On Fri, Sep 15, 2017 at 20:30:17 -0400, John Ferlan wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1425757 > > The blockdev-add code provides a mechanism to sanely provide user > and password-secret arguments for iscsi without placing them on the > command line to be viewable by a 'ps -ef' type command or needing > to create separate -iscsi devices for each disk/volume found. > > So modify the iSCSI command line building to check for the presence > of the capability in order properly setup and use the domain master > secret object to encrypt the password in a secret object and alter > the parameters for the command line to utilize. > > Modify the xml2argvtest to exhibit the syntax for both disk and > hostdev configurations. > > Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx> > --- > src/qemu/qemu_block.c | 64 ++++++++++++++++++- > src/qemu/qemu_command.c | 73 +++++++++++++++++++--- > src/qemu/qemu_command.h | 3 +- > src/qemu/qemu_domain.c | 4 ++ > src/qemu/qemu_hotplug.c | 49 ++++++++++++++- > ...xml2argv-disk-drive-network-iscsi-auth-AES.args | 41 ++++++++++++ > ...uxml2argv-disk-drive-network-iscsi-auth-AES.xml | 43 +++++++++++++ > ...ml2argv-hostdev-scsi-virtio-iscsi-auth-AES.args | 45 +++++++++++++ > ...xml2argv-hostdev-scsi-virtio-iscsi-auth-AES.xml | 48 ++++++++++++++ > tests/qemuxml2argvtest.c | 10 +++ > 10 files changed, 366 insertions(+), 14 deletions(-) > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.args > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.xml > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.args > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.xml > > diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c > index 7fb12ea5a..057fb8233 100644 > --- a/src/qemu/qemu_block.c > +++ b/src/qemu/qemu_block.c > @@ -482,6 +482,64 @@ qemuBlockStorageSourceGetGlusterProps(virStorageSourcePtr src) > } > > > +static virJSONValuePtr > +qemuBlockStorageSourceGetISCSIProps(virStorageSourcePtr src) > +{ > + const char *protocol = virStorageNetProtocolTypeToString(src->protocol); > + char *target = NULL; > + char *lunStr = NULL; > + char *username = NULL; > + char *objalias = NULL; > + unsigned int lun = 0; > + virJSONValuePtr ret = NULL; > + qemuDomainDiskSrcPrivatePtr diskSrcPriv = QEMU_DOMAIN_DISK_SRC_PRIVATE(src); > + > + /* { driver:"iscsi", > + * transport:"tcp" ("iser" also possible) > + * portal:"example.com", > + * target:"iqn.2017-04.com.example:iscsi-disks", > + * lun:1, > + * [user:"username", > + * password-secret:"secret-alias",] As I've pointed out in the VxHS series, using array designators in an json example to mark "optional" fields is not a great idea. > + * } > + */ > + > + if (VIR_STRDUP(target, src->path) < 0) > + goto cleanup; > + > + /* Separate the target and lun */ > + if ((lunStr = strchr(target, '/'))) { > + *(lunStr++) = '\0'; > + if (virStrToLong_ui(lunStr, NULL, 10, &lun) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("cannot parse target for lunStr '%s'"), > + target); > + goto cleanup; > + } > + } > + > + if (src->auth) { > + username = src->auth->username; > + objalias = diskSrcPriv->secinfo->s.aes.alias; > + } > + > + ignore_value(virJSONValueObjectCreate(&ret, > + "s:driver", protocol, > + "s:portal", src->hosts[0].name, > + "s:target", target, > + "u:lun", lun, > + "s:transport", "tcp", > + "S:user", username, > + "S:password-secret", objalias, > + NULL)); > + goto cleanup; > + > + cleanup: > + VIR_FREE(target); > + return ret; > +} [...] > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index c851823e7..f9edf623c 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c [...] > @@ -4846,10 +4854,13 @@ qemuBuildSCSIHostHostdevDrvStr(virDomainHostdevDefPtr dev) > } > > static char * > -qemuBuildSCSIiSCSIHostdevDrvStr(virDomainHostdevDefPtr dev) > +qemuBuildSCSIiSCSIHostdevDrvStr(virDomainHostdevDefPtr dev, > + virQEMUCapsPtr qemuCaps) > { > + char *netsource = NULL; > char *source = NULL; > - virStorageSource src; > + virStorageSource src = { 0 }; > + virJSONValuePtr srcprops = NULL; > qemuDomainHostdevPrivatePtr hostdevPriv = QEMU_DOMAIN_HOSTDEV_PRIVATE(dev); > > memset(&src, 0, sizeof(src)); > @@ -4857,14 +4868,51 @@ qemuBuildSCSIiSCSIHostdevDrvStr(virDomainHostdevDefPtr dev) > virDomainHostdevSubsysSCSIPtr scsisrc = &dev->source.subsys.u.scsi; > virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = &scsisrc->u.iscsi; > > + src.type = VIR_STORAGE_TYPE_NETWORK; > src.protocol = VIR_STORAGE_NET_PROTOCOL_ISCSI; > src.path = iscsisrc->path; > src.hosts = iscsisrc->hosts; > src.nhosts = iscsisrc->nhosts; > > /* Rather than pull what we think we want - use the network disk code */ > - source = qemuBuildNetworkDriveStr(&src, hostdevPriv->secinfo); > + if (qemuDiskSourceNeedsProps(&src, qemuCaps)) { > + /* The next pile of code hunts and gathers as if @src were a disk. > + * In particular, using private data... So a bit more chicanery is > + * going to be required */ > + qemuDomainDiskSrcPrivatePtr diskSrcPriv; > + > + if (!iscsisrc->auth->username) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("missing username for iSCSI auth")); > + goto cleanup; > + } > + src.auth = iscsisrc->auth; > + > + if (VIR_ALLOC(src.privateData) < 0) > + goto cleanup; src.privateData is a virObjectPtr. This won't work as you expect it to work. I'd say it'll crash (or corrupt heap) since ... > + diskSrcPriv = QEMU_DOMAIN_DISK_SRC_PRIVATE(&src); this typecasts the virObjectPtr into a _qemuDomainDiskSrcPrivate pointer .. > + diskSrcPriv->secinfo = hostdevPriv->secinfo; And this points 8 bytes after the allocated data, as _qemuDomainDiskSrcPrivate has a virObject folowed by the "secinfo" pointer. > + srcprops = qemuBlockStorageSourceGetBackendProps(&src); > + VIR_FREE(src.privateData); > + if (!srcprops) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("failed to build the backend props")); > + goto cleanup; > + } > > + if (!(netsource = virQEMUBuildDriveCommandlineFromJSON(srcprops))) > + goto cleanup; > + if (virAsprintf(&source, "%s,if=none,format=raw", netsource) < 0) > + goto cleanup; > + } else { > + if (!(netsource = qemuBuildNetworkDriveStr(&src, hostdevPriv->secinfo))) > + goto cleanup; > + if (virAsprintf(&source, "file=%s,if=none,format=raw", netsource) < 0) > + goto cleanup; > + } > + > + cleanup: > + VIR_FREE(netsource); > return source; > } > > @@ -4907,7 +4955,8 @@ qemuBuildSCSIVHostHostdevDevStr(const virDomainDef *def, > } > > char * > -qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev) > +qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev, > + virQEMUCapsPtr qemuCaps) > { > virBuffer buf = VIR_BUFFER_INITIALIZER; > char *source = NULL; > @@ -4915,9 +4964,9 @@ qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev) > virDomainHostdevSubsysSCSIPtr scsisrc = &dev->source.subsys.u.scsi; > > if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) { > - if (!(source = qemuBuildSCSIiSCSIHostdevDrvStr(dev))) > + if (!(source = qemuBuildSCSIiSCSIHostdevDrvStr(dev, qemuCaps))) > goto error; > - virBufferAsprintf(&buf, "file=%s,if=none,format=raw", source); > + virBufferAsprintf(&buf, "%s", source); > } else { > if (!(source = qemuBuildSCSIHostHostdevDrvStr(dev))) > goto error; > @@ -5414,10 +5463,14 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd, > /* SCSI */ > if (virHostdevIsSCSIDevice(hostdev)) { > if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_SCSI_GENERIC)) { > + qemuDomainHostdevPrivatePtr hostdevPriv = QEMU_DOMAIN_HOSTDEV_PRIVATE(hostdev); > char *drvstr; > > + if (qemuBuildDiskSecinfoCommandLine(cmd, hostdevPriv->secinfo) < 0) > + return -1; > + > virCommandAddArg(cmd, "-drive"); > - if (!(drvstr = qemuBuildSCSIHostdevDrvStr(hostdev))) > + if (!(drvstr = qemuBuildSCSIHostdevDrvStr(hostdev, qemuCaps))) > return -1; > virCommandAddArg(cmd, drvstr); > VIR_FREE(drvstr); > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c > index a544cecb9..17228d1b4 100644 > --- a/src/qemu/qemu_hotplug.c > +++ b/src/qemu/qemu_hotplug.c [...] > @@ -3879,8 +3909,22 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, > if (!(drivealias = qemuAliasFromHostdev(hostdev))) > goto cleanup; > > + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_ISCSI_PASSWORD_SECRET)) { > + if (!(objAlias = > + qemuDomainGetSecretAESAlias(hostdev->info->alias, false))) { > + return -1; > + } This will leak stuff since you skip cleanup. Also don't break that line above. > + } > + > qemuDomainObjEnterMonitor(driver, vm); > qemuMonitorDriveDel(priv->mon, drivealias); > + > + /* If it fails, then so be it - it was a best shot */ > + if (objAlias) { > + ignore_value(qemuMonitorDelObject(priv->mon, objAlias)); > + qemuDomainObjDiskSecretObjectAliasEntryRemove(priv, objAlias); > + } > + > if (qemuDomainObjExitMonitor(driver, vm) < 0) > goto cleanup; > }
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
