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 <[email protected]> > --- > 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;
> }
signature.asc
Description: PGP signature
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
