[libvirt] [PATCHv2 4/4] qemu: Refactor qemuTranslatePool source

Osier Yang jyang at redhat.com
Thu Nov 28 09:03:19 UTC 2013


On 27/11/13 23:14, Peter Krempa wrote:
> Before this patch, the translation function still needs a second ugly
> helper function to actually format the command line for qemu. But if we
> do the right stuff in the translation functio, we don't have to bother

s/functio/function/,

> with the second function any more.
>
> This patch removes the messy qemuBuildVolumeString() function and
> changes qemuTranslatePool() to set stuff up correctly so that the

s/qemuTranslatePool/qemuTranslateDiskSourcePool/,

> regular code paths meant for volumes can be used to format the command
> line correctly.
>
> For this purpose a new helper "qemuDiskGetActualType()" is introduced to
> return the type of the volume in a pool.
>
> As a part of the refactor the qemuTranslatePool function is fixed to do

Same as above.

> decisions based on the pool type instead of the volume type. This allows
> to separate pool-type-specific stuff more clearly and will ease addition
> of other pool types that will require certain other operations to get
> the correct pool source.
>
> The previously fixed tests should make sure that we don't break stuff
> that was working before.
> ---
>   src/conf/domain_conf.h   |   1 +
>   src/libvirt_private.syms |   1 +
>   src/qemu/qemu_command.c  |  76 +++-------------------------
>   src/qemu/qemu_conf.c     | 129 ++++++++++++++++++++++++++++++++---------------
>   src/qemu/qemu_conf.h     |   2 +
>   5 files changed, 98 insertions(+), 111 deletions(-)
>
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 4561ccc..a5ef2ca 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -686,6 +686,7 @@ struct _virDomainDiskSourcePoolDef {
>       char *volume; /* volume name */
>       int voltype; /* enum virStorageVolType, internal only */
>       int pooltype; /* enum virStoragePoolType, internal only */
> +    int actualtype; /* enum virDomainDiskType, internal only */
>       int mode; /* enum virDomainDiskSourcePoolMode */
>   };
>   typedef virDomainDiskSourcePoolDef *virDomainDiskSourcePoolDefPtr;
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 205fe56..aeb3568 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -693,6 +693,7 @@ virStoragePoolSourceFree;
>   virStoragePoolSourceListFormat;
>   virStoragePoolSourceListNewSource;
>   virStoragePoolTypeFromString;
> +virStoragePoolTypeToString;
>   virStorageVolDefFindByKey;
>   virStorageVolDefFindByName;
>   virStorageVolDefFindByPath;
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 763417f..b8129a3 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3775,69 +3775,6 @@ qemuBuildNBDString(virConnectPtr conn, virDomainDiskDefPtr disk, virBufferPtr op
>       return 0;
>   }
>
> -static int
> -qemuBuildVolumeString(virConnectPtr conn,
> -                      virDomainDiskDefPtr disk,
> -                      virBufferPtr opt)

I admit I was confused when reviewing v2 without the commit log.

> -{
> -    int ret = -1;
> -
> -    switch ((virStorageVolType) disk->srcpool->voltype) {
> -    case VIR_STORAGE_VOL_DIR:
> -        if (!disk->readonly) {
> -            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                           _("cannot create virtual FAT disks in read-write mode"));
> -            goto cleanup;
> -        }
> -        if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY)
> -            virBufferEscape(opt, ',', ",", "file=fat:floppy:%s,", disk->src);
> -        else
> -            virBufferEscape(opt, ',', ",", "file=fat:%s,", disk->src);
> -        break;
> -    case VIR_STORAGE_VOL_BLOCK:
> -        if (disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN) {
> -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -                           _("tray status 'open' is invalid for "
> -                             "block type volume"));

[1]

I keep my opinion that it should provide an exact error, either "block"
or "volume" is the term we used in the XML and documents.  One
will be confused to see "tray status 'open' is invalid for block type disk",
since it's actually defined as type="volume". It doesn't have to be
the same, but there should be a way to differentiate them.

