[PATCH 00/18] qemu: Switch (i)SCSI host devices to -blockdev

Peter Krempa (18): qemuBlockStorageSourceGetBackendProps: Convert boolean arguments to flags qemuBlockStorageSourceGetBackendProps: Allow skipping "discard":"unmap" qemuBlockStorageSourceAttachData: Add field for ad-hoc storage node name virDomainHostdevDefFormatSubsys: Format private data for a virStorageSource virDomainHostdevSubsysSCSIiSCSIDefParseXML: Parse private data of virStorageSource qemustatusxml2xmltest: Add tests for iSCSI hostdev private data handling qemuDomainSecretHostdevDestroy: Don't clear secinfo alias qemu: domain: Regenerate hostdev source private data qemu: hotplug: Don't regenerate iSCSI secret alias qemuBuildHostdevCommandLine: Extract (i)SCSI code qemuBuildSCSIHostdevDevStr: Pass in backend alias qemu: capabilities: Add QEMU_CAPS_BLOCKDEV_HOSTDEV_SCSI qemu: command: Create qemuBlockStorageSourceAttachData for (i)SCSI hostdevs qemuBuildHostdevSCSICommandLine: Use new infrastructure qemuDomainAttachHostSCSIDevice: Use new infrastructure qemuDomainRemoveHostDevice: Use new infrastructure for (i)SCSI qemu: caps: Enable QEMU_CAPS_BLOCKDEV_HOSTDEV_SCSI qemuBuildSCSIHostdevDrvStr: unexport src/conf/domain_conf.c | 71 +++++--- src/qemu/qemu_block.c | 41 +++-- src/qemu/qemu_block.h | 12 +- src/qemu/qemu_capabilities.c | 4 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 167 ++++++++++++------ src/qemu/qemu_command.h | 15 +- src/qemu/qemu_domain.c | 58 +++++- src/qemu/qemu_hotplug.c | 70 ++------ tests/qemublocktest.c | 18 +- .../caps_4.2.0.aarch64.xml | 1 + .../qemucapabilitiesdata/caps_4.2.0.s390x.xml | 1 + .../caps_4.2.0.x86_64.xml | 1 + .../caps_5.0.0.aarch64.xml | 1 + .../qemucapabilitiesdata/caps_5.0.0.ppc64.xml | 1 + .../caps_5.0.0.riscv64.xml | 1 + .../caps_5.0.0.x86_64.xml | 1 + .../caps_5.1.0.x86_64.xml | 1 + .../disk-secinfo-upgrade-in.xml | 20 +++ .../disk-secinfo-upgrade-out.xml | 30 ++++ tests/qemustatusxml2xmldata/modern-in.xml | 18 ++ .../hostdev-scsi-lsi.x86_64-latest.args | 52 +++--- ...ostdev-scsi-virtio-scsi.x86_64-latest.args | 46 ++--- 23 files changed, 426 insertions(+), 205 deletions(-) -- 2.26.2

Upcoming commit will need to add another flag for the function so convert it to a bitwise-or'd array of flags to prevent having 4 booleans. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 32 +++++++++++++++++++++----------- src/qemu/qemu_block.h | 10 +++++++--- src/qemu/qemu_command.c | 3 ++- tests/qemublocktest.c | 18 ++++++++++++------ 4 files changed, 42 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index a2eabbcd64..10ddf53b3b 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -1052,26 +1052,32 @@ qemuBlockStorageSourceGetBlockdevGetCacheProps(virStorageSourcePtr src, /** * qemuBlockStorageSourceGetBackendProps: * @src: disk source - * @legacy: use legacy formatting of attributes (for -drive / old qemus) - * @onlytarget: omit any data which does not identify the image itself - * @autoreadonly: use the auto-read-only feature of qemu + * @flags: bitwise-or of qemuBlockStorageSourceBackendPropsFlags + * + * Flags: + * QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_LEGACY: + * use legacy formatting of attributes (for -drive / old qemus) + * QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_TARGET_ONLY: + * omit any data which does not identify the image itself + * QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_AUTO_READONLY: + * use the auto-read-only feature of qemu * * Creates a JSON object describing the underlying storage or protocol of a * storage source. Returns NULL on error and reports an appropriate error message. */ virJSONValuePtr qemuBlockStorageSourceGetBackendProps(virStorageSourcePtr src, - bool legacy, - bool onlytarget, - bool autoreadonly) + unsigned int flags) { int actualType = virStorageSourceGetActualType(src); g_autoptr(virJSONValue) fileprops = NULL; const char *driver = NULL; virTristateBool aro = VIR_TRISTATE_BOOL_ABSENT; virTristateBool ro = VIR_TRISTATE_BOOL_ABSENT; + bool onlytarget = flags & QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_TARGET_ONLY; + bool legacy = flags & QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_LEGACY; - if (autoreadonly) { + if (flags & QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_AUTO_READONLY) { aro = VIR_TRISTATE_BOOL_YES; } else { if (src->readonly) @@ -1576,15 +1582,18 @@ qemuBlockStorageSourceAttachPrepareBlockdev(virStorageSourcePtr src, bool autoreadonly) { g_autoptr(qemuBlockStorageSourceAttachData) data = NULL; + unsigned int backendpropsflags = 0; + + if (autoreadonly) + backendpropsflags |= QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_AUTO_READONLY; if (VIR_ALLOC(data) < 0) return NULL; if (!(data->formatProps = qemuBlockStorageSourceGetBlockdevProps(src, backingStore)) || - !(data->storageProps = qemuBlockStorageSourceGetBackendProps(src, false, - false, - autoreadonly))) + !(data->storageProps = qemuBlockStorageSourceGetBackendProps(src, + backendpropsflags))) return NULL; data->storageNodeName = src->nodestorage; @@ -2108,7 +2117,8 @@ qemuBlockGetBackingStoreString(virStorageSourcePtr src, } /* use json: pseudo protocol otherwise */ - if (!(backingProps = qemuBlockStorageSourceGetBackendProps(src, false, true, false))) + if (!(backingProps = qemuBlockStorageSourceGetBackendProps(src, + QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_TARGET_ONLY))) return NULL; props = backingProps; diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h index b1bdb39613..715739e59c 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -56,11 +56,15 @@ qemuBlockGetNodeData(virJSONValuePtr data); bool qemuBlockStorageSourceSupportsConcurrentAccess(virStorageSourcePtr src); +typedef enum { + QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_LEGACY = 1 << 0, + QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_TARGET_ONLY = 1 << 1, + QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_AUTO_READONLY = 1 << 2, +} qemuBlockStorageSourceBackendPropsFlags; + virJSONValuePtr qemuBlockStorageSourceGetBackendProps(virStorageSourcePtr src, - bool legacy, - bool onlytarget, - bool autoreadonly); + unsigned int flags); virURIPtr qemuBlockStorageSourceGetURI(virStorageSourcePtr src); diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 24e99e13b8..c41976b903 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1193,7 +1193,8 @@ qemuDiskSourceGetProps(virStorageSourcePtr src) g_autoptr(virJSONValue) props = NULL; virJSONValuePtr ret; - if (!(props = qemuBlockStorageSourceGetBackendProps(src, true, false, false))) + if (!(props = qemuBlockStorageSourceGetBackendProps(src, + QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_LEGACY))) return NULL; if (virJSONValueObjectCreate(&ret, "a:file", &props, NULL) < 0) diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c index 51283793e9..025e96ab4b 100644 --- a/tests/qemublocktest.c +++ b/tests/qemublocktest.c @@ -62,6 +62,10 @@ testBackingXMLjsonXML(const void *args) g_autoptr(virStorageSource) xmlsrc = NULL; g_autoptr(virStorageSource) jsonsrc = NULL; g_auto(virBuffer) debug = VIR_BUFFER_INITIALIZER; + unsigned int backendpropsflags = 0; + + if (data->legacy) + backendpropsflags |= QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_LEGACY; if (!(xmlsrc = virStorageSourceNew())) return -1; @@ -77,9 +81,7 @@ testBackingXMLjsonXML(const void *args) } if (!(backendprops = qemuBlockStorageSourceGetBackendProps(xmlsrc, - data->legacy, - false, - false))) { + backendpropsflags))) { fprintf(stderr, "failed to format disk source json\n"); return -1; } @@ -159,7 +161,8 @@ testJSONtoJSON(const void *args) return -1; } - if (!(jsonsrcout = qemuBlockStorageSourceGetBackendProps(src, false, false, true))) { + if (!(jsonsrcout = qemuBlockStorageSourceGetBackendProps(src, + QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_AUTO_READONLY))) { fprintf(stderr, "failed to format disk source json\n"); return -1; } @@ -290,6 +293,9 @@ testQemuDiskXMLToProps(const void *opaque) for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) { g_autofree char *backingstore = NULL; + unsigned int backendpropsflagsnormal = QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_AUTO_READONLY; + unsigned int backendpropsflagstarget = QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_AUTO_READONLY | + QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_TARGET_ONLY; if (testQemuDiskXMLToJSONFakeSecrets(n) < 0) return -1; @@ -300,8 +306,8 @@ testQemuDiskXMLToProps(const void *opaque) qemuDomainPrepareDiskSourceData(disk, n); if (!(formatProps = qemuBlockStorageSourceGetBlockdevProps(n, n->backingStore)) || - !(storageSrcOnlyProps = qemuBlockStorageSourceGetBackendProps(n, false, true, true)) || - !(storageProps = qemuBlockStorageSourceGetBackendProps(n, false, false, true)) || + !(storageSrcOnlyProps = qemuBlockStorageSourceGetBackendProps(n, backendpropsflagstarget)) || + !(storageProps = qemuBlockStorageSourceGetBackendProps(n, backendpropsflagsnormal)) || !(backingstore = qemuBlockGetBackingStoreString(n, true))) { if (!data->fail) { VIR_TEST_VERBOSE("failed to generate qemu blockdev props"); -- 2.26.2

