[PATCH 00/17] qemu: Fix startup of VM with SCSI hostdev with long user-provided alias
Device alias was used to generate the backend nodename. This doesn't work well if somebody specifies a very long useralias since qemu limits nodename to 32 bytes. Stop basing the nodename on the alias. Peter Krempa (17): virDomainHostdevSubsysSCSIiSCSIClear: Inline contents into only caller virDomainStorageNetworkParseHosts: Switch to a more modern XML parsing approach virDomainHostdevSubsysSCSIHostDefParseXML: Switch to a more modern XML parsing approach syntax-check: Don't forbid curly braces around single line condition body qemuxml2argvtest: hostdev-scsi-virtio-scsi: Add hostdev with useralias conf: Add virStorageSource member for SCSI host device config data tests: qemustatusxml2xmldata: Rename 'disk-secinfo-upgrade' case to 'upgrade' tests: qemustatusxml2xmldata: Add local SCSI hostdev to 'upgrade' case qemu: domain: Fill in (i)SCSI backend nodename if it is not present in status XML qemuBuildHostdevSCSI(A|De)tachPrepare: Use virStorageSource in def for SCSI hostdevs qemuBlockStorageSourceAttachData: remove 'storageNodeNameCopy' qemu: domain: Extract preparation of hostdev specific data to a separate function qemuDomainSecretHostdevPrepare: remove qemuDomainPrepareHostdev: Allocate backend nodenames in the prepare function qemuDomainPrepareHostdev: base hostdev secret object names on backend alias qemuDomainPrepareHostdev: Don't base backend nodename on device alias qemuxml2argvtest: hostdev-scsi-virtio-scsi: Use longer user-alias for SCSI hostdev build-aux/check-spacing.pl | 36 ---- docs/coding-style.rst | 8 +- src/conf/domain_conf.c | 170 ++++++++--------- src/conf/domain_conf.h | 1 + src/qemu/qemu_block.c | 1 - src/qemu/qemu_block.h | 1 - src/qemu/qemu_command.c | 74 +++++--- src/qemu/qemu_domain.c | 176 ++++++++++++------ src/qemu/qemu_domain.h | 8 +- src/qemu/qemu_hotplug.c | 2 +- src/qemu/qemu_process.c | 21 +++ tests/qemustatusxml2xmldata/modern-in.xml | 1 + ...-secinfo-upgrade-in.xml => upgrade-in.xml} | 9 + ...ecinfo-upgrade-out.xml => upgrade-out.xml} | 20 ++ .../hostdev-scsi-lsi.x86_64-latest.args | 38 ++-- ...hostdev-scsi-virtio-scsi.x86_64-2.8.0.args | 5 + ...hostdev-scsi-virtio-scsi.x86_64-4.1.0.args | 5 + ...ostdev-scsi-virtio-scsi.x86_64-latest.args | 36 ++-- .../hostdev-scsi-virtio-scsi.xml | 8 + .../hostdev-scsi-virtio-scsi.xml | 8 + tests/qemuxml2xmltest.c | 2 +- 21 files changed, 370 insertions(+), 260 deletions(-) rename tests/qemustatusxml2xmldata/{disk-secinfo-upgrade-in.xml => upgrade-in.xml} (98%) rename tests/qemustatusxml2xmldata/{disk-secinfo-upgrade-out.xml => upgrade-out.xml} (96%) -- 2.26.2
There's just one caller for the function. Move the code into the caller. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 72ac4f4191..acbc3f1c1e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3011,24 +3011,15 @@ virDomainHostdevDefNew(void) } -static void -virDomainHostdevSubsysSCSIiSCSIClear(virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc) -{ - if (!iscsisrc) - return; - - virObjectUnref(iscsisrc->src); - iscsisrc->src = NULL; -} - - static void virDomainHostdevSubsysSCSIClear(virDomainHostdevSubsysSCSIPtr scsisrc) { - if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) - virDomainHostdevSubsysSCSIiSCSIClear(&scsisrc->u.iscsi); - else + if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) { + virObjectUnref(scsisrc->u.iscsi.src); + scsisrc->u.iscsi.src = NULL; + } else { VIR_FREE(scsisrc->u.host.adapter); + } } -- 2.26.2
On a Friday in 2020, Peter Krempa wrote:
There's just one caller for the function. Move the code into the caller.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Use XPath to get the host list instead of iterating through the nodes. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index acbc3f1c1e..ae7cb1e1c5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8234,23 +8234,26 @@ virDomainStorageNetworkParseHost(xmlNodePtr hostnode, static int virDomainStorageNetworkParseHosts(xmlNodePtr node, + xmlXPathContextPtr ctxt, virStorageNetHostDefPtr *hosts, size_t *nhosts) { - xmlNodePtr child; + g_autofree xmlNodePtr *hostnodes = NULL; + ssize_t nhostnodes; + size_t i; + VIR_XPATH_NODE_AUTORESTORE(ctxt); - for (child = node->children; child; child = child->next) { - if (child->type == XML_ELEMENT_NODE && - virXMLNodeNameEqual(child, "host")) { - virStorageNetHostDef host; + ctxt->node = node; - if (virDomainStorageNetworkParseHost(child, &host) < 0) - return -1; - if (VIR_APPEND_ELEMENT(*hosts, *nhosts, host) < 0) { - virStorageNetHostDefClear(&host); - return -1; - } - } + if ((nhostnodes = virXPathNodeSet("./host", ctxt, &hostnodes)) <= 0) + return nhostnodes; + + *hosts = g_new0(virStorageNetHostDef, nhostnodes); + *nhosts = nhostnodes; + + for (i = 0; i < nhostnodes; i++) { + if (virDomainStorageNetworkParseHost(hostnodes[i], *hosts + i) < 0) + return -1; } return 0; @@ -8370,7 +8373,7 @@ virDomainHostdevSubsysSCSIiSCSIDefParseXML(xmlNodePtr sourcenode, return -1; } - if (virDomainStorageNetworkParseHosts(sourcenode, &iscsisrc->src->hosts, + if (virDomainStorageNetworkParseHosts(sourcenode, ctxt, &iscsisrc->src->hosts, &iscsisrc->src->nhosts) < 0) return -1; @@ -9643,7 +9646,7 @@ virDomainDiskSourceNetworkParse(xmlNodePtr node, src->protocol == VIR_STORAGE_NET_PROTOCOL_HTTPS) src->query = virXMLPropString(node, "query"); - if (virDomainStorageNetworkParseHosts(node, &src->hosts, &src->nhosts) < 0) + if (virDomainStorageNetworkParseHosts(node, ctxt, &src->hosts, &src->nhosts) < 0) return -1; virStorageSourceNetworkAssignDefaultPorts(src); -- 2.26.2
On a Friday in 2020, Peter Krempa wrote:
Use XPath to get the host list instead of iterating through the nodes.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index acbc3f1c1e..ae7cb1e1c5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8234,23 +8234,26 @@ virDomainStorageNetworkParseHost(xmlNodePtr hostnode,
static int virDomainStorageNetworkParseHosts(xmlNodePtr node, + xmlXPathContextPtr ctxt, virStorageNetHostDefPtr *hosts, size_t *nhosts) { - xmlNodePtr child; + g_autofree xmlNodePtr *hostnodes = NULL; + ssize_t nhostnodes; + size_t i; + VIR_XPATH_NODE_AUTORESTORE(ctxt);
Please drop the ending semicolon so that other declarations can be added without triggering -Wdeclaration-after-statement. It contains a pragma to suppress the -Wunused-variable warning (with Clang, IIRC): commit 8cc177fc5d2c1ac76b256bd8d104d894fa9845ec util: xml: use pragma in VIR_XPATH_NODE_AUTORESTORE And placing a semicolon after it creates an empty statement. (This was the only way I found that VIR_XPATH_NODE_AUTORESTORE would not be considered a statement and -Wdeclaration-after-statement could be enabled with both GCC and Clang) Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Use XPath instead of iterating through the nodes. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 98 +++++++++++++++--------------------------- 1 file changed, 35 insertions(+), 63 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ae7cb1e1c5..05c2c63f58 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8262,89 +8262,61 @@ virDomainStorageNetworkParseHosts(xmlNodePtr node, static int virDomainHostdevSubsysSCSIHostDefParseXML(xmlNodePtr sourcenode, + xmlXPathContextPtr ctxt, virDomainHostdevSubsysSCSIPtr scsisrc) { - bool got_address = false, got_adapter = false; - xmlNodePtr cur; virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host; g_autofree char *bus = NULL; g_autofree char *target = NULL; g_autofree char *unit = NULL; + xmlNodePtr addressnode = NULL; + VIR_XPATH_NODE_AUTORESTORE(ctxt); - cur = sourcenode->children; - while (cur != NULL) { - if (cur->type == XML_ELEMENT_NODE) { - if (virXMLNodeNameEqual(cur, "address")) { - if (got_address) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("more than one source addresses is " - "specified for scsi hostdev")); - return -1; - } - - if (!(bus = virXMLPropString(cur, "bus")) || - !(target = virXMLPropString(cur, "target")) || - !(unit = virXMLPropString(cur, "unit"))) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("'bus', 'target', and 'unit' must be specified " - "for scsi hostdev source address")); - return -1; - } + ctxt->node = sourcenode; - if (virStrToLong_uip(bus, NULL, 0, &scsihostsrc->bus) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot parse bus '%s'"), bus); - return -1; - } + if (!(addressnode = virXPathNode("./address", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("'address' must be specified for scsi hostdev source")); + return -1; + } - if (virStrToLong_uip(target, NULL, 0, - &scsihostsrc->target) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot parse target '%s'"), target); - return -1; - } + if (!(bus = virXMLPropString(addressnode, "bus")) || + !(target = virXMLPropString(addressnode, "target")) || + !(unit = virXMLPropString(addressnode, "unit"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("'bus', 'target', and 'unit' must be specified " + "for scsi hostdev source address")); + return -1; + } - if (virStrToLong_ullp(unit, NULL, 0, &scsihostsrc->unit) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot parse unit '%s'"), unit); - return -1; - } + if (virStrToLong_uip(bus, NULL, 0, &scsihostsrc->bus) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse bus '%s'"), bus); + return -1; + } - got_address = true; - } else if (virXMLNodeNameEqual(cur, "adapter")) { - if (got_adapter) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("more than one adapters is specified " - "for scsi hostdev source")); - return -1; - } - if (!(scsihostsrc->adapter = virXMLPropString(cur, "name"))) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("'adapter' must be specified for scsi hostdev source")); - return -1; - } + if (virStrToLong_uip(target, NULL, 0, &scsihostsrc->target) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse target '%s'"), target); + return -1; + } - got_adapter = true; - } else { - virReportError(VIR_ERR_XML_ERROR, - _("unsupported element '%s' of scsi hostdev source"), - cur->name); - return -1; - } - } - cur = cur->next; + if (virStrToLong_ullp(unit, NULL, 0, &scsihostsrc->unit) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse unit '%s'"), unit); + return -1; } - if (!got_address || !got_adapter) { + if (!(scsihostsrc->adapter = virXPathString("string(./adapter/@name)", ctxt))) { virReportError(VIR_ERR_XML_ERROR, "%s", - _("'adapter' and 'address' must be specified for scsi " - "hostdev source")); + _("'adapter' name must be specified for scsi hostdev source")); return -1; } return 0; } + static int virDomainHostdevSubsysSCSIiSCSIDefParseXML(xmlNodePtr sourcenode, virDomainHostdevSubsysSCSIPtr def, @@ -8441,7 +8413,7 @@ virDomainHostdevSubsysSCSIDefParseXML(xmlNodePtr sourcenode, switch ((virDomainHostdevSCSIProtocolType) scsisrc->protocol) { case VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_NONE: - return virDomainHostdevSubsysSCSIHostDefParseXML(sourcenode, scsisrc); + return virDomainHostdevSubsysSCSIHostDefParseXML(sourcenode, ctxt, scsisrc); case VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI: return virDomainHostdevSubsysSCSIiSCSIDefParseXML(sourcenode, scsisrc, ctxt, -- 2.26.2
On a Friday in 2020, Peter Krempa wrote:
Use XPath instead of iterating through the nodes.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 98 +++++++++++++++--------------------------- 1 file changed, 35 insertions(+), 63 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ae7cb1e1c5..05c2c63f58 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c
[...]
- if (got_adapter) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("more than one adapters is specified " - "for scsi hostdev source")); - return -1; - } - if (!(scsihostsrc->adapter = virXMLPropString(cur, "name"))) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("'adapter' must be specified for scsi hostdev source")); - return -1; - }
[...]
- if (!got_address || !got_adapter) { + if (!(scsihostsrc->adapter = virXPathString("string(./adapter/@name)", ctxt))) { virReportError(VIR_ERR_XML_ERROR, "%s", - _("'adapter' and 'address' must be specified for scsi " - "hostdev source")); + _("'adapter' name must be specified for scsi hostdev source")); return -1; }
You changed the error message for a missing adapter name. Either split it out into a separate commit, or at least mention the change in the commit message. With that: Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
This syntax rule doesn't make much sense, especially if there are so much exceptions to it. Just remove it and adjust the coding style. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- build-aux/check-spacing.pl | 36 ------------------------------------ docs/coding-style.rst | 8 ++++---- 2 files changed, 4 insertions(+), 40 deletions(-) diff --git a/build-aux/check-spacing.pl b/build-aux/check-spacing.pl index 33377f3dd3..72901b75f9 100755 --- a/build-aux/check-spacing.pl +++ b/build-aux/check-spacing.pl @@ -24,11 +24,6 @@ my $ret = 0; my $incomment = 0; foreach my $file (@ARGV) { - # Per-file variables for multiline Curly Bracket (cb_) check - my $cb_linenum = 0; - my $cb_code = ""; - my $cb_scolon = 0; - open FILE, $file; while (defined (my $line = <FILE>)) { @@ -160,37 +155,6 @@ foreach my $file (@ARGV) { print "$file:$.: $line"; $ret = 1; } - - # One line conditional statements with one line bodies should - # not use curly brackets. - if ($data =~ /^\s*(if|while|for)\b.*\{$/) { - $cb_linenum = $.; - $cb_code = $line; - $cb_scolon = 0; - } - - # We need to check for exactly one semicolon inside the body, - # because empty statements (e.g. with comment only) are - # allowed - if ($cb_linenum == $. - 1 && $data =~ /^[^;]*;[^;]*$/) { - $cb_code .= $line; - $cb_scolon = 1; - } - - if ($data =~ /^\s*}\s*$/ && - $cb_linenum == $. - 2 && - $cb_scolon) { - - print "Curly brackets around single-line body:\n"; - print "$file:$cb_linenum-$.:\n$cb_code$line"; - $ret = 1; - - # There _should_ be no need to reset the values; but to - # keep my inner peace... - $cb_linenum = 0; - $cb_scolon = 0; - $cb_code = ""; - } } close FILE; } diff --git a/docs/coding-style.rst b/docs/coding-style.rst index 942caf4e09..44e5265a60 100644 --- a/docs/coding-style.rst +++ b/docs/coding-style.rst @@ -258,15 +258,15 @@ comment, although use of a semicolon is not currently rejected. Curly braces ------------ -Omit the curly braces around an ``if``, ``while``, ``for`` etc. -body only when both that body and the condition itself occupy a -single line. In every other case we require the braces. This +Curly braces around an ``if``, ``while``, ``for`` etc. can be omitted if the +body and the condition itself occupy only a single line. +In every other case we require the braces. This ensures that it is trivially easy to identify a single-\ *statement* loop: each has only one *line* in its body. :: - while (expr) // single line body; {} is forbidden + while (expr) // single line body; {} is optional single_line_stmt(); :: -- 2.26.2
On a Friday in 2020, Peter Krempa wrote:
This syntax rule doesn't make much sense, especially if there are so much exceptions to it. Just remove it and adjust the coding style.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- build-aux/check-spacing.pl | 36 ------------------------------------
before: $ hyperfine 'make -C build/build-aux sc_spacing-check' Benchmark #1: make -C build/build-aux sc_spacing-check Time (mean ± σ): 1.385 s ± 0.023 s [User: 1.386 s, System: 0.022 s] Range (min … max): 1.356 s … 1.425 s 10 runs after: $ hyperfine 'make -C build/build-aux sc_spacing-check' Benchmark #1: make -C build/build-aux sc_spacing-check Time (mean ± σ): 1.215 s ± 0.025 s [User: 1.217 s, System: 0.024 s] Range (min … max): 1.179 s … 1.259 s 10 runs Yay, less wasted CPU cycles.
docs/coding-style.rst | 8 ++++---- 2 files changed, 4 insertions(+), 40 deletions(-)
diff --git a/docs/coding-style.rst b/docs/coding-style.rst index 942caf4e09..44e5265a60 100644 --- a/docs/coding-style.rst +++ b/docs/coding-style.rst @@ -258,15 +258,15 @@ comment, although use of a semicolon is not currently rejected. Curly braces ------------
-Omit the curly braces around an ``if``, ``while``, ``for`` etc. -body only when both that body and the condition itself occupy a -single line. In every other case we require the braces. This +Curly braces around an ``if``, ``while``, ``for`` etc. can be omitted if the +body and the condition itself occupy only a single line. +In every other case we require the braces. This ensures that it is trivially easy to identify a single-\ *statement* loop: each has only one *line* in its body.
::
- while (expr) // single line body; {} is forbidden + while (expr) // single line body; {} is optional single_line_stmt();
::
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Add a SCSI host device with a user-specified alias to illustrate the upcoming changes. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- .../hostdev-scsi-virtio-scsi.x86_64-2.8.0.args | 3 +++ .../hostdev-scsi-virtio-scsi.x86_64-4.1.0.args | 3 +++ .../hostdev-scsi-virtio-scsi.x86_64-latest.args | 4 ++++ tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.xml | 8 ++++++++ tests/qemuxml2xmloutdata/hostdev-scsi-virtio-scsi.xml | 8 ++++++++ 5 files changed, 26 insertions(+) diff --git a/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-2.8.0.args b/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-2.8.0.args index c5a3c0ce61..07b7a5b113 100644 --- a/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-2.8.0.args +++ b/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-2.8.0.args @@ -37,6 +37,9 @@ drive=drive-hostdev0,id=hostdev0 \ -drive file=/dev/sg0,if=none,format=raw,id=drive-hostdev1,readonly=on \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=4,lun=7,\ drive=drive-hostdev1,id=hostdev1 \ +-drive file=/dev/sg0,if=none,format=raw,id=drive-ua-test \ +-device scsi-generic,bus=scsi0.0,channel=0,scsi-id=4,lun=6,drive=drive-ua-test,\ +id=ua-test \ -drive file=iscsi://example.org:3260/iqn.1992-01.com.example/0,if=none,\ format=raw,id=drive-hostdev2 \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=2,lun=4,\ diff --git a/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-4.1.0.args b/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-4.1.0.args index f2591d6956..421edf90d0 100644 --- a/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-4.1.0.args +++ b/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-4.1.0.args @@ -36,6 +36,9 @@ drive=drive-hostdev0,id=hostdev0 \ -drive file=/dev/sg0,if=none,format=raw,id=drive-hostdev1,readonly=on \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=4,lun=7,\ drive=drive-hostdev1,id=hostdev1 \ +-drive file=/dev/sg0,if=none,format=raw,id=drive-ua-test \ +-device scsi-generic,bus=scsi0.0,channel=0,scsi-id=4,lun=6,drive=drive-ua-test,\ +id=ua-test \ -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 \ 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 f86cbd7314..a2302d1089 100644 --- a/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-latest.args +++ b/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-latest.args @@ -42,6 +42,10 @@ drive=libvirt-hostdev0-backend,id=hostdev0 \ "node-name":"libvirt-hostdev1-backend","read-only":true}' \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=4,lun=7,\ drive=libvirt-hostdev1-backend,id=hostdev1 \ +-blockdev '{"driver":"host_device","filename":"/dev/sg0",\ +"node-name":"libvirt-ua-test-backend","read-only":false}' \ +-device scsi-generic,bus=scsi0.0,channel=0,scsi-id=4,lun=6,\ +drive=libvirt-ua-test-backend,id=ua-test \ -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}' \ diff --git a/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.xml b/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.xml index f1caf80644..8da3fb1bfc 100644 --- a/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.xml +++ b/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.xml @@ -40,6 +40,14 @@ <readonly/> <address type='drive' controller='0' bus='0' target='4' unit='7'/> </hostdev> + <hostdev mode='subsystem' type='scsi' managed='no'> + <source> + <adapter name='scsi_host0'/> + <address bus='0' target='0' unit='2'/> + </source> + <alias name='ua-test'/> + <address type='drive' controller='0' bus='0' target='4' unit='6'/> + </hostdev> <hostdev mode='subsystem' type='scsi' managed='yes'> <source protocol='iscsi' name='iqn.1992-01.com.example'> <host name='example.org' port='3260'/> diff --git a/tests/qemuxml2xmloutdata/hostdev-scsi-virtio-scsi.xml b/tests/qemuxml2xmloutdata/hostdev-scsi-virtio-scsi.xml index 6c7e22d0c3..733d1d72a0 100644 --- a/tests/qemuxml2xmloutdata/hostdev-scsi-virtio-scsi.xml +++ b/tests/qemuxml2xmloutdata/hostdev-scsi-virtio-scsi.xml @@ -47,6 +47,14 @@ <readonly/> <address type='drive' controller='0' bus='0' target='4' unit='7'/> </hostdev> + <hostdev mode='subsystem' type='scsi' managed='no'> + <source> + <adapter name='scsi_host0'/> + <address bus='0' target='0' unit='2'/> + </source> + <alias name='ua-test'/> + <address type='drive' controller='0' bus='0' target='4' unit='6'/> + </hostdev> <hostdev mode='subsystem' type='scsi' managed='yes'> <source protocol='iscsi' name='iqn.1992-01.com.example/0'> <host name='example.org' port='3260'/> -- 2.26.2
On a Friday in 2020, Peter Krempa wrote:
Add a SCSI host device with a user-specified alias to illustrate the upcoming changes.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- .../hostdev-scsi-virtio-scsi.x86_64-2.8.0.args | 3 +++ .../hostdev-scsi-virtio-scsi.x86_64-4.1.0.args | 3 +++ .../hostdev-scsi-virtio-scsi.x86_64-latest.args | 4 ++++ tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.xml | 8 ++++++++ tests/qemuxml2xmloutdata/hostdev-scsi-virtio-scsi.xml | 8 ++++++++ 5 files changed, 26 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The backend for the SCSI host device is a storage source. While the definition doesn't look like that it's converted to a storage source when the VM is running. Add the storage source to the definition object and also parse/format it's private data which will be used for internal state storage while the VM is running. Note that the virStorageSourcePtr may not be allocated all the time so the private data parser allocates it if there is any private data present. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 24 ++++++++++++++++++++++-- src/conf/domain_conf.h | 1 + 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 05c2c63f58..f93cd60c2b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3019,6 +3019,8 @@ virDomainHostdevSubsysSCSIClear(virDomainHostdevSubsysSCSIPtr scsisrc) scsisrc->u.iscsi.src = NULL; } else { VIR_FREE(scsisrc->u.host.adapter); + virObjectUnref(scsisrc->u.host.src); + scsisrc->u.host.src = NULL; } } @@ -8263,7 +8265,9 @@ virDomainStorageNetworkParseHosts(xmlNodePtr node, static int virDomainHostdevSubsysSCSIHostDefParseXML(xmlNodePtr sourcenode, xmlXPathContextPtr ctxt, - virDomainHostdevSubsysSCSIPtr scsisrc) + virDomainHostdevSubsysSCSIPtr scsisrc, + unsigned int flags, + virDomainXMLOptionPtr xmlopt) { virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host; g_autofree char *bus = NULL; @@ -8313,6 +8317,16 @@ virDomainHostdevSubsysSCSIHostDefParseXML(xmlNodePtr sourcenode, return -1; } + if (flags & VIR_DOMAIN_DEF_PARSE_STATUS && + xmlopt && xmlopt->privateData.storageParse) { + if ((ctxt->node = virXPathNode("./privateData", ctxt))) { + if (!scsihostsrc->src && + !(scsihostsrc->src = virStorageSourceNew())) + return -1; + if (xmlopt->privateData.storageParse(ctxt, scsihostsrc->src) < 0) + return -1; + } + } return 0; } @@ -8413,7 +8427,8 @@ virDomainHostdevSubsysSCSIDefParseXML(xmlNodePtr sourcenode, switch ((virDomainHostdevSCSIProtocolType) scsisrc->protocol) { case VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_NONE: - return virDomainHostdevSubsysSCSIHostDefParseXML(sourcenode, ctxt, scsisrc); + return virDomainHostdevSubsysSCSIHostDefParseXML(sourcenode, ctxt, scsisrc, + flags, xmlopt); case VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI: return virDomainHostdevSubsysSCSIiSCSIDefParseXML(sourcenode, scsisrc, ctxt, @@ -26305,6 +26320,11 @@ virDomainHostdevDefFormatSubsysSCSI(virBufferPtr buf, virBufferAsprintf(&sourceChildBuf, " bus='%u' target='%u' unit='%llu'", scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit); virBufferAddLit(&sourceChildBuf, "/>\n"); + + if (scsihostsrc->src && + virDomainDiskSourceFormatPrivateData(&sourceChildBuf, scsihostsrc->src, + flags, xmlopt) < 0) + return -1; } virXMLFormatElement(buf, "source", &sourceAttrBuf, &sourceChildBuf); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 14a376350c..cf76f340ee 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -245,6 +245,7 @@ struct _virDomainHostdevSubsysSCSIHost { unsigned bus; unsigned target; unsigned long long unit; + virStorageSourcePtr src; }; struct _virDomainHostdevSubsysSCSIiSCSI { -- 2.26.2
On a Friday in 2020, Peter Krempa wrote:
The backend for the SCSI host device is a storage source. While the definition doesn't look like that it's converted to a storage source when the VM is running.
Add the storage source to the definition object and also parse/format it's private data which will be used for internal state storage while
s/it's/its/
the VM is running.
Note that the virStorageSourcePtr may not be allocated all the time so the private data parser allocates it if there is any private data present.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 24 ++++++++++++++++++++++-- src/conf/domain_conf.h | 1 + 2 files changed, 23 insertions(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The test case tests other things besides disk secinfos, so we can make it more universal. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- .../{disk-secinfo-upgrade-in.xml => upgrade-in.xml} | 0 .../{disk-secinfo-upgrade-out.xml => upgrade-out.xml} | 0 tests/qemuxml2xmltest.c | 2 +- 3 files changed, 1 insertion(+), 1 deletion(-) rename tests/qemustatusxml2xmldata/{disk-secinfo-upgrade-in.xml => upgrade-in.xml} (100%) rename tests/qemustatusxml2xmldata/{disk-secinfo-upgrade-out.xml => upgrade-out.xml} (100%) diff --git a/tests/qemustatusxml2xmldata/disk-secinfo-upgrade-in.xml b/tests/qemustatusxml2xmldata/upgrade-in.xml similarity index 100% rename from tests/qemustatusxml2xmldata/disk-secinfo-upgrade-in.xml rename to tests/qemustatusxml2xmldata/upgrade-in.xml diff --git a/tests/qemustatusxml2xmldata/disk-secinfo-upgrade-out.xml b/tests/qemustatusxml2xmldata/upgrade-out.xml similarity index 100% rename from tests/qemustatusxml2xmldata/disk-secinfo-upgrade-out.xml rename to tests/qemustatusxml2xmldata/upgrade-out.xml diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 39a9da874f..2bf8dd5b14 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -1435,7 +1435,7 @@ mymain(void) DO_TEST_STATUS("migration-in-params"); DO_TEST_STATUS("migration-out-params"); DO_TEST_STATUS("migration-out-nbd-tls"); - DO_TEST_STATUS("disk-secinfo-upgrade"); + DO_TEST_STATUS("upgrade"); DO_TEST_STATUS("blockjob-blockdev"); -- 2.26.2
On a Friday in 2020, Peter Krempa wrote:
The test case tests other things besides disk secinfos, so we can make it more universal.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- .../{disk-secinfo-upgrade-in.xml => upgrade-in.xml} | 0 .../{disk-secinfo-upgrade-out.xml => upgrade-out.xml} | 0 tests/qemuxml2xmltest.c | 2 +- 3 files changed, 1 insertion(+), 1 deletion(-) rename tests/qemustatusxml2xmldata/{disk-secinfo-upgrade-in.xml => upgrade-in.xml} (100%) rename tests/qemustatusxml2xmldata/{disk-secinfo-upgrade-out.xml => upgrade-out.xml} (100%)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Add a local SCSI host device to validate upcoming generated data. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemustatusxml2xmldata/upgrade-in.xml | 8 ++++++++ tests/qemustatusxml2xmldata/upgrade-out.xml | 8 ++++++++ 2 files changed, 16 insertions(+) diff --git a/tests/qemustatusxml2xmldata/upgrade-in.xml b/tests/qemustatusxml2xmldata/upgrade-in.xml index 8023857ffe..1942eacfa4 100644 --- a/tests/qemustatusxml2xmldata/upgrade-in.xml +++ b/tests/qemustatusxml2xmldata/upgrade-in.xml @@ -511,6 +511,14 @@ <alias name='hostdev1'/> <address type='drive' controller='0' bus='0' target='2' unit='5'/> </hostdev> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source> + <adapter name='scsi_host0'/> + <address bus='0' target='0' unit='0'/> + </source> + <alias name='hostdev2'/> + <address type='drive' controller='0' bus='0' target='2' unit='6'/> + </hostdev> <redirdev bus='usb' type='spicevmc'> <alias name='redir0'/> <address type='usb' bus='0' port='2'/> diff --git a/tests/qemustatusxml2xmldata/upgrade-out.xml b/tests/qemustatusxml2xmldata/upgrade-out.xml index d5534fb0f5..ee3c1b49b0 100644 --- a/tests/qemustatusxml2xmldata/upgrade-out.xml +++ b/tests/qemustatusxml2xmldata/upgrade-out.xml @@ -543,6 +543,14 @@ <alias name='hostdev1'/> <address type='drive' controller='0' bus='0' target='2' unit='5'/> </hostdev> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source> + <adapter name='scsi_host0'/> + <address bus='0' target='0' unit='0'/> + </source> + <alias name='hostdev2'/> + <address type='drive' controller='0' bus='0' target='2' unit='6'/> + </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:
Add a local SCSI host device to validate upcoming generated data.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemustatusxml2xmldata/upgrade-in.xml | 8 ++++++++ tests/qemustatusxml2xmldata/upgrade-out.xml | 8 ++++++++ 2 files changed, 16 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
For upgrade reasons so that we can modify the used nodename we must generate the old version for all status XMLs which don't have it stored explicitly. The change will be required as using the user-provided alias may result in too-long nodenames which will be rejected by qemu. Add code which fills in the appropriate old value and add test cases to validate that it's added and also that existing nodenames are not overwritten. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 55 +++++++++++++++++++++ tests/qemustatusxml2xmldata/modern-in.xml | 1 + tests/qemustatusxml2xmldata/upgrade-in.xml | 1 + tests/qemustatusxml2xmldata/upgrade-out.xml | 12 +++++ 4 files changed, 69 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 0ed132a829..89f2c2c09b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5301,6 +5301,57 @@ qemuDomainDeviceHostdevDefPostParseRestoreSecAlias(virDomainHostdevDefPtr hostde } +/** + * qemuDomainDeviceHostdevDefPostParseRestoreBackendAlias: + * + * Re-generate backend alias if it wasn't stored in the status XML by an older + * libvirtd. + * + * Note that qemuCaps should be always present for a status XML. + */ +static int +qemuDomainDeviceHostdevDefPostParseRestoreBackendAlias(virDomainHostdevDefPtr hostdev, + virQEMUCapsPtr qemuCaps, + unsigned int parseFlags) +{ + virDomainHostdevSubsysSCSIPtr scsisrc = &hostdev->source.subsys.u.scsi; + virStorageSourcePtr src; + + if (!(parseFlags & VIR_DOMAIN_DEF_PARSE_STATUS)) + return 0; + + if (!qemuCaps || + hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS || + hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI || + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_BLOCKDEV_HOSTDEV_SCSI)) + return 0; + + switch ((virDomainHostdevSCSIProtocolType) scsisrc->protocol) { + case VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_NONE: + if (!scsisrc->u.host.src && + !(scsisrc->u.host.src = virStorageSourceNew())) + return -1; + + src = scsisrc->u.host.src; + break; + + case VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI: + src = scsisrc->u.iscsi.src; + break; + + case VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_LAST: + default: + virReportEnumRangeError(virDomainHostdevSCSIProtocolType, scsisrc->protocol); + return -1; + } + + if (!src->nodestorage) + src->nodestorage = g_strdup_printf("libvirt-%s-backend", hostdev->info->alias); + + return 0; +} + + static int qemuDomainHostdevDefMdevPostParse(virDomainHostdevSubsysMediatedDevPtr mdevsrc, virQEMUCapsPtr qemuCaps) @@ -5327,6 +5378,10 @@ qemuDomainHostdevDefPostParse(virDomainHostdevDefPtr hostdev, parseFlags) < 0) return -1; + if (qemuDomainDeviceHostdevDefPostParseRestoreBackendAlias(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) diff --git a/tests/qemustatusxml2xmldata/modern-in.xml b/tests/qemustatusxml2xmldata/modern-in.xml index 0a3990bd3e..5a5d5b8983 100644 --- a/tests/qemustatusxml2xmldata/modern-in.xml +++ b/tests/qemustatusxml2xmldata/modern-in.xml @@ -234,6 +234,7 @@ <flag name='dump-completed'/> <flag name='hda-output'/> <flag name='blockdev'/> + <flag name='blockdev-hostdev-scsi'/> </qemuCaps> <devices> <device alias='rng0'/> diff --git a/tests/qemustatusxml2xmldata/upgrade-in.xml b/tests/qemustatusxml2xmldata/upgrade-in.xml index 1942eacfa4..7fa56429f4 100644 --- a/tests/qemustatusxml2xmldata/upgrade-in.xml +++ b/tests/qemustatusxml2xmldata/upgrade-in.xml @@ -232,6 +232,7 @@ <flag name='isa-serial'/> <flag name='dump-completed'/> <flag name='hda-output'/> + <flag name='blockdev-hostdev-scsi'/> </qemuCaps> <devices> <device alias='rng0'/> diff --git a/tests/qemustatusxml2xmldata/upgrade-out.xml b/tests/qemustatusxml2xmldata/upgrade-out.xml index ee3c1b49b0..962d25ce2e 100644 --- a/tests/qemustatusxml2xmldata/upgrade-out.xml +++ b/tests/qemustatusxml2xmldata/upgrade-out.xml @@ -232,6 +232,7 @@ <flag name='isa-serial'/> <flag name='dump-completed'/> <flag name='hda-output'/> + <flag name='blockdev-hostdev-scsi'/> </qemuCaps> <devices> <device alias='rng0'/> @@ -517,6 +518,9 @@ <source protocol='iscsi' name='iqn.1992-01.com.example:storage/1'> <host name='example.org' port='3260'/> <privateData> + <nodenames> + <nodename type='storage' name='libvirt-hostdev0-backend'/> + </nodenames> <objects> <secret type='auth' alias='hostdev0-secret0'/> </objects> @@ -532,6 +536,9 @@ <source protocol='iscsi' name='iqn.1992-01.com.example:storage/2'> <host name='example.org' port='3260'/> <privateData> + <nodenames> + <nodename type='storage' name='libvirt-hostdev1-backend'/> + </nodenames> <objects> <secret type='auth' alias='hostdev1-secret0'/> </objects> @@ -547,6 +554,11 @@ <source> <adapter name='scsi_host0'/> <address bus='0' target='0' unit='0'/> + <privateData> + <nodenames> + <nodename type='storage' name='libvirt-hostdev2-backend'/> + </nodenames> + </privateData> </source> <alias name='hostdev2'/> <address type='drive' controller='0' bus='0' target='2' unit='6'/> -- 2.26.2
On a Friday in 2020, Peter Krempa wrote:
For upgrade reasons so that we can modify the used nodename we must generate the old version for all status XMLs which don't have it stored explicitly.
The change will be required as using the user-provided alias may result in too-long nodenames which will be rejected by qemu.
Add code which fills in the appropriate old value and add test cases to validate that it's added and also that existing nodenames are not overwritten.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 55 +++++++++++++++++++++ tests/qemustatusxml2xmldata/modern-in.xml | 1 + tests/qemustatusxml2xmldata/upgrade-in.xml | 1 + tests/qemustatusxml2xmldata/upgrade-out.xml | 12 +++++ 4 files changed, 69 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On a Friday in 2020, Peter Krempa wrote:
For upgrade reasons so that we can modify the used nodename we must generate the old version for all status XMLs which don't have it stored explicitly.
The change will be required as using the user-provided alias may result in too-long nodenames which will be rejected by qemu.
Add code which fills in the appropriate old value and add test cases to validate that it's added and also that existing nodenames are not overwritten.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 55 +++++++++++++++++++++ tests/qemustatusxml2xmldata/modern-in.xml | 1 + tests/qemustatusxml2xmldata/upgrade-in.xml | 1 + tests/qemustatusxml2xmldata/upgrade-out.xml | 12 +++++ 4 files changed, 69 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Modify the attach/detach data generators to actually use the virStorageSourceStructure embedded in the SCSI config data rather than creating an ad-hoc internal one. The modification will allow us to properly store the nodename used for the backend in the status XML rather than re-generating it all the time. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 71 +++++++++++++++++++++++++++-------------- 1 file changed, 47 insertions(+), 24 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index bd98b0a97c..d92b967419 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5053,24 +5053,35 @@ 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); + virStorageSourcePtr src; + qemuDomainStorageSourcePrivatePtr srcpriv; - ret->storageNodeName = iscsisrc->src->nodestorage; + switch ((virDomainHostdevSCSIProtocolType) scsisrc->protocol) { + case VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_NONE: + src = scsisrc->u.host.src; + break; - 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; + case VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI: + src = scsisrc->u.iscsi.src; + break; + + case VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_LAST: + default: + virReportEnumRangeError(virDomainHostdevSCSIProtocolType, scsisrc->protocol); + return NULL; } + srcpriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src); + ret->storageNodeName = src->nodestorage; ret->storageAttached = true; + + if (srcpriv && srcpriv->secinfo && + srcpriv->secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) + ret->authsecretAlias = g_strdup(srcpriv->secinfo->s.aes.alias); + } else { ret->driveAlias = qemuAliasFromHostdev(hostdev); ret->driveAdded = true; @@ -5086,45 +5097,57 @@ qemuBuildHostdevSCSIAttachPrepare(virDomainHostdevDefPtr hostdev, 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); + virStorageSourcePtr src = NULL; 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); + g_autofree char *devstr = NULL; - if (!devstr) + switch ((virDomainHostdevSCSIProtocolType) scsisrc->protocol) { + case VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_NONE: + if (!scsisrc->u.host.src && + !(scsisrc->u.host.src = virStorageSourceNew())) return NULL; - if (!(src = localsrc = virStorageSourceNew())) + if (!(devstr = qemuBuildSCSIHostHostdevDrvStr(hostdev))) return NULL; + src = scsisrc->u.host.src; + src->type = VIR_STORAGE_TYPE_BLOCK; src->readonly = hostdev->readonly; src->path = g_strdup_printf("/dev/%s", devstr); + + break; + + case VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI: + src = scsisrc->u.iscsi.src; + break; + + case VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_LAST: + default: + virReportEnumRangeError(virDomainHostdevSCSIProtocolType, scsisrc->protocol); + return NULL; } src->nodestorage = g_strdup_printf("libvirt-%s-backend", hostdev->info->alias); - ret->storageNodeNameCopy = g_strdup(src->nodestorage); - ret->storageNodeName = ret->storageNodeNameCopy; - *backendAlias = ret->storageNodeNameCopy; + ret->storageNodeName = src->nodestorage; + *backendAlias = src->nodestorage; if (!(ret->storageProps = qemuBlockStorageSourceGetBackendProps(src, QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_SKIP_UNMAP))) return NULL; } else { + if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) + src = scsisrc->u.iscsi.src; 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) + if (src && + qemuBuildStorageSourceAttachPrepareCommon(src, ret, qemuCaps) < 0) return NULL; return g_steal_pointer(&ret); -- 2.26.2
On a Friday in 2020, Peter Krempa wrote:
Modify the attach/detach data generators to actually use the virStorageSourceStructure embedded in the SCSI config data rather than creating an ad-hoc internal one.
The modification will allow us to properly store the nodename used for the backend in the status XML rather than re-generating it all the time.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 71 +++++++++++++++++++++++++++-------------- 1 file changed, 47 insertions(+), 24 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
This was a hack when we were locally regenerating the nodename so that it's not leaked. Now that we use proper virStorageSource with persistence it's no longer required. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 1 - src/qemu/qemu_block.h | 1 - 2 files changed, 2 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 9095986f73..487b0a72e7 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -1557,7 +1557,6 @@ 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 9aab620947..0701fc18d1 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -85,7 +85,6 @@ 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:
This was a hack when we were locally regenerating the nodename so that it's not leaked. Now that we use proper virStorageSource with persistence it's no longer required.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 1 - src/qemu/qemu_block.h | 1 - 2 files changed, 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Historically we've prepared secrets for all objects in one place. This doesn't make much sense and it's semantically more appealing to prepare everything for a single device type in one place. Move the setup of the (iSCSI|SCSI) hostdev secrets into a new function which will be used to setup other things as well in the future. This is a similar approach we do for disks. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 59 ++++++++++++++++++++++++++++++++++++----- src/qemu/qemu_domain.h | 4 +++ src/qemu/qemu_hotplug.c | 2 +- src/qemu/qemu_process.c | 21 +++++++++++++++ 4 files changed, 78 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 89f2c2c09b..1289201764 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1596,13 +1596,7 @@ qemuDomainSecretPrepare(virQEMUDriverPtr driver, g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); size_t i; - /* disk secrets are prepared when preparing disks */ - - for (i = 0; i < vm->def->nhostdevs; i++) { - if (qemuDomainSecretHostdevPrepare(priv, - vm->def->hostdevs[i]) < 0) - return -1; - } + /* disk and hostdev secrets are prepared when preparing internal data */ for (i = 0; i < vm->def->nserials; i++) { if (qemuDomainSecretChardevPrepare(cfg, priv, @@ -10455,6 +10449,57 @@ qemuDomainPrepareDiskSource(virDomainDiskDefPtr disk, } +int +qemuDomainPrepareHostdev(virDomainHostdevDefPtr hostdev, + qemuDomainObjPrivatePtr priv) +{ + if (virHostdevIsSCSIDevice(hostdev)) { + virDomainHostdevSubsysSCSIPtr scsisrc = &hostdev->source.subsys.u.scsi; + virStorageSourcePtr src = NULL; + + switch ((virDomainHostdevSCSIProtocolType) scsisrc->protocol) { + case VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_NONE: + break; + + case VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI: + src = scsisrc->u.iscsi.src; + break; + + case VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_LAST: + default: + virReportEnumRangeError(virDomainHostdevSCSIProtocolType, scsisrc->protocol); + return -1; + } + + if (src) { + if (src->auth) { + bool iscsiHasPS = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_ISCSI_PASSWORD_SECRET); + virSecretUsageType usageType = VIR_SECRET_USAGE_TYPE_ISCSI; + qemuDomainStorageSourcePrivatePtr srcPriv = qemuDomainStorageSourcePrivateFetch(src); + + if (!qemuDomainSupportsEncryptedSecret(priv) || !iscsiHasPS) { + srcPriv->secinfo = qemuDomainSecretInfoNewPlain(usageType, + src->auth->username, + &src->auth->seclookupdef); + } else { + srcPriv->secinfo = qemuDomainSecretAESSetupFromSecret(priv, + hostdev->info->alias, + NULL, + usageType, + src->auth->username, + &src->auth->seclookupdef); + } + + if (!srcPriv->secinfo) + return -1; + } + } + } + + return 0; +} + + /** * qemuDomainDiskCachemodeFlags: * diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index adba79aded..6abd896119 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -970,6 +970,10 @@ qemuDomainDiskCachemodeFlags(int cachemode, bool *direct, bool *noflush); +int +qemuDomainPrepareHostdev(virDomainHostdevDefPtr hostdev, + qemuDomainObjPrivatePtr priv); + char * qemuDomainGetManagedPRSocketPath(qemuDomainObjPrivatePtr priv); bool qemuDomainDefHasManagedPR(virDomainObjPtr vm); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index e2c6e14c2e..f20b8e9a56 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2604,7 +2604,7 @@ qemuDomainAttachHostSCSIDevice(virQEMUDriverPtr driver, if (qemuAssignDeviceHostdevAlias(vm->def, &hostdev->info->alias, -1) < 0) goto cleanup; - if (qemuDomainSecretHostdevPrepare(priv, hostdev) < 0) + if (qemuDomainPrepareHostdev(hostdev, priv) < 0) goto cleanup; if (!(data = qemuBuildHostdevSCSIAttachPrepare(hostdev, &backendalias, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index dd60fb0ddf..79e72aaf2a 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6213,6 +6213,23 @@ qemuProcessPrepareDomainStorage(virQEMUDriverPtr driver, } +static int +qemuProcessPrepareDomainHostdevs(virDomainObjPtr vm, + qemuDomainObjPrivatePtr priv) +{ + size_t i; + + for (i = 0; i < vm->def->nhostdevs; i++) { + virDomainHostdevDefPtr hostdev = vm->def->hostdevs[i]; + + if (qemuDomainPrepareHostdev(hostdev, priv) < 0) + return -1; + } + + return 0; +} + + static void qemuProcessPrepareAllowReboot(virDomainObjPtr vm) { @@ -6315,6 +6332,10 @@ qemuProcessPrepareDomain(virQEMUDriverPtr driver, if (qemuProcessPrepareDomainStorage(driver, vm, priv, cfg, flags) < 0) return -1; + VIR_DEBUG("Setting up host devices"); + if (qemuProcessPrepareDomainHostdevs(vm, priv) < 0) + return -1; + VIR_DEBUG("Prepare chardev source backends for TLS"); qemuDomainPrepareChardevSource(vm->def, cfg); -- 2.26.2
On a Friday in 2020, Peter Krempa wrote:
Historically we've prepared secrets for all objects in one place. This doesn't make much sense and it's semantically more appealing to prepare everything for a single device type in one place.
Move the setup of the (iSCSI|SCSI) hostdev secrets into a new function which will be used to setup other things as well in the future.
This is a similar approach we do for disks.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 59 ++++++++++++++++++++++++++++++++++++----- src/qemu/qemu_domain.h | 4 +++ src/qemu/qemu_hotplug.c | 2 +- src/qemu/qemu_process.c | 21 +++++++++++++++ 4 files changed, 78 insertions(+), 8 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The function is no longer used once we setup per-hostdev data. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 50 ------------------------------------------ src/qemu/qemu_domain.h | 4 ---- 2 files changed, 54 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 1289201764..a751d0c205 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1381,56 +1381,6 @@ qemuDomainSecretHostdevDestroy(virDomainHostdevDefPtr hostdev) } -/* qemuDomainSecretHostdevPrepare: - * @priv: pointer to domain private object - * @hostdev: Pointer to a hostdev definition - * - * For the right host device, generate the qemuDomainSecretInfo structure. - * - * Returns 0 on success, -1 on failure - */ -int -qemuDomainSecretHostdevPrepare(qemuDomainObjPrivatePtr priv, - virDomainHostdevDefPtr hostdev) -{ - if (virHostdevIsSCSIDevice(hostdev)) { - virDomainHostdevSubsysSCSIPtr scsisrc = &hostdev->source.subsys.u.scsi; - virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = &scsisrc->u.iscsi; - virStorageSourcePtr src = iscsisrc->src; - - if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI && - src->auth) { - bool iscsiHasPS = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_ISCSI_PASSWORD_SECRET); - virSecretUsageType usageType = VIR_SECRET_USAGE_TYPE_ISCSI; - qemuDomainStorageSourcePrivatePtr srcPriv; - - if (!(src->privateData = qemuDomainStorageSourcePrivateNew())) - return -1; - - srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src); - - if (!qemuDomainSupportsEncryptedSecret(priv) || !iscsiHasPS) { - srcPriv->secinfo = qemuDomainSecretInfoNewPlain(usageType, - src->auth->username, - &src->auth->seclookupdef); - } else { - srcPriv->secinfo = qemuDomainSecretAESSetupFromSecret(priv, - hostdev->info->alias, - NULL, - usageType, - src->auth->username, - &src->auth->seclookupdef); - } - - if (!srcPriv->secinfo) - return -1; - } - } - - return 0; -} - - void qemuDomainSecretChardevDestroy(virDomainChrSourceDefPtr dev) { diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 6abd896119..77d6bfc810 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -851,10 +851,6 @@ qemuDomainSecretInfoTLSNew(qemuDomainObjPrivatePtr priv, void qemuDomainSecretHostdevDestroy(virDomainHostdevDefPtr disk) ATTRIBUTE_NONNULL(1); -int qemuDomainSecretHostdevPrepare(qemuDomainObjPrivatePtr priv, - virDomainHostdevDefPtr hostdev) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); - void qemuDomainSecretChardevDestroy(virDomainChrSourceDefPtr dev) ATTRIBUTE_NONNULL(1); -- 2.26.2
On a Friday in 2020, Peter Krempa wrote:
The function is no longer used once we setup per-hostdev data.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 50 ------------------------------------------ src/qemu/qemu_domain.h | 4 ---- 2 files changed, 54 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Allocate the nodename in the setup function rather than in the command line generator. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 7 ++++--- src/qemu/qemu_domain.c | 8 ++++++++ 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d92b967419..c5c587b97d 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5105,9 +5105,11 @@ qemuBuildHostdevSCSIAttachPrepare(virDomainHostdevDefPtr hostdev, switch ((virDomainHostdevSCSIProtocolType) scsisrc->protocol) { case VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_NONE: - if (!scsisrc->u.host.src && - !(scsisrc->u.host.src = virStorageSourceNew())) + if (!scsisrc->u.host.src) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("SCSI host device data structure was not initialized")); return NULL; + } if (!(devstr = qemuBuildSCSIHostHostdevDrvStr(hostdev))) return NULL; @@ -5130,7 +5132,6 @@ qemuBuildHostdevSCSIAttachPrepare(virDomainHostdevDefPtr hostdev, return NULL; } - src->nodestorage = g_strdup_printf("libvirt-%s-backend", hostdev->info->alias); ret->storageNodeName = src->nodestorage; *backendAlias = src->nodestorage; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index a751d0c205..d570ba892b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -10409,6 +10409,10 @@ qemuDomainPrepareHostdev(virDomainHostdevDefPtr hostdev, switch ((virDomainHostdevSCSIProtocolType) scsisrc->protocol) { case VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_NONE: + virObjectUnref(scsisrc->u.host.src); + if (!(scsisrc->u.host.src = virStorageSourceNew())) + return -1; + src = scsisrc->u.host.src; break; case VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI: @@ -10422,6 +10426,10 @@ qemuDomainPrepareHostdev(virDomainHostdevDefPtr hostdev, } if (src) { + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV_HOSTDEV_SCSI)) { + src->nodestorage = g_strdup_printf("libvirt-%s-backend", hostdev->info->alias); + } + if (src->auth) { bool iscsiHasPS = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_ISCSI_PASSWORD_SECRET); virSecretUsageType usageType = VIR_SECRET_USAGE_TYPE_ISCSI; -- 2.26.2
On a Friday in 2020, Peter Krempa wrote:
Allocate the nodename in the setup function rather than in the command line generator.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 7 ++++--- src/qemu/qemu_domain.c | 8 ++++++++ 2 files changed, 12 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The secret object is used to pass data to the backend so it's better fitting to base the secret object name on the SCSI host device backend name. Since we store the object alias in the status XML this modification is safe in regards to existing guests. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 5 ++++- .../qemuxml2argvdata/hostdev-scsi-lsi.x86_64-latest.args | 8 ++++---- .../hostdev-scsi-virtio-scsi.x86_64-latest.args | 8 ++++---- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index d570ba892b..46f7caeb09 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -10426,8 +10426,11 @@ qemuDomainPrepareHostdev(virDomainHostdevDefPtr hostdev, } if (src) { + const char *backendalias = hostdev->info->alias; + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV_HOSTDEV_SCSI)) { src->nodestorage = g_strdup_printf("libvirt-%s-backend", hostdev->info->alias); + backendalias = src->nodestorage; } if (src->auth) { @@ -10441,7 +10444,7 @@ qemuDomainPrepareHostdev(virDomainHostdevDefPtr hostdev, &src->auth->seclookupdef); } else { srcPriv->secinfo = qemuDomainSecretAESSetupFromSecret(priv, - hostdev->info->alias, + backendalias, NULL, usageType, src->auth->username, diff --git a/tests/qemuxml2argvdata/hostdev-scsi-lsi.x86_64-latest.args b/tests/qemuxml2argvdata/hostdev-scsi-lsi.x86_64-latest.args index d4599f6002..f768c2471b 100644 --- a/tests/qemuxml2argvdata/hostdev-scsi-lsi.x86_64-latest.args +++ b/tests/qemuxml2argvdata/hostdev-scsi-lsi.x86_64-latest.args @@ -52,21 +52,21 @@ id=hostdev2 \ "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,\ +-object secret,id=libvirt-hostdev4-backend-secret0,\ data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ -blockdev '{"driver":"iscsi","portal":"example.org:3260",\ "target":"iqn.1992-01.com.example:storage","lun":1,"transport":"tcp",\ -"user":"myname","password-secret":"hostdev4-secret0",\ +"user":"myname","password-secret":"libvirt-hostdev4-backend-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,\ +-object secret,id=libvirt-hostdev5-backend-secret0,\ data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ -blockdev '{"driver":"iscsi","portal":"example.org:3260",\ "target":"iqn.1992-01.com.example:storage","lun":2,"transport":"tcp",\ -"user":"myname","password-secret":"hostdev5-secret0",\ +"user":"myname","password-secret":"libvirt-hostdev5-backend-secret0",\ "node-name":"libvirt-hostdev5-backend","read-only":false}' \ -device scsi-generic,bus=scsi0.0,scsi-id=2,drive=libvirt-hostdev5-backend,\ id=hostdev5 \ 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 a2302d1089..0beefabd27 100644 --- a/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-latest.args +++ b/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-latest.args @@ -56,21 +56,21 @@ drive=libvirt-hostdev2-backend,id=hostdev2 \ "node-name":"libvirt-hostdev3-backend","read-only":false}' \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=2,lun=5,\ drive=libvirt-hostdev3-backend,id=hostdev3 \ --object secret,id=hostdev4-secret0,\ +-object secret,id=libvirt-hostdev4-backend-secret0,\ data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ -blockdev '{"driver":"iscsi","portal":"example.org:3260",\ "target":"iqn.1992-01.com.example:storage","lun":1,"transport":"tcp",\ -"user":"myname","password-secret":"hostdev4-secret0",\ +"user":"myname","password-secret":"libvirt-hostdev4-backend-secret0",\ "node-name":"libvirt-hostdev4-backend","read-only":false}' \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=3,lun=4,\ drive=libvirt-hostdev4-backend,id=hostdev4 \ --object secret,id=hostdev5-secret0,\ +-object secret,id=libvirt-hostdev5-backend-secret0,\ data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ -blockdev '{"driver":"iscsi","portal":"example.org:3260",\ "target":"iqn.1992-01.com.example:storage","lun":2,"transport":"tcp",\ -"user":"myname","password-secret":"hostdev5-secret0",\ +"user":"myname","password-secret":"libvirt-hostdev5-backend-secret0",\ "initiator-name":"iqn.2020-07.com.example:test",\ "node-name":"libvirt-hostdev5-backend","read-only":false}' \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=3,lun=5,\ -- 2.26.2
On a Friday in 2020, Peter Krempa wrote:
The secret object is used to pass data to the backend so it's better fitting to base the secret object name on the SCSI host device backend name.
Since we store the object alias in the status XML this modification is safe in regards to existing guests.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 5 ++++- .../qemuxml2argvdata/hostdev-scsi-lsi.x86_64-latest.args | 8 ++++---- .../hostdev-scsi-virtio-scsi.x86_64-latest.args | 8 ++++---- 3 files changed, 12 insertions(+), 9 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
QEMU's blockdev nodenames which are used to back SCSI/iSCSI hostdevs are limited to 32 characters. If a user passes a very long user alias as name of the host device it's easy to end up with a too-long nodename. To prevent this from happening don't base the nodename on the possibly user-specified alias but on the normal sequential node name generator. We then store the name in the status XML for further use. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 3 +- .../hostdev-scsi-lsi.x86_64-latest.args | 38 ++++++++----------- ...ostdev-scsi-virtio-scsi.x86_64-latest.args | 36 +++++++++--------- 3 files changed, 36 insertions(+), 41 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 46f7caeb09..2444e47173 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -10429,7 +10429,8 @@ qemuDomainPrepareHostdev(virDomainHostdevDefPtr hostdev, const char *backendalias = hostdev->info->alias; if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV_HOSTDEV_SCSI)) { - src->nodestorage = g_strdup_printf("libvirt-%s-backend", hostdev->info->alias); + src->id = qemuDomainStorageIdNew(priv); + src->nodestorage = g_strdup_printf("libvirt-%d-backend", src->id); backendalias = src->nodestorage; } diff --git a/tests/qemuxml2argvdata/hostdev-scsi-lsi.x86_64-latest.args b/tests/qemuxml2argvdata/hostdev-scsi-lsi.x86_64-latest.args index f768c2471b..a5b200543a 100644 --- a/tests/qemuxml2argvdata/hostdev-scsi-lsi.x86_64-latest.args +++ b/tests/qemuxml2argvdata/hostdev-scsi-lsi.x86_64-latest.args @@ -35,41 +35,35 @@ file=/tmp/lib/domain--1-QEMUGuest2/master-key.aes \ "file":"libvirt-1-storage"}' \ -device ide-hd,bus=ide.0,unit=0,drive=libvirt-1-format,id=ide0-0-0,bootindex=1 \ -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 \ +"node-name":"libvirt-2-backend","read-only":false}' \ +-device scsi-generic,bus=scsi0.0,scsi-id=7,drive=libvirt-2-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 \ +"node-name":"libvirt-3-backend","read-only":true}' \ +-device scsi-generic,bus=scsi0.0,scsi-id=6,drive=libvirt-3-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 \ +"node-name":"libvirt-4-backend","read-only":false}' \ +-device scsi-generic,bus=scsi0.0,scsi-id=4,drive=libvirt-4-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=libvirt-hostdev4-backend-secret0,\ +"node-name":"libvirt-5-backend","read-only":false}' \ +-device scsi-generic,bus=scsi0.0,scsi-id=5,drive=libvirt-5-backend,id=hostdev3 \ +-object secret,id=libvirt-6-backend-secret0,\ data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ -blockdev '{"driver":"iscsi","portal":"example.org:3260",\ "target":"iqn.1992-01.com.example:storage","lun":1,"transport":"tcp",\ -"user":"myname","password-secret":"libvirt-hostdev4-backend-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=libvirt-hostdev5-backend-secret0,\ +"user":"myname","password-secret":"libvirt-6-backend-secret0",\ +"node-name":"libvirt-6-backend","read-only":false}' \ +-device scsi-generic,bus=scsi0.0,scsi-id=3,drive=libvirt-6-backend,id=hostdev4 \ +-object secret,id=libvirt-7-backend-secret0,\ data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ -blockdev '{"driver":"iscsi","portal":"example.org:3260",\ "target":"iqn.1992-01.com.example:storage","lun":2,"transport":"tcp",\ -"user":"myname","password-secret":"libvirt-hostdev5-backend-secret0",\ -"node-name":"libvirt-hostdev5-backend","read-only":false}' \ --device scsi-generic,bus=scsi0.0,scsi-id=2,drive=libvirt-hostdev5-backend,\ -id=hostdev5 \ +"user":"myname","password-secret":"libvirt-7-backend-secret0",\ +"node-name":"libvirt-7-backend","read-only":false}' \ +-device scsi-generic,bus=scsi0.0,scsi-id=2,drive=libvirt-7-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 0beefabd27..147ab40c9a 100644 --- a/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-latest.args +++ b/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-latest.args @@ -35,46 +35,46 @@ file=/tmp/lib/domain--1-QEMUGuest2/master-key.aes \ "file":"libvirt-1-storage"}' \ -device ide-hd,bus=ide.0,unit=0,drive=libvirt-1-format,id=ide0-0-0,bootindex=1 \ -blockdev '{"driver":"host_device","filename":"/dev/sg0",\ -"node-name":"libvirt-hostdev0-backend","read-only":false}' \ +"node-name":"libvirt-2-backend","read-only":false}' \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=4,lun=8,\ -drive=libvirt-hostdev0-backend,id=hostdev0 \ +drive=libvirt-2-backend,id=hostdev0 \ -blockdev '{"driver":"host_device","filename":"/dev/sg0",\ -"node-name":"libvirt-hostdev1-backend","read-only":true}' \ +"node-name":"libvirt-3-backend","read-only":true}' \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=4,lun=7,\ -drive=libvirt-hostdev1-backend,id=hostdev1 \ +drive=libvirt-3-backend,id=hostdev1 \ -blockdev '{"driver":"host_device","filename":"/dev/sg0",\ -"node-name":"libvirt-ua-test-backend","read-only":false}' \ +"node-name":"libvirt-4-backend","read-only":false}' \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=4,lun=6,\ -drive=libvirt-ua-test-backend,id=ua-test \ +drive=libvirt-4-backend,id=ua-test \ -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}' \ +"node-name":"libvirt-5-backend","read-only":false}' \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=2,lun=4,\ -drive=libvirt-hostdev2-backend,id=hostdev2 \ +drive=libvirt-5-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}' \ +"node-name":"libvirt-6-backend","read-only":false}' \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=2,lun=5,\ -drive=libvirt-hostdev3-backend,id=hostdev3 \ --object secret,id=libvirt-hostdev4-backend-secret0,\ +drive=libvirt-6-backend,id=hostdev3 \ +-object secret,id=libvirt-7-backend-secret0,\ data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ -blockdev '{"driver":"iscsi","portal":"example.org:3260",\ "target":"iqn.1992-01.com.example:storage","lun":1,"transport":"tcp",\ -"user":"myname","password-secret":"libvirt-hostdev4-backend-secret0",\ -"node-name":"libvirt-hostdev4-backend","read-only":false}' \ +"user":"myname","password-secret":"libvirt-7-backend-secret0",\ +"node-name":"libvirt-7-backend","read-only":false}' \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=3,lun=4,\ -drive=libvirt-hostdev4-backend,id=hostdev4 \ --object secret,id=libvirt-hostdev5-backend-secret0,\ +drive=libvirt-7-backend,id=hostdev4 \ +-object secret,id=libvirt-8-backend-secret0,\ data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ -blockdev '{"driver":"iscsi","portal":"example.org:3260",\ "target":"iqn.1992-01.com.example:storage","lun":2,"transport":"tcp",\ -"user":"myname","password-secret":"libvirt-hostdev5-backend-secret0",\ +"user":"myname","password-secret":"libvirt-8-backend-secret0",\ "initiator-name":"iqn.2020-07.com.example:test",\ -"node-name":"libvirt-hostdev5-backend","read-only":false}' \ +"node-name":"libvirt-8-backend","read-only":false}' \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=3,lun=5,\ -drive=libvirt-hostdev5-backend,id=hostdev5 \ +drive=libvirt-8-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:
QEMU's blockdev nodenames which are used to back SCSI/iSCSI hostdevs are limited to 32 characters. If a user passes a very long user alias as name of the host device it's easy to end up with a too-long nodename.
To prevent this from happening don't base the nodename on the possibly user-specified alias but on the normal sequential node name generator.
We then store the name in the status XML for further use.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 3 +- .../hostdev-scsi-lsi.x86_64-latest.args | 38 ++++++++----------- ...ostdev-scsi-virtio-scsi.x86_64-latest.args | 36 +++++++++--------- 3 files changed, 36 insertions(+), 41 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Test that we can cope with a long useralias when generating SCSI hostdev commandline. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- .../hostdev-scsi-virtio-scsi.x86_64-2.8.0.args | 8 +++++--- .../hostdev-scsi-virtio-scsi.x86_64-4.1.0.args | 8 +++++--- .../hostdev-scsi-virtio-scsi.x86_64-latest.args | 2 +- tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.xml | 2 +- tests/qemuxml2xmloutdata/hostdev-scsi-virtio-scsi.xml | 2 +- 5 files changed, 13 insertions(+), 9 deletions(-) diff --git a/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-2.8.0.args b/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-2.8.0.args index 07b7a5b113..3f007db2c3 100644 --- a/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-2.8.0.args +++ b/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-2.8.0.args @@ -37,9 +37,11 @@ drive=drive-hostdev0,id=hostdev0 \ -drive file=/dev/sg0,if=none,format=raw,id=drive-hostdev1,readonly=on \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=4,lun=7,\ drive=drive-hostdev1,id=hostdev1 \ --drive file=/dev/sg0,if=none,format=raw,id=drive-ua-test \ --device scsi-generic,bus=scsi0.0,channel=0,scsi-id=4,lun=6,drive=drive-ua-test,\ -id=ua-test \ +-drive file=/dev/sg0,if=none,format=raw,\ +id=drive-ua-7996c8dc-a4fa-4012-b76f-043d20144263 \ +-device scsi-generic,bus=scsi0.0,channel=0,scsi-id=4,lun=6,\ +drive=drive-ua-7996c8dc-a4fa-4012-b76f-043d20144263,\ +id=ua-7996c8dc-a4fa-4012-b76f-043d20144263 \ -drive file=iscsi://example.org:3260/iqn.1992-01.com.example/0,if=none,\ format=raw,id=drive-hostdev2 \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=2,lun=4,\ diff --git a/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-4.1.0.args b/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-4.1.0.args index 421edf90d0..eb11d8f733 100644 --- a/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-4.1.0.args +++ b/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-4.1.0.args @@ -36,9 +36,11 @@ drive=drive-hostdev0,id=hostdev0 \ -drive file=/dev/sg0,if=none,format=raw,id=drive-hostdev1,readonly=on \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=4,lun=7,\ drive=drive-hostdev1,id=hostdev1 \ --drive file=/dev/sg0,if=none,format=raw,id=drive-ua-test \ --device scsi-generic,bus=scsi0.0,channel=0,scsi-id=4,lun=6,drive=drive-ua-test,\ -id=ua-test \ +-drive file=/dev/sg0,if=none,format=raw,\ +id=drive-ua-7996c8dc-a4fa-4012-b76f-043d20144263 \ +-device scsi-generic,bus=scsi0.0,channel=0,scsi-id=4,lun=6,\ +drive=drive-ua-7996c8dc-a4fa-4012-b76f-043d20144263,\ +id=ua-7996c8dc-a4fa-4012-b76f-043d20144263 \ -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 \ 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 147ab40c9a..47c3f09db5 100644 --- a/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-latest.args +++ b/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-latest.args @@ -45,7 +45,7 @@ drive=libvirt-3-backend,id=hostdev1 \ -blockdev '{"driver":"host_device","filename":"/dev/sg0",\ "node-name":"libvirt-4-backend","read-only":false}' \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=4,lun=6,\ -drive=libvirt-4-backend,id=ua-test \ +drive=libvirt-4-backend,id=ua-7996c8dc-a4fa-4012-b76f-043d20144263 \ -blockdev '{"driver":"iscsi","portal":"example.org:3260",\ "target":"iqn.1992-01.com.example","lun":0,"transport":"tcp",\ "node-name":"libvirt-5-backend","read-only":false}' \ diff --git a/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.xml b/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.xml index 8da3fb1bfc..4903a75d13 100644 --- a/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.xml +++ b/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.xml @@ -45,7 +45,7 @@ <adapter name='scsi_host0'/> <address bus='0' target='0' unit='2'/> </source> - <alias name='ua-test'/> + <alias name='ua-7996c8dc-a4fa-4012-b76f-043d20144263'/> <address type='drive' controller='0' bus='0' target='4' unit='6'/> </hostdev> <hostdev mode='subsystem' type='scsi' managed='yes'> diff --git a/tests/qemuxml2xmloutdata/hostdev-scsi-virtio-scsi.xml b/tests/qemuxml2xmloutdata/hostdev-scsi-virtio-scsi.xml index 733d1d72a0..3a2b10d815 100644 --- a/tests/qemuxml2xmloutdata/hostdev-scsi-virtio-scsi.xml +++ b/tests/qemuxml2xmloutdata/hostdev-scsi-virtio-scsi.xml @@ -52,7 +52,7 @@ <adapter name='scsi_host0'/> <address bus='0' target='0' unit='2'/> </source> - <alias name='ua-test'/> + <alias name='ua-7996c8dc-a4fa-4012-b76f-043d20144263'/> <address type='drive' controller='0' bus='0' target='4' unit='6'/> </hostdev> <hostdev mode='subsystem' type='scsi' managed='yes'> -- 2.26.2
On a Friday in 2020, Peter Krempa wrote:
Test that we can cope with a long useralias when generating SCSI hostdev commandline.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- .../hostdev-scsi-virtio-scsi.x86_64-2.8.0.args | 8 +++++--- .../hostdev-scsi-virtio-scsi.x86_64-4.1.0.args | 8 +++++--- .../hostdev-scsi-virtio-scsi.x86_64-latest.args | 2 +- tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.xml | 2 +- tests/qemuxml2xmloutdata/hostdev-scsi-virtio-scsi.xml | 2 +- 5 files changed, 13 insertions(+), 9 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa