On 08/21/2012 10:26 AM, Daniel P. Berrange wrote:
> On Mon, Aug 20, 2012 at 03:58:50PM +0200, Viktor Mihajlovski wrote:
>> From: J.B. Joret <jb@xxxxxxxxxxxxxxxxxx>
>>
>> A hypervisor may allow to override the disk geometry of drives.
>> Qemu, as an example with cyls=,heads=,secs=[,trans=].
>> This patch extends the domain config to allow the specification of
>> disk geometry with libvirt.
>>
>> V2 Changes: Split out qemu specific code, add documentation and schema.
>>
>> V3 Changes: Moved geometry element to diskspec and use virReportError now.
>>
>> V6 Changes: Generate geometry for all host device types.
We tend to put the patch changelog...
>>
>> Signed-off-by: J.B. Joret <jb@xxxxxxxxxxxxxxxxxx>
>> Signed-off-by: Viktor Mihajlovski <mihajlov@xxxxxxxxxxxxxxxxxx>
>> ---
after the --- line, as then it doesn't pollute the git history. It's
great for review purposes in email, but once the thread is over, no one
cares any longer how many revisions it took to get to the patch in git.
>> docs/formatdomain.html.in | 25 ++++++++++++++
>> docs/schemas/domaincommon.rng | 25 ++++++++++++++
>> src/conf/domain_conf.c | 71 +++++++++++++++++++++++++++++++++++++++++
>> src/conf/domain_conf.h | 17 ++++++++++
>> src/libvirt_private.syms | 2 +
>> 5 files changed, 140 insertions(+), 0 deletions(-)
> + <dt><code>geometry</code></dt>
> + <dd>The optional <code>geometry</code> element provides the
> + ability to override geometry settings. This mostly useful for
> + S390 DASD-disks or older DOS-disks
Trailing '.'
> + <dl>
> + <dt><code>cyls</code></dt>
> + <dd>The <code>cyls</code> element is the
s/element/attribute/
> + number of cylinders. </dd>
> + <dt><code>heads</code></dt>
> + <dd>The <code>heads</code> element is the
> + number of heads. </dd>
> + <dt><code>secs</code></dt>
> + <dd>The <code>secs</code> element is the
> + number of sectors per track. </dd>
> + <dt><code>trans</code></dt>
> + <dd>The optional <code>trans</code> element is the
> + BIOS-Translation-Modus (none, lba or auto)</dd>
> + </dl>
Missing a 'since 0.10.0' designation.
> + trans = virXMLPropString(cur, "trans");
> + if (trans != NULL) {
> + if (STREQ(trans, "none"))
> + def->geometry.trans = VIR_DOMAIN_DISK_TRANS_NONE;
> + else if (STREQ(trans, "auto"))
> + def->geometry.trans = VIR_DOMAIN_DISK_TRANS_AUTO;
> + else if (STREQ(trans, "lba"))
> + def->geometry.trans = VIR_DOMAIN_DISK_TRANS_LBA;
> + else {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("invalid translation value '%s'"),
> + trans);
> + goto error;
Why is this open-coded, instead of using...
> +++ b/src/libvirt_private.syms
> @@ -320,6 +320,8 @@ virDomainDiskDefAssignAddress;
> virDomainDiskDefForeachPath;
> virDomainDiskDefFree;
> virDomainDiskDeviceTypeToString;
> +virDomainDiskGeometryTransTypeToString;
> +virDomainDiskGeometryTransTypeFromString;
> virDomainDiskErrorPolicyTypeFromString;
the new TypeFromString function? Also, this isn't sorted.
>
> ACK
Pushed with the following tweaks, and with a listing for you in AUTHORS;
let me know if I need to alter the spelling that you prefer for name or
email.
diff --git i/AUTHORS w/AUTHORS
index 5dec3a2..289c984 100644
--- i/AUTHORS
+++ w/AUTHORS
@@ -258,6 +258,8 @@ Patches have also been contributed by:
...
diff --git i/docs/formatdomain.html.in w/docs/formatdomain.html.in
index 5bb9a76..d87ca6b 100644
--- i/docs/formatdomain.html.in
+++ w/docs/formatdomain.html.in
@@ -1580,19 +1580,19 @@
<dt><code>geometry</code></dt>
<dd>The optional <code>geometry</code> element provides the
ability to override geometry settings. This mostly useful for
- S390 DASD-disks or older DOS-disks
+ S390 DASD-disks or older DOS-disks. <span
class="since">0.10.0</span>
<dl>
<dt><code>cyls</code></dt>
- <dd>The <code>cyls</code> element is the
+ <dd>The <code>cyls</code> attribute is the
number of cylinders. </dd>
<dt><code>heads</code></dt>
- <dd>The <code>heads</code> element is the
+ <dd>The <code>heads</code> attribute is the
number of heads. </dd>
<dt><code>secs</code></dt>
- <dd>The <code>secs</code> element is the
+ <dd>The <code>secs</code> attribute is the
number of sectors per track. </dd>
<dt><code>trans</code></dt>
- <dd>The optional <code>trans</code> element is the
+ <dd>The optional <code>trans</code> attribute is the
BIOS-Translation-Modus (none, lba or auto)</dd>
</dl>
</dd>
diff --git i/src/conf/domain_conf.c w/src/conf/domain_conf.c
index 754b1f2..c516685 100644
--- i/src/conf/domain_conf.c
+++ w/src/conf/domain_conf.c
@@ -3520,14 +3520,9 @@ virDomainDiskDefParseXML(virCapsPtr caps,
goto error;
}
trans = virXMLPropString(cur, "trans");
- if (trans != NULL) {
- if (STREQ(trans, "none"))
- def->geometry.trans = VIR_DOMAIN_DISK_TRANS_NONE;
- else if (STREQ(trans, "auto"))
- def->geometry.trans = VIR_DOMAIN_DISK_TRANS_AUTO;
- else if (STREQ(trans, "lba"))
- def->geometry.trans = VIR_DOMAIN_DISK_TRANS_LBA;
- else {
+ if (trans) {
+ def->geometry.trans =
virDomainDiskGeometryTransTypeFromString(trans);
+ if (def->geometry.trans <= 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("invalid translation value '%s'"),
trans);
diff --git i/src/libvirt_private.syms w/src/libvirt_private.syms
index 054f550..b20a754 100644
--- i/src/libvirt_private.syms
+++ w/src/libvirt_private.syms
@@ -323,11 +323,11 @@ virDomainDiskDefAssignAddress;
virDomainDiskDefForeachPath;
virDomainDiskDefFree;
virDomainDiskDeviceTypeToString;
-virDomainDiskGeometryTransTypeToString;
-virDomainDiskGeometryTransTypeFromString;
virDomainDiskErrorPolicyTypeFromString;
virDomainDiskErrorPolicyTypeToString;
virDomainDiskFindControllerModel;
+virDomainDiskGeometryTransTypeFromString;
+virDomainDiskGeometryTransTypeToString;
virDomainDiskIndexByName;
virDomainDiskInsert;
virDomainDiskInsertPreAlloced;
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list