[libvirt] [PATCH 1/3] qemu: Support for Disk Geometry Override

[Daniel P. Berrange]

On Fri, Jul 13, 2012 at 10:45:28AM +0200, Viktor Mihajlovski wrote:
> From: J.B. Joret <jb at linux.vnet.ibm.com>
> 
> Qemu allows to override the disk geometry in the drive specification
> with cyls=,heads=,secs=[,trans=].
> This patch extends the domain config and the qemu driver to allow
> the specification of disk geometry with libvirt.
> 
> Signed-off-by: J.B. Joret <jb at linux.vnet.ibm.com>
> Signed-off-by: Viktor Mihajlovski <mihajlov at linux.vnet.ibm.com>
> ---
>  src/conf/domain_conf.c   |   71 ++++++++++++++++++++++++++++++++++++++++++++++
>  src/conf/domain_conf.h   |   17 +++++++++++
>  src/libvirt_private.syms |    2 +
>  src/qemu/qemu_command.c  |   59 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 149 insertions(+), 0 deletions(-)

Since you're adding to the public XML schema, please also update
the files docs/schemas/domaincommon.rng and docs/formatdomain.html.in
with appropriate info. It would also be desirable to add some test
cases to the qemuxml2argvtest.c test to validate this.


> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 4f8c57a..4b208fc 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -170,6 +170,12 @@ VIR_ENUM_IMPL(virDomainDiskDevice, VIR_DOMAIN_DISK_DEVICE_LAST,
>                "floppy",
>                "lun")
>  
> +VIR_ENUM_IMPL(virDomainDiskGeometryTrans, VIR_DOMAIN_DISK_TRANS_LAST,
> +              "default",
> +              "none",
> +              "auto",
> +              "lba")
> +
>  VIR_ENUM_IMPL(virDomainDiskBus, VIR_DOMAIN_DISK_BUS_LAST,
>                "ide",
>                "fdc",
> @@ -3347,6 +3353,7 @@ virDomainDiskDefParseXML(virCapsPtr caps,
>      char *source = NULL;
>      char *target = NULL;
>      char *protocol = NULL;
> +    char *trans = NULL;
>      virDomainDiskHostDefPtr hosts = NULL;
>      int nhosts = 0;
>      char *bus = NULL;
> @@ -3375,6 +3382,11 @@ virDomainDiskDefParseXML(virCapsPtr caps,
>          return NULL;
>      }
>  
> +    def->geometry.cylinders = 0;
> +    def->geometry.heads = 0;
> +    def->geometry.sectors = 0;
> +    def->geometry.trans = VIR_DOMAIN_DISK_TRANS_DEFAULT;
> +
>      ctxt->node = node;
>  
>      type = virXMLPropString(node, "type");
> @@ -3484,6 +3496,40 @@ virDomainDiskDefParseXML(virCapsPtr caps,
>                  if (target &&
>                      STRPREFIX(target, "ioemu:"))
>                      memmove(target, target+6, strlen(target)-5);
> +            } else if (xmlStrEqual(cur->name, BAD_CAST "geometry")) {
> +                if (virXPathUInt("string(./geometry/@cyls)",
> +                                 ctxt, &def->geometry.cylinders) < 0) {
> +                    virDomainReportError(VIR_ERR_INTERNAL_ERROR,
> +                                         _("invalid geometry settings (cyls)"));
> +                    goto error;
> +                }
> +                if (virXPathUInt("string(./geometry/@heads)",
> +                                 ctxt, &def->geometry.heads) < 0) {
> +                    virDomainReportError(VIR_ERR_INTERNAL_ERROR,
> +                                         _("invalid geometry settings (heads)"));
> +                    goto error;
> +                }
> +                if (virXPathUInt("string(./geometry/@secs)",
> +                                 ctxt, &def->geometry.sectors) < 0) {
> +                    virDomainReportError(VIR_ERR_INTERNAL_ERROR,
> +                                         _("invalid geometry settings (secs)"));
> +                    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 {
> +                        virDomainReportError(VIR_ERR_INTERNAL_ERROR,
> +                                             _("invalid translation value '%s'"),
> +                                             trans);
> +                        goto error;

Since you went to trouble of defining VIR_ENUM, you should be using
virDomainDiskGeometryTransTypeFromString() here instead of all these
STREQs.

> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index ae48678..23d1aba 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -2016,6 +2016,8 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
>  {
>      virBuffer opt = VIR_BUFFER_INITIALIZER;
>      const char *bus = virDomainDiskQEMUBusTypeToString(disk->bus);
> +    const char *trans =
> +        virDomainDiskGeometryTransTypeToString(disk->geometry.trans);
>      int idx = virDiskNameToIndex(disk->dst);
>      int busid = -1, unitid = -1;
>  
> @@ -2218,6 +2220,24 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
>          disk->type != VIR_DOMAIN_DISK_TYPE_DIR &&
>          qemuCapsGet(qemuCaps, QEMU_CAPS_DRIVE_FORMAT))
>          virBufferAsprintf(&opt, ",format=%s", disk->driverType);
> +
> +    /* generate geometry command string*/
> +    if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK ||
> +        disk->type == VIR_DOMAIN_DISK_TYPE_FILE) {

The geometry information is used to set guest device properties.
The disk->type setting is about the host backend image type.
So I don't see why there's a dependancy between them. IMHO you
should just remove this if, and unconditionally set the geometry
regardless of what the backend type is.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|