[libvirt] [PATCH 3/8] qemu: Translate the pool disk source when building drive string

[John Ferlan]

On 04/04/2013 03:37 PM, Osier Yang wrote:
> This adds a new helper qemuTranslateDiskSourcePool which uses the
> storage pool/vol APIs to tranlsate the disk source before building

s/tranlsate/translate/

> the drive string. Network volume is not supported yet. Disk chain
> for volume type disk may be supported later, but before I'm confident
> it doesn't break anything, it's just disabled now.


How would 'network volume' differ from "<disk type='network'"? And all
the variants therein?

Still trying to figure the 'benefit' of the volume tag since each of the
types looked up in the pool could also be specified as "<disk
type='xxx'" types (where xxx is file, block, dir).

But I'll press on with at least a mechanical review.  Maybe someone else
can chime in with the product level viewpoint.

> ---
>  src/qemu/qemu_command.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_domain.c  |  4 ++-
>  2 files changed, 82 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 693d30d..03c7195 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -2681,6 +2681,49 @@ qemuBuildNBDString(virConnectPtr conn, virDomainDiskDefPtr disk, virBufferPtr op
>      return 0;
>  }
>  
> +static int
> +qemuTranslateDiskSourcePool(virConnectPtr conn,
> +                            virDomainDiskDefPtr def,
> +                            int *voltype)
> +{
> +    virStoragePoolPtr pool = NULL;
> +    virStorageVolPtr vol = NULL;
> +    virStorageVolInfo info;
> +    int ret = -1;
> +
> +    if (def->type != VIR_DOMAIN_DISK_TYPE_VOLUME)
> +        return 0;
> +
> +    if (!(pool = virStoragePoolLookupByName(conn, def->srcpool->pool)))
> +        return -1;
> +
> +    if (!(vol = virStorageVolLookupByName(pool, def->srcpool->volume)))
> +        goto cleanup;
> +
> +    if (virStorageVolGetInfo(vol, &info) < 0)
> +        goto cleanup;
> +
> +    switch (info.type) {
> +    case VIR_STORAGE_VOL_FILE:
> +    case VIR_STORAGE_VOL_BLOCK:
> +    case VIR_STORAGE_VOL_DIR:
> +        if (!(def->src = virStorageVolGetPath(vol)))
> +            goto cleanup;
> +        break;
> +    case VIR_STORAGE_VOL_NETWORK:
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("Using network volume as disk source is not supported"));
> +        goto cleanup;
> +    }
> +
> +    *voltype = info.type;
> +    ret = 0;
> +cleanup:
> +    virStoragePoolFree(pool);
> +    virStorageVolFree(vol);
> +    return ret;
> +}
> +
>  char *
>  qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
>                    virDomainDiskDefPtr disk,
> @@ -2693,6 +2736,7 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
>          virDomainDiskGeometryTransTypeToString(disk->geometry.trans);
>      int idx = virDiskNameToIndex(disk->dst);
>      int busid = -1, unitid = -1;
> +    int voltype = -1;

Does initializing this matter?  Ah perhaps the compiler complains...

>  
>      if (idx < 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -2700,6 +2744,9 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
>          goto error;
>      }
>  
> +    if (qemuTranslateDiskSourcePool(conn, disk, &voltype) < 0)
> +        goto error;
> +
>      switch (disk->bus) {
>      case VIR_DOMAIN_DISK_BUS_SCSI:
>          if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) {
> @@ -2834,6 +2881,38 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
>                  }
>                  break;
>              }
> +        } else if (disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME) {
> +            switch (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 error;
> +                }
> +                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"));
> +                    goto error;
> +                }
> +                virBufferEscape(&opt, ',', ",", "file=%s,", disk->src);
> +                break;
> +            case VIR_STORAGE_VOL_FILE:
> +                virBufferEscape(&opt, ',', ",", "file=%s,", disk->src);
> +                break;
> +            case VIR_STORAGE_VOL_NETWORK:
> +                /* Let the compiler shutup, qemuTranslateDiskSourcePool already

consider "keep the compiler quiet" :-) although I understand the
sentiment :-)

> +                 * reported the unsupported error.
> +                 */
> +                break;
> +            }
>          } else {
>              if ((disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK) &&
>                  (disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN)) {
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index c79b05d..6762152 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1943,7 +1943,9 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
>      virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>      int ret = 0;
>  
> -    if (!disk->src || disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK)
> +    if (!disk->src ||
> +        disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK ||
> +        disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME)
>          goto cleanup;
>  
>      if (disk->backingChain) {
> 

ACK, mechnically it seems what's here covers things.

John