Alter the example to remove the <auth> from: <disk type='volume' device='disk'> <driver name='qemu' type='raw'/> <source pool='iscsi-pool' volume='unit:0:0:1' mode='host'/> <auth username='myuser'> <secret type='iscsi' usage='libvirtiscsi'/> </auth> <target dev='vdb' bus='virtio'/> </disk> and <disk type='volume' device='disk'> <driver name='qemu' type='raw'/> <source pool='iscsi-pool' volume='unit:0:0:2' mode='direct'/> <auth username='myuser'> <secret type='iscsi' usage='libvirtiscsi'/> </auth> <target dev='vdc' bus='virtio'/> </disk> The reality is, it's not even used. For a <source pool> the authdef from the storage source pool will supercede whatever is in the <disk> definition during virStorageTranslateDiskSourcePool processing. In fact, if the pool doesn't have/need authentication, then the authdef would be removed anyway as the storage pool would be handling things. The "proof" for this is in the adjustment to the test to add an <auth> for a disk. The resulting .args file won't add what normally would be added "myname:encodedpassword@" prior to the hostname in the IQN (e.g. iscsi://myname:encodedpassword@iscsi.example.org:3260/... Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- docs/formatdomain.html.in | 6 ------ .../qemuxml2argv-disk-source-pool-mode.args | 3 +++ .../qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml | 13 +++++++++++++ .../qemuxml2xmlout-disk-source-pool-mode.xml | 13 +++++++++++++ 4 files changed, 29 insertions(+), 6 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 8ca7637a4..3b78bbeb8 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2385,17 +2385,11 @@ &lt;disk type='volume' device='disk'&gt; &lt;driver name='qemu' type='raw'/&gt; &lt;source pool='iscsi-pool' volume='unit:0:0:1' mode='host'/&gt; - &lt;auth username='myuser'&gt; - &lt;secret type='iscsi' usage='libvirtiscsi'/&gt; - &lt;/auth&gt; &lt;target dev='vdb' bus='virtio'/&gt; &lt;/disk&gt; &lt;disk type='volume' device='disk'&gt; &lt;driver name='qemu' type='raw'/&gt; &lt;source pool='iscsi-pool' volume='unit:0:0:2' mode='direct'/&gt; - &lt;auth username='myuser'&gt; - &lt;secret type='iscsi' usage='libvirtiscsi'/&gt; - &lt;/auth&gt; &lt;target dev='vdc' bus='virtio'/&gt; &lt;/disk&gt; &lt;disk type='file' device='disk'&gt; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.args index 7cda627f2..5b4e65e10 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.args @@ -28,4 +28,7 @@ id=drive-ide0-0-2,readonly=on \ -device ide-drive,bus=ide.0,unit=2,drive=drive-ide0-0-2,id=ide0-0-2 \ -drive file=/tmp/idedisk.img,format=raw,if=none,id=drive-ide0-0-3 \ -device ide-drive,bus=ide.0,unit=3,drive=drive-ide0-0-3,id=ide0-0-3 \ +-drive file=iscsi://iscsi.example.com:3260/demo-target/3,if=none,media=cdrom,\ +id=drive-ide0-0-4,readonly=on \ +-device ide-drive,bus=ide.0,unit=4,drive=drive-ide0-0-4,id=ide0-0-4 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml index eaf411c8b..3f5a2d524 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml @@ -39,6 +39,19 @@ <target dev='hdc' bus='ide'/> <address type='drive' controller='0' bus='0' target='0' unit='3'/> </disk> + <disk type='volume' device='cdrom'> + <auth username='myname'> + <secret type='iscsi' usage='mycluster_myname'/> + </auth> + <source pool='pool-iscsi' volume='unit:0:0:3' mode='direct'> + <seclabel model='selinux' relabel='yes'> + <label>system_u:system_r:public_content_t:s0</label> + </seclabel> + </source> + <target dev='hdd' bus='ide'/> + <readonly/> + <address type='drive' controller='0' bus='0' target='0' unit='4'/> + </disk> <controller type='usb' index='0'/> <controller type='ide' index='0'/> <controller type='pci' index='0' model='pci-root'/> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-source-pool-mode.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-source-pool-mode.xml index 1ca56fbb9..a14ed7b97 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-source-pool-mode.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-source-pool-mode.xml @@ -39,6 +39,19 @@ <target dev='hdc' bus='ide'/> <address type='drive' controller='0' bus='0' target='0' unit='3'/> </disk> + <disk type='volume' device='cdrom'> + <auth username='myname'> + <secret type='iscsi' usage='mycluster_myname'/> + </auth> + <source pool='pool-iscsi' volume='unit:0:0:3' mode='direct'> + <seclabel model='selinux' relabel='yes'> + <label>system_u:system_r:public_content_t:s0</label> + </seclabel> + </source> + <target dev='hdd' bus='ide'/> + <readonly/> + <address type='drive' controller='0' bus='0' target='0' unit='4'/> + </disk> <controller type='usb' index='0'> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> </controller> -- 2.13.5