On a Friday in 2020, Peter Krempa wrote:
Upcoming commit will need to add another flag for the function so convert it to a bitwise-or'd array of flags to prevent having 4 booleans.
false true false true false true false false false true true false true false false false false true true false false false false true false true true false true true true false false true true false true false true true false false true false false false false false false true true true true false false true false true true false true true true true false true true true false true false true false false true false false false false true
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 32 +++++++++++++++++++++----------- src/qemu/qemu_block.h | 10 +++++++--- src/qemu/qemu_command.c | 3 ++- tests/qemublocktest.c | 18 ++++++++++++------ 4 files changed, 42 insertions(+), 21 deletions(-)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index a2eabbcd64..10ddf53b3b 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -1052,26 +1052,32 @@ qemuBlockStorageSourceGetBlockdevGetCacheProps(virStorageSourcePtr src, /** * qemuBlockStorageSourceGetBackendProps: * @src: disk source - * @legacy: use legacy formatting of attributes (for -drive / old qemus) - * @onlytarget: omit any data which does not identify the image itself - * @autoreadonly: use the auto-read-only feature of qemu + * @flags: bitwise-or of qemuBlockStorageSourceBackendPropsFlags + * + * Flags: + * QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_LEGACY:
This flag has three spaces in front of it.
+ * use legacy formatting of attributes (for -drive / old qemus) + * QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_TARGET_ONLY: + * omit any data which does not identify the image itself + * QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_AUTO_READONLY: + * use the auto-read-only feature of qemu *
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

It doesn't make sense to format "discard" when doing a -blockdev backend of scsi-generic used with SCSI hostdevs. Add a way to skip it. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 8 ++++++++ src/qemu/qemu_block.h | 1 + 2 files changed, 9 insertions(+) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 10ddf53b3b..bdd07d9925 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -1061,6 +1061,9 @@ qemuBlockStorageSourceGetBlockdevGetCacheProps(virStorageSourcePtr src, * omit any data which does not identify the image itself * QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_AUTO_READONLY: * use the auto-read-only feature of qemu + * QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_SKIP_UNMAP: + * don't enable 'discard:unmap' option for passing through dicards + * (note that this is disabled also for _LEGACY and _TARGET_ONLY options) * * Creates a JSON object describing the underlying storage or protocol of a * storage source. Returns NULL on error and reports an appropriate error message. @@ -1202,6 +1205,11 @@ qemuBlockStorageSourceGetBackendProps(virStorageSourcePtr src, if (virJSONValueObjectAdd(fileprops, "T:read-only", ro, "T:auto-read-only", aro, + NULL) < 0) + return NULL; + + if (!(flags & QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_SKIP_UNMAP) && + virJSONValueObjectAdd(fileprops, "s:discard", "unmap", NULL) < 0) return NULL; diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h index 715739e59c..0701fc18d1 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -60,6 +60,7 @@ typedef enum { QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_LEGACY = 1 << 0, QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_TARGET_ONLY = 1 << 1, QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_AUTO_READONLY = 1 << 2, + QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_SKIP_UNMAP = 1 << 3, } qemuBlockStorageSourceBackendPropsFlags; virJSONValuePtr -- 2.26.2

On a Friday in 2020, Peter Krempa wrote:
It doesn't make sense to format "discard" when doing a -blockdev backend of scsi-generic used with SCSI hostdevs. Add a way to skip it.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 8 ++++++++ src/qemu/qemu_block.h | 1 + 2 files changed, 9 insertions(+)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 10ddf53b3b..bdd07d9925 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -1061,6 +1061,9 @@ qemuBlockStorageSourceGetBlockdevGetCacheProps(virStorageSourcePtr src, * omit any data which does not identify the image itself * QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_AUTO_READONLY: * use the auto-read-only feature of qemu + * QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_SKIP_UNMAP: + * don't enable 'discard:unmap' option for passing through dicards
discards
+ * (note that this is disabled also for _LEGACY and _TARGET_ONLY options) *
So outside of tests, this is (currently) used by one out of three callers. Would it make sense to negate the option?
* Creates a JSON object describing the underlying storage or protocol of a * storage source. Returns NULL on error and reports an appropriate error message.
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

SCSI hostdevs don't have a virStorageSource associated with the backend in certain cases. Adding a separate field to hold memory for a copy of the nodename of the storage backend will allow reusing the blockdev machinery also for SCSI hostdevs. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 1 + src/qemu/qemu_block.h | 1 + 2 files changed, 2 insertions(+) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index bdd07d9925..34406aa743 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -1557,6 +1557,7 @@ qemuBlockStorageSourceAttachDataFree(qemuBlockStorageSourceAttachDataPtr data) virJSONValueFree(data->encryptsecretProps); virJSONValueFree(data->tlsProps); virJSONValueFree(data->tlsKeySecretProps); + VIR_FREE(data->storageNodeNameCopy); VIR_FREE(data->tlsAlias); VIR_FREE(data->tlsKeySecretAlias); VIR_FREE(data->authsecretAlias); diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h index 0701fc18d1..9aab620947 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -85,6 +85,7 @@ struct qemuBlockStorageSourceAttachData { virJSONValuePtr storageProps; const char *storageNodeName; + char *storageNodeNameCopy; /* in some cases we don't have the corresponding storage source */ bool storageAttached; virJSONValuePtr storageSliceProps; -- 2.26.2

On a Friday in 2020, Peter Krempa wrote:
SCSI hostdevs don't have a virStorageSource associated with the backend in certain cases. Adding a separate field to hold memory for a copy of the nodename of the storage backend will allow reusing the blockdev machinery also for SCSI hostdevs.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 1 + src/qemu/qemu_block.h | 1 + 2 files changed, 2 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

iSCSI subsystem hostdevs store the data as a virStorageSource. Format the private data part of the virStorageSource in the appropriate place. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d14485f18d..bda9375f13 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -25987,7 +25987,8 @@ static int virDomainHostdevDefFormatSubsys(virBufferPtr buf, virDomainHostdevDefPtr def, unsigned int flags, - bool includeTypeInAddr) + bool includeTypeInAddr, + virDomainXMLOptionPtr xmlopt) { bool closedSource = false; virDomainHostdevSubsysUSBPtr usbsrc = &def->source.subsys.u.usb; @@ -26086,6 +26087,10 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf, if (iscsisrc->src->hosts[0].port) virBufferAsprintf(buf, " port='%u'", iscsisrc->src->hosts[0].port); virBufferAddLit(buf, "/>\n"); + + if (virDomainDiskSourceFormatPrivateData(buf, iscsisrc->src, + flags, xmlopt) < 0) + return -1; } else { virBufferAsprintf(buf, "<adapter name='%s'/>\n", scsihostsrc->adapter); @@ -26167,13 +26172,14 @@ static int virDomainActualNetDefContentsFormat(virBufferPtr buf, virDomainNetDefPtr def, bool inSubelement, - unsigned int flags) + unsigned int flags, + virDomainXMLOptionPtr xmlopt) { virDomainNetType actualType = virDomainNetGetActualType(def); if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) { if (virDomainHostdevDefFormatSubsys(buf, virDomainNetGetActualHostdev(def), - flags, true) < 0) { + flags, true, xmlopt) < 0) { return -1; } } else { @@ -26253,7 +26259,8 @@ virDomainActualNetDefContentsFormat(virBufferPtr buf, static int virDomainActualNetDefFormat(virBufferPtr buf, virDomainNetDefPtr def, - unsigned int flags) + unsigned int flags, + virDomainXMLOptionPtr xmlopt) { virDomainNetType type; const char *typeStr; @@ -26281,7 +26288,7 @@ virDomainActualNetDefFormat(virBufferPtr buf, virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); - if (virDomainActualNetDefContentsFormat(buf, def, true, flags) < 0) + if (virDomainActualNetDefContentsFormat(buf, def, true, flags, xmlopt) < 0) return -1; virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</actual>\n"); @@ -26479,7 +26486,7 @@ virDomainNetDefFormat(virBufferPtr buf, * the standard place... (this is for public reporting of * interface status) */ - if (virDomainActualNetDefContentsFormat(buf, def, false, flags) < 0) + if (virDomainActualNetDefContentsFormat(buf, def, false, flags, xmlopt) < 0) return -1; } else { /* ...but if we've asked for the inactive XML (rather than @@ -26581,7 +26588,7 @@ virDomainNetDefFormat(virBufferPtr buf, case VIR_DOMAIN_NET_TYPE_HOSTDEV: if (virDomainHostdevDefFormatSubsys(buf, &def->data.hostdev.def, - flags, true) < 0) { + flags, true, xmlopt) < 0) { return -1; } break; @@ -26627,7 +26634,7 @@ virDomainNetDefFormat(virBufferPtr buf, */ if (def->type == VIR_DOMAIN_NET_TYPE_NETWORK && (flags & VIR_DOMAIN_DEF_FORMAT_ACTUAL_NET) && - (virDomainActualNetDefFormat(buf, def, flags) < 0)) + (virDomainActualNetDefFormat(buf, def, flags, xmlopt) < 0)) return -1; } @@ -28204,7 +28211,8 @@ virDomainGraphicsDefFormat(virBufferPtr buf, static int virDomainHostdevDefFormat(virBufferPtr buf, virDomainHostdevDefPtr def, - unsigned int flags) + unsigned int flags, + virDomainXMLOptionPtr xmlopt) { const char *mode = virDomainHostdevModeTypeToString(def->mode); virDomainHostdevSubsysSCSIPtr scsisrc = &def->source.subsys.u.scsi; @@ -28283,7 +28291,7 @@ virDomainHostdevDefFormat(virBufferPtr buf, switch (def->mode) { case VIR_DOMAIN_HOSTDEV_MODE_SUBSYS: - if (virDomainHostdevDefFormatSubsys(buf, def, flags, false) < 0) + if (virDomainHostdevDefFormatSubsys(buf, def, flags, false, xmlopt) < 0) return -1; break; case VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES: @@ -29893,7 +29901,7 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr def, * and will have already been formatted there. */ if (!def->hostdevs[n]->parentnet && - virDomainHostdevDefFormat(buf, def->hostdevs[n], flags) < 0) { + virDomainHostdevDefFormat(buf, def->hostdevs[n], flags, xmlopt) < 0) { return -1; } } @@ -31016,7 +31024,7 @@ virDomainDeviceDefCopy(virDomainDeviceDefPtr src, rc = virDomainVideoDefFormat(&buf, src->data.video, flags); break; case VIR_DOMAIN_DEVICE_HOSTDEV: - rc = virDomainHostdevDefFormat(&buf, src->data.hostdev, flags); + rc = virDomainHostdevDefFormat(&buf, src->data.hostdev, flags, xmlopt); break; case VIR_DOMAIN_DEVICE_WATCHDOG: rc = virDomainWatchdogDefFormat(&buf, src->data.watchdog, flags); -- 2.26.2

On a Friday in 2020, Peter Krempa wrote:
iSCSI subsystem hostdevs store the data as a virStorageSource. Format the private data part of the virStorageSource in the appropriate place.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

We store the config of an iSCSI hostdev in a virStorageSource structure. Parse the private data portion. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 39 +++++++++++++++++++++++++++++---------- 1 file changed, 29 insertions(+), 10 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bda9375f13..ceaf73772d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8283,7 +8283,9 @@ virDomainHostdevSubsysSCSIHostDefParseXML(xmlNodePtr sourcenode, static int virDomainHostdevSubsysSCSIiSCSIDefParseXML(xmlNodePtr sourcenode, virDomainHostdevSubsysSCSIPtr def, - xmlXPathContextPtr ctxt) + xmlXPathContextPtr ctxt, + unsigned int flags, + virDomainXMLOptionPtr xmlopt) { int auth_secret_usage = -1; xmlNodePtr cur; @@ -8348,13 +8350,27 @@ virDomainHostdevSubsysSCSIiSCSIDefParseXML(xmlNodePtr sourcenode, } cur = cur->next; } + + if ((flags & VIR_DOMAIN_DEF_PARSE_STATUS) && + xmlopt && xmlopt->privateData.storageParse) { + VIR_XPATH_NODE_AUTORESTORE(ctxt); + + ctxt->node = sourcenode; + + if ((ctxt->node = virXPathNode("./privateData", ctxt)) && + xmlopt->privateData.storageParse(ctxt, iscsisrc->src) < 0) + return -1; + } + return 0; } static int virDomainHostdevSubsysSCSIDefParseXML(xmlNodePtr sourcenode, virDomainHostdevSubsysSCSIPtr scsisrc, - xmlXPathContextPtr ctxt) + xmlXPathContextPtr ctxt, + unsigned int flags, + virDomainXMLOptionPtr xmlopt) { g_autofree char *protocol = NULL; @@ -8370,7 +8386,8 @@ virDomainHostdevSubsysSCSIDefParseXML(xmlNodePtr sourcenode, } if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) - return virDomainHostdevSubsysSCSIiSCSIDefParseXML(sourcenode, scsisrc, ctxt); + return virDomainHostdevSubsysSCSIiSCSIDefParseXML(sourcenode, scsisrc, ctxt, + flags, xmlopt); return virDomainHostdevSubsysSCSIHostDefParseXML(sourcenode, scsisrc); } @@ -8461,7 +8478,8 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, xmlXPathContextPtr ctxt, const char *type, virDomainHostdevDefPtr def, - unsigned int flags) + unsigned int flags, + virDomainXMLOptionPtr xmlopt) { xmlNodePtr sourcenode; int backend; @@ -8633,7 +8651,7 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, break; case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: - if (virDomainHostdevSubsysSCSIDefParseXML(sourcenode, scsisrc, ctxt) < 0) + if (virDomainHostdevSubsysSCSIDefParseXML(sourcenode, scsisrc, ctxt, flags, xmlopt) < 0) return -1; break; @@ -11645,7 +11663,8 @@ virDomainActualNetDefParseXML(xmlNodePtr node, xmlXPathContextPtr ctxt, virDomainNetDefPtr parent, virDomainActualNetDefPtr *def, - unsigned int flags) + unsigned int flags, + virDomainXMLOptionPtr xmlopt) { virDomainActualNetDefPtr actual = NULL; int ret = -1; @@ -11750,7 +11769,7 @@ virDomainActualNetDefParseXML(xmlNodePtr node, addrtype = g_strdup("usb"); hostdev->mode = VIR_DOMAIN_HOSTDEV_MODE_SUBSYS; if (virDomainHostdevDefParseXMLSubsys(node, ctxt, addrtype, - hostdev, flags) < 0) { + hostdev, flags, xmlopt) < 0) { goto error; } } else if (actual->type == VIR_DOMAIN_NET_TYPE_BRIDGE || @@ -12124,7 +12143,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, def->type == VIR_DOMAIN_NET_TYPE_NETWORK && virXMLNodeNameEqual(cur, "actual")) { if (virDomainActualNetDefParseXML(cur, ctxt, def, - &actual, flags) < 0) { + &actual, flags, xmlopt) < 0) { goto error; } } else if (virXMLNodeNameEqual(cur, "bandwidth")) { @@ -12388,7 +12407,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, addrtype = g_strdup("usb"); hostdev->mode = VIR_DOMAIN_HOSTDEV_MODE_SUBSYS; if (virDomainHostdevDefParseXMLSubsys(node, ctxt, addrtype, - hostdev, flags) < 0) { + hostdev, flags, xmlopt) < 0) { goto error; } break; @@ -16140,7 +16159,7 @@ virDomainHostdevDefParseXML(virDomainXMLOptionPtr xmlopt, switch (def->mode) { case VIR_DOMAIN_HOSTDEV_MODE_SUBSYS: /* parse managed/mode/type, and the <source> element */ - if (virDomainHostdevDefParseXMLSubsys(node, ctxt, type, def, flags) < 0) + if (virDomainHostdevDefParseXMLSubsys(node, ctxt, type, def, flags, xmlopt) < 0) goto error; break; case VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES: -- 2.26.2

On a Friday in 2020, Peter Krempa wrote:
We store the config of an iSCSI hostdev in a virStorageSource structure. Parse the private data portion.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 39 +++++++++++++++++++++++++++++---------- 1 file changed, 29 insertions(+), 10 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bda9375f13..ceaf73772d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8283,7 +8283,9 @@ virDomainHostdevSubsysSCSIHostDefParseXML(xmlNodePtr sourcenode, static int virDomainHostdevSubsysSCSIiSCSIDefParseXML(xmlNodePtr sourcenode, virDomainHostdevSubsysSCSIPtr def, - xmlXPathContextPtr ctxt) + xmlXPathContextPtr ctxt, + unsigned int flags, + virDomainXMLOptionPtr xmlopt) { int auth_secret_usage = -1; xmlNodePtr cur; @@ -8348,13 +8350,27 @@ virDomainHostdevSubsysSCSIiSCSIDefParseXML(xmlNodePtr sourcenode, } cur = cur->next; } + + if ((flags & VIR_DOMAIN_DEF_PARSE_STATUS) &&
Extra parentheses.
+ xmlopt && xmlopt->privateData.storageParse) { + VIR_XPATH_NODE_AUTORESTORE(ctxt); + + ctxt->node = sourcenode; + + if ((ctxt->node = virXPathNode("./privateData", ctxt)) && + xmlopt->privateData.storageParse(ctxt, iscsisrc->src) < 0) + return -1; + } + return 0; }
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemustatusxml2xmldata/modern-in.xml | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/qemustatusxml2xmldata/modern-in.xml b/tests/qemustatusxml2xmldata/modern-in.xml index 63ef9caed3..0a3990bd3e 100644 --- a/tests/qemustatusxml2xmldata/modern-in.xml +++ b/tests/qemustatusxml2xmldata/modern-in.xml @@ -458,6 +458,24 @@ <alias name='video0'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> </video> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source protocol='iscsi' name='iqn.1992-01.com.example:storage/1'> + <host name='example.org' port='3260'/> + <privateData> + <nodenames> + <nodename type='storage' name='hostdev-backend'/> + </nodenames> + <objects> + <secret type='auth' alias='custom-secret'/> + </objects> + </privateData> + <auth username='myname'> + <secret type='iscsi' usage='mycluster_myname'/> + </auth> + </source> + <alias name='hostdev0'/> + <address type='drive' controller='0' bus='0' target='2' unit='4'/> + </hostdev> <redirdev bus='usb' type='spicevmc'> <alias name='redir0'/> <address type='usb' bus='0' port='2'/> -- 2.26.2

On a Friday in 2020, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemustatusxml2xmldata/modern-in.xml | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

We need the alias to deal with hot-unplug of the hostdev. Use qemuDomainSecretInfoDestroy which clears only the secrets and not the alias. The same function is used also for handling disk secrets. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 248e259634..3141a94136 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1216,8 +1216,8 @@ qemuDomainSecretHostdevDestroy(virDomainHostdevDefPtr hostdev) if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) { srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(iscsisrc->src); - if (srcPriv && srcPriv->secinfo) - g_clear_pointer(&srcPriv->secinfo, qemuDomainSecretInfoFree); + if (srcPriv) + qemuDomainSecretInfoDestroy(srcPriv->secinfo); } } } -- 2.26.2

