[libvirt] [PATCH 0/5 v2] Corrections to SCSI logical unit handling
While working with the hostdev tag and SCSI LUNs, a problem was discovered with the XML schema (see commit message in patch 4). This spawned some further corrections to the handling of the logical unit field throughout libvirt. This series was split from a single patch, from this feedback: http://www.redhat.com/archives/libvir-list/2015-June/msg00489.html Eric Farman (5): Print SCSI logical unit as a positive integer Print SCSI logical unit as unsigned integer Convert SCSI logical unit from int to long long docs: Fix XML schema handling of LUN address in hostdev tag docs: Correct typos in scsi hostdev and address elements 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(-) -- 1.9.1
A logical unit address is a positive integer, so it would be wise to reject any negative inputs. Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com> --- src/conf/domain_conf.c | 2 +- tools/virsh-domain.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ca55981..9e77b87 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_uip(unit, NULL, 0, &scsihostsrc->unit) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot parse unit '%s'"), unit); goto cleanup; diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 4c47473..0bea462 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -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_uip(unit, NULL, 0, &scsiAddr->unit) != 0) return -1; return 0; -- 1.9.1
On 06/16/2015 11:29 PM, Eric Farman wrote:
A logical unit address is a positive integer, so it would be wise to reject any negative inputs.
Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com> --- src/conf/domain_conf.c | 2 +- tools/virsh-domain.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
While we're at it - why not bus and target as well? That's doable in one patch... I'll adjust this in order Looking back through history it seems commit id '5c811dce' added this functionality to domain.conf.c and commit id 'e962a579' added address parsing for the attach-disk command. I also peeked at the review cycle of the domain_conf.c change and found my name, but at the time I didn't notice that we were expecting a non negative value storing as unsigned, but printing as signed... In my defense I was newer to the group though ;-) So, I'll add bus/target as _uip and then adjust the title/commit message appropriately when I push the changes. John
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ca55981..9e77b87 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_uip(unit, NULL, 0, &scsihostsrc->unit) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot parse unit '%s'"), unit); goto cleanup; diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 4c47473..0bea462 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -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_uip(unit, NULL, 0, &scsiAddr->unit) != 0) return -1;
return 0;
On 06/18/2015 12:47 PM, John Ferlan wrote:
On 06/16/2015 11:29 PM, Eric Farman wrote:
A logical unit address is a positive integer, so it would be wise to reject any negative inputs.
Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com> --- src/conf/domain_conf.c | 2 +- tools/virsh-domain.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
While we're at it - why not bus and target as well? That's doable in one patch... I'll adjust this in order
The linux kernel/qemu code have some more confusion in how the bus and target are printed, but that's certainly not the responsibility of this patch series. The LUN is always printed as %llu in the other layers of the stack, so I just wanted to get them in sync. So yes, I'm fine with cleaning up bus and target in both these patches. Thanks! - Eric
Looking back through history it seems commit id '5c811dce' added this functionality to domain.conf.c and commit id 'e962a579' added address parsing for the attach-disk command.
I also peeked at the review cycle of the domain_conf.c change and found my name, but at the time I didn't notice that we were expecting a non negative value storing as unsigned, but printing as signed... In my defense I was newer to the group though ;-)
So, I'll add bus/target as _uip and then adjust the title/commit message appropriately when I push the changes.
John
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ca55981..9e77b87 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_uip(unit, NULL, 0, &scsihostsrc->unit) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot parse unit '%s'"), unit); goto cleanup; diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 4c47473..0bea462 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -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_uip(unit, NULL, 0, &scsiAddr->unit) != 0) return -1;
return 0;
The logical unit field is an unsigned integer, we should use the appropriate substitution when printing it. Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com> --- src/conf/domain_audit.c | 2 +- src/conf/domain_conf.c | 2 +- src/qemu/qemu_hotplug.c | 4 ++-- src/util/virhostdev.c | 6 +++--- src/util/virscsi.c | 6 +++--- tools/virsh-domain.c | 2 +- 6 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c index 1900039..c94cae8 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:%u", scsihostsrc->adapter, scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit) < 0) { diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9e77b87..7e3ca36 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18940,7 +18940,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='%u'/>\n", includeTypeInAddr ? "type='scsi' " : "", scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index cc86a3b..1d538a0 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:%u"), scsihostsrc->adapter, scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit); } @@ -3873,7 +3873,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.%u not found"), scsihostsrc->adapter, scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit); } diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 1c8f31e..ea0076c 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:%u 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:%u " "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:%u 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..6c8b6ce 100644 --- a/src/util/virscsi.c +++ b/src/util/virscsi.c @@ -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:%u/scsi_generic", prefix, adapter_id, bus, target, unit) < 0) return NULL; @@ -170,7 +170,7 @@ virSCSIDeviceGetDevName(const char *sysfs_prefix, return NULL; if (virAsprintf(&path, - "%s/%d:%d:%d:%d/block", + "%s/%d:%d:%d:%u/block", prefix, adapter_id, bus, target, unit) < 0) return NULL; @@ -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:%u", dev->adapter, dev->bus, dev->target, dev->unit) < 0 || virAsprintf(&dev->sg_path, "%s/%s", sysfs_prefix ? sysfs_prefix : "/dev", sg) < 0) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 0bea462..e9dbcd9 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -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='%u' />\n", diskAddr.addr.scsi.controller, diskAddr.addr.scsi.bus, diskAddr.addr.scsi.unit); } else { -- 1.9.1
On 06/16/2015 11:29 PM, Eric Farman wrote:
The logical unit field is an unsigned integer, we should use the appropriate substitution when printing it.
Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com> --- src/conf/domain_audit.c | 2 +- src/conf/domain_conf.c | 2 +- src/qemu/qemu_hotplug.c | 4 ++-- src/util/virhostdev.c | 6 +++--- src/util/virscsi.c | 6 +++--- tools/virsh-domain.c | 2 +- 6 files changed, 11 insertions(+), 11 deletions(-)
Similar to 1/5 - why only adjust unit, adjust bus & target too since they're incorrect. I will adjust when I push (and change commit message to reflect that). John
diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c index 1900039..c94cae8 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:%u", scsihostsrc->adapter, scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit) < 0) { diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9e77b87..7e3ca36 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18940,7 +18940,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='%u'/>\n", includeTypeInAddr ? "type='scsi' " : "", scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index cc86a3b..1d538a0 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:%u"), scsihostsrc->adapter, scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit); } @@ -3873,7 +3873,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.%u not found"), scsihostsrc->adapter, scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit); } diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 1c8f31e..ea0076c 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:%u 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:%u " "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:%u 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..6c8b6ce 100644 --- a/src/util/virscsi.c +++ b/src/util/virscsi.c @@ -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:%u/scsi_generic", prefix, adapter_id, bus, target, unit) < 0) return NULL;
@@ -170,7 +170,7 @@ virSCSIDeviceGetDevName(const char *sysfs_prefix, return NULL;
if (virAsprintf(&path, - "%s/%d:%d:%d:%d/block", + "%s/%d:%d:%d:%u/block", prefix, adapter_id, bus, target, unit) < 0) return NULL;
@@ -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:%u", dev->adapter, dev->bus, dev->target, dev->unit) < 0 || virAsprintf(&dev->sg_path, "%s/%s", sysfs_prefix ? sysfs_prefix : "/dev", sg) < 0) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 0bea462..e9dbcd9 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -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='%u' />\n", diskAddr.addr.scsi.controller, diskAddr.addr.scsi.bus, diskAddr.addr.scsi.unit); } else {
The SCSI Architecture Model defines a logical unit address as 64-bits in length, so change the field accordingly so that the entire value could be stored. Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com> --- 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 +++--- 10 files changed, 26 insertions(+), 26 deletions(-) diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c index c94cae8..e5d7e15 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:%u", + if (virAsprintfQuiet(&address, "%s:%d:%d:%llu", scsihostsrc->adapter, scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit) < 0) { diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7e3ca36..3f0f175 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4954,7 +4954,7 @@ virDomainHostdevSubsysSCSIHostDefParseXML(xmlNodePtr sourcenode, goto cleanup; } - if (virStrToLong_uip(unit, NULL, 0, &scsihostsrc->unit) < 0) { + if (virStrToLong_ullp(unit, NULL, 0, &scsihostsrc->unit) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot parse unit '%s'"), unit); goto cleanup; @@ -18940,7 +18940,7 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf, virBufferAsprintf(buf, "<adapter name='%s'/>\n", scsihostsrc->adapter); virBufferAsprintf(buf, - "<address %sbus='%d' target='%d' unit='%u'/>\n", + "<address %sbus='%d' target='%d' unit='%llu'/>\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 1d538a0..d5a40aa 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:%u"), + _("Unable to prepare scsi hostdev: %s:%d:%d:%llu"), scsihostsrc->adapter, scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit); } @@ -3873,7 +3873,7 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver, virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host; virReportError(VIR_ERR_OPERATION_FAILED, - _("host scsi device %s:%d:%d.%u not found"), + _("host scsi device %s:%d:%d.%llu not found"), scsihostsrc->adapter, scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit); } diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index ea0076c..aa06b4a 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:%u on domain %s", + VIR_WARN("Unable to reattach SCSI device %s:%d:%d:%llu 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:%u " + VIR_WARN("Unable to find device %s:%d:%d:%llu " "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:%u dom=%s from activeSCSIHostdevs", + VIR_DEBUG("Removing %s:%d:%d:%llu 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 6c8b6ce..ea4a738 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:%u/scsi_generic", + "%s/%d:%d:%d:%llu/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:%u/block", + "%s/%d:%d:%d:%llu/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:%u", dev->adapter, + if (virAsprintf(&dev->name, "%d:%d:%d:%llu", 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 e9dbcd9..a4620bf 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_uip(unit, NULL, 0, &scsiAddr->unit) != 0) + if (virStrToLong_ullp(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='%u' />\n", + " bus='%d' unit='%llu' />\n", diskAddr.addr.scsi.controller, diskAddr.addr.scsi.bus, diskAddr.addr.scsi.unit); } else { -- 1.9.1
On 06/16/2015 11:29 PM, Eric Farman wrote:
The SCSI Architecture Model defines a logical unit address as 64-bits in length, so change the field accordingly so that the entire value could be stored.
Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com> --- 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 +++--- 10 files changed, 26 insertions(+), 26 deletions(-)
ACK - John
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. 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 | 6 +++++- docs/schemas/domaincommon.rng | 14 ++++++++++++-- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 4e85b51..c88c4a6 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3256,7 +3256,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 f0f7400..b3c5cb8 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3833,10 +3833,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"> @@ -5129,11 +5129,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> -- 1.9.1
On 06/16/2015 11:29 PM, 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. 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 | 6 +++++- docs/schemas/domaincommon.rng | 14 ++++++++++++-- 2 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 4e85b51..c88c4a6 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3256,7 +3256,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).
Since we know from the v1 comments that qemu has a max of 16384 units, add: Not all hypervisors support larger <code>unit</code> values. It is up to each hypervisor to determine the maximum value supported for the adapter. I could add "<code>target</code> and " if you think that would be better..
<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 f0f7400..b3c5cb8 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3833,10 +3833,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"> @@ -5129,11 +5129,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>
I'm going to add a test as well. It's essentially a copy of tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-scsi.xml adjusted to have a larger unit value (it'll be attached)
On 06/18/2015 02:38 PM, John Ferlan wrote:
On 06/16/2015 11:29 PM, 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. 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 | 6 +++++- docs/schemas/domaincommon.rng | 14 ++++++++++++-- 2 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 4e85b51..c88c4a6 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3256,7 +3256,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). Since we know from the v1 comments that qemu has a max of 16384 units, add:
Not all hypervisors support larger <code>unit</code> values. It is up to each hypervisor to determine the maximum value supported for the adapter.
I could add "<code>target</code> and " if you think that would be better..
The text with both target and unit sounds good to me.
<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 f0f7400..b3c5cb8 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3833,10 +3833,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"> @@ -5129,11 +5129,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>
I'm going to add a test as well. It's essentially a copy of tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-scsi.xml adjusted to have a larger unit value (it'll be attached)
Ok. If you push it as a separate patch, feel free to add: Reviewed-by: Eric Farman <farman@linux.vnet.ibm.com> Thanks, Eric
The type='scsi' parameter of an address element is ignored if placed within a hostdev section, and rejected by the XML schema used by virt-xml-validate. Remove it from the doc, and correct a typo in the remaining address arguments. 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 | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index c88c4a6..f97a049 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2839,7 +2839,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> @@ -3148,7 +3148,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'/> -- 1.9.1
On 06/16/2015 11:29 PM, Eric Farman wrote:
The type='scsi' parameter of an address element is ignored if placed within a hostdev section, and rejected by the XML schema used by virt-xml-validate. Remove it from the doc, and correct a typo in the remaining address arguments.
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 | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
ACK John
On 06/16/2015 11:29 PM, Eric Farman wrote:
While working with the hostdev tag and SCSI LUNs, a problem was discovered with the XML schema (see commit message in patch 4). This spawned some further corrections to the handling of the logical unit field throughout libvirt.
This series was split from a single patch, from this feedback: http://www.redhat.com/archives/libvir-list/2015-June/msg00489.html
Eric Farman (5): Print SCSI logical unit as a positive integer Print SCSI logical unit as unsigned integer Convert SCSI logical unit from int to long long docs: Fix XML schema handling of LUN address in hostdev tag docs: Correct typos in scsi hostdev and address elements
I get the value of small patches & agree with the way patches 4 and 5 are split out, but patch 1 and 2 are completely replaced by patch 3 with a different type. These are pretty straightforward changes, so I'd suggest squashing patches 1-3 as a single patch that just goes from signed int --> unsigned long long and has a commit that explains why we needed to change both size and sign... I see now that it was a v1 comment from John, so I'll leave it to his judgment. Regardless, code still looks good to me so you can feel free to keep my reviewed-by tag for the set, whether patches 1-3 are squashed or not. Reviewed-by: Matthew Rosato <mjrosato@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(-)
On 06/17/2015 04:29 PM, Matthew Rosato wrote:
On 06/16/2015 11:29 PM, Eric Farman wrote:
While working with the hostdev tag and SCSI LUNs, a problem was discovered with the XML schema (see commit message in patch 4). This spawned some further corrections to the handling of the logical unit field throughout libvirt.
This series was split from a single patch, from this feedback: http://www.redhat.com/archives/libvir-list/2015-June/msg00489.html
Eric Farman (5): Print SCSI logical unit as a positive integer Print SCSI logical unit as unsigned integer Convert SCSI logical unit from int to long long docs: Fix XML schema handling of LUN address in hostdev tag docs: Correct typos in scsi hostdev and address elements
I get the value of small patches & agree with the way patches 4 and 5 are split out, but patch 1 and 2 are completely replaced by patch 3 with a different type. These are pretty straightforward changes, so I'd suggest squashing patches 1-3 as a single patch that just goes from signed int --> unsigned long long and has a commit that explains why we needed to change both size and sign... I see now that it was a v1 comment from John, so I'll leave it to his judgment. The recommendation came from John and since he can push... following it to get it upstream is ... OK. :-)
Regardless, code still looks good to me so you can feel free to keep my reviewed-by tag for the set, whether patches 1-3 are squashed or not.
Reviewed-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
I agree with Mat! Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Another side info: I will currently not revert your former patch and apply this patch set on branches devel and rebased but wait until I update to libvirt v1.3.0! If you disagree with this approach please let me know. -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Wed, 2015-06-17 at 10:29 -0400, Matthew Rosato wrote:
On 06/16/2015 11:29 PM, Eric Farman wrote:
While working with the hostdev tag and SCSI LUNs, a problem was discovered with the XML schema (see commit message in patch 4). This spawned some further corrections to the handling of the logical unit field throughout libvirt.
This series was split from a single patch, from this feedback: http://www.redhat.com/archives/libvir-list/2015-June/msg00489.html
Eric Farman (5): Print SCSI logical unit as a positive integer Print SCSI logical unit as unsigned integer Convert SCSI logical unit from int to long long docs: Fix XML schema handling of LUN address in hostdev tag docs: Correct typos in scsi hostdev and address elements
I get the value of small patches & agree with the way patches 4 and 5 are split out, but patch 1 and 2 are completely replaced by patch 3 with a different type. These are pretty straightforward changes, so I'd suggest squashing patches 1-3 as a single patch that just goes from signed int --> unsigned long long and has a commit that explains why we needed to change both size and sign... I see now that it was a v1 comment from John, so I'll leave it to his judgment.
Regardless, code still looks good to me so you can feel free to keep my reviewed-by tag for the set, whether patches 1-3 are squashed or not.
Reviewed-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com> I'm with you. Reviewed-by: Stefan Zimmermann <stzi@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(-)
On 06/16/2015 11:29 PM, Eric Farman wrote:
While working with the hostdev tag and SCSI LUNs, a problem was discovered with the XML schema (see commit message in patch 4). This spawned some further corrections to the handling of the logical unit field throughout libvirt.
This series was split from a single patch, from this feedback: http://www.redhat.com/archives/libvir-list/2015-June/msg00489.html
Eric Farman (5): Print SCSI logical unit as a positive integer Print SCSI logical unit as unsigned integer Convert SCSI logical unit from int to long long docs: Fix XML schema handling of LUN address in hostdev tag docs: Correct typos in scsi hostdev and address elements
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(-)
While I understand the comments regarding the step by step process - consider bisection when/if something went wrong with these patches in the future... Was the issue because we parsed a positive integer? Was the issue because we printed an unsigned value? Was the issue because we printed a long long unsigned? Easier to revert something smaller and rework, than have to go through everything. I've made adjustments in my local branch to add the unsigned for bus and target, as well as a couple of other adjustments I've noted. I'll give it a day or so to percolate before pushing though just in case I've missed something... Tks - John John
On 06/16/2015 11:29 PM, Eric Farman wrote:
While working with the hostdev tag and SCSI LUNs, a problem was discovered with the XML schema (see commit message in patch 4). This spawned some further corrections to the handling of the logical unit field throughout libvirt.
This series was split from a single patch, from this feedback: http://www.redhat.com/archives/libvir-list/2015-June/msg00489.html
Eric Farman (5): Print SCSI logical unit as a positive integer Print SCSI logical unit as unsigned integer Convert SCSI logical unit from int to long long docs: Fix XML schema handling of LUN address in hostdev tag docs: Correct typos in scsi hostdev and address elements
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(-)
FYI: Just pushed upstream. John
participants (5)
-
Boris Fiuczynski -
Eric Farman -
John Ferlan -
Matthew Rosato -
Stefan Zimmermann