Add a couple of tests to "validate" checks in domain_conf that either a missing secrettype (CONFIG_UNSUPPORTED) or an mismatched secrettype of ceph for an iSCSI disk (INTERNAL_ERROR) will cause a parsing error. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- ...drive-network-iscsi-auth-secrettype-invalid.xml | 33 ++++++++++++++++++++++ ...k-drive-network-iscsi-auth-wrong-secrettype.xml | 33 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 2 ++ 3 files changed, 68 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-secrettype-invalid.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-wrong-secrettype.xml diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-secrettype-invalid.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-secrettype-invalid.xml new file mode 100644 index 000000000..7e6b623c3 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-secrettype-invalid.xml @@ -0,0 +1,33 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-i686</emulator> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <auth username='myname'> + <secret usage='mycluster_myname'/> + </auth> + <source protocol='iscsi' name='iqn.1992-01.com.example:storage/1'> + <host name='example.org' port='6000'/> + </source> + <target dev='vda' bus='virtio'/> + </disk> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-wrong-secrettype.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-wrong-secrettype.xml new file mode 100644 index 000000000..4854abd6c --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-wrong-secrettype.xml @@ -0,0 +1,33 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-i686</emulator> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <auth username='myname'> + <secret type='ceph' usage='mycluster_myname'/> + </auth> + <source protocol='iscsi' name='iqn.1992-01.com.example:storage/1'> + <host name='example.org' port='6000'/> + </source> + <target dev='vda' bus='virtio'/> + </disk> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 2c040e4c0..fd05155ef 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -917,6 +917,8 @@ mymain(void) DO_TEST("disk-drive-network-nbd-unix", NONE); DO_TEST("disk-drive-network-iscsi", NONE); DO_TEST("disk-drive-network-iscsi-auth", NONE); + DO_TEST_PARSE_ERROR("disk-drive-network-iscsi-auth-secrettype-invalid", NONE); + DO_TEST_PARSE_ERROR("disk-drive-network-iscsi-auth-wrong-secrettype", NONE); DO_TEST("disk-drive-network-iscsi-lun", QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_SCSI_BLOCK); -- 2.13.5