On a Friday in 2020, Peter Krempa wrote:
We need the alias to deal with hot-unplug of the hostdev. Use qemuDomainSecretInfoDestroy which clears only the secrets and not the alias. The same function is used also for handling disk secrets.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

When upgrading from a libvirt which didn't format private data of a virStorageSource representing an iSCSI hostdev source, we might need to generate some internal data so that the code still works as if it was present in the status XML. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 54 ++++++++++++++++++- .../disk-secinfo-upgrade-in.xml | 20 +++++++ .../disk-secinfo-upgrade-out.xml | 30 +++++++++++ 3 files changed, 102 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 3141a94136..41e4d5dd0b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5319,6 +5319,51 @@ qemuDomainVsockDefPostParse(virDomainVsockDefPtr vsock) } +/** + * qemuDomainDeviceHostdevDefPostParseRestoreSecAlias: + * + * Re-generate aliases for objects related to the storage source if they + * were not stored in the status XML by an older libvirt. + * + * Note that qemuCaps should be always present for a status XML. + */ +static int +qemuDomainDeviceHostdevDefPostParseRestoreSecAlias(virDomainHostdevDefPtr hostdev, + virQEMUCapsPtr qemuCaps, + unsigned int parseFlags) +{ + qemuDomainStorageSourcePrivatePtr priv; + virDomainHostdevSubsysSCSIPtr scsisrc = &hostdev->source.subsys.u.scsi; + virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = &scsisrc->u.iscsi; + g_autofree char *authalias = NULL; + + if (!(parseFlags & VIR_DOMAIN_DEF_PARSE_STATUS) || + !qemuCaps || + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_SECRET)) + return 0; + + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS || + hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI || + scsisrc->protocol != VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI || + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_ISCSI_PASSWORD_SECRET) || + !qemuDomainStorageSourceHasAuth(iscsisrc->src)) + return 0; + + if (!(priv = qemuDomainStorageSourcePrivateFetch(iscsisrc->src))) + return -1; + + if (priv->secinfo) + return 0; + + authalias = g_strdup_printf("%s-secret0", hostdev->info->alias); + + if (qemuStorageSourcePrivateDataAssignSecinfo(&priv->secinfo, &authalias) < 0) + return -1; + + return 0; +} + + static int qemuDomainHostdevDefMdevPostParse(virDomainHostdevSubsysMediatedDevPtr mdevsrc, virQEMUCapsPtr qemuCaps) @@ -5336,10 +5381,15 @@ qemuDomainHostdevDefMdevPostParse(virDomainHostdevSubsysMediatedDevPtr mdevsrc, static int qemuDomainHostdevDefPostParse(virDomainHostdevDefPtr hostdev, - virQEMUCapsPtr qemuCaps) + virQEMUCapsPtr qemuCaps, + unsigned int parseFlags) { virDomainHostdevSubsysPtr subsys = &hostdev->source.subsys; + if (qemuDomainDeviceHostdevDefPostParseRestoreSecAlias(hostdev, qemuCaps, + parseFlags) < 0) + return -1; + if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV && qemuDomainHostdevDefMdevPostParse(&subsys->u.mdev, qemuCaps) < 0) @@ -5414,7 +5464,7 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, break; case VIR_DOMAIN_DEVICE_HOSTDEV: - ret = qemuDomainHostdevDefPostParse(dev->data.hostdev, qemuCaps); + ret = qemuDomainHostdevDefPostParse(dev->data.hostdev, qemuCaps, parseFlags); break; case VIR_DOMAIN_DEVICE_TPM: diff --git a/tests/qemustatusxml2xmldata/disk-secinfo-upgrade-in.xml b/tests/qemustatusxml2xmldata/disk-secinfo-upgrade-in.xml index ce55a70637..8023857ffe 100644 --- a/tests/qemustatusxml2xmldata/disk-secinfo-upgrade-in.xml +++ b/tests/qemustatusxml2xmldata/disk-secinfo-upgrade-in.xml @@ -491,6 +491,26 @@ <alias name='video0'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> </video> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source protocol='iscsi' name='iqn.1992-01.com.example:storage/1'> + <host name='example.org' port='3260'/> + <auth username='myname'> + <secret type='iscsi' usage='mycluster_myname'/> + </auth> + </source> + <alias name='hostdev0'/> + <address type='drive' controller='0' bus='0' target='2' unit='4'/> + </hostdev> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source protocol='iscsi' name='iqn.1992-01.com.example:storage/2'> + <host name='example.org' port='3260'/> + <auth username='myname'> + <secret type='iscsi' usage='mycluster_myname'/> + </auth> + </source> + <alias name='hostdev1'/> + <address type='drive' controller='0' bus='0' target='2' unit='5'/> + </hostdev> <redirdev bus='usb' type='spicevmc'> <alias name='redir0'/> <address type='usb' bus='0' port='2'/> diff --git a/tests/qemustatusxml2xmldata/disk-secinfo-upgrade-out.xml b/tests/qemustatusxml2xmldata/disk-secinfo-upgrade-out.xml index 5d3287606f..d5534fb0f5 100644 --- a/tests/qemustatusxml2xmldata/disk-secinfo-upgrade-out.xml +++ b/tests/qemustatusxml2xmldata/disk-secinfo-upgrade-out.xml @@ -513,6 +513,36 @@ <alias name='video0'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> </video> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source protocol='iscsi' name='iqn.1992-01.com.example:storage/1'> + <host name='example.org' port='3260'/> + <privateData> + <objects> + <secret type='auth' alias='hostdev0-secret0'/> + </objects> + </privateData> + <auth username='myname'> + <secret type='iscsi' usage='mycluster_myname'/> + </auth> + </source> + <alias name='hostdev0'/> + <address type='drive' controller='0' bus='0' target='2' unit='4'/> + </hostdev> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source protocol='iscsi' name='iqn.1992-01.com.example:storage/2'> + <host name='example.org' port='3260'/> + <privateData> + <objects> + <secret type='auth' alias='hostdev1-secret0'/> + </objects> + </privateData> + <auth username='myname'> + <secret type='iscsi' usage='mycluster_myname'/> + </auth> + </source> + <alias name='hostdev1'/> + <address type='drive' controller='0' bus='0' target='2' unit='5'/> + </hostdev> <redirdev bus='usb' type='spicevmc'> <alias name='redir0'/> <address type='usb' bus='0' port='2'/> -- 2.26.2

