[libvirt] [PATCHv1.5 04/27] qemuxml2argv: Add test for disk type='volume' with iSCSI pools
Peter Krempa
pkrempa at redhat.com
Wed Nov 27 09:54:30 UTC 2013
On 11/27/13 09:29, Osier Yang wrote:
> On 27/11/13 00:48, Peter Krempa wrote:
>> Tweak the existing file so that it can be tested for command line
>> corectness.
>
> It's actually lots of change, should provide detailed commit log.
Hmmm, this patch actually should only touch the test files, but I
somehow accidentally squashed patch named:
qemu: Refactor qemuTranslatePool source (seen in v1 posting of this series)
with a more verbose error message. I'll split them again in next posting.
>
>> ---
>> 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 +
>> .../qemuxml2argv-disk-source-pool-mode.args | 10 ++
>> .../qemuxml2argv-disk-source-pool-mode.xml | 4 +-
>> tests/qemuxml2argvtest.c | 2 +
>> 8 files changed, 112 insertions(+), 113 deletions(-)
>> create mode 100644
>> tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.args
>>
>> 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)
>
> Wondering why this is completely removed, before going ahead...
Well a function like this should never be necessary. The translation
function has to populate the fields in such way, that the regular
generator then does the same thing without any need for ugly hacks like
this.
>
>> -{
>> - 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"));
>
> With this removed, I guess one will see different error like:
>
> _("tray status 'open' is invalid for block type disk"));
>
> I'm doubting it's what we want for a "volume" type disk.
I don't think this is a serious problem. If you insist I can change the
error message in the normal command line generator, but in this case I
think we would be gilding the lily here [1].
>
>> - 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);
>
> Especially here, I'm wondering if the "mode" attribute for iscsi pool
> disk still works.
Why do it here if we can while translating the volume from the pool? see
below.
Also ... the test file that is touched in this patch tests it. It's not
that apparent, but these changes should be after the file was used to
test command line generation and after changing this piece of code the
test still passes.
>
>> - }
>> - } 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,
>
> Not sure if it's wise to use "VIR_ERR_CONFIG_UNSUPPORTED" here.
Do we have a special one for this reason? Or is it worth inventing a new
one?
>
>> + _("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) {
>
> Why this check is removed?
Shouldn't this actually be checked in the function that generates the
commandline? It seemed to me as duplicated code.
>
>> + 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"));
>> + _("disk source mode is only valid when "
>> + "storage pool is of iscsi type"));
>> goto cleanup;
>> }
>>
>> - switch ((virStorageVolType) info.type) {
>> - case VIR_STORAGE_VOL_FILE:
>> - case VIR_STORAGE_VOL_DIR:
>> + switch ((enum virStoragePoolType) pooldef->type) {
>> + case VIR_STORAGE_POOL_DIR:
>> + case VIR_STORAGE_POOL_FS:
>> + case VIR_STORAGE_POOL_NETFS:
>> + case VIR_STORAGE_POOL_LOGICAL:
>> + case VIR_STORAGE_POOL_DISK:
>> + case VIR_STORAGE_POOL_SCSI:
>> if (!(def->src = virStorageVolGetPath(vol)))
>> goto cleanup;
>> - break;
>> - case VIR_STORAGE_VOL_BLOCK:
>> - if (!(poolxml = virStoragePoolGetXMLDesc(pool, 0)))
>> - goto cleanup;
>> -
>> - if (!(pooldef = virStoragePoolDefParseString(poolxml)))
>> - goto cleanup;
>>
>> - if (def->srcpool->mode && pooldef->type !=
>> VIR_STORAGE_POOL_ISCSI) {
>> - virReportError(VIR_ERR_XML_ERROR, "%s",
>> - _("disk source mode is only valid when "
>> - "storage pool is of iscsi type"));
>> + switch (info.type) {
>> + case VIR_STORAGE_VOL_FILE:
>> + def->srcpool->actualtype = VIR_DOMAIN_DISK_TYPE_FILE;
>> + break;
>> +
>> + case VIR_STORAGE_VOL_DIR:
>> + def->srcpool->actualtype = VIR_DOMAIN_DISK_TYPE_DIR;
>> + break;
>> +
>> + case VIR_STORAGE_VOL_BLOCK:
>> + def->srcpool->actualtype = VIR_DOMAIN_DISK_TYPE_BLOCK;
>> + break;
>> +
>> + case VIR_STORAGE_VOL_NETWORK:
>> + case VIR_STORAGE_VOL_NETDIR:
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("unexpected storage volume type '%s' "
>> + "for storage pool type '%s'"),
>> + virStorageVolTypeToString(info.type),
>> + virStoragePoolTypeToString(pooldef->type));
>> goto cleanup;
>> }
>>
>> - def->srcpool->pooltype = pooldef->type;
>> - if (pooldef->type == VIR_STORAGE_POOL_ISCSI) {
>> - /* Default to use the LUN's path on host */
>> - if (!def->srcpool->mode)
>> - def->srcpool->mode =
>> VIR_DOMAIN_DISK_SOURCE_POOL_MODE_HOST;
>> -
>> - if (def->srcpool->mode ==
>> - VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DIRECT) {
>> - if (qemuAddISCSIPoolSourceHost(def, pooldef) < 0)
>> - goto cleanup;
>> - } else if (def->srcpool->mode ==
>> - VIR_DOMAIN_DISK_SOURCE_POOL_MODE_HOST) {
>> - if (!(def->src = virStorageVolGetPath(vol)))
>> - goto cleanup;
>> - }
>> + break;
>> +
>> + case VIR_STORAGE_POOL_ISCSI:
>> + switch (def->srcpool->mode) {
>> + case VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DEFAULT:
>> + case VIR_DOMAIN_DISK_SOURCE_POOL_MODE_LAST:
>> + def->srcpool->mode = VIR_DOMAIN_DISK_SOURCE_POOL_MODE_HOST;
>> + def->srcpool->actualtype = VIR_DOMAIN_DISK_TYPE_BLOCK;
>> + /* fallthrough */
>
> I'd prefer the old "if...else" statements instead of a "fallthrough", which
> explicitly sets the "mode" as VIR_DOMAIN_DISK_SOURCE_POOL_MODE_HOST
> before going ahead.
Well this is functionally same and less messy when attempting to add new
stuff.
>
>> + case VIR_DOMAIN_DISK_SOURCE_POOL_MODE_HOST:
>> + if (!(def->src = virStorageVolGetPath(vol)))
>> + goto cleanup;
>> + break;
>> +
>> + case VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DIRECT:
>> + def->srcpool->actualtype = VIR_DOMAIN_DISK_TYPE_NETWORK;
>> + def->protocol = VIR_DOMAIN_DISK_PROTOCOL_ISCSI;
>>
>> if (qemuTranslateDiskSourcePoolAuth(def, pooldef) < 0)
>> goto cleanup;
>> - } else {
>> - if (!(def->src = virStorageVolGetPath(vol)))
>> +
>> + if (qemuAddISCSIPoolSourceHost(def, pooldef) < 0)
>> goto cleanup;
>> + break;
>> }
>> -
>> break;
>> - case VIR_STORAGE_VOL_NETWORK:
>> - case VIR_STORAGE_VOL_NETDIR:
>> - case VIR_STORAGE_VOL_LAST:
>> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> - _("Using network volume as disk source is not
>> supported"));
>> +
>> + case VIR_STORAGE_POOL_MPATH:
>> + case VIR_STORAGE_POOL_RBD:
>> + case VIR_STORAGE_POOL_SHEEPDOG:
>> + case VIR_STORAGE_POOL_GLUSTER:
>> + case VIR_STORAGE_POOL_LAST:
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> + _("using '%s' pools for backing 'volume' disks "
>> + "isn't yet supported"),
>> + virStoragePoolTypeToString(pooldef->type));
>> goto cleanup;
>> }
>>
>> - def->srcpool->voltype = info.type;
>> ret = 0;
>> cleanup:
>> if (ret < 0)
>> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
>> index f9ff7af..b9786b1 100644
>> --- a/src/qemu/qemu_conf.h
>> +++ b/src/qemu/qemu_conf.h
>> @@ -304,6 +304,8 @@ int qemuSetUnprivSGIO(virDomainDeviceDefPtr dev);
>> int qemuDriverAllocateID(virQEMUDriverPtr driver);
>> virDomainXMLOptionPtr virQEMUDriverCreateXMLConf(virQEMUDriverPtr
>> driver);
>>
>> +int qemuDiskGetActualType(virDomainDiskDefPtr def);
>> +
>> int qemuTranslateDiskSourcePool(virConnectPtr conn,
>> virDomainDiskDefPtr def);
>>
>> diff --git
>> a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.args
>> b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.args
>> new file mode 100644
>> index 0000000..8f6a3dd
>> --- /dev/null
>> +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.args
>> @@ -0,0 +1,10 @@
>> +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test
>> QEMU_AUDIO_DRV=none \
>> +/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefconfig
>> -nodefaults \
>> +-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb
>> -drive \
>> +file=/some/block/device/unit:0:0:1,if=none,media=cdrom,id=drive-ide0-0-1
>> -device \
>> +ide-drive,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1 -drive \
>> +file=iscsi://iscsi.example.com:3260/demo-target/2,if=none,media=cdrom,id=drive-ide0-0-2
>> \
>> +-device ide-drive,bus=ide.0,unit=2,drive=drive-ide0-0-2,id=ide0-0-2
>> -drive \
>> +file=/tmp/idedisk.img,if=none,id=drive-ide0-0-3 -device \
>> +ide-drive,bus=ide.0,unit=3,drive=drive-ide0-0-3,id=ide0-0-3 -device \
>> +virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4
>> diff --git
>> a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml
>> b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml
>> index b907633..9f90293 100644
>> --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml
>> +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml
>> @@ -15,7 +15,7 @@
>> <devices>
>> <emulator>/usr/bin/qemu</emulator>
>> <disk type='volume' device='cdrom'>
>> - <source pool='blk-pool0' volume='blk-pool0-vol0' mode='host'
>> startupPolicy='optional'>
>> + <source pool='pool-iscsi-auth' volume='unit:0:0:1' mode='host'>
>
> Same question as above.
>
> It seems this patch destroyed the "mode" attr for iscsi pool disk
> completely
If you think so, please provide a test case that proves this that would
be applied before this patch that would show the problem. Otherwise I'm
pretty confident that it works as it's supposed.
> with this patch. It again asks for the detailed commit log, since the
> changes
> looks rather aggressive, which may cause regression.
I'll split those patches again as they should be.
>
> Osier
Peter
[1]: gild the lily
* 1. To adorn unnecessarily something already beautiful.
* 2. To make superfluous additions to what is already complete.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 901 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20131127/0ae695a4/attachment-0001.sig>
More information about the libvir-list
mailing list