> -            goto cleanup;
> -        }
> -        if (disk->srcpool->pooltype == VIR_STORAGE_POOL_ISCSI) {
> -            if (disk->srcpool->mode ==
> -                VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DIRECT) {
> -                if (qemuBuildISCSIString(conn, disk, opt) < 0)
> -                    goto cleanup;
> -                virBufferAddChar(opt, ',');
> -            } else if (disk->srcpool->mode ==
> -                       VIR_DOMAIN_DISK_SOURCE_POOL_MODE_HOST) {
> -                virBufferEscape(opt, ',', ",", "file=%s,", disk->src);
> -            }
> -        } else {
> -            virBufferEscape(opt, ',', ",", "file=%s,", disk->src);
> -        }
> -        break;
> -    case VIR_STORAGE_VOL_FILE:
> -        if (disk->auth.username) {
> -            if (qemuBuildISCSIString(conn, disk, opt) < 0)
> -                goto cleanup;
> -            virBufferAddChar(opt, ',');
> -        } else {
> -            virBufferEscape(opt, ',', ",", "file=%s,", disk->src);
> -        }
> -        break;
> -    case VIR_STORAGE_VOL_NETWORK:
> -    case VIR_STORAGE_VOL_NETDIR:
> -    case VIR_STORAGE_VOL_LAST:
> -        /* Keep the compiler quiet, qemuTranslateDiskSourcePool already
> -         * reported the unsupported error.
> -         */
> -        goto cleanup;
> -    }
> -
> -    ret = 0;
> -
> -cleanup:
> -    return ret;
> -}
>
>   char *
>   qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
> @@ -3851,6 +3788,7 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
>           virDomainDiskGeometryTransTypeToString(disk->geometry.trans);
>       int idx = virDiskNameToIndex(disk->dst);
>       int busid = -1, unitid = -1;
> +    int actualType = qemuDiskGetActualType(disk);
>
>       if (idx < 0) {
>           virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -3934,12 +3872,13 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
>
>       /* disk->src is NULL when we use nbd disks */
>       if ((disk->src ||
> -        (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK &&
> +        (actualType == VIR_DOMAIN_DISK_TYPE_NETWORK &&
>            disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_NBD)) &&
>           !((disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY ||
>              disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) &&
>             disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN)) {
> -        if (disk->type == VIR_DOMAIN_DISK_TYPE_DIR) {
> +
> +        if (actualType == VIR_DOMAIN_DISK_TYPE_DIR) {
>               /* QEMU only supports magic FAT format for now */
>               if (disk->format > 0 && disk->format != VIR_STORAGE_FILE_FAT) {
>                   virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -3957,7 +3896,7 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
>                                   disk->src);
>               else
>                   virBufferEscape(&opt, ',', ",", "file=fat:%s,", disk->src);
> -        } else if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) {
> +        } else if (actualType == VIR_DOMAIN_DISK_TYPE_NETWORK) {
>               switch (disk->protocol) {
>               case VIR_DOMAIN_DISK_PROTOCOL_NBD:
>                   if (qemuBuildNBDString(conn, disk, &opt) < 0)
> @@ -4025,11 +3964,8 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
>                   virBufferEscape(&opt, ',', ",", "%s,", disk->src);
>                   break;
>               }
> -        } else if (disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME) {
> -            if (qemuBuildVolumeString(conn, disk, &opt) < 0)
> -                goto error;
>           } else {
> -            if ((disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK) &&
> +            if ((actualType == VIR_DOMAIN_DISK_TYPE_BLOCK) &&
>                   (disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN)) {
>                   virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>                                  _("tray status 'open' is invalid for "
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 77df370..58a0500 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -1298,6 +1298,17 @@ cleanup:
>       return ret;
>   }
>
> +
> +int
> +qemuDiskGetActualType(virDomainDiskDefPtr def)
> +{
> +    if (def->type == VIR_DOMAIN_DISK_TYPE_VOLUME)
> +        return def->srcpool->actualtype;
> +
> +    return def->type;
> +}
> +
> +
>   int
>   qemuTranslateDiskSourcePool(virConnectPtr conn,
>                               virDomainDiskDefPtr def)
> @@ -1319,72 +1330,108 @@ qemuTranslateDiskSourcePool(virConnectPtr conn,
>       if (!(pool = virStoragePoolLookupByName(conn, def->srcpool->pool)))
>           return -1;
>
> +    if (virStoragePoolIsActive(pool) != 1) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("storage pool '%s' containing volume '%s' "
> +                         "is not active"),
> +                       def->srcpool->pool, def->srcpool->volume);
> +        goto cleanup;
> +    }
> +
>       if (!(vol = virStorageVolLookupByName(pool, def->srcpool->volume)))
>           goto cleanup;
>
>       if (virStorageVolGetInfo(vol, &info) < 0)
>           goto cleanup;
>
> -    if (def->startupPolicy &&
> -        info.type != VIR_STORAGE_VOL_FILE) {

There is no checking for startupPolicy in qemu driver actually, the checking
is done when parsing, see src/conf/domain_conf.c.  That means previously
we report error for volume not of "file" type if it has "startupPolicy" 
defined.
(looks like the checking should be more complex than before with a glance
of the current checkings in parsing code, btw). After this changing, we just
ignore the checking, whether it will cause problem when the code flow 
forward
is a question.

For "volume" type disk, we have no way to check when parsing, since you
don't know what's the volume type yet. So it must be do here. I don't think
you will want to mix the checking in command line building, since it only
makes sense for "volume" type disk, and the command line build helper
is already quite big.

> +    if (!(poolxml = virStoragePoolGetXMLDesc(pool, 0)))
> +        goto cleanup;
> +
> +    if (!(pooldef = virStoragePoolDefParseString(poolxml)))
> +        goto cleanup;
> +
> +    def->srcpool->pooltype = pooldef->type;
> +    def->srcpool->voltype = info.type;
> +
> +    if (def->srcpool->mode && pooldef->type != VIR_STORAGE_POOL_ISCSI) {
>           virReportError(VIR_ERR_XML_ERROR, "%s",
> -                       _("'startupPolicy' is only valid for 'file' type volume"));

Same as [1]

Others look fine.

Osier




More information about the libvir-list mailing list