On a Friday in 2020, Peter Krempa wrote:
When upgrading from a libvirt which didn't format private data of a virStorageSource representing an iSCSI hostdev source, we might need to generate some internal data so that the code still works as if it was present in the status XML.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 54 ++++++++++++++++++- .../disk-secinfo-upgrade-in.xml | 20 +++++++ .../disk-secinfo-upgrade-out.xml | 30 +++++++++++ 3 files changed, 102 insertions(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

We now store the alias of the secrets in the status XML so there's no need to generate it again. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index e897c059dd..cbd4c88abe 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -4473,7 +4473,7 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, size_t i; qemuDomainObjPrivatePtr priv = vm->privateData; g_autofree char *drivealias = NULL; - g_autofree char *objAlias = NULL; + const char *secretObjAlias = NULL; VIR_DEBUG("Removing host device %s from domain %p %s", hostdev->info->alias, vm, vm->def->name); @@ -4485,22 +4485,17 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, if (!(drivealias = qemuAliasFromHostdev(hostdev))) return -1; - /* Look for the markers that the iSCSI hostdev was added with a - * secret object to manage the username/password. If present, let's - * attempt to remove the object as well. */ - if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI && - virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_ISCSI_PASSWORD_SECRET) && - qemuDomainStorageSourceHasAuth(iscsisrc->src)) { - if (!(objAlias = qemuAliasForSecret(hostdev->info->alias, NULL))) - return -1; + if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) { + qemuDomainStorageSourcePrivatePtr srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(iscsisrc->src); + if (srcPriv && srcPriv->secinfo) + secretObjAlias = srcPriv->secinfo->s.aes.alias; } 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, false)); + if (secretObjAlias) + ignore_value(qemuMonitorDelObject(priv->mon, secretObjAlias, false)); if (qemuDomainObjExitMonitor(driver, vm) < 0) return -1; -- 2.26.2

On a Friday in 2020, Peter Krempa wrote:
We now store the alias of the secrets in the status XML so there's no need to generate it again.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

