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

[Osier Yang]

On 06/04/13 07:24, John Ferlan wrote:
> 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).

Except Eric's feedback, see the discussion about "Migration with NPIV"
here:

http://www.redhat.com/archives/libvir-list/2012-November/msg00826.html

>
> 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...

0 means the VIR_STORAGE_VOL_FILE.

>
>>   
>>       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 :-)

That's more polite, changed. :-)

>
>> +                 * 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
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list