[libvirt] [PATCH] Fix virsh edit and virt-xml-validate with SCSI LUN
Defining a domain with a SCSI disk attached via a hostdev tag and a source address unit value longer than two digits causes an error when editing the domain with virsh edit, even if no changes are made to the domain definition. The error suggests invalid XML, somewhere: # virsh edit lmb_guest error: XML document failed to validate against schema: Unable to validate doc against /usr/local/share/libvirt/schemas/domain.rng Extra element devices in interleave Element domain failed to validate content The virt-xml-validate tool fails with a similar error: # virt-xml-validate lmb_guest.xml Relax-NG validity error : Extra element devices in interleave lmb_guest.xml:17: element devices: Relax-NG validity error : Element domain failed to validate content lmb_guest.xml fails to validate The hostdev tag requires a source address to be specified, which includes bus, target, and unit address attributes. According to the SCSI Architecture Model spec (section 4.9 of SAM-2), a LUN address is 64 bits and thus could be up to 20 decimal digits long. Unfortunately, the XML schema limits this string to just two digits. Similarly, the target field can be up to 32 bits in length, which would be 10 decimal digits. # lsscsi -xx [0:0:19:0x4022401100000000] disk IBM 2107900 3.44 /dev/sda # lsscsi [0:0:19:1074872354]disk IBM 2107900 3.44 /dev/sda # cat lmb_guest.xml <domain type='kvm'> <name>lmb_guest</name> <memory unit='MiB'>1024</memory> ...trimmed... <devices> <controller type='scsi' model='virtio-scsi' index='0'/> <hostdev mode='subsystem' type='scsi'> <source> <adapter name='scsi_host0'/> <address bus='0' target='19' unit='1074872354'/> </source> </hostdev> ...trimmed... Since the reference unit and target fields are used in several places in the XML schema, create a separate one specific for SCSI Logical Units that will permit the greater length. Also, expand the definition of the SCSI unit field from an int to a long long, to cover the possible size. This permits both the validation utility and the virsh edit command to succeed when a hostdev tag is included. Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com> Reviewed-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- docs/formatdomain.html.in | 10 +++++++--- docs/schemas/domaincommon.rng | 14 ++++++++++++-- src/conf/domain_audit.c | 2 +- src/conf/domain_conf.c | 4 ++-- src/conf/domain_conf.h | 2 +- src/qemu/qemu_command.h | 2 +- src/qemu/qemu_hotplug.c | 4 ++-- src/util/virhostdev.c | 6 +++--- src/util/virscsi.c | 16 ++++++++-------- src/util/virscsi.h | 8 ++++---- tests/testutilsqemu.c | 2 +- tools/virsh-domain.c | 6 +++--- 12 files changed, 45 insertions(+), 31 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 1781996..e7a8e1a 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2827,7 +2827,7 @@ <dd>Drive addresses have the following additional attributes: <code>controller</code> (a 2-digit controller number), <code>bus</code> (a 2-digit bus number), - <code>target</code> (a 2-digit bus number), + <code>target</code> (a 2-digit target number), and <code>unit</code> (a 2-digit unit number on the bus). </dd> <dt><code>type='virtio-serial'</code></dt> @@ -3136,7 +3136,7 @@ <hostdev mode='subsystem' type='scsi' sgio='filtered' rawio='yes'> <source> <adapter name='scsi_host0'/> - <address type='scsi' bus='0' target='0' unit='0'/> + <address bus='0' target='0' unit='0'/> </source> <readonly/> <address type='drive' controller='0' bus='0' target='0' unit='0'/> @@ -3244,7 +3244,11 @@ </dd> <dt>scsi</dt> <dd>SCSI devices are described by both the <code>adapter</code> - and <code>address</code> elements. + and <code>address</code> elements. The <code>address</code> + element includes a <code>bus</code> attribute (a 2-digit bus + number), a <code>target</code> attribute (a 10-digit target + number), and a <code>unit</code> attribute (a 20-digit unit + number on the bus). <p> <span class="since">Since 1.2.8</span>, the <code>source</code> element of a SCSI device may contain the <code>protocol</code> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 5dc48f7..dccd3fd 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3846,10 +3846,10 @@ <ref name="driveBus"/> </attribute> <attribute name="target"> - <ref name="driveTarget"/> + <ref name="driveSCSITarget"/> </attribute> <attribute name="unit"> - <ref name="driveUnit"/> + <ref name="driveSCSIUnit"/> </attribute> </define> <define name="usbportaddress"> @@ -5142,11 +5142,21 @@ <param name="pattern">[0-9]{1,2}</param> </data> </define> + <define name="driveSCSITarget"> + <data type="string"> + <param name="pattern">[0-9]{1,10}</param> + </data> + </define> <define name="driveUnit"> <data type="string"> <param name="pattern">[0-9]{1,2}</param> </data> </define> + <define name="driveSCSIUnit"> + <data type="string"> + <param name="pattern">[0-9]{1,20}</param> + </data> + </define> <define name="featureName"> <data type="string"> <param name='pattern'>[a-zA-Z0-9\-_\.]+</param> diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c index 1900039..844297c 100644 --- a/src/conf/domain_audit.c +++ b/src/conf/domain_audit.c @@ -427,7 +427,7 @@ virDomainAuditHostdev(virDomainObjPtr vm, virDomainHostdevDefPtr hostdev, } else { virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host; - if (virAsprintfQuiet(&address, "%s:%d:%d:%d", + if (virAsprintfQuiet(&address, "%s:%d:%d:%lld", scsihostsrc->adapter, scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit) < 0) { diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 36de844..1f2bfca 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4954,7 +4954,7 @@ virDomainHostdevSubsysSCSIHostDefParseXML(xmlNodePtr sourcenode, goto cleanup; } - if (virStrToLong_ui(unit, NULL, 0, &scsihostsrc->unit) < 0) { + if (virStrToLong_ull(unit, NULL, 0, &scsihostsrc->unit) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot parse unit '%s'"), unit); goto cleanup; @@ -18844,7 +18844,7 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf, virBufferAsprintf(buf, "<adapter name='%s'/>\n", scsihostsrc->adapter); virBufferAsprintf(buf, - "<address %sbus='%d' target='%d' unit='%d'/>\n", + "<address %sbus='%d' target='%d' unit='%lld'/>\n", includeTypeInAddr ? "type='scsi' " : "", scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ba17a8d..f677c2e 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -463,7 +463,7 @@ struct _virDomainHostdevSubsysSCSIHost { char *adapter; unsigned bus; unsigned target; - unsigned unit; + unsigned long long unit; }; typedef struct _virDomainHostdevSubsysSCSIiSCSI virDomainHostdevSubsysSCSIiSCSI; diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 0fc59a8..6e0c3a3 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -63,7 +63,7 @@ struct _qemuBuildCommandLineCallbacks { const char *adapter, unsigned int bus, unsigned int target, - unsigned int unit); + unsigned long long unit); }; extern qemuBuildCommandLineCallbacks buildCommandLineCallbacks; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 3562de6..bbdf0b0 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1938,7 +1938,7 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn, } else { virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host; virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to prepare scsi hostdev: %s:%d:%d:%d"), + _("Unable to prepare scsi hostdev: %s:%d:%d:%lld"), scsihostsrc->adapter, scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit); } @@ -3882,7 +3882,7 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver, virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host; virReportError(VIR_ERR_OPERATION_FAILED, - _("host scsi device %s:%d:%d.%d not found"), + _("host scsi device %s:%d:%d:%lld not found"), scsihostsrc->adapter, scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit); } diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 1c8f31e..e67fe18 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -1482,7 +1482,7 @@ virHostdevReAttachSCSIHostDevices(virHostdevManagerPtr hostdev_mgr, scsihostsrc->adapter, scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit, hostdev->readonly, hostdev->shareable))) { - VIR_WARN("Unable to reattach SCSI device %s:%d:%d:%d on domain %s", + VIR_WARN("Unable to reattach SCSI device %s:%d:%d:%lld on domain %s", scsihostsrc->adapter, scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit, dom_name); return; @@ -1492,7 +1492,7 @@ virHostdevReAttachSCSIHostDevices(virHostdevManagerPtr hostdev_mgr, * because qemuProcessStart could fail half way through. */ if (!(tmp = virSCSIDeviceListFind(hostdev_mgr->activeSCSIHostdevs, scsi))) { - VIR_WARN("Unable to find device %s:%d:%d:%d " + VIR_WARN("Unable to find device %s:%d:%d:%lld " "in list of active SCSI devices", scsihostsrc->adapter, scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit); @@ -1500,7 +1500,7 @@ virHostdevReAttachSCSIHostDevices(virHostdevManagerPtr hostdev_mgr, return; } - VIR_DEBUG("Removing %s:%d:%d:%d dom=%s from activeSCSIHostdevs", + VIR_DEBUG("Removing %s:%d:%d:%lld dom=%s from activeSCSIHostdevs", scsihostsrc->adapter, scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit, dom_name); diff --git a/src/util/virscsi.c b/src/util/virscsi.c index 9f5cf0d..5fd4380 100644 --- a/src/util/virscsi.c +++ b/src/util/virscsi.c @@ -56,7 +56,7 @@ struct _virSCSIDevice { unsigned int adapter; unsigned int bus; unsigned int target; - unsigned int unit; + unsigned long long unit; char *name; /* adapter:bus:target:unit */ char *id; /* model:vendor */ @@ -110,7 +110,7 @@ virSCSIDeviceGetSgName(const char *sysfs_prefix, const char *adapter, unsigned int bus, unsigned int target, - unsigned int unit) + unsigned long long unit) { DIR *dir = NULL; struct dirent *entry; @@ -123,7 +123,7 @@ virSCSIDeviceGetSgName(const char *sysfs_prefix, return NULL; if (virAsprintf(&path, - "%s/%d:%d:%d:%d/scsi_generic", + "%s/%d:%d:%d:%lld/scsi_generic", prefix, adapter_id, bus, target, unit) < 0) return NULL; @@ -157,7 +157,7 @@ virSCSIDeviceGetDevName(const char *sysfs_prefix, const char *adapter, unsigned int bus, unsigned int target, - unsigned int unit) + unsigned long long unit) { DIR *dir = NULL; struct dirent *entry; @@ -170,7 +170,7 @@ virSCSIDeviceGetDevName(const char *sysfs_prefix, return NULL; if (virAsprintf(&path, - "%s/%d:%d:%d:%d/block", + "%s/%d:%d:%d:%lld/block", prefix, adapter_id, bus, target, unit) < 0) return NULL; @@ -200,7 +200,7 @@ virSCSIDeviceNew(const char *sysfs_prefix, const char *adapter, unsigned int bus, unsigned int target, - unsigned int unit, + unsigned long long unit, bool readonly, bool shareable) { @@ -227,7 +227,7 @@ virSCSIDeviceNew(const char *sysfs_prefix, if (virSCSIDeviceGetAdapterId(adapter, &dev->adapter) < 0) goto cleanup; - if (virAsprintf(&dev->name, "%d:%d:%d:%d", dev->adapter, + if (virAsprintf(&dev->name, "%d:%d:%d:%lld", dev->adapter, dev->bus, dev->target, dev->unit) < 0 || virAsprintf(&dev->sg_path, "%s/%s", sysfs_prefix ? sysfs_prefix : "/dev", sg) < 0) @@ -347,7 +347,7 @@ virSCSIDeviceGetTarget(virSCSIDevicePtr dev) return dev->target; } -unsigned int +unsigned long long virSCSIDeviceGetUnit(virSCSIDevicePtr dev) { return dev->unit; diff --git a/src/util/virscsi.h b/src/util/virscsi.h index c67837f..df40d7f 100644 --- a/src/util/virscsi.h +++ b/src/util/virscsi.h @@ -37,18 +37,18 @@ char *virSCSIDeviceGetSgName(const char *sysfs_prefix, const char *adapter, unsigned int bus, unsigned int target, - unsigned int unit); + unsigned long long unit); char *virSCSIDeviceGetDevName(const char *sysfs_prefix, const char *adapter, unsigned int bus, unsigned int target, - unsigned int unit); + unsigned long long unit); virSCSIDevicePtr virSCSIDeviceNew(const char *sysfs_prefix, const char *adapter, unsigned int bus, unsigned int target, - unsigned int unit, + unsigned long long unit, bool readonly, bool shareable); @@ -61,7 +61,7 @@ const char *virSCSIDeviceGetName(virSCSIDevicePtr dev); unsigned int virSCSIDeviceGetAdapter(virSCSIDevicePtr dev); unsigned int virSCSIDeviceGetBus(virSCSIDevicePtr dev); unsigned int virSCSIDeviceGetTarget(virSCSIDevicePtr dev); -unsigned int virSCSIDeviceGetUnit(virSCSIDevicePtr dev); +unsigned long long virSCSIDeviceGetUnit(virSCSIDevicePtr dev); bool virSCSIDeviceGetReadonly(virSCSIDevicePtr dev); bool virSCSIDeviceGetShareable(virSCSIDevicePtr dev); diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index d067bca..ceaabb6 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -462,7 +462,7 @@ testSCSIDeviceGetSgName(const char *sysfs_prefix ATTRIBUTE_UNUSED, const char *adapter ATTRIBUTE_UNUSED, unsigned int bus ATTRIBUTE_UNUSED, unsigned int target ATTRIBUTE_UNUSED, - unsigned int unit ATTRIBUTE_UNUSED) + unsigned long long unit ATTRIBUTE_UNUSED) { char *sg = NULL; diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 4c47473..aa4d58c 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -415,7 +415,7 @@ struct PCIAddress { struct SCSIAddress { unsigned int controller; unsigned int bus; - unsigned int unit; + unsigned long long unit; }; struct IDEAddress { @@ -488,7 +488,7 @@ static int str2SCSIAddress(const char *str, struct SCSIAddress *scsiAddr) return -1; unit++; - if (virStrToLong_ui(unit, NULL, 0, &scsiAddr->unit) != 0) + if (virStrToLong_ull(unit, NULL, 0, &scsiAddr->unit) != 0) return -1; return 0; @@ -725,7 +725,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) if (diskAddr.type == DISK_ADDR_TYPE_SCSI) { virBufferAsprintf(&buf, "<address type='drive' controller='%d'" - " bus='%d' unit='%d' />\n", + " bus='%d' unit='%lld' />\n", diskAddr.addr.scsi.controller, diskAddr.addr.scsi.bus, diskAddr.addr.scsi.unit); } else { -- 1.9.1
On 06/10/2015 11:22 AM, Eric Farman wrote:
Defining a domain with a SCSI disk attached via a hostdev tag and a source address unit value longer than two digits causes an error when editing the domain with virsh edit, even if no changes are made to the domain definition. The error suggests invalid XML, somewhere:
# virsh edit lmb_guest error: XML document failed to validate against schema: Unable to validate doc against /usr/local/share/libvirt/schemas/domain.rng Extra element devices in interleave Element domain failed to validate content
The virt-xml-validate tool fails with a similar error:
# virt-xml-validate lmb_guest.xml Relax-NG validity error : Extra element devices in interleave lmb_guest.xml:17: element devices: Relax-NG validity error : Element domain failed to validate content lmb_guest.xml fails to validate
The hostdev tag requires a source address to be specified, which includes bus, target, and unit address attributes. According to the SCSI Architecture Model spec (section 4.9 of SAM-2), a LUN address is 64 bits and thus could be up to 20 decimal digits long. Unfortunately, the XML schema limits this string to just two digits. Similarly, the target field can be up to 32 bits in length, which would be 10 decimal digits.
# lsscsi -xx [0:0:19:0x4022401100000000] disk IBM 2107900 3.44 /dev/sda # lsscsi [0:0:19:1074872354]disk IBM 2107900 3.44 /dev/sda # cat lmb_guest.xml <domain type='kvm'> <name>lmb_guest</name> <memory unit='MiB'>1024</memory> ...trimmed... <devices> <controller type='scsi' model='virtio-scsi' index='0'/> <hostdev mode='subsystem' type='scsi'> <source> <adapter name='scsi_host0'/> <address bus='0' target='19' unit='1074872354'/> </source> </hostdev> ...trimmed...
Since the reference unit and target fields are used in several places in the XML schema, create a separate one specific for SCSI Logical Units that will permit the greater length. Also, expand the definition of the SCSI unit field from an int to a long long, to cover the possible size. This permits both the validation utility and the virsh edit command to succeed when a hostdev tag is included.
Nice explanation - helped set the stage quite well! Of course you've just opened Pandora's box of questions...
Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com> Reviewed-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- docs/formatdomain.html.in | 10 +++++++--- docs/schemas/domaincommon.rng | 14 ++++++++++++-- src/conf/domain_audit.c | 2 +- src/conf/domain_conf.c | 4 ++-- src/conf/domain_conf.h | 2 +- src/qemu/qemu_command.h | 2 +- src/qemu/qemu_hotplug.c | 4 ++-- src/util/virhostdev.c | 6 +++--- src/util/virscsi.c | 16 ++++++++-------- src/util/virscsi.h | 8 ++++---- tests/testutilsqemu.c | 2 +- tools/virsh-domain.c | 6 +++--- 12 files changed, 45 insertions(+), 31 deletions(-)
Need to add some XML in order to "test" the longer values - eg, xml2xml and/or xml2args in order to validate that what you read in is what gets printed out and that what you read in gets sent to the hypervisor correctly. <me> wonders what qemu dos with a 20 digit unit number... So speaking of qemu... If you look at qemuBuildDriveStr you will see how the device address is passed down to qemu... That function needs some adjustment too it seems. I didn't dig into the qemu sources yet, but
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 1781996..e7a8e1a 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2827,7 +2827,7 @@ <dd>Drive addresses have the following additional attributes: <code>controller</code> (a 2-digit controller number), <code>bus</code> (a 2-digit bus number), - <code>target</code> (a 2-digit bus number), + <code>target</code> (a 2-digit target number), and <code>unit</code> (a 2-digit unit number on the bus). </dd> <dt><code>type='virtio-serial'</code></dt>
Interesting there's no 'scsi' in here (of course you're removing it below, but that leaves the address as unknown
@@ -3136,7 +3136,7 @@ <hostdev mode='subsystem' type='scsi' sgio='filtered' rawio='yes'> <source> <adapter name='scsi_host0'/> - <address type='scsi' bus='0' target='0' unit='0'/> + <address bus='0' target='0' unit='0'/>
These first two don't seem relevant to the changes below and should have been their own patch. See [1] below for a side comment on this particular changes
</source> <readonly/> <address type='drive' controller='0' bus='0' target='0' unit='0'/> @@ -3244,7 +3244,11 @@ </dd> <dt>scsi</dt> <dd>SCSI devices are described by both the <code>adapter</code> - and <code>address</code> elements. + and <code>address</code> elements. The <code>address</code> + element includes a <code>bus</code> attribute (a 2-digit bus + number), a <code>target</code> attribute (a 10-digit target + number), and a <code>unit</code> attribute (a 20-digit unit + number on the bus). <p> <span class="since">Since 1.2.8</span>, the <code>source</code> element of a SCSI device may contain the <code>protocol</code> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 5dc48f7..dccd3fd 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3846,10 +3846,10 @@ <ref name="driveBus"/> </attribute> <attribute name="target"> - <ref name="driveTarget"/> + <ref name="driveSCSITarget"/> </attribute> <attribute name="unit"> - <ref name="driveUnit"/> + <ref name="driveSCSIUnit"/> </attribute> </define> <define name="usbportaddress"> @@ -5142,11 +5142,21 @@ <param name="pattern">[0-9]{1,2}</param> </data> </define> + <define name="driveSCSITarget"> + <data type="string"> + <param name="pattern">[0-9]{1,10}</param> + </data> + </define> <define name="driveUnit"> <data type="string"> <param name="pattern">[0-9]{1,2}</param> </data> </define> + <define name="driveSCSIUnit"> + <data type="string"> + <param name="pattern">[0-9]{1,20}</param> + </data> + </define> <define name="featureName"> <data type="string"> <param name='pattern'>[a-zA-Z0-9\-_\.]+</param> diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c index 1900039..844297c 100644 --- a/src/conf/domain_audit.c +++ b/src/conf/domain_audit.c @@ -427,7 +427,7 @@ virDomainAuditHostdev(virDomainObjPtr vm, virDomainHostdevDefPtr hostdev, } else { virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host; - if (virAsprintfQuiet(&address, "%s:%d:%d:%d", + if (virAsprintfQuiet(&address, "%s:%d:%d:%lld", scsihostsrc->adapter, scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit) < 0) { diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 36de844..1f2bfca 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4954,7 +4954,7 @@ virDomainHostdevSubsysSCSIHostDefParseXML(xmlNodePtr sourcenode, goto cleanup; }
- if (virStrToLong_ui(unit, NULL, 0, &scsihostsrc->unit) < 0) { + if (virStrToLong_ull(unit, NULL, 0, &scsihostsrc->unit) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot parse unit '%s'"), unit);
Existing - read as "ui" (or now "ull"), but could be "uip" and "ullp" (eg, don't allow reading a negative value). I would be remiss if I didn't point out UINT_MAX is 4294967295U, which is 10 digits, but means 9999999999 which would be 'valid' according to the 10 digit rule in RNG but invalid once you read it in...
goto cleanup; @@ -18844,7 +18844,7 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf, virBufferAsprintf(buf, "<adapter name='%s'/>\n", scsihostsrc->adapter); virBufferAsprintf(buf, - "<address %sbus='%d' target='%d' unit='%d'/>\n", + "<address %sbus='%d' target='%d' unit='%lld'/>\n", includeTypeInAddr ? "type='scsi' " : "",
Printed as "%d" not "%u" (of course that only means something for -1... Without even considering the math, my eyes saw the commit message "unit='1074872354'" and I immediately thought - is this one of those negative value type issues... BTW: I wonder if this deserves a patch prior to this to use _uip and avoid negative values and a second patch to change the formatting of %d to %u. [1] Doesn't this provide an example of the 2nd of the two formatdomain.html.in changes you made? That is you removed having 'scsi' in the "<address...", but this shows that it is printed.... It seems you've covered most of the various areas. It may be easier to review them if they were smaller units. When I started reading this, I figured this wouldn't be too bad, but the more I dug and thought I had more questions. I wanted to provide some feedback before I had to stop reading code for the day/weekend and perhaps see where this goes from here. Maybe I'm reading too much into it. John
scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ba17a8d..f677c2e 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -463,7 +463,7 @@ struct _virDomainHostdevSubsysSCSIHost { char *adapter; unsigned bus; unsigned target; - unsigned unit; + unsigned long long unit; };
typedef struct _virDomainHostdevSubsysSCSIiSCSI virDomainHostdevSubsysSCSIiSCSI; diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 0fc59a8..6e0c3a3 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -63,7 +63,7 @@ struct _qemuBuildCommandLineCallbacks { const char *adapter, unsigned int bus, unsigned int target, - unsigned int unit); + unsigned long long unit); };
extern qemuBuildCommandLineCallbacks buildCommandLineCallbacks; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 3562de6..bbdf0b0 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1938,7 +1938,7 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn, } else { virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host; virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to prepare scsi hostdev: %s:%d:%d:%d"), + _("Unable to prepare scsi hostdev: %s:%d:%d:%lld"), scsihostsrc->adapter, scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit); } @@ -3882,7 +3882,7 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver, virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host; virReportError(VIR_ERR_OPERATION_FAILED, - _("host scsi device %s:%d:%d.%d not found"), + _("host scsi device %s:%d:%d:%lld not found"), scsihostsrc->adapter, scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit); } diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 1c8f31e..e67fe18 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -1482,7 +1482,7 @@ virHostdevReAttachSCSIHostDevices(virHostdevManagerPtr hostdev_mgr, scsihostsrc->adapter, scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit, hostdev->readonly, hostdev->shareable))) { - VIR_WARN("Unable to reattach SCSI device %s:%d:%d:%d on domain %s", + VIR_WARN("Unable to reattach SCSI device %s:%d:%d:%lld on domain %s", scsihostsrc->adapter, scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit, dom_name); return; @@ -1492,7 +1492,7 @@ virHostdevReAttachSCSIHostDevices(virHostdevManagerPtr hostdev_mgr, * because qemuProcessStart could fail half way through. */
if (!(tmp = virSCSIDeviceListFind(hostdev_mgr->activeSCSIHostdevs, scsi))) { - VIR_WARN("Unable to find device %s:%d:%d:%d " + VIR_WARN("Unable to find device %s:%d:%d:%lld " "in list of active SCSI devices", scsihostsrc->adapter, scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit); @@ -1500,7 +1500,7 @@ virHostdevReAttachSCSIHostDevices(virHostdevManagerPtr hostdev_mgr, return; }
- VIR_DEBUG("Removing %s:%d:%d:%d dom=%s from activeSCSIHostdevs", + VIR_DEBUG("Removing %s:%d:%d:%lld dom=%s from activeSCSIHostdevs", scsihostsrc->adapter, scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit, dom_name);
diff --git a/src/util/virscsi.c b/src/util/virscsi.c index 9f5cf0d..5fd4380 100644 --- a/src/util/virscsi.c +++ b/src/util/virscsi.c @@ -56,7 +56,7 @@ struct _virSCSIDevice { unsigned int adapter; unsigned int bus; unsigned int target; - unsigned int unit; + unsigned long long unit;
char *name; /* adapter:bus:target:unit */ char *id; /* model:vendor */ @@ -110,7 +110,7 @@ virSCSIDeviceGetSgName(const char *sysfs_prefix, const char *adapter, unsigned int bus, unsigned int target, - unsigned int unit) + unsigned long long unit) { DIR *dir = NULL; struct dirent *entry; @@ -123,7 +123,7 @@ virSCSIDeviceGetSgName(const char *sysfs_prefix, return NULL;
if (virAsprintf(&path, - "%s/%d:%d:%d:%d/scsi_generic", + "%s/%d:%d:%d:%lld/scsi_generic", prefix, adapter_id, bus, target, unit) < 0) return NULL;
@@ -157,7 +157,7 @@ virSCSIDeviceGetDevName(const char *sysfs_prefix, const char *adapter, unsigned int bus, unsigned int target, - unsigned int unit) + unsigned long long unit) { DIR *dir = NULL; struct dirent *entry; @@ -170,7 +170,7 @@ virSCSIDeviceGetDevName(const char *sysfs_prefix, return NULL;
if (virAsprintf(&path, - "%s/%d:%d:%d:%d/block", + "%s/%d:%d:%d:%lld/block", prefix, adapter_id, bus, target, unit) < 0) return NULL;
@@ -200,7 +200,7 @@ virSCSIDeviceNew(const char *sysfs_prefix, const char *adapter, unsigned int bus, unsigned int target, - unsigned int unit, + unsigned long long unit, bool readonly, bool shareable) { @@ -227,7 +227,7 @@ virSCSIDeviceNew(const char *sysfs_prefix, if (virSCSIDeviceGetAdapterId(adapter, &dev->adapter) < 0) goto cleanup;
- if (virAsprintf(&dev->name, "%d:%d:%d:%d", dev->adapter, + if (virAsprintf(&dev->name, "%d:%d:%d:%lld", dev->adapter, dev->bus, dev->target, dev->unit) < 0 || virAsprintf(&dev->sg_path, "%s/%s", sysfs_prefix ? sysfs_prefix : "/dev", sg) < 0) @@ -347,7 +347,7 @@ virSCSIDeviceGetTarget(virSCSIDevicePtr dev) return dev->target; }
-unsigned int +unsigned long long virSCSIDeviceGetUnit(virSCSIDevicePtr dev) { return dev->unit; diff --git a/src/util/virscsi.h b/src/util/virscsi.h index c67837f..df40d7f 100644 --- a/src/util/virscsi.h +++ b/src/util/virscsi.h @@ -37,18 +37,18 @@ char *virSCSIDeviceGetSgName(const char *sysfs_prefix, const char *adapter, unsigned int bus, unsigned int target, - unsigned int unit); + unsigned long long unit); char *virSCSIDeviceGetDevName(const char *sysfs_prefix, const char *adapter, unsigned int bus, unsigned int target, - unsigned int unit); + unsigned long long unit);
virSCSIDevicePtr virSCSIDeviceNew(const char *sysfs_prefix, const char *adapter, unsigned int bus, unsigned int target, - unsigned int unit, + unsigned long long unit, bool readonly, bool shareable);
@@ -61,7 +61,7 @@ const char *virSCSIDeviceGetName(virSCSIDevicePtr dev); unsigned int virSCSIDeviceGetAdapter(virSCSIDevicePtr dev); unsigned int virSCSIDeviceGetBus(virSCSIDevicePtr dev); unsigned int virSCSIDeviceGetTarget(virSCSIDevicePtr dev); -unsigned int virSCSIDeviceGetUnit(virSCSIDevicePtr dev); +unsigned long long virSCSIDeviceGetUnit(virSCSIDevicePtr dev); bool virSCSIDeviceGetReadonly(virSCSIDevicePtr dev); bool virSCSIDeviceGetShareable(virSCSIDevicePtr dev);
diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index d067bca..ceaabb6 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -462,7 +462,7 @@ testSCSIDeviceGetSgName(const char *sysfs_prefix ATTRIBUTE_UNUSED, const char *adapter ATTRIBUTE_UNUSED, unsigned int bus ATTRIBUTE_UNUSED, unsigned int target ATTRIBUTE_UNUSED, - unsigned int unit ATTRIBUTE_UNUSED) + unsigned long long unit ATTRIBUTE_UNUSED) { char *sg = NULL;
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 4c47473..aa4d58c 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -415,7 +415,7 @@ struct PCIAddress { struct SCSIAddress { unsigned int controller; unsigned int bus; - unsigned int unit; + unsigned long long unit; };
struct IDEAddress { @@ -488,7 +488,7 @@ static int str2SCSIAddress(const char *str, struct SCSIAddress *scsiAddr) return -1;
unit++; - if (virStrToLong_ui(unit, NULL, 0, &scsiAddr->unit) != 0) + if (virStrToLong_ull(unit, NULL, 0, &scsiAddr->unit) != 0) return -1;
return 0; @@ -725,7 +725,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) if (diskAddr.type == DISK_ADDR_TYPE_SCSI) { virBufferAsprintf(&buf, "<address type='drive' controller='%d'" - " bus='%d' unit='%d' />\n", + " bus='%d' unit='%lld' />\n", diskAddr.addr.scsi.controller, diskAddr.addr.scsi.bus, diskAddr.addr.scsi.unit); } else {
On 06/12/2015 04:49 PM, John Ferlan wrote:
On 06/10/2015 11:22 AM, Eric Farman wrote:
Defining a domain with a SCSI disk attached via a hostdev tag and a source address unit value longer than two digits causes an error when editing the domain with virsh edit, even if no changes are made to the domain definition. The error suggests invalid XML, somewhere:
# virsh edit lmb_guest error: XML document failed to validate against schema: Unable to validate doc against /usr/local/share/libvirt/schemas/domain.rng Extra element devices in interleave Element domain failed to validate content
The virt-xml-validate tool fails with a similar error:
# virt-xml-validate lmb_guest.xml Relax-NG validity error : Extra element devices in interleave lmb_guest.xml:17: element devices: Relax-NG validity error : Element domain failed to validate content lmb_guest.xml fails to validate
The hostdev tag requires a source address to be specified, which includes bus, target, and unit address attributes. According to the SCSI Architecture Model spec (section 4.9 of SAM-2), a LUN address is 64 bits and thus could be up to 20 decimal digits long. Unfortunately, the XML schema limits this string to just two digits. Similarly, the target field can be up to 32 bits in length, which would be 10 decimal digits.
# lsscsi -xx [0:0:19:0x4022401100000000] disk IBM 2107900 3.44 /dev/sda # lsscsi [0:0:19:1074872354]disk IBM 2107900 3.44 /dev/sda # cat lmb_guest.xml <domain type='kvm'> <name>lmb_guest</name> <memory unit='MiB'>1024</memory> ...trimmed... <devices> <controller type='scsi' model='virtio-scsi' index='0'/> <hostdev mode='subsystem' type='scsi'> <source> <adapter name='scsi_host0'/> <address bus='0' target='19' unit='1074872354'/> </source> </hostdev> ...trimmed...
Since the reference unit and target fields are used in several places in the XML schema, create a separate one specific for SCSI Logical Units that will permit the greater length. Also, expand the definition of the SCSI unit field from an int to a long long, to cover the possible size. This permits both the validation utility and the virsh edit command to succeed when a hostdev tag is included.
Nice explanation - helped set the stage quite well!
Of course you've just opened Pandora's box of questions...
Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com> Reviewed-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- docs/formatdomain.html.in | 10 +++++++--- docs/schemas/domaincommon.rng | 14 ++++++++++++-- src/conf/domain_audit.c | 2 +- src/conf/domain_conf.c | 4 ++-- src/conf/domain_conf.h | 2 +- src/qemu/qemu_command.h | 2 +- src/qemu/qemu_hotplug.c | 4 ++-- src/util/virhostdev.c | 6 +++--- src/util/virscsi.c | 16 ++++++++-------- src/util/virscsi.h | 8 ++++---- tests/testutilsqemu.c | 2 +- tools/virsh-domain.c | 6 +++--- 12 files changed, 45 insertions(+), 31 deletions(-)
Need to add some XML in order to "test" the longer values - eg, xml2xml and/or xml2args in order to validate that what you read in is what gets printed out and that what you read in gets sent to the hypervisor correctly.
I wrestled with this part, because the xml2args tests pass the SCSI [host:bus:target:unit] address to testSCSIDeviceGetSgName, which ignores all inputs and returns /dev/sg0. The reverse had a similar gotcha. I'll dig at xml2xml, and see what I can come up with.
<me> wonders what qemu dos with a 20 digit unit number...
So speaking of qemu... If you look at qemuBuildDriveStr you will see how the device address is passed down to qemu... That function needs some adjustment too it seems.
This function doesn't appear to be invoked for an <address> tag within a <hostdev> element, either as part of the <source> subelements or as the address being created for the guest. Rather, it seems to turn up as part of a <disk> element, which (unless I'm mistaken) relies on the host "/dev" address instead of [host:bus:target:unit]. Speaking of the address that is created in the guest, though, I left that as-is because virtio defines LUN addresses up to 16,384 and is enforced in KVM/QEMU. Since I can't create a unit address in the guest larger than that, leaving that defined as an int is okay.
I didn't dig into the qemu sources yet, but
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 1781996..e7a8e1a 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2827,7 +2827,7 @@ <dd>Drive addresses have the following additional attributes: <code>controller</code> (a 2-digit controller number), <code>bus</code> (a 2-digit bus number), - <code>target</code> (a 2-digit bus number), + <code>target</code> (a 2-digit target number), and <code>unit</code> (a 2-digit unit number on the bus). </dd> <dt><code>type='virtio-serial'</code></dt> Interesting there's no 'scsi' in here (of course you're removing it below, but that leaves the address as unknown
Is it? See below, but I thought that got hardcoded somewhere because it's in a hostdev. I'll doublecheck.
@@ -3136,7 +3136,7 @@ <hostdev mode='subsystem' type='scsi' sgio='filtered' rawio='yes'> <source> <adapter name='scsi_host0'/> - <address type='scsi' bus='0' target='0' unit='0'/> + <address bus='0' target='0' unit='0'/> These first two don't seem relevant to the changes below and should have been their own patch.
Fair enough, my apologies. I'll break them out.
See [1] below for a side comment on this particular changes
</source> <readonly/> <address type='drive' controller='0' bus='0' target='0' unit='0'/> @@ -3244,7 +3244,11 @@ </dd> <dt>scsi</dt> <dd>SCSI devices are described by both the <code>adapter</code> - and <code>address</code> elements. + and <code>address</code> elements. The <code>address</code> + element includes a <code>bus</code> attribute (a 2-digit bus + number), a <code>target</code> attribute (a 10-digit target + number), and a <code>unit</code> attribute (a 20-digit unit + number on the bus). <p> <span class="since">Since 1.2.8</span>, the <code>source</code> element of a SCSI device may contain the <code>protocol</code> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 5dc48f7..dccd3fd 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3846,10 +3846,10 @@ <ref name="driveBus"/> </attribute> <attribute name="target"> - <ref name="driveTarget"/> + <ref name="driveSCSITarget"/> </attribute> <attribute name="unit"> - <ref name="driveUnit"/> + <ref name="driveSCSIUnit"/> </attribute> </define> <define name="usbportaddress"> @@ -5142,11 +5142,21 @@ <param name="pattern">[0-9]{1,2}</param> </data> </define> + <define name="driveSCSITarget"> + <data type="string"> + <param name="pattern">[0-9]{1,10}</param> + </data> + </define> <define name="driveUnit"> <data type="string"> <param name="pattern">[0-9]{1,2}</param> </data> </define> + <define name="driveSCSIUnit"> + <data type="string"> + <param name="pattern">[0-9]{1,20}</param> + </data> + </define> <define name="featureName"> <data type="string"> <param name='pattern'>[a-zA-Z0-9\-_\.]+</param> diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c index 1900039..844297c 100644 --- a/src/conf/domain_audit.c +++ b/src/conf/domain_audit.c @@ -427,7 +427,7 @@ virDomainAuditHostdev(virDomainObjPtr vm, virDomainHostdevDefPtr hostdev, } else { virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host; - if (virAsprintfQuiet(&address, "%s:%d:%d:%d", + if (virAsprintfQuiet(&address, "%s:%d:%d:%lld", scsihostsrc->adapter, scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit) < 0) { diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 36de844..1f2bfca 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4954,7 +4954,7 @@ virDomainHostdevSubsysSCSIHostDefParseXML(xmlNodePtr sourcenode, goto cleanup; }
- if (virStrToLong_ui(unit, NULL, 0, &scsihostsrc->unit) < 0) { + if (virStrToLong_ull(unit, NULL, 0, &scsihostsrc->unit) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot parse unit '%s'"), unit);
Existing - read as "ui" (or now "ull"), but could be "uip" and "ullp" (eg, don't allow reading a negative value).
I would be remiss if I didn't point out UINT_MAX is 4294967295U, which is 10 digits, but means 9999999999 which would be 'valid' according to the 10 digit rule in RNG but invalid once you read it in...
goto cleanup; @@ -18844,7 +18844,7 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf, virBufferAsprintf(buf, "<adapter name='%s'/>\n", scsihostsrc->adapter); virBufferAsprintf(buf, - "<address %sbus='%d' target='%d' unit='%d'/>\n", + "<address %sbus='%d' target='%d' unit='%lld'/>\n", includeTypeInAddr ? "type='scsi' " : "",
Printed as "%d" not "%u" (of course that only means something for -1... Without even considering the math, my eyes saw the commit message "unit='1074872354'" and I immediately thought - is this one of those negative value type issues...
BTW: I wonder if this deserves a patch prior to this to use _uip and avoid negative values and a second patch to change the formatting of %d to %u.
I like this idea. Will work through it.
[1] Doesn't this provide an example of the 2nd of the two formatdomain.html.in changes you made? That is you removed having 'scsi' in the "<address...", but this shows that it is printed....
I see three places that call virDomainHostdevDefFormatSubsys, but the one where a <hostdev mode='subsystem'> is handled makes the call with includeTypeInAddr set to false. The other two callers handle network definitions and make the call with it set to true, but those won't drop into this section of code. This is why I pulled the type='scsi' from the doc above, because the existing code doesn't allow a type='scsi' to be specified. The routine virDomainHostdevSubsysSCSIHostDefParseXML only parses bus, target, and unit parameters in its address tag.
It seems you've covered most of the various areas. It may be easier to review them if they were smaller units.
Just making sure that "units" here is in reference to breaking up this patch for the ui->uip and %d->%u changes, and not the "logical unit" in the commit message?
When I started reading this, I figured this wouldn't be too bad, but the more I dug and thought I had more questions.
I wanted to provide some feedback before I had to stop reading code for the day/weekend and perhaps see where this goes from here. Maybe I'm reading too much into it.
Much appreciated, thank you!
John
scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ba17a8d..f677c2e 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -463,7 +463,7 @@ struct _virDomainHostdevSubsysSCSIHost { char *adapter; unsigned bus; unsigned target; - unsigned unit; + unsigned long long unit; };
typedef struct _virDomainHostdevSubsysSCSIiSCSI virDomainHostdevSubsysSCSIiSCSI; diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 0fc59a8..6e0c3a3 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -63,7 +63,7 @@ struct _qemuBuildCommandLineCallbacks { const char *adapter, unsigned int bus, unsigned int target, - unsigned int unit); + unsigned long long unit); };
extern qemuBuildCommandLineCallbacks buildCommandLineCallbacks; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 3562de6..bbdf0b0 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1938,7 +1938,7 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn, } else { virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host; virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to prepare scsi hostdev: %s:%d:%d:%d"), + _("Unable to prepare scsi hostdev: %s:%d:%d:%lld"), scsihostsrc->adapter, scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit); } @@ -3882,7 +3882,7 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver, virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host; virReportError(VIR_ERR_OPERATION_FAILED, - _("host scsi device %s:%d:%d.%d not found"), + _("host scsi device %s:%d:%d:%lld not found"), scsihostsrc->adapter, scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit); } diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 1c8f31e..e67fe18 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -1482,7 +1482,7 @@ virHostdevReAttachSCSIHostDevices(virHostdevManagerPtr hostdev_mgr, scsihostsrc->adapter, scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit, hostdev->readonly, hostdev->shareable))) { - VIR_WARN("Unable to reattach SCSI device %s:%d:%d:%d on domain %s", + VIR_WARN("Unable to reattach SCSI device %s:%d:%d:%lld on domain %s", scsihostsrc->adapter, scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit, dom_name); return; @@ -1492,7 +1492,7 @@ virHostdevReAttachSCSIHostDevices(virHostdevManagerPtr hostdev_mgr, * because qemuProcessStart could fail half way through. */
if (!(tmp = virSCSIDeviceListFind(hostdev_mgr->activeSCSIHostdevs, scsi))) { - VIR_WARN("Unable to find device %s:%d:%d:%d " + VIR_WARN("Unable to find device %s:%d:%d:%lld " "in list of active SCSI devices", scsihostsrc->adapter, scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit); @@ -1500,7 +1500,7 @@ virHostdevReAttachSCSIHostDevices(virHostdevManagerPtr hostdev_mgr, return; }
- VIR_DEBUG("Removing %s:%d:%d:%d dom=%s from activeSCSIHostdevs", + VIR_DEBUG("Removing %s:%d:%d:%lld dom=%s from activeSCSIHostdevs", scsihostsrc->adapter, scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit, dom_name);
diff --git a/src/util/virscsi.c b/src/util/virscsi.c index 9f5cf0d..5fd4380 100644 --- a/src/util/virscsi.c +++ b/src/util/virscsi.c @@ -56,7 +56,7 @@ struct _virSCSIDevice { unsigned int adapter; unsigned int bus; unsigned int target; - unsigned int unit; + unsigned long long unit;
char *name; /* adapter:bus:target:unit */ char *id; /* model:vendor */ @@ -110,7 +110,7 @@ virSCSIDeviceGetSgName(const char *sysfs_prefix, const char *adapter, unsigned int bus, unsigned int target, - unsigned int unit) + unsigned long long unit) { DIR *dir = NULL; struct dirent *entry; @@ -123,7 +123,7 @@ virSCSIDeviceGetSgName(const char *sysfs_prefix, return NULL;
if (virAsprintf(&path, - "%s/%d:%d:%d:%d/scsi_generic", + "%s/%d:%d:%d:%lld/scsi_generic", prefix, adapter_id, bus, target, unit) < 0) return NULL;
@@ -157,7 +157,7 @@ virSCSIDeviceGetDevName(const char *sysfs_prefix, const char *adapter, unsigned int bus, unsigned int target, - unsigned int unit) + unsigned long long unit) { DIR *dir = NULL; struct dirent *entry; @@ -170,7 +170,7 @@ virSCSIDeviceGetDevName(const char *sysfs_prefix, return NULL;
if (virAsprintf(&path, - "%s/%d:%d:%d:%d/block", + "%s/%d:%d:%d:%lld/block", prefix, adapter_id, bus, target, unit) < 0) return NULL;
@@ -200,7 +200,7 @@ virSCSIDeviceNew(const char *sysfs_prefix, const char *adapter, unsigned int bus, unsigned int target, - unsigned int unit, + unsigned long long unit, bool readonly, bool shareable) { @@ -227,7 +227,7 @@ virSCSIDeviceNew(const char *sysfs_prefix, if (virSCSIDeviceGetAdapterId(adapter, &dev->adapter) < 0) goto cleanup;
- if (virAsprintf(&dev->name, "%d:%d:%d:%d", dev->adapter, + if (virAsprintf(&dev->name, "%d:%d:%d:%lld", dev->adapter, dev->bus, dev->target, dev->unit) < 0 || virAsprintf(&dev->sg_path, "%s/%s", sysfs_prefix ? sysfs_prefix : "/dev", sg) < 0) @@ -347,7 +347,7 @@ virSCSIDeviceGetTarget(virSCSIDevicePtr dev) return dev->target; }
-unsigned int +unsigned long long virSCSIDeviceGetUnit(virSCSIDevicePtr dev) { return dev->unit; diff --git a/src/util/virscsi.h b/src/util/virscsi.h index c67837f..df40d7f 100644 --- a/src/util/virscsi.h +++ b/src/util/virscsi.h @@ -37,18 +37,18 @@ char *virSCSIDeviceGetSgName(const char *sysfs_prefix, const char *adapter, unsigned int bus, unsigned int target, - unsigned int unit); + unsigned long long unit); char *virSCSIDeviceGetDevName(const char *sysfs_prefix, const char *adapter, unsigned int bus, unsigned int target, - unsigned int unit); + unsigned long long unit);
virSCSIDevicePtr virSCSIDeviceNew(const char *sysfs_prefix, const char *adapter, unsigned int bus, unsigned int target, - unsigned int unit, + unsigned long long unit, bool readonly, bool shareable);
@@ -61,7 +61,7 @@ const char *virSCSIDeviceGetName(virSCSIDevicePtr dev); unsigned int virSCSIDeviceGetAdapter(virSCSIDevicePtr dev); unsigned int virSCSIDeviceGetBus(virSCSIDevicePtr dev); unsigned int virSCSIDeviceGetTarget(virSCSIDevicePtr dev); -unsigned int virSCSIDeviceGetUnit(virSCSIDevicePtr dev); +unsigned long long virSCSIDeviceGetUnit(virSCSIDevicePtr dev); bool virSCSIDeviceGetReadonly(virSCSIDevicePtr dev); bool virSCSIDeviceGetShareable(virSCSIDevicePtr dev);
diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index d067bca..ceaabb6 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -462,7 +462,7 @@ testSCSIDeviceGetSgName(const char *sysfs_prefix ATTRIBUTE_UNUSED, const char *adapter ATTRIBUTE_UNUSED, unsigned int bus ATTRIBUTE_UNUSED, unsigned int target ATTRIBUTE_UNUSED, - unsigned int unit ATTRIBUTE_UNUSED) + unsigned long long unit ATTRIBUTE_UNUSED) { char *sg = NULL;
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 4c47473..aa4d58c 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -415,7 +415,7 @@ struct PCIAddress { struct SCSIAddress { unsigned int controller; unsigned int bus; - unsigned int unit; + unsigned long long unit; };
struct IDEAddress { @@ -488,7 +488,7 @@ static int str2SCSIAddress(const char *str, struct SCSIAddress *scsiAddr) return -1;
unit++; - if (virStrToLong_ui(unit, NULL, 0, &scsiAddr->unit) != 0) + if (virStrToLong_ull(unit, NULL, 0, &scsiAddr->unit) != 0) return -1;
return 0; @@ -725,7 +725,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) if (diskAddr.type == DISK_ADDR_TYPE_SCSI) { virBufferAsprintf(&buf, "<address type='drive' controller='%d'" - " bus='%d' unit='%d' />\n", + " bus='%d' unit='%lld' />\n", diskAddr.addr.scsi.controller, diskAddr.addr.scsi.bus, diskAddr.addr.scsi.unit); } else {
Something I didn't catch on pass1, but perhaps an important distinction title: Resolve issues with SCSI LUN hostdev device addressing Coincidentally - there's a bz regarding the <address> setting between SCSI <disk> and <hostdev> which is assigned to me and was near the top of my todo list... https://bugzilla.redhat.com/show_bug.cgi?id=1210587 so this has helped put me in the right frame of reference! ...
Need to add some XML in order to "test" the longer values - eg, xml2xml and/or xml2args in order to validate that what you read in is what gets printed out and that what you read in gets sent to the hypervisor correctly.
I wrestled with this part, because the xml2args tests pass the SCSI [host:bus:target:unit] address to testSCSIDeviceGetSgName, which ignores all inputs and returns /dev/sg0. The reverse had a similar gotcha. I'll dig at xml2xml, and see what I can come up with.
<me> wonders what qemu dos with a 20 digit unit number...
So speaking of qemu... If you look at qemuBuildDriveStr you will see how the device address is passed down to qemu... That function needs some adjustment too it seems.
This function doesn't appear to be invoked for an <address> tag within a <hostdev> element, either as part of the <source> subelements or as the address being created for the guest. Rather, it seems to turn up as part of a <disk> element, which (unless I'm mistaken) relies on the host "/dev" address instead of [host:bus:target:unit].
Ah... so back to my title comment ;-) - hostdev... That would mean qemuBuildSCSIHostdevDevStr, which does have a printing of bus, target, unit. Still wondering about how qemu would handle it (haven't got that far yet), but that seems to be what you point out in the next paragraph.
Speaking of the address that is created in the guest, though, I left that as-is because virtio defines LUN addresses up to 16,384 and is enforced in KVM/QEMU. Since I can't create a unit address in the guest larger than that, leaving that defined as an int is okay.
And so regardless of what you did for libvirt, kvm/qemu wouldn't be able to handle it? Perhaps then that needs to be checked somehow... Is this something qemu/kvm needs to support? Again I haven't looked at those sources yet. Is there a specific hypervisor that isn't working because libvirt isn't passing the correct address value? This is the "what is the bug you're trying to fix" type question...
I didn't dig into the qemu sources yet, but
Looks like I also lost my train of thought here ;-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 1781996..e7a8e1a 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2827,7 +2827,7 @@ <dd>Drive addresses have the following additional attributes: <code>controller</code> (a 2-digit controller number), <code>bus</code> (a 2-digit bus number), - <code>target</code> (a 2-digit bus number), + <code>target</code> (a 2-digit target number), and <code>unit</code> (a 2-digit unit number on the bus). </dd> <dt><code>type='virtio-serial'</code></dt> Interesting there's no 'scsi' in here (of course you're removing it below, but that leaves the address as unknown
Is it? See below, but I thought that got hardcoded somewhere because it's in a hostdev. I'll doublecheck.
It seems virDomainHostdevSubsysSCSIHostDefParseXML ignores the type field and the RNG supports that.
@@ -3136,7 +3136,7 @@ <hostdev mode='subsystem' type='scsi' sgio='filtered' rawio='yes'> <source> <adapter name='scsi_host0'/> - <address type='scsi' bus='0' target='0' unit='0'/> + <address bus='0' target='0' unit='0'/> These first two don't seem relevant to the changes below and should have been their own patch.
Fair enough, my apologies. I'll break them out.
We all fall into the same trap at some point in time.... ;-)
See [1] below for a side comment on this particular changes
...
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 36de844..1f2bfca 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4954,7 +4954,7 @@ virDomainHostdevSubsysSCSIHostDefParseXML(xmlNodePtr sourcenode, goto cleanup; } - if (virStrToLong_ui(unit, NULL, 0, &scsihostsrc->unit) < 0) { + if (virStrToLong_ull(unit, NULL, 0, &scsihostsrc->unit) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot parse unit '%s'"), unit); Existing - read as "ui" (or now "ull"), but could be "uip" and "ullp" (eg, don't allow reading a negative value).
I would be remiss if I didn't point out UINT_MAX is 4294967295U, which is 10 digits, but means 9999999999 which would be 'valid' according to the 10 digit rule in RNG but invalid once you read it in...
goto cleanup; @@ -18844,7 +18844,7 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf, virBufferAsprintf(buf, "<adapter name='%s'/>\n", scsihostsrc->adapter); virBufferAsprintf(buf, - "<address %sbus='%d' target='%d' unit='%d'/>\n", + "<address %sbus='%d' target='%d' unit='%lld'/>\n", includeTypeInAddr ? "type='scsi' " : "",
Printed as "%d" not "%u" (of course that only means something for -1... Without even considering the math, my eyes saw the commit message "unit='1074872354'" and I immediately thought - is this one of those negative value type issues...
BTW: I wonder if this deserves a patch prior to this to use _uip and avoid negative values and a second patch to change the formatting of %d to %u.
I like this idea. Will work through it.
[1] Doesn't this provide an example of the 2nd of the two formatdomain.html.in changes you made? That is you removed having 'scsi' in the "<address...", but this shows that it is printed....
I see three places that call virDomainHostdevDefFormatSubsys, but the one where a <hostdev mode='subsystem'> is handled makes the call with includeTypeInAddr set to false. The other two callers handle network definitions and make the call with it set to true, but those won't drop into this section of code.
This is why I pulled the type='scsi' from the doc above, because the existing code doesn't allow a type='scsi' to be specified. The routine virDomainHostdevSubsysSCSIHostDefParseXML only parses bus, target, and unit parameters in its address tag.
yep - you are correct... furthermore for the "<source..." processing for non iSCSI devices. So we're getting more and more specific.
It seems you've covered most of the various areas. It may be easier to review them if they were smaller units.
Just making sure that "units" here is in reference to breaking up this patch for the ui->uip and %d->%u changes, and not the "logical unit" in the commit message?
This patch may be tough to do it, but perhaps a general comment about if it's possible to make smaller chunks of changes, in the long run it makes it easier to apply and make progress while perhaps working out the "fine details". Of course changing a data type representation causes a ripple effect throughout the code and probably isn't possible. I certainly think the uip changes will help... followed perhaps by just changes to print %u instead of %d... then followed by the change from %u to %llu... It's a natural progression of changes that can be built and checked. You may end up changing the same file twice in followup patches, but seeing how you get from point A to point B is helpful as is providing tests. John
On 06/15/2015 05:03 PM, John Ferlan wrote:
Something I didn't catch on pass1, but perhaps an important distinction
title:
Resolve issues with SCSI LUN hostdev device addressing
Sounds good! I'll work that into the split patches.
Coincidentally - there's a bz regarding the <address> setting between SCSI <disk> and <hostdev> which is assigned to me and was near the top of my todo list... https://bugzilla.redhat.com/show_bug.cgi?id=1210587 so this has helped put me in the right frame of reference!
...
Need to add some XML in order to "test" the longer values - eg, xml2xml and/or xml2args in order to validate that what you read in is what gets printed out and that what you read in gets sent to the hypervisor correctly. I wrestled with this part, because the xml2args tests pass the SCSI [host:bus:target:unit] address to testSCSIDeviceGetSgName, which ignores all inputs and returns /dev/sg0. The reverse had a similar gotcha. I'll dig at xml2xml, and see what I can come up with.
<me> wonders what qemu dos with a 20 digit unit number...
So speaking of qemu... If you look at qemuBuildDriveStr you will see how the device address is passed down to qemu... That function needs some adjustment too it seems.
This function doesn't appear to be invoked for an <address> tag within a <hostdev> element, either as part of the <source> subelements or as the address being created for the guest. Rather, it seems to turn up as part of a <disk> element, which (unless I'm mistaken) relies on the host "/dev" address instead of [host:bus:target:unit]. Ah... so back to my title comment ;-) - hostdev... That would mean qemuBuildSCSIHostdevDevStr, which does have a printing of bus, target, unit. Still wondering about how qemu would handle it (haven't got that far yet), but that seems to be what you point out in the next paragraph.
Speaking of the address that is created in the guest, though, I left that as-is because virtio defines LUN addresses up to 16,384 and is enforced in KVM/QEMU. Since I can't create a unit address in the guest larger than that, leaving that defined as an int is okay.
And so regardless of what you did for libvirt, kvm/qemu wouldn't be able to handle it? Perhaps then that needs to be checked somehow... Is this something qemu/kvm needs to support?
I'll defer to you, but kvm/qemu enforces things okay and libvirt fails gracefully. Example: # cat disk.xml <hostdev mode='subsystem' type='scsi'> <source> <adapter name='scsi_host0'/> <address bus='0' target='14' unit='1074872354'/> </source> <address type='drive' controller='0' bus='0' target='0' unit='16384'/> </hostdev> # virsh attach-device lmb_guest disk.xml error: Failed to attach device from disk.xml error: internal error: unable to execute QEMU command 'device_add': bad scsi device lun: 16384 # vim disk.xml # cat disk.xml <hostdev mode='subsystem' type='scsi'> <source> <adapter name='scsi_host0'/> <address bus='0' target='14' unit='1074872354'/> </source> <address type='drive' controller='0' bus='0' target='0' unit='16383'/> </hostdev> # virsh attach-device lmb_guest disk.xml Device attached successfully This looks reasonable to me because virtio only permits 256 targets, and 16,384 units per targets. So there's no concern with the corresponding "int" fields being overrun.
Again I haven't looked at those sources yet. Is there a specific hypervisor that isn't working because libvirt isn't passing the correct address value? This is the "what is the bug you're trying to fix" type question...
I'm using qemu/kvm, but I haven't seen anything besides the "virsh edit" and "virt-xml-validate" errors when the hostdev->source->address->unit value is larger than two digits. But this really boils to your later comment about the patch being split, to explain the sequence of events that brought me here. I have it broken into the patches you had suggested, plus two others (one doc, one xml schema which is what started this), but haven't tested them independently yet. Tomorrow, I hope. Eric
I didn't dig into the qemu sources yet, but
Looks like I also lost my train of thought here ;-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 1781996..e7a8e1a 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2827,7 +2827,7 @@ <dd>Drive addresses have the following additional attributes: <code>controller</code> (a 2-digit controller number), <code>bus</code> (a 2-digit bus number), - <code>target</code> (a 2-digit bus number), + <code>target</code> (a 2-digit target number), and <code>unit</code> (a 2-digit unit number on the bus). </dd> <dt><code>type='virtio-serial'</code></dt> Interesting there's no 'scsi' in here (of course you're removing it below, but that leaves the address as unknown Is it? See below, but I thought that got hardcoded somewhere because it's in a hostdev. I'll doublecheck.
It seems virDomainHostdevSubsysSCSIHostDefParseXML ignores the type field and the RNG supports that.
@@ -3136,7 +3136,7 @@ <hostdev mode='subsystem' type='scsi' sgio='filtered' rawio='yes'> <source> <adapter name='scsi_host0'/> - <address type='scsi' bus='0' target='0' unit='0'/> + <address bus='0' target='0' unit='0'/> These first two don't seem relevant to the changes below and should have been their own patch.
Fair enough, my apologies. I'll break them out.
We all fall into the same trap at some point in time.... ;-)
See [1] below for a side comment on this particular changes
...
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 36de844..1f2bfca 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4954,7 +4954,7 @@ virDomainHostdevSubsysSCSIHostDefParseXML(xmlNodePtr sourcenode, goto cleanup; } - if (virStrToLong_ui(unit, NULL, 0, &scsihostsrc->unit) < 0) { + if (virStrToLong_ull(unit, NULL, 0, &scsihostsrc->unit) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot parse unit '%s'"), unit); Existing - read as "ui" (or now "ull"), but could be "uip" and "ullp" (eg, don't allow reading a negative value).
I would be remiss if I didn't point out UINT_MAX is 4294967295U, which is 10 digits, but means 9999999999 which would be 'valid' according to the 10 digit rule in RNG but invalid once you read it in...
goto cleanup; @@ -18844,7 +18844,7 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf, virBufferAsprintf(buf, "<adapter name='%s'/>\n", scsihostsrc->adapter); virBufferAsprintf(buf, - "<address %sbus='%d' target='%d' unit='%d'/>\n", + "<address %sbus='%d' target='%d' unit='%lld'/>\n", includeTypeInAddr ? "type='scsi' " : "",
Printed as "%d" not "%u" (of course that only means something for -1... Without even considering the math, my eyes saw the commit message "unit='1074872354'" and I immediately thought - is this one of those negative value type issues...
BTW: I wonder if this deserves a patch prior to this to use _uip and avoid negative values and a second patch to change the formatting of %d to %u. I like this idea. Will work through it.
[1] Doesn't this provide an example of the 2nd of the two formatdomain.html.in changes you made? That is you removed having 'scsi' in the "<address...", but this shows that it is printed.... I see three places that call virDomainHostdevDefFormatSubsys, but the one where a <hostdev mode='subsystem'> is handled makes the call with includeTypeInAddr set to false. The other two callers handle network definitions and make the call with it set to true, but those won't drop into this section of code.
This is why I pulled the type='scsi' from the doc above, because the existing code doesn't allow a type='scsi' to be specified. The routine virDomainHostdevSubsysSCSIHostDefParseXML only parses bus, target, and unit parameters in its address tag.
yep - you are correct... furthermore for the "<source..." processing for non iSCSI devices. So we're getting more and more specific.
It seems you've covered most of the various areas. It may be easier to review them if they were smaller units. Just making sure that "units" here is in reference to breaking up this patch for the ui->uip and %d->%u changes, and not the "logical unit" in the commit message?
This patch may be tough to do it, but perhaps a general comment about if it's possible to make smaller chunks of changes, in the long run it makes it easier to apply and make progress while perhaps working out the "fine details". Of course changing a data type representation causes a ripple effect throughout the code and probably isn't possible.
I certainly think the uip changes will help... followed perhaps by just changes to print %u instead of %d... then followed by the change from %u to %llu... It's a natural progression of changes that can be built and checked. You may end up changing the same file twice in followup patches, but seeing how you get from point A to point B is helpful as is providing tests.
John
participants (2)
-
Eric Farman -
John Ferlan