Rather than checking during XML processing, move the checks for correct and valid auth into virDomainDiskDefParseValidate. This will introduce virDomainDiskSourceDefParseAuthValidate to validate that the authdef stored for the virStorageSource is valid. This can then be expanded to service backingStore sources as well. Alter the message text slightly as well to distinguish between an unknown name and an incorrectly used name. Since type is not a mandatory field, add the NULLSTR() around the output of the unknown error. NB, a config using unknown formatting would fail virschematest since it only accepts 'iscsi' and 'ceph' as "valid" types. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/conf/domain_conf.c | 67 +++++++++++++++++++++++++------------------------- 1 file changed, 34 insertions(+), 33 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a43b25c31..07bda1a36 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8500,6 +8500,39 @@ virDomainDiskDefGeometryParse(virDomainDiskDefPtr def, static int +virDomainDiskSourceDefParseAuthValidate(const virStorageSource *src) +{ + virStorageAuthDefPtr authdef = src->auth; + int actUsage; + + /* Disk volume types won't have the secrettype filled in until + * after virStorageTranslateDiskSourcePool is run + */ + if (src->type == VIR_STORAGE_TYPE_VOLUME || !authdef) + return 0; + + if ((actUsage = virSecretUsageTypeFromString(authdef->secrettype)) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown secret type '%s'"), + NULLSTR(authdef->secrettype)); + return -1; + } + + if ((src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI && + actUsage != VIR_SECRET_USAGE_TYPE_ISCSI) || + (src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD && + actUsage != VIR_SECRET_USAGE_TYPE_CEPH)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("invalid secret type '%s'"), + virSecretUsageTypeToString(actUsage)); + return -1; + } + + return 0; +} + + +static int virDomainDiskDefParseValidate(const virDomainDiskDef *def) { if (def->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) { @@ -8572,7 +8605,7 @@ virDomainDiskDefParseValidate(const virDomainDiskDef *def) } } - return 0; + return virDomainDiskSourceDefParseAuthValidate(def->src); } @@ -8731,8 +8764,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, char *vendor = NULL; char *product = NULL; char *domain_name = NULL; - int expected_secret_usage = -1; - int auth_secret_usage = -1; if (!(def = virDomainDiskDefNew(xmlopt))) return NULL; @@ -8776,13 +8807,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, source = true; - if (def->src->type == VIR_STORAGE_TYPE_NETWORK) { - if (def->src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI) - expected_secret_usage = VIR_SECRET_USAGE_TYPE_ISCSI; - else if (def->src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD) - expected_secret_usage = VIR_SECRET_USAGE_TYPE_CEPH; - } - startupPolicy = virXMLPropString(cur, "startupPolicy"); } else if (!target && @@ -8840,17 +8864,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, virXMLNodeNameEqual(cur, "auth")) { if (!(authdef = virStorageAuthDefParse(node->doc, cur))) goto error; - /* Disk volume types won't have the secrettype filled in until - * after virStorageTranslateDiskSourcePool is run - */ - if (def->src->type != VIR_STORAGE_TYPE_VOLUME && - (auth_secret_usage = - virSecretUsageTypeFromString(authdef->secrettype)) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("invalid secret type %s"), - authdef->secrettype); - goto error; - } } else if (virXMLNodeNameEqual(cur, "iotune")) { if (virDomainDiskDefIotuneParse(def, ctxt) < 0) goto error; @@ -8914,18 +8927,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, } } - /* Disk volume types will have authentication information handled in - * virStorageTranslateDiskSourcePool - */ - if (def->src->type != VIR_STORAGE_TYPE_VOLUME && - auth_secret_usage != -1 && auth_secret_usage != expected_secret_usage) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("invalid secret type '%s'"), - virSecretUsageTypeToString(auth_secret_usage)); - goto error; - } - - /* Only CDROM and Floppy devices are allowed missing source path * to indicate no media present. LUN is for raw access CD-ROMs * that are not attached to a physical device presently */ -- 2.13.5

On 09/14/2017 11:58 PM, Peter Krempa wrote: That check should work too... Not sure it'd work/look quite right as an || though if (type == VOLUME || type != NETWORK || !authdef) I'll just adjust to be if (src->type != VIR_STORAGE_TYPE_NETWORK || !authdef) and remove the comment. True - I wrote it that way first, but changed it... Only because I had a recollection of having it said I should just return directly, I'll change it though. if (!function) return -1; return 0; Tks - John

On Thu, Sep 14, 2017 at 14:03:09 -0400, John Ferlan wrote: This is the exact reason why I did not like this style in the patch that added it. Perhaps we can improve the message now. How about: "supplying <cipher> for domain disk definition is unnecessary" ? ACK

Commit id 'e02ff020cac' neglected to use the attrBuf and childBuf in the virDomainDiskSourceFormatNetwork call. So make the necessary alterations to allow usage. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/conf/domain_conf.c | 34 ++++++++++++++-------------------- 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 09c5bc1ae..a8771a3a4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -21674,13 +21674,14 @@ virDomainSourceDefFormatSeclabel(virBufferPtr buf, static int -virDomainDiskSourceFormatNetwork(virBufferPtr buf, +virDomainDiskSourceFormatNetwork(virBufferPtr attrBuf, + virBufferPtr childBuf, virStorageSourcePtr src) { size_t n; char *path = NULL; - virBufferAsprintf(buf, "<source protocol='%s'", + virBufferAsprintf(attrBuf, " protocol='%s'", virStorageNetProtocolTypeToString(src->protocol)); if (src->volume) { @@ -21688,36 +21689,29 @@ virDomainDiskSourceFormatNetwork(virBufferPtr buf, return -1; } - virBufferEscapeString(buf, " name='%s'", path ? path : src->path); + virBufferEscapeString(attrBuf, " name='%s'", path ? path : src->path); VIR_FREE(path); - if (src->nhosts == 0 && !src->snapshot && !src->configFile) { - virBufferAddLit(buf, "/>\n"); - } else { - virBufferAddLit(buf, ">\n"); - virBufferAdjustIndent(buf, 2); + if (src->nhosts > 0 || src->snapshot || src->configFile) { for (n = 0; n < src->nhosts; n++) { - virBufferAddLit(buf, "<host"); - virBufferEscapeString(buf, " name='%s'", src->hosts[n].name); + virBufferAddLit(childBuf, "<host"); + virBufferEscapeString(childBuf, " name='%s'", src->hosts[n].name); if (src->hosts[n].port) - virBufferAsprintf(buf, " port='%u'", src->hosts[n].port); + virBufferAsprintf(childBuf, " port='%u'", src->hosts[n].port); if (src->hosts[n].transport) - virBufferAsprintf(buf, " transport='%s'", + virBufferAsprintf(childBuf, " transport='%s'", virStorageNetHostTransportTypeToString(src->hosts[n].transport)); - virBufferEscapeString(buf, " socket='%s'", src->hosts[n].socket); - virBufferAddLit(buf, "/>\n"); + virBufferEscapeString(childBuf, " socket='%s'", src->hosts[n].socket); + virBufferAddLit(childBuf, "/>\n"); } - virBufferEscapeString(buf, "<snapshot name='%s'/>\n", src->snapshot); - virBufferEscapeString(buf, "<config file='%s'/>\n", src->configFile); - - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</source>\n"); + virBufferEscapeString(childBuf, "<snapshot name='%s'/>\n", src->snapshot); + virBufferEscapeString(childBuf, "<config file='%s'/>\n", src->configFile); } return 0; @@ -21766,7 +21760,7 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf, break; case VIR_STORAGE_TYPE_NETWORK: - if (virDomainDiskSourceFormatNetwork(buf, src) < 0) + if (virDomainDiskSourceFormatNetwork(&attrBuf, &childBuf, src) < 0) goto error; break; -- 2.13.5

On 09/15/2017 12:10 AM, Peter Krempa wrote: Oh yeah right... The EscapeString's won't format NULL Tks- John