Move all (i)SCSI related code into a new function named 'qemuBuildHostdevSCSICommandLine'. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 61 +++++++++++++++++++++++++---------------- 1 file changed, 38 insertions(+), 23 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c41976b903..3d9479f863 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5063,6 +5063,43 @@ qemuBuildHostdevMediatedDevStr(const virDomainDef *def, return virBufferContentAndReset(&buf); } + +static int +qemuBuildHostdevSCSICommandLine(virCommandPtr cmd, + const virDomainDef *def, + virDomainHostdevDefPtr hostdev, + virQEMUCapsPtr qemuCaps) +{ + virDomainHostdevSubsysSCSIPtr scsisrc = &hostdev->source.subsys.u.scsi; + g_autofree char *devstr = NULL; + g_autofree char *drvstr = NULL; + + if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) { + virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = + &scsisrc->u.iscsi; + qemuDomainStorageSourcePrivatePtr srcPriv = + QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(iscsisrc->src); + + if (qemuBuildDiskSecinfoCommandLine(cmd, srcPriv ? + srcPriv->secinfo : + NULL) < 0) + return -1; + } + + virCommandAddArg(cmd, "-drive"); + if (!(drvstr = qemuBuildSCSIHostdevDrvStr(hostdev, qemuCaps))) + return -1; + virCommandAddArg(cmd, drvstr); + + virCommandAddArg(cmd, "-device"); + if (!(devstr = qemuBuildSCSIHostdevDevStr(def, hostdev))) + return -1; + virCommandAddArg(cmd, devstr); + + return 0; +} + + static int qemuBuildHostdevCommandLine(virCommandPtr cmd, const virDomainDef *def, @@ -5074,10 +5111,8 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd, for (i = 0; i < def->nhostdevs; i++) { virDomainHostdevDefPtr hostdev = def->hostdevs[i]; virDomainHostdevSubsysPtr subsys = &hostdev->source.subsys; - virDomainHostdevSubsysSCSIPtr scsisrc = &hostdev->source.subsys.u.scsi; virDomainHostdevSubsysMediatedDevPtr mdevsrc = &subsys->u.mdev; g_autofree char *devstr = NULL; - g_autofree char *drvstr = NULL; g_autofree char *vhostfdName = NULL; unsigned int bootIndex = hostdev->info->bootIndex; int vhostfd = -1; @@ -5123,28 +5158,8 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd, /* SCSI */ case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: - if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) { - virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = - &scsisrc->u.iscsi; - qemuDomainStorageSourcePrivatePtr srcPriv = - QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(iscsisrc->src); - - if (qemuBuildDiskSecinfoCommandLine(cmd, srcPriv ? - srcPriv->secinfo : - NULL) < 0) - return -1; - } - - virCommandAddArg(cmd, "-drive"); - if (!(drvstr = qemuBuildSCSIHostdevDrvStr(hostdev, qemuCaps))) + if (qemuBuildHostdevSCSICommandLine(cmd, def, hostdev, qemuCaps) < 0) return -1; - virCommandAddArg(cmd, drvstr); - - virCommandAddArg(cmd, "-device"); - if (!(devstr = qemuBuildSCSIHostdevDevStr(def, hostdev))) - return -1; - virCommandAddArg(cmd, devstr); - break; /* SCSI_host */ -- 2.26.2

On a Friday in 2020, Peter Krempa wrote:
Move all (i)SCSI related code into a new function named 'qemuBuildHostdevSCSICommandLine'.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 61 +++++++++++++++++++++++++---------------- 1 file changed, 38 insertions(+), 23 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

Don't (re)generate the backend alias (alias of the -drive backend for now) internally but rather pass it in. Later on it will be replaced by the nodename when blockdev is used depending on the capabilities. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 14 ++++++++------ src/qemu/qemu_command.h | 4 +++- src/qemu/qemu_hotplug.c | 2 +- 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 3d9479f863..200decd6e0 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4641,11 +4641,11 @@ qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev, char * qemuBuildSCSIHostdevDevStr(const virDomainDef *def, - virDomainHostdevDefPtr dev) + virDomainHostdevDefPtr dev, + const char *backendAlias) { g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; int model = -1; - g_autofree char *driveAlias = NULL; const char *contAlias; model = qemuDomainFindSCSIControllerModel(def, dev->info); @@ -4687,9 +4687,7 @@ qemuBuildSCSIHostdevDevStr(const virDomainDef *def, dev->info->addr.drive.unit); } - if (!(driveAlias = qemuAliasFromHostdev(dev))) - return NULL; - virBufferAsprintf(&buf, ",drive=%s,id=%s", driveAlias, dev->info->alias); + virBufferAsprintf(&buf, ",drive=%s,id=%s", backendAlias, dev->info->alias); if (dev->info->bootIndex) virBufferAsprintf(&buf, ",bootindex=%u", dev->info->bootIndex); @@ -5073,6 +5071,7 @@ qemuBuildHostdevSCSICommandLine(virCommandPtr cmd, virDomainHostdevSubsysSCSIPtr scsisrc = &hostdev->source.subsys.u.scsi; g_autofree char *devstr = NULL; g_autofree char *drvstr = NULL; + g_autofree char *backendAlias = NULL; if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) { virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = @@ -5091,8 +5090,11 @@ qemuBuildHostdevSCSICommandLine(virCommandPtr cmd, return -1; virCommandAddArg(cmd, drvstr); + if (!(backendAlias = qemuAliasFromHostdev(hostdev))) + return -1; + virCommandAddArg(cmd, "-device"); - if (!(devstr = qemuBuildSCSIHostdevDevStr(def, hostdev))) + if (!(devstr = qemuBuildSCSIHostdevDevStr(def, hostdev, backendAlias))) return -1; virCommandAddArg(cmd, devstr); diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 55bc67072a..d0de27d9b0 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -179,7 +179,9 @@ char *qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev, virQEMUCapsPtr qemuCaps); char *qemuBuildSCSIHostdevDevStr(const virDomainDef *def, - virDomainHostdevDefPtr dev); + virDomainHostdevDefPtr dev, + const char *backendAlias); + char * qemuBuildSCSIVHostHostdevDevStr(const virDomainDef *def, virDomainHostdevDefPtr dev, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index cbd4c88abe..fbbd6a533c 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2629,7 +2629,7 @@ qemuDomainAttachHostSCSIDevice(virQEMUDriverPtr driver, if (!(drivealias = qemuAliasFromHostdev(hostdev))) goto cleanup; - if (!(devstr = qemuBuildSCSIHostdevDevStr(vm->def, hostdev))) + if (!(devstr = qemuBuildSCSIHostdevDevStr(vm->def, hostdev, drivealias))) goto cleanup; if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) < 0) -- 2.26.2

On a Friday in 2020, Peter Krempa wrote:
Don't (re)generate the backend alias (alias of the -drive backend for now) internally but rather pass it in. Later on it will be replaced by the nodename when blockdev is used depending on the capabilities.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 14 ++++++++------ src/qemu/qemu_command.h | 4 +++- src/qemu/qemu_hotplug.c | 2 +- 3 files changed, 12 insertions(+), 8 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

We want to instantiate hostdevs via -blockdev too. Add a separate capability for them for a clean transition. The new capability will be enabled when QEMU_CAPS_BLOCKDEV is present once all code is prepared. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_capabilities.c | 1 + src/qemu/qemu_capabilities.h | 1 + 2 files changed, 2 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index b27a5e5c81..cc54ad9283 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -596,6 +596,7 @@ VIR_ENUM_IMPL(virQEMUCaps, "intel-iommu.aw-bits", "spapr-tpm-proxy", "numa.hmat", + "blockdev-hostdev-scsi" ); diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index dac36b33d9..5d08941538 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -576,6 +576,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ QEMU_CAPS_INTEL_IOMMU_AW_BITS, /* intel-iommu.aw-bits */ QEMU_CAPS_DEVICE_SPAPR_TPM_PROXY, /* -device spapr-tpm-proxy */ QEMU_CAPS_NUMA_HMAT, /* -numa hmat */ + QEMU_CAPS_BLOCKDEV_HOSTDEV_SCSI, /* -blockdev used for (i)SCSI hostdevs */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; -- 2.26.2

On a Friday in 2020, Peter Krempa wrote:
We want to instantiate hostdevs via -blockdev too. Add a separate capability for them for a clean transition. The new capability will be enabled when QEMU_CAPS_BLOCKDEV is present once all code is prepared.
What is the benefit here compared to using QEMU_CAPS_BLOCKDEV directly? Seems more like a libvirt capability than a QEMU capability. Jano
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_capabilities.c | 1 + src/qemu/qemu_capabilities.h | 1 + 2 files changed, 2 insertions(+)

On Thu, Jul 16, 2020 at 00:16:23 +0200, Ján Tomko wrote:
On a Friday in 2020, Peter Krempa wrote:
We want to instantiate hostdevs via -blockdev too. Add a separate capability for them for a clean transition. The new capability will be enabled when QEMU_CAPS_BLOCKDEV is present once all code is prepared.
What is the benefit here compared to using QEMU_CAPS_BLOCKDEV directly?
It encodes whether the particular VM instance was already started with -blockdev used for hostdevs. The benefit ... or rather necessity is that we need to handle cases when the VM was started with -drive differently on hot-unplug of the hostdev. I'd have to encode that bit of information somehow regardless of which approach is used to actually store it.
Seems more like a libvirt capability than a QEMU capability.
It is a capability of that particular instance of the VM the same way as others. A VM started with pre-blockdev libvirt but modern qemu will not use -blockdev for hotplug after libvirt upgrade and this is exactly te same situation.
Jano
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_capabilities.c | 1 + src/qemu/qemu_capabilities.h | 1 + 2 files changed, 2 insertions(+)

On a Thursday in 2020, Peter Krempa wrote:
On Thu, Jul 16, 2020 at 00:16:23 +0200, Ján Tomko wrote:
On a Friday in 2020, Peter Krempa wrote:
We want to instantiate hostdevs via -blockdev too. Add a separate capability for them for a clean transition. The new capability will be enabled when QEMU_CAPS_BLOCKDEV is present once all code is prepared.
What is the benefit here compared to using QEMU_CAPS_BLOCKDEV directly?
It encodes whether the particular VM instance was already started with -blockdev used for hostdevs. The benefit ... or rather necessity is that we need to handle cases when the VM was started with -drive differently on hot-unplug of the hostdev.
I'd have to encode that bit of information somehow regardless of which approach is used to actually store it.
Seems more like a libvirt capability than a QEMU capability.
It is a capability of that particular instance of the VM the same way as others. A VM started with pre-blockdev libvirt but modern qemu will not use -blockdev for hotplug after libvirt upgrade and this is exactly te same situation.
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

Add convertor for creating qemuBlockStorageSourceAttachData which will allow reusing the infrastructure which we have for attaching disks also for hostdevs. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 83 +++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_command.h | 8 ++++ 2 files changed, 91 insertions(+) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 200decd6e0..6b3b1b39ca 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5062,6 +5062,89 @@ qemuBuildHostdevMediatedDevStr(const virDomainDef *def, } +qemuBlockStorageSourceAttachData * +qemuBuildHostdevSCSIDetachPrepare(virDomainHostdevDefPtr hostdev, + virQEMUCapsPtr qemuCaps) +{ + virDomainHostdevSubsysSCSIPtr scsisrc = &hostdev->source.subsys.u.scsi; + virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = &scsisrc->u.iscsi; + g_autoptr(qemuBlockStorageSourceAttachData) ret = g_new0(qemuBlockStorageSourceAttachData, 1); + + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_BLOCKDEV_HOSTDEV_SCSI)) { + if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) { + qemuDomainStorageSourcePrivatePtr srcpriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(iscsisrc->src); + + ret->storageNodeName = iscsisrc->src->nodestorage; + + if (srcpriv && srcpriv->secinfo && + srcpriv->secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) + ret->authsecretAlias = g_strdup(srcpriv->secinfo->s.aes.alias); + } else { + ret->storageNodeNameCopy = g_strdup_printf("libvirt-%s-backend", hostdev->info->alias); + ret->storageNodeName = ret->storageNodeNameCopy; + } + + ret->storageAttached = true; + } else { + ret->driveAlias = qemuAliasFromHostdev(hostdev); + ret->driveAdded = true; + } + + return g_steal_pointer(&ret); +} + + +qemuBlockStorageSourceAttachData * +qemuBuildHostdevSCSIAttachPrepare(virDomainHostdevDefPtr hostdev, + const char **backendAlias, + virQEMUCapsPtr qemuCaps) +{ + virDomainHostdevSubsysSCSIPtr scsisrc = &hostdev->source.subsys.u.scsi; + virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = &scsisrc->u.iscsi; + virStorageSourcePtr src; + g_autoptr(virStorageSource) localsrc = NULL; + g_autoptr(qemuBlockStorageSourceAttachData) ret = g_new0(qemuBlockStorageSourceAttachData, 1); + + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_BLOCKDEV_HOSTDEV_SCSI)) { + if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) { + src = iscsisrc->src; + } else { + g_autofree char *devstr = qemuBuildSCSIHostHostdevDrvStr(hostdev); + + if (!devstr) + return NULL; + + if (!(src = localsrc = virStorageSourceNew())) + return NULL; + + src->type = VIR_STORAGE_TYPE_BLOCK; + src->readonly = hostdev->readonly; + src->path = g_strdup_printf("/dev/%s", devstr); + } + + src->nodestorage = g_strdup_printf("libvirt-%s-backend", hostdev->info->alias); + ret->storageNodeNameCopy = g_strdup(src->nodestorage); + ret->storageNodeName = ret->storageNodeNameCopy; + *backendAlias = ret->storageNodeNameCopy; + + if (!(ret->storageProps = qemuBlockStorageSourceGetBackendProps(src, + QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_SKIP_UNMAP))) + return NULL; + + } else { + ret->driveCmd = qemuBuildSCSIHostdevDrvStr(hostdev, qemuCaps); + ret->driveAlias = qemuAliasFromHostdev(hostdev); + *backendAlias = ret->driveAlias; + } + + if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI && + qemuBuildStorageSourceAttachPrepareCommon(iscsisrc->src, ret, qemuCaps) < 0) + return NULL; + + return g_steal_pointer(&ret); +} + + static int qemuBuildHostdevSCSICommandLine(virCommandPtr cmd, const virDomainDef *def, diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index d0de27d9b0..a43cdb2733 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -182,6 +182,14 @@ char *qemuBuildSCSIHostdevDevStr(const virDomainDef *def, virDomainHostdevDefPtr dev, const char *backendAlias); +qemuBlockStorageSourceAttachData * +qemuBuildHostdevSCSIAttachPrepare(virDomainHostdevDefPtr hostdev, + const char **backendAlias, + virQEMUCapsPtr qemuCaps); +qemuBlockStorageSourceAttachData * +qemuBuildHostdevSCSIDetachPrepare(virDomainHostdevDefPtr hostdev, + virQEMUCapsPtr qemuCaps); + char * qemuBuildSCSIVHostHostdevDevStr(const virDomainDef *def, virDomainHostdevDefPtr dev, -- 2.26.2

On a Friday in 2020, Peter Krempa wrote:
Add convertor for creating qemuBlockStorageSourceAttachData which will allow reusing the infrastructure which we have for attaching disks also for hostdevs.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 83 +++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_command.h | 8 ++++ 2 files changed, 91 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

In preparation for instantiating (i)SCSI hostdevs via -blockdev, refactor qemuBuildHostdevSCSICommandLine to use the new infrastructure which will do it automatically. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 44 ++++------------------------------------- 1 file changed, 4 insertions(+), 40 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6b3b1b39ca..7f624a031e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -695,27 +695,6 @@ qemuBuildObjectSecretCommandLine(virCommandPtr cmd, } -/* qemuBuildDiskSecinfoCommandLine: - * @cmd: Pointer to the command string - * @secinfo: Pointer to a possible secinfo - * - * Add the secret object for the disks that will be using it to perform - * their authentication. - * - * Returns 0 on success, -1 w/ error on some sort of failure. - */ -static int -qemuBuildDiskSecinfoCommandLine(virCommandPtr cmd, - qemuDomainSecretInfoPtr secinfo) -{ - /* Not necessary for non AES secrets */ - if (!secinfo || secinfo->type != VIR_DOMAIN_SECRET_INFO_TYPE_AES) - return 0; - - return qemuBuildObjectSecretCommandLine(cmd, secinfo); -} - - /* qemuBuildGeneralSecinfoURI: * @uri: Pointer to the URI structure to add to * @secinfo: Pointer to the secret info data (if present) @@ -5151,29 +5130,14 @@ qemuBuildHostdevSCSICommandLine(virCommandPtr cmd, virDomainHostdevDefPtr hostdev, virQEMUCapsPtr qemuCaps) { - virDomainHostdevSubsysSCSIPtr scsisrc = &hostdev->source.subsys.u.scsi; + g_autoptr(qemuBlockStorageSourceAttachData) data = NULL; g_autofree char *devstr = NULL; - g_autofree char *drvstr = NULL; - g_autofree char *backendAlias = NULL; + const char *backendAlias = NULL; - if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) { - virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = - &scsisrc->u.iscsi; - qemuDomainStorageSourcePrivatePtr srcPriv = - QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(iscsisrc->src); - - if (qemuBuildDiskSecinfoCommandLine(cmd, srcPriv ? - srcPriv->secinfo : - NULL) < 0) - return -1; - } - - virCommandAddArg(cmd, "-drive"); - if (!(drvstr = qemuBuildSCSIHostdevDrvStr(hostdev, qemuCaps))) + if (!(data = qemuBuildHostdevSCSIAttachPrepare(hostdev, &backendAlias, qemuCaps))) return -1; - virCommandAddArg(cmd, drvstr); - if (!(backendAlias = qemuAliasFromHostdev(hostdev))) + if (qemuBuildBlockStorageSourceAttachDataCommandline(cmd, data) < 0) return -1; virCommandAddArg(cmd, "-device"); -- 2.26.2

On a Friday in 2020, Peter Krempa wrote:
In preparation for instantiating (i)SCSI hostdevs via -blockdev, refactor qemuBuildHostdevSCSICommandLine to use the new infrastructure which will do it automatically.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 44 ++++------------------------------------- 1 file changed, 4 insertions(+), 40 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

Similarly to command line creation, use the blockdev helpers when hotplugging an (i)SCSI hostdev. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 45 +++++++---------------------------------- 1 file changed, 7 insertions(+), 38 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index fbbd6a533c..06b3b94ff2 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2566,17 +2566,12 @@ qemuDomainAttachHostSCSIDevice(virQEMUDriverPtr driver, int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; virErrorPtr orig_err; + g_autoptr(qemuBlockStorageSourceAttachData) data = NULL; + const char *backendalias = NULL; g_autofree char *devstr = NULL; - g_autofree char *drvstr = NULL; - g_autofree char *drivealias = NULL; - g_autofree char *secobjAlias = NULL; bool teardowncgroup = false; bool teardownlabel = false; bool teardowndevice = false; - bool driveAdded = false; - virJSONValuePtr secobjProps = NULL; - virDomainHostdevSubsysSCSIPtr scsisrc = &hostdev->source.subsys.u.scsi; - qemuDomainSecretInfoPtr secinfo = NULL; /* Let's make sure the disk has a controller defined and loaded before * trying to add it. The controller used by the disk must exist before a @@ -2611,25 +2606,11 @@ qemuDomainAttachHostSCSIDevice(virQEMUDriverPtr driver, if (qemuDomainSecretHostdevPrepare(priv, hostdev) < 0) goto cleanup; - if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) { - qemuDomainStorageSourcePrivatePtr srcPriv = - QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(scsisrc->u.iscsi.src); - if (srcPriv) - secinfo = srcPriv->secinfo; - } - - if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) { - if (qemuBuildSecretInfoProps(secinfo, &secobjProps) < 0) - goto cleanup; - } - - if (!(drvstr = qemuBuildSCSIHostdevDrvStr(hostdev, priv->qemuCaps))) - goto cleanup; - - if (!(drivealias = qemuAliasFromHostdev(hostdev))) + if (!(data = qemuBuildHostdevSCSIAttachPrepare(hostdev, &backendalias, + priv->qemuCaps))) goto cleanup; - if (!(devstr = qemuBuildSCSIHostdevDevStr(vm->def, hostdev, drivealias))) + if (!(devstr = qemuBuildSCSIHostdevDevStr(vm->def, hostdev, backendalias))) goto cleanup; if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) < 0) @@ -2637,14 +2618,9 @@ qemuDomainAttachHostSCSIDevice(virQEMUDriverPtr driver, qemuDomainObjEnterMonitor(driver, vm); - if (secobjProps && - qemuMonitorAddObject(priv->mon, &secobjProps, &secobjAlias) < 0) + if (qemuBlockStorageSourceAttachApply(priv->mon, data) < 0) goto exit_monitor; - if (qemuMonitorAddDrive(priv->mon, drvstr) < 0) - goto exit_monitor; - driveAdded = true; - if (qemuMonitorAddDevice(priv->mon, devstr) < 0) goto exit_monitor; @@ -2670,18 +2646,11 @@ qemuDomainAttachHostSCSIDevice(virQEMUDriverPtr driver, VIR_WARN("Unable to remove host device from /dev"); } qemuDomainSecretHostdevDestroy(hostdev); - virJSONValueFree(secobjProps); return ret; exit_monitor: virErrorPreserveLast(&orig_err); - if (driveAdded && qemuMonitorDriveDel(priv->mon, drivealias) < 0) { - VIR_WARN("Unable to remove drive %s (%s) after failed " - "qemuMonitorAddDevice", - drvstr, devstr); - } - if (secobjAlias) - ignore_value(qemuMonitorDelObject(priv->mon, secobjAlias, false)); + qemuBlockStorageSourceAttachRollback(priv->mon, data); ignore_value(qemuDomainObjExitMonitor(driver, vm)); virErrorRestore(&orig_err); -- 2.26.2

On a Friday in 2020, Peter Krempa wrote:
Similarly to command line creation, use the blockdev helpers when hotplugging an (i)SCSI hostdev.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 45 +++++++---------------------------------- 1 file changed, 7 insertions(+), 38 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

Similarly to previous commits, modify the hostdev detach code to use blockdev infrastructure to detach (i)SCSI hostdevs. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 20 +++----------------- 1 file changed, 3 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 06b3b94ff2..607f9fdc0f 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -4441,31 +4441,17 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, virDomainNetDefPtr net = NULL; size_t i; qemuDomainObjPrivatePtr priv = vm->privateData; - g_autofree char *drivealias = NULL; - const char *secretObjAlias = NULL; VIR_DEBUG("Removing host device %s from domain %p %s", hostdev->info->alias, vm, vm->def->name); if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) { - virDomainHostdevSubsysSCSIPtr scsisrc = &hostdev->source.subsys.u.scsi; - virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = &scsisrc->u.iscsi; + g_autoptr(qemuBlockStorageSourceAttachData) detachscsi = NULL; - if (!(drivealias = qemuAliasFromHostdev(hostdev))) - return -1; - - if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) { - qemuDomainStorageSourcePrivatePtr srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(iscsisrc->src); - if (srcPriv && srcPriv->secinfo) - secretObjAlias = srcPriv->secinfo->s.aes.alias; - } + detachscsi = qemuBuildHostdevSCSIDetachPrepare(hostdev, priv->qemuCaps); qemuDomainObjEnterMonitor(driver, vm); - qemuMonitorDriveDel(priv->mon, drivealias); - - if (secretObjAlias) - ignore_value(qemuMonitorDelObject(priv->mon, secretObjAlias, false)); - + qemuBlockStorageSourceAttachRollback(priv->mon, detachscsi); if (qemuDomainObjExitMonitor(driver, vm) < 0) return -1; } -- 2.26.2

On a Friday in 2020, Peter Krempa wrote:
Similarly to previous commits, modify the hostdev detach code to use blockdev infrastructure to detach (i)SCSI hostdevs.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 20 +++----------------- 1 file changed, 3 insertions(+), 17 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

Enable it when regular QEMU_CAPS_BLOCKDEV is present. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_capabilities.c | 3 ++ .../caps_4.2.0.aarch64.xml | 1 + .../qemucapabilitiesdata/caps_4.2.0.s390x.xml | 1 + .../caps_4.2.0.x86_64.xml | 1 + .../caps_5.0.0.aarch64.xml | 1 + .../qemucapabilitiesdata/caps_5.0.0.ppc64.xml | 1 + .../caps_5.0.0.riscv64.xml | 1 + .../caps_5.0.0.x86_64.xml | 1 + .../caps_5.1.0.x86_64.xml | 1 + .../hostdev-scsi-lsi.x86_64-latest.args | 52 +++++++++++-------- ...ostdev-scsi-virtio-scsi.x86_64-latest.args | 46 ++++++++-------- 11 files changed, 65 insertions(+), 44 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index cc54ad9283..d6bfec9996 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -5124,6 +5124,9 @@ virQEMUCapsInitProcessCapsInterlock(virQEMUCapsPtr qemuCaps) !virQEMUCapsGet(qemuCaps, QEMU_CAPS_USB_STORAGE_WERROR)) { virQEMUCapsClear(qemuCaps, QEMU_CAPS_STORAGE_WERROR); } + + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_BLOCKDEV)) + virQEMUCapsSet(qemuCaps, QEMU_CAPS_BLOCKDEV_HOSTDEV_SCSI); } diff --git a/tests/qemucapabilitiesdata/caps_4.2.0.aarch64.xml b/tests/qemucapabilitiesdata/caps_4.2.0.aarch64.xml index 98cee36669..11a964ed39 100644 --- a/tests/qemucapabilitiesdata/caps_4.2.0.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_4.2.0.aarch64.xml @@ -188,6 +188,7 @@ <flag name='migration-param.bandwidth'/> <flag name='migration-param.downtime'/> <flag name='migration-param.xbzrle-cache-size'/> + <flag name='blockdev-hostdev-scsi'/> <version>4001050</version> <kvmVersion>0</kvmVersion> <microcodeVersion>61700242</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.2.0.s390x.xml b/tests/qemucapabilitiesdata/caps_4.2.0.s390x.xml index 0b174ffeec..76e2747b65 100644 --- a/tests/qemucapabilitiesdata/caps_4.2.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_4.2.0.s390x.xml @@ -152,6 +152,7 @@ <flag name='migration-param.bandwidth'/> <flag name='migration-param.downtime'/> <flag name='migration-param.xbzrle-cache-size'/> + <flag name='blockdev-hostdev-scsi'/> <version>4002000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>39100242</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml index 91ce30ba44..fd63a0ee02 100644 --- a/tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml @@ -235,6 +235,7 @@ <flag name='migration-param.downtime'/> <flag name='migration-param.xbzrle-cache-size'/> <flag name='intel-iommu.aw-bits'/> + <flag name='blockdev-hostdev-scsi'/> <version>4002000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100242</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml b/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml index 9446ddc22a..928af2a01c 100644 --- a/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml @@ -198,6 +198,7 @@ <flag name='migration-param.downtime'/> <flag name='migration-param.xbzrle-cache-size'/> <flag name='numa.hmat'/> + <flag name='blockdev-hostdev-scsi'/> <version>5000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>61700241</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml b/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml index 8c371a045b..e8668a25a9 100644 --- a/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml @@ -207,6 +207,7 @@ <flag name='migration-param.xbzrle-cache-size'/> <flag name='spapr-tpm-proxy'/> <flag name='numa.hmat'/> + <flag name='blockdev-hostdev-scsi'/> <version>5000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>42900241</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml b/tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml index 15c9d54332..85a8a46dac 100644 --- a/tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml +++ b/tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml @@ -194,6 +194,7 @@ <flag name='migration-param.downtime'/> <flag name='migration-param.xbzrle-cache-size'/> <flag name='numa.hmat'/> + <flag name='blockdev-hostdev-scsi'/> <version>5000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>0</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml index dafca26b89..546b9b0422 100644 --- a/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml @@ -242,6 +242,7 @@ <flag name='migration-param.xbzrle-cache-size'/> <flag name='intel-iommu.aw-bits'/> <flag name='numa.hmat'/> + <flag name='blockdev-hostdev-scsi'/> <version>5000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100241</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml index bd6e83ccaf..e05290fcfe 100644 --- a/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml @@ -242,6 +242,7 @@ <flag name='migration-param.xbzrle-cache-size'/> <flag name='intel-iommu.aw-bits'/> <flag name='numa.hmat'/> + <flag name='blockdev-hostdev-scsi'/> <version>5000050</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100242</microcodeVersion> diff --git a/tests/qemuxml2argvdata/hostdev-scsi-lsi.x86_64-latest.args b/tests/qemuxml2argvdata/hostdev-scsi-lsi.x86_64-latest.args index 76a0708336..d4599f6002 100644 --- a/tests/qemuxml2argvdata/hostdev-scsi-lsi.x86_64-latest.args +++ b/tests/qemuxml2argvdata/hostdev-scsi-lsi.x86_64-latest.args @@ -34,34 +34,42 @@ file=/tmp/lib/domain--1-QEMUGuest2/master-key.aes \ -blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"raw",\ "file":"libvirt-1-storage"}' \ -device ide-hd,bus=ide.0,unit=0,drive=libvirt-1-format,id=ide0-0-0,bootindex=1 \ --drive file=/dev/sg0,if=none,format=raw,id=drive-hostdev0 \ --device scsi-generic,bus=scsi0.0,scsi-id=7,drive=drive-hostdev0,id=hostdev0 \ --drive file=/dev/sg0,if=none,format=raw,id=drive-hostdev1,readonly=on \ --device scsi-generic,bus=scsi0.0,scsi-id=6,drive=drive-hostdev1,id=hostdev1 \ --drive file.driver=iscsi,file.portal=example.org:3260,\ -file.target=iqn.1992-01.com.example,file.lun=0,file.transport=tcp,if=none,\ -format=raw,id=drive-hostdev2 \ --device scsi-generic,bus=scsi0.0,scsi-id=4,drive=drive-hostdev2,id=hostdev2 \ --drive file.driver=iscsi,file.portal=example.org:3260,\ -file.target=iqn.1992-01.com.example,file.lun=1,file.transport=tcp,if=none,\ -format=raw,id=drive-hostdev3 \ --device scsi-generic,bus=scsi0.0,scsi-id=5,drive=drive-hostdev3,id=hostdev3 \ +-blockdev '{"driver":"host_device","filename":"/dev/sg0",\ +"node-name":"libvirt-hostdev0-backend","read-only":false}' \ +-device scsi-generic,bus=scsi0.0,scsi-id=7,drive=libvirt-hostdev0-backend,\ +id=hostdev0 \ +-blockdev '{"driver":"host_device","filename":"/dev/sg0",\ +"node-name":"libvirt-hostdev1-backend","read-only":true}' \ +-device scsi-generic,bus=scsi0.0,scsi-id=6,drive=libvirt-hostdev1-backend,\ +id=hostdev1 \ +-blockdev '{"driver":"iscsi","portal":"example.org:3260",\ +"target":"iqn.1992-01.com.example","lun":0,"transport":"tcp",\ +"node-name":"libvirt-hostdev2-backend","read-only":false}' \ +-device scsi-generic,bus=scsi0.0,scsi-id=4,drive=libvirt-hostdev2-backend,\ +id=hostdev2 \ +-blockdev '{"driver":"iscsi","portal":"example.org:3260",\ +"target":"iqn.1992-01.com.example","lun":1,"transport":"tcp",\ +"node-name":"libvirt-hostdev3-backend","read-only":false}' \ +-device scsi-generic,bus=scsi0.0,scsi-id=5,drive=libvirt-hostdev3-backend,\ +id=hostdev3 \ -object secret,id=hostdev4-secret0,\ data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ --drive file.driver=iscsi,file.portal=example.org:3260,\ -file.target=iqn.1992-01.com.example:storage,file.lun=1,file.transport=tcp,\ -file.user=myname,file.password-secret=hostdev4-secret0,if=none,format=raw,\ -id=drive-hostdev4 \ --device scsi-generic,bus=scsi0.0,scsi-id=3,drive=drive-hostdev4,id=hostdev4 \ +-blockdev '{"driver":"iscsi","portal":"example.org:3260",\ +"target":"iqn.1992-01.com.example:storage","lun":1,"transport":"tcp",\ +"user":"myname","password-secret":"hostdev4-secret0",\ +"node-name":"libvirt-hostdev4-backend","read-only":false}' \ +-device scsi-generic,bus=scsi0.0,scsi-id=3,drive=libvirt-hostdev4-backend,\ +id=hostdev4 \ -object secret,id=hostdev5-secret0,\ data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ --drive file.driver=iscsi,file.portal=example.org:3260,\ -file.target=iqn.1992-01.com.example:storage,file.lun=2,file.transport=tcp,\ -file.user=myname,file.password-secret=hostdev5-secret0,if=none,format=raw,\ -id=drive-hostdev5 \ --device scsi-generic,bus=scsi0.0,scsi-id=2,drive=drive-hostdev5,id=hostdev5 \ +-blockdev '{"driver":"iscsi","portal":"example.org:3260",\ +"target":"iqn.1992-01.com.example:storage","lun":2,"transport":"tcp",\ +"user":"myname","password-secret":"hostdev5-secret0",\ +"node-name":"libvirt-hostdev5-backend","read-only":false}' \ +-device scsi-generic,bus=scsi0.0,scsi-id=2,drive=libvirt-hostdev5-backend,\ +id=hostdev5 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 \ -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ resourcecontrol=deny \ diff --git a/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-latest.args b/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-latest.args index 99b0117447..72980d58b8 100644 --- a/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-latest.args +++ b/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-latest.args @@ -34,40 +34,42 @@ file=/tmp/lib/domain--1-QEMUGuest2/master-key.aes \ -blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"raw",\ "file":"libvirt-1-storage"}' \ -device ide-hd,bus=ide.0,unit=0,drive=libvirt-1-format,id=ide0-0-0,bootindex=1 \ --drive file=/dev/sg0,if=none,format=raw,id=drive-hostdev0 \ +-blockdev '{"driver":"host_device","filename":"/dev/sg0",\ +"node-name":"libvirt-hostdev0-backend","read-only":false}' \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=4,lun=8,\ -drive=drive-hostdev0,id=hostdev0 \ --drive file=/dev/sg0,if=none,format=raw,id=drive-hostdev1,readonly=on \ +drive=libvirt-hostdev0-backend,id=hostdev0 \ +-blockdev '{"driver":"host_device","filename":"/dev/sg0",\ +"node-name":"libvirt-hostdev1-backend","read-only":true}' \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=4,lun=7,\ -drive=drive-hostdev1,id=hostdev1 \ --drive file.driver=iscsi,file.portal=example.org:3260,\ -file.target=iqn.1992-01.com.example,file.lun=0,file.transport=tcp,if=none,\ -format=raw,id=drive-hostdev2 \ +drive=libvirt-hostdev1-backend,id=hostdev1 \ +-blockdev '{"driver":"iscsi","portal":"example.org:3260",\ +"target":"iqn.1992-01.com.example","lun":0,"transport":"tcp",\ +"node-name":"libvirt-hostdev2-backend","read-only":false}' \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=2,lun=4,\ -drive=drive-hostdev2,id=hostdev2 \ --drive file.driver=iscsi,file.portal=example.org:3260,\ -file.target=iqn.1992-01.com.example,file.lun=1,file.transport=tcp,if=none,\ -format=raw,id=drive-hostdev3 \ +drive=libvirt-hostdev2-backend,id=hostdev2 \ +-blockdev '{"driver":"iscsi","portal":"example.org:3260",\ +"target":"iqn.1992-01.com.example","lun":1,"transport":"tcp",\ +"node-name":"libvirt-hostdev3-backend","read-only":false}' \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=2,lun=5,\ -drive=drive-hostdev3,id=hostdev3 \ +drive=libvirt-hostdev3-backend,id=hostdev3 \ -object secret,id=hostdev4-secret0,\ data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ --drive file.driver=iscsi,file.portal=example.org:3260,\ -file.target=iqn.1992-01.com.example:storage,file.lun=1,file.transport=tcp,\ -file.user=myname,file.password-secret=hostdev4-secret0,if=none,format=raw,\ -id=drive-hostdev4 \ +-blockdev '{"driver":"iscsi","portal":"example.org:3260",\ +"target":"iqn.1992-01.com.example:storage","lun":1,"transport":"tcp",\ +"user":"myname","password-secret":"hostdev4-secret0",\ +"node-name":"libvirt-hostdev4-backend","read-only":false}' \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=3,lun=4,\ -drive=drive-hostdev4,id=hostdev4 \ +drive=libvirt-hostdev4-backend,id=hostdev4 \ -object secret,id=hostdev5-secret0,\ data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ --drive file.driver=iscsi,file.portal=example.org:3260,\ -file.target=iqn.1992-01.com.example:storage,file.lun=2,file.transport=tcp,\ -file.user=myname,file.password-secret=hostdev5-secret0,if=none,format=raw,\ -id=drive-hostdev5 \ +-blockdev '{"driver":"iscsi","portal":"example.org:3260",\ +"target":"iqn.1992-01.com.example:storage","lun":2,"transport":"tcp",\ +"user":"myname","password-secret":"hostdev5-secret0",\ +"node-name":"libvirt-hostdev5-backend","read-only":false}' \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=3,lun=5,\ -drive=drive-hostdev5,id=hostdev5 \ +drive=libvirt-hostdev5-backend,id=hostdev5 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 \ -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ resourcecontrol=deny \ -- 2.26.2

On Fri, Jul 10, 2020 at 16:33:38 +0200, Peter Krempa wrote:
Enable it when regular QEMU_CAPS_BLOCKDEV is present.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_capabilities.c | 3 ++ .../caps_4.2.0.aarch64.xml | 1 + .../qemucapabilitiesdata/caps_4.2.0.s390x.xml | 1 + .../caps_4.2.0.x86_64.xml | 1 + .../caps_5.0.0.aarch64.xml | 1 + .../qemucapabilitiesdata/caps_5.0.0.ppc64.xml | 1 + .../caps_5.0.0.riscv64.xml | 1 + .../caps_5.0.0.x86_64.xml | 1 + .../caps_5.1.0.x86_64.xml | 1 + .../hostdev-scsi-lsi.x86_64-latest.args | 52 +++++++++++-------- ...ostdev-scsi-virtio-scsi.x86_64-latest.args | 46 ++++++++-------- 11 files changed, 65 insertions(+), 44 deletions(-)
Kevin, Markus, could you please check, that he change from -drive if=none to -blockdev is equivalent/makes sense in these cases. Usage of SCSI hostdevs is finnicky as it tends to be used with weird devices such as tape libraries. I've tested it only with scsi_debug. Thanks.
diff --git a/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-latest.args b/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-latest.args index 99b0117447..72980d58b8 100644 --- a/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-latest.args +++ b/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-latest.args @@ -34,40 +34,42 @@ file=/tmp/lib/domain--1-QEMUGuest2/master-key.aes \ -blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"raw",\ "file":"libvirt-1-storage"}' \ -device ide-hd,bus=ide.0,unit=0,drive=libvirt-1-format,id=ide0-0-0,bootindex=1 \ --drive file=/dev/sg0,if=none,format=raw,id=drive-hostdev0 \ +-blockdev '{"driver":"host_device","filename":"/dev/sg0",\ +"node-name":"libvirt-hostdev0-backend","read-only":false}' \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=4,lun=8,\ -drive=drive-hostdev0,id=hostdev0 \ --drive file=/dev/sg0,if=none,format=raw,id=drive-hostdev1,readonly=on \ +drive=libvirt-hostdev0-backend,id=hostdev0 \ +-blockdev '{"driver":"host_device","filename":"/dev/sg0",\ +"node-name":"libvirt-hostdev1-backend","read-only":true}' \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=4,lun=7,\ -drive=drive-hostdev1,id=hostdev1 \ --drive file.driver=iscsi,file.portal=example.org:3260,\ -file.target=iqn.1992-01.com.example,file.lun=0,file.transport=tcp,if=none,\ -format=raw,id=drive-hostdev2 \ +drive=libvirt-hostdev1-backend,id=hostdev1 \ +-blockdev '{"driver":"iscsi","portal":"example.org:3260",\ +"target":"iqn.1992-01.com.example","lun":0,"transport":"tcp",\ +"node-name":"libvirt-hostdev2-backend","read-only":false}' \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=2,lun=4,\ -drive=drive-hostdev2,id=hostdev2 \ --drive file.driver=iscsi,file.portal=example.org:3260,\ -file.target=iqn.1992-01.com.example,file.lun=1,file.transport=tcp,if=none,\ -format=raw,id=drive-hostdev3 \ +drive=libvirt-hostdev2-backend,id=hostdev2 \ +-blockdev '{"driver":"iscsi","portal":"example.org:3260",\ +"target":"iqn.1992-01.com.example","lun":1,"transport":"tcp",\ +"node-name":"libvirt-hostdev3-backend","read-only":false}' \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=2,lun=5,\ -drive=drive-hostdev3,id=hostdev3 \ +drive=libvirt-hostdev3-backend,id=hostdev3 \ -object secret,id=hostdev4-secret0,\ data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ --drive file.driver=iscsi,file.portal=example.org:3260,\ -file.target=iqn.1992-01.com.example:storage,file.lun=1,file.transport=tcp,\ -file.user=myname,file.password-secret=hostdev4-secret0,if=none,format=raw,\ -id=drive-hostdev4 \ +-blockdev '{"driver":"iscsi","portal":"example.org:3260",\ +"target":"iqn.1992-01.com.example:storage","lun":1,"transport":"tcp",\ +"user":"myname","password-secret":"hostdev4-secret0",\ +"node-name":"libvirt-hostdev4-backend","read-only":false}' \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=3,lun=4,\ -drive=drive-hostdev4,id=hostdev4 \ +drive=libvirt-hostdev4-backend,id=hostdev4 \ -object secret,id=hostdev5-secret0,\ data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ --drive file.driver=iscsi,file.portal=example.org:3260,\ -file.target=iqn.1992-01.com.example:storage,file.lun=2,file.transport=tcp,\ -file.user=myname,file.password-secret=hostdev5-secret0,if=none,format=raw,\ -id=drive-hostdev5 \ +-blockdev '{"driver":"iscsi","portal":"example.org:3260",\ +"target":"iqn.1992-01.com.example:storage","lun":2,"transport":"tcp",\ +"user":"myname","password-secret":"hostdev5-secret0",\ +"node-name":"libvirt-hostdev5-backend","read-only":false}' \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=3,lun=5,\ -drive=drive-hostdev5,id=hostdev5 \ +drive=libvirt-hostdev5-backend,id=hostdev5 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 \ -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ resourcecontrol=deny \ -- 2.26.2

Am 10.07.2020 um 16:44 hat Peter Krempa geschrieben:
On Fri, Jul 10, 2020 at 16:33:38 +0200, Peter Krempa wrote:
Enable it when regular QEMU_CAPS_BLOCKDEV is present.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_capabilities.c | 3 ++ .../caps_4.2.0.aarch64.xml | 1 + .../qemucapabilitiesdata/caps_4.2.0.s390x.xml | 1 + .../caps_4.2.0.x86_64.xml | 1 + .../caps_5.0.0.aarch64.xml | 1 + .../qemucapabilitiesdata/caps_5.0.0.ppc64.xml | 1 + .../caps_5.0.0.riscv64.xml | 1 + .../caps_5.0.0.x86_64.xml | 1 + .../caps_5.1.0.x86_64.xml | 1 + .../hostdev-scsi-lsi.x86_64-latest.args | 52 +++++++++++-------- ...ostdev-scsi-virtio-scsi.x86_64-latest.args | 46 ++++++++-------- 11 files changed, 65 insertions(+), 44 deletions(-)
Kevin, Markus, could you please check, that he change from -drive if=none to -blockdev is equivalent/makes sense in these cases.
Usage of SCSI hostdevs is finnicky as it tends to be used with weird devices such as tape libraries. I've tested it only with scsi_debug.
Apart from the obvious differences shared with other cases (like differences in QMP for using node names instead of -drive IDs), it looks equivalent to me. Kevin
diff --git a/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-latest.args b/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-latest.args index 99b0117447..72980d58b8 100644 --- a/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-latest.args +++ b/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-latest.args @@ -34,40 +34,42 @@ file=/tmp/lib/domain--1-QEMUGuest2/master-key.aes \ -blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"raw",\ "file":"libvirt-1-storage"}' \ -device ide-hd,bus=ide.0,unit=0,drive=libvirt-1-format,id=ide0-0-0,bootindex=1 \ --drive file=/dev/sg0,if=none,format=raw,id=drive-hostdev0 \ +-blockdev '{"driver":"host_device","filename":"/dev/sg0",\ +"node-name":"libvirt-hostdev0-backend","read-only":false}' \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=4,lun=8,\ -drive=drive-hostdev0,id=hostdev0 \ --drive file=/dev/sg0,if=none,format=raw,id=drive-hostdev1,readonly=on \ +drive=libvirt-hostdev0-backend,id=hostdev0 \ +-blockdev '{"driver":"host_device","filename":"/dev/sg0",\ +"node-name":"libvirt-hostdev1-backend","read-only":true}' \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=4,lun=7,\ -drive=drive-hostdev1,id=hostdev1 \ --drive file.driver=iscsi,file.portal=example.org:3260,\ -file.target=iqn.1992-01.com.example,file.lun=0,file.transport=tcp,if=none,\ -format=raw,id=drive-hostdev2 \ +drive=libvirt-hostdev1-backend,id=hostdev1 \ +-blockdev '{"driver":"iscsi","portal":"example.org:3260",\ +"target":"iqn.1992-01.com.example","lun":0,"transport":"tcp",\ +"node-name":"libvirt-hostdev2-backend","read-only":false}' \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=2,lun=4,\ -drive=drive-hostdev2,id=hostdev2 \ --drive file.driver=iscsi,file.portal=example.org:3260,\ -file.target=iqn.1992-01.com.example,file.lun=1,file.transport=tcp,if=none,\ -format=raw,id=drive-hostdev3 \ +drive=libvirt-hostdev2-backend,id=hostdev2 \ +-blockdev '{"driver":"iscsi","portal":"example.org:3260",\ +"target":"iqn.1992-01.com.example","lun":1,"transport":"tcp",\ +"node-name":"libvirt-hostdev3-backend","read-only":false}' \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=2,lun=5,\ -drive=drive-hostdev3,id=hostdev3 \ +drive=libvirt-hostdev3-backend,id=hostdev3 \ -object secret,id=hostdev4-secret0,\ data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ --drive file.driver=iscsi,file.portal=example.org:3260,\ -file.target=iqn.1992-01.com.example:storage,file.lun=1,file.transport=tcp,\ -file.user=myname,file.password-secret=hostdev4-secret0,if=none,format=raw,\ -id=drive-hostdev4 \ +-blockdev '{"driver":"iscsi","portal":"example.org:3260",\ +"target":"iqn.1992-01.com.example:storage","lun":1,"transport":"tcp",\ +"user":"myname","password-secret":"hostdev4-secret0",\ +"node-name":"libvirt-hostdev4-backend","read-only":false}' \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=3,lun=4,\ -drive=drive-hostdev4,id=hostdev4 \ +drive=libvirt-hostdev4-backend,id=hostdev4 \ -object secret,id=hostdev5-secret0,\ data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ --drive file.driver=iscsi,file.portal=example.org:3260,\ -file.target=iqn.1992-01.com.example:storage,file.lun=2,file.transport=tcp,\ -file.user=myname,file.password-secret=hostdev5-secret0,if=none,format=raw,\ -id=drive-hostdev5 \ +-blockdev '{"driver":"iscsi","portal":"example.org:3260",\ +"target":"iqn.1992-01.com.example:storage","lun":2,"transport":"tcp",\ +"user":"myname","password-secret":"hostdev5-secret0",\ +"node-name":"libvirt-hostdev5-backend","read-only":false}' \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=3,lun=5,\ -drive=drive-hostdev5,id=hostdev5 \ +drive=libvirt-hostdev5-backend,id=hostdev5 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 \ -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ resourcecontrol=deny \ -- 2.26.2

On a Friday in 2020, Peter Krempa wrote:
Enable it when regular QEMU_CAPS_BLOCKDEV is present.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_capabilities.c | 3 ++ .../caps_4.2.0.aarch64.xml | 1 + .../qemucapabilitiesdata/caps_4.2.0.s390x.xml | 1 + .../caps_4.2.0.x86_64.xml | 1 + .../caps_5.0.0.aarch64.xml | 1 + .../qemucapabilitiesdata/caps_5.0.0.ppc64.xml | 1 + .../caps_5.0.0.riscv64.xml | 1 + .../caps_5.0.0.x86_64.xml | 1 + .../caps_5.1.0.x86_64.xml | 1 + .../hostdev-scsi-lsi.x86_64-latest.args | 52 +++++++++++-------- ...ostdev-scsi-virtio-scsi.x86_64-latest.args | 46 ++++++++-------- 11 files changed, 65 insertions(+), 44 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

The function is no longer called from other modules. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_command.h | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 7f624a031e..839c93bfb4 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4589,7 +4589,7 @@ qemuBuildSCSIVHostHostdevDevStr(const virDomainDef *def, return virBufferContentAndReset(&buf); } -char * +static char * qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev, virQEMUCapsPtr qemuCaps) { diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index a43cdb2733..b579817b44 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -175,9 +175,6 @@ char *qemuBuildUSBHostdevDevStr(const virDomainDef *def, virDomainHostdevDefPtr dev, virQEMUCapsPtr qemuCaps); -char *qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev, - virQEMUCapsPtr qemuCaps); - char *qemuBuildSCSIHostdevDevStr(const virDomainDef *def, virDomainHostdevDefPtr dev, const char *backendAlias); -- 2.26.2

On a Friday in 2020, Peter Krempa wrote:
The function is no longer called from other modules.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_command.h | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (3)
-
Ján Tomko
-
Kevin Wolf
-
Peter Krempa