[libvirt] [PATCH v2 00/12] Adjust SCSI generated device address checks

[libvirt] [PATCH] conf: Remove...

[libvirt] [PATCH] Drive...

John Ferlan

Wednesday, 22 July 2015 Wed, 22 Jul '15

9:54 a.m.

v1 here: http://www.redhat.com/archives/libvir-list/2015-June/msg01104.html Some followups into July resulted in the request to move the Hostdev and Disk default (or _NONE) address creation/assignment into domain/ device post processing rather than during XML parsing. Changes in v2 are numerous, quite a bit of patch and code motion in order to accomplish the requested task in small enough and reviewable chunks John Ferlan (12): conf: Remove extraneous check in virDomainHostdevAssignAddress conf: Add 'bus' and 'target' to SCSI address conflict checks conf: Move hostdev and disk address validations conf: Add xmlopt to virDomainDeviceDefPostParseInternal conf: Add check for host address type while checking in use conf: Try controller add when searching hostdev bus for unit conf: Change when virDomainHostdevAssignAddress is called conf: Remove unused param from virDomainHostdevDefParseXML conf: Add SCSI hostdev check for disk drive address already in use conf: Change when virDomainDiskDefAssignAddress is called conf: Create locals for virDomainDiskDefAssignAddress conf: Check for hostdev conflicts when assign default disk address docs/formatdomain.html.in | 4 +- src/conf/domain_conf.c | 396 +++++++++++++++++++++++++++------------------- src/conf/domain_conf.h | 3 +- src/qemu/qemu_command.c | 4 +- src/vmx/vmx.c | 22 +-- src/vmx/vmx.h | 3 +- 6 files changed, 253 insertions(+), 179 deletions(-) -- 2.1.0

Show replies by date

John Ferlan

Wednesday, 22 July Wed, 22 Jul

9:54 a.m.

New subject: [libvirt] [PATCH v2 01/12] conf: Remove extraneous check in virDomainHostdevAssignAddress

Since the only way virDomainHostdevAssignAddress can be called is from within virDomainHostdevDefParseXML when hostdev->source.subsys.type is VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI, thus there's no need for redundancy. Signed-off-by: John Ferlan <jferlan(a)redhat.com> --- src/conf/domain_conf.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 73ac537..f7bc540 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5310,9 +5310,6 @@ virDomainHostdevAssignAddress(virDomainXMLOptionPtr xmlopt, size_t i; int ret; - if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) - return -1; - for (i = 0; i < def->ncontrollers; i++) { if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) continue; -- 2.1.0

John Ferlan

9:54 a.m.

New subject: [libvirt] [PATCH v2 02/12] conf: Add 'bus' and 'target' to SCSI address conflict checks

Modify virDomainDriveAddressIsUsedBy{Disk|Hostdev} and virDomainSCSIDriveAddressIsUsed to take 'bus' and 'target' parameters. Will be used by future patches for more complete address conflict checks Signed-off-by: John Ferlan <jferlan(a)redhat.com> --- src/conf/domain_conf.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f7bc540..f3d8f87 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5204,13 +5204,15 @@ virDomainHostdevSubsysSCSIDefParseXML(xmlNodePtr sourcenode, return ret; } -/* Check if a drive type address $controller:0:0:$unit is already +/* Check if a drive type address $controller:$bus:$target:$unit is already * taken by a disk or not. */ static bool virDomainDriveAddressIsUsedByDisk(const virDomainDef *def, virDomainDiskBus type, unsigned int controller, + unsigned int bus, + unsigned int target, unsigned int unit) { virDomainDiskDefPtr disk; @@ -5225,21 +5227,23 @@ virDomainDriveAddressIsUsedByDisk(const virDomainDef *def, if (disk->info.addr.drive.controller == controller && disk->info.addr.drive.unit == unit && - disk->info.addr.drive.bus == 0 && - disk->info.addr.drive.target == 0) + disk->info.addr.drive.bus == bus && + disk->info.addr.drive.target == target) return true; } return false; } -/* Check if a drive type address $controller:0:0:$unit is already +/* Check if a drive type address $controller:$target:$bus:$unit is already * taken by a host device or not. */ static bool virDomainDriveAddressIsUsedByHostdev(const virDomainDef *def, virDomainHostdevSubsysType type, unsigned int controller, + unsigned int bus, + unsigned int target, unsigned int unit) { virDomainHostdevDefPtr hostdev; @@ -5253,8 +5257,8 @@ virDomainDriveAddressIsUsedByHostdev(const virDomainDef *def, if (hostdev->info->addr.drive.controller == controller && hostdev->info->addr.drive.unit == unit && - hostdev->info->addr.drive.bus == 0 && - hostdev->info->addr.drive.target == 0) + hostdev->info->addr.drive.bus == bus && + hostdev->info->addr.drive.target == target) return true; } @@ -5264,6 +5268,8 @@ virDomainDriveAddressIsUsedByHostdev(const virDomainDef *def, static bool virDomainSCSIDriveAddressIsUsed(const virDomainDef *def, unsigned int controller, + unsigned int bus, + unsigned int target, unsigned int unit) { /* In current implementation, the maximum unit number of a controller @@ -5273,9 +5279,10 @@ virDomainSCSIDriveAddressIsUsed(const virDomainDef *def, return true; if (virDomainDriveAddressIsUsedByDisk(def, VIR_DOMAIN_DISK_BUS_SCSI, - controller, unit) || - virDomainDriveAddressIsUsedByHostdev(def, VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI, - controller, unit)) + controller, bus, target, unit) || + virDomainDriveAddressIsUsedByHostdev(def, + VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI, + controller, bus, target, unit)) return true; return false; @@ -5290,7 +5297,8 @@ virDomainControllerSCSINextUnit(const virDomainDef *def, size_t i; for (i = 0; i < max_unit; i++) { - if (!virDomainSCSIDriveAddressIsUsed(def, controller, i)) + /* Default to assigning addresses using bus = target = 0 */ + if (!virDomainSCSIDriveAddressIsUsed(def, controller, 0, 0, i)) return i; } -- 2.1.0

John Ferlan

9:54 a.m.

New subject: [libvirt] [PATCH v2 03/12] conf: Move hostdev and disk address validations

Move the functions above the post processing for upcoming patch Signed-off-by: John Ferlan <jferlan(a)redhat.com> --- src/conf/domain_conf.c | 284 +++++++++++++++++++++++++------------------------ 1 file changed, 145 insertions(+), 139 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f3d8f87..a3e266d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3836,6 +3836,151 @@ virDomainDefPostParseInternal(virDomainDefPtr def, } +/* Check if a drive type address $controller:$bus:$target:$unit is already + * taken by a disk or not. + */ +static bool +virDomainDriveAddressIsUsedByDisk(const virDomainDef *def, + virDomainDiskBus type, + unsigned int controller, + unsigned int bus, + unsigned int target, + unsigned int unit) +{ + virDomainDiskDefPtr disk; + size_t i; + + for (i = 0; i < def->ndisks; i++) { + disk = def->disks[i]; + + if (disk->bus != type || + disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) + continue; + + if (disk->info.addr.drive.controller == controller && + disk->info.addr.drive.unit == unit && + disk->info.addr.drive.bus == bus && + disk->info.addr.drive.target == target) + return true; + } + + return false; +} + + +/* Check if a drive type address $controller:$target:$bus:$unit is already + * taken by a host device or not. + */ +static bool +virDomainDriveAddressIsUsedByHostdev(const virDomainDef *def, + virDomainHostdevSubsysType type, + unsigned int controller, + unsigned int bus, + unsigned int target, + unsigned int unit) +{ + virDomainHostdevDefPtr hostdev; + size_t i; + + for (i = 0; i < def->nhostdevs; i++) { + hostdev = def->hostdevs[i]; + + if (hostdev->source.subsys.type != type) + continue; + + if (hostdev->info->addr.drive.controller == controller && + hostdev->info->addr.drive.unit == unit && + hostdev->info->addr.drive.bus == bus && + hostdev->info->addr.drive.target == target) + return true; + } + + return false; +} + + +static bool +virDomainSCSIDriveAddressIsUsed(const virDomainDef *def, + unsigned int controller, + unsigned int bus, + unsigned int target, + unsigned int unit) +{ + /* In current implementation, the maximum unit number of a controller + * is either 16 or 7 (narrow SCSI bus), and if the maximum unit number + * is 16, the controller itself is on unit 7 */ + if (unit == 7) + return true; + + if (virDomainDriveAddressIsUsedByDisk(def, VIR_DOMAIN_DISK_BUS_SCSI, + controller, bus, target, unit) || + virDomainDriveAddressIsUsedByHostdev(def, + VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI, + controller, bus, target, unit)) + return true; + + return false; +} + + +/* Find out the next usable "unit" of a specific controller */ +static int +virDomainControllerSCSINextUnit(const virDomainDef *def, + unsigned int max_unit, + unsigned int controller) +{ + size_t i; + + for (i = 0; i < max_unit; i++) { + /* Default to assigning addresses using bus = target = 0 */ + if (!virDomainSCSIDriveAddressIsUsed(def, controller, 0, 0, i)) + return i; + } + + return -1; +} + + +#define SCSI_WIDE_BUS_MAX_CONT_UNIT 16 +#define SCSI_NARROW_BUS_MAX_CONT_UNIT 7 + +static int +virDomainHostdevAssignAddress(virDomainXMLOptionPtr xmlopt, + const virDomainDef *def, + virDomainHostdevDefPtr hostdev) +{ + int next_unit = 0; + unsigned controller = 0; + size_t i; + int ret; + + for (i = 0; i < def->ncontrollers; i++) { + if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) + continue; + + controller++; + ret = virDomainControllerSCSINextUnit(def, + xmlopt->config.hasWideSCSIBus ? + SCSI_WIDE_BUS_MAX_CONT_UNIT : + SCSI_NARROW_BUS_MAX_CONT_UNIT, + def->controllers[i]->idx); + if (ret >= 0) { + next_unit = ret; + controller = def->controllers[i]->idx; + break; + } + } + + hostdev->info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE; + hostdev->info->addr.drive.controller = controller; + hostdev->info->addr.drive.bus = 0; + hostdev->info->addr.drive.target = 0; + hostdev->info->addr.drive.unit = next_unit; + + return 0; +} + + static int virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev, const virDomainDef *def, @@ -5204,145 +5349,6 @@ virDomainHostdevSubsysSCSIDefParseXML(xmlNodePtr sourcenode, return ret; } -/* Check if a drive type address $controller:$bus:$target:$unit is already - * taken by a disk or not. - */ -static bool -virDomainDriveAddressIsUsedByDisk(const virDomainDef *def, - virDomainDiskBus type, - unsigned int controller, - unsigned int bus, - unsigned int target, - unsigned int unit) -{ - virDomainDiskDefPtr disk; - size_t i; - - for (i = 0; i < def->ndisks; i++) { - disk = def->disks[i]; - - if (disk->bus != type || - disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) - continue; - - if (disk->info.addr.drive.controller == controller && - disk->info.addr.drive.unit == unit && - disk->info.addr.drive.bus == bus && - disk->info.addr.drive.target == target) - return true; - } - - return false; -} - -/* Check if a drive type address $controller:$target:$bus:$unit is already - * taken by a host device or not. - */ -static bool -virDomainDriveAddressIsUsedByHostdev(const virDomainDef *def, - virDomainHostdevSubsysType type, - unsigned int controller, - unsigned int bus, - unsigned int target, - unsigned int unit) -{ - virDomainHostdevDefPtr hostdev; - size_t i; - - for (i = 0; i < def->nhostdevs; i++) { - hostdev = def->hostdevs[i]; - - if (hostdev->source.subsys.type != type) - continue; - - if (hostdev->info->addr.drive.controller == controller && - hostdev->info->addr.drive.unit == unit && - hostdev->info->addr.drive.bus == bus && - hostdev->info->addr.drive.target == target) - return true; - } - - return false; -} - -static bool -virDomainSCSIDriveAddressIsUsed(const virDomainDef *def, - unsigned int controller, - unsigned int bus, - unsigned int target, - unsigned int unit) -{ - /* In current implementation, the maximum unit number of a controller - * is either 16 or 7 (narrow SCSI bus), and if the maximum unit number - * is 16, the controller itself is on unit 7 */ - if (unit == 7) - return true; - - if (virDomainDriveAddressIsUsedByDisk(def, VIR_DOMAIN_DISK_BUS_SCSI, - controller, bus, target, unit) || - virDomainDriveAddressIsUsedByHostdev(def, - VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI, - controller, bus, target, unit)) - return true; - - return false; -} - -/* Find out the next usable "unit" of a specific controller */ -static int -virDomainControllerSCSINextUnit(const virDomainDef *def, - unsigned int max_unit, - unsigned int controller) -{ - size_t i; - - for (i = 0; i < max_unit; i++) { - /* Default to assigning addresses using bus = target = 0 */ - if (!virDomainSCSIDriveAddressIsUsed(def, controller, 0, 0, i)) - return i; - } - - return -1; -} - -#define SCSI_WIDE_BUS_MAX_CONT_UNIT 16 -#define SCSI_NARROW_BUS_MAX_CONT_UNIT 7 - -static int -virDomainHostdevAssignAddress(virDomainXMLOptionPtr xmlopt, - const virDomainDef *def, - virDomainHostdevDefPtr hostdev) -{ - int next_unit = 0; - unsigned controller = 0; - size_t i; - int ret; - - for (i = 0; i < def->ncontrollers; i++) { - if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) - continue; - - controller++; - ret = virDomainControllerSCSINextUnit(def, - xmlopt->config.hasWideSCSIBus ? - SCSI_WIDE_BUS_MAX_CONT_UNIT : - SCSI_NARROW_BUS_MAX_CONT_UNIT, - def->controllers[i]->idx); - if (ret >= 0) { - next_unit = ret; - controller = def->controllers[i]->idx; - break; - } - } - - hostdev->info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE; - hostdev->info->addr.drive.controller = controller; - hostdev->info->addr.drive.bus = 0; - hostdev->info->addr.drive.target = 0; - hostdev->info->addr.drive.unit = next_unit; - - return 0; -} static int virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, -- 2.1.0

John Ferlan

9:54 a.m.

New subject: [libvirt] [PATCH v2 04/12] conf: Add xmlopt to virDomainDeviceDefPostParseInternal

Add the xmlopt parameter that was saved during virDomainDefPostParse to the parameters. A future patch will use it. Signed-off-by: John Ferlan <jferlan(a)redhat.com> --- src/conf/domain_conf.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a3e266d..6dbe05e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3984,7 +3984,8 @@ virDomainHostdevAssignAddress(virDomainXMLOptionPtr xmlopt, static int virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev, const virDomainDef *def, - virCapsPtr caps ATTRIBUTE_UNUSED) + virCapsPtr caps ATTRIBUTE_UNUSED, + virDomainXMLOptionPtr xmlopt ATTRIBUTE_UNUSED) { if (dev->type == VIR_DOMAIN_DEVICE_CHR) { virDomainChrDefPtr chr = dev->data.chr; @@ -4091,7 +4092,7 @@ virDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, return ret; } - if ((ret = virDomainDeviceDefPostParseInternal(dev, def, caps)) < 0) + if ((ret = virDomainDeviceDefPostParseInternal(dev, def, caps, xmlopt)) < 0) return ret; return 0; -- 2.1.0

John Ferlan

9:54 a.m.

New subject: [libvirt] [PATCH v2 05/12] conf: Add check for host address type while checking in use

While searching the hostdevs the drive type can be either *_TYPE_DRIVE or *_TYPE_NONE. If the type is _TYPE_NONE on the first scsi_host, then there is an erroneous "match" that the address already exists. Although this works by chance currently because hostdev's are added one at a time and 'nhostdevs' would be zero, thus returning false for the first hostdev added, a future patch will move the hostdev address assignment into post processing resulting in the bad match. This code is only called by path's expecting either drive or none. Signed-off-by: John Ferlan <jferlan(a)redhat.com> --- src/conf/domain_conf.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6dbe05e..80daba9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3885,7 +3885,8 @@ virDomainDriveAddressIsUsedByHostdev(const virDomainDef *def, for (i = 0; i < def->nhostdevs; i++) { hostdev = def->hostdevs[i]; - if (hostdev->source.subsys.type != type) + if (hostdev->source.subsys.type != type || + hostdev->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) continue; if (hostdev->info->addr.drive.controller == controller && -- 2.1.0

John Ferlan

9:54 a.m.

New subject: [libvirt] [PATCH v2 06/12] conf: Try controller add when searching hostdev bus for unit

If virDomainControllerSCSINextUnit failed to find a slot on the current VIR_DOMAIN_CONTROLLER_TYPE_SCSI controller(s), try to add a new controller; otherwise, there may be multiple unit=0 entries for the same "next" controller. Signed-off-by: John Ferlan <jferlan(a)redhat.com> --- src/conf/domain_conf.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 80daba9..9c6c739 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3953,7 +3953,7 @@ virDomainHostdevAssignAddress(virDomainXMLOptionPtr xmlopt, int next_unit = 0; unsigned controller = 0; size_t i; - int ret; + int ret = -1; for (i = 0; i < def->ncontrollers; i++) { if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) @@ -3972,6 +3972,17 @@ virDomainHostdevAssignAddress(virDomainXMLOptionPtr xmlopt, } } + /* If failed to find any VIR_DOMAIN_CONTROLLER_TYPE_SCSI or any space + * on existing VIR_DOMAIN_CONTROLLER_TYPE_SCSI controller(s), then + * try to add a new controller resulting in placement of this entry + * as unit=0 + */ + if (ret == -1 && + virDomainDefMaybeAddController((virDomainDefPtr) def, + VIR_DOMAIN_CONTROLLER_TYPE_SCSI, + controller, -1) < 0) + return -1; + hostdev->info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE; hostdev->info->addr.drive.controller = controller; hostdev->info->addr.drive.bus = 0; -- 2.1.0

John Ferlan

9:54 a.m.

New subject: [libvirt] [PATCH v2 07/12] conf: Change when virDomainHostdevAssignAddress is called

Rather than calling virDomainHostdevAssignAddress during the parsing of the XML, move the setting of a default hostdev address to domain/ device post processing. Since the parse code no longer generates an address, we can remove the virDomainDefMaybeAddHostdevSCSIcontroller since the call to virDomainHostdevAssignAddress will attempt to add the controllers that were not already defined in the XML. This patch will also enforce that the address type is type 'drive' when a SCSI subsystem <hostdev> element is provided with an <address>. Signed-off-by: John Ferlan <jferlan(a)redhat.com> --- docs/formatdomain.html.in | 4 ++-- src/conf/domain_conf.c | 29 +++++++++++++++++++---------- 2 files changed, 21 insertions(+), 12 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index d0c1741..e78fb26 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3355,8 +3355,8 @@ (starting with 0x) or octal (starting with 0) form. For PCI devices the element carries 4 attributes allowing to designate the device as can be found with the <code>lspci</code> or - with <code>virsh - nodedev-list</code>. <a href="#elementsAddress">See above</a> for + with <code>virsh nodedev-list</code>. For SCSI devices a 'drive' + address type must be used. <a href="#elementsAddress">See above</a> for more details on the address element.</dd> <dt><code>driver</code></dt> <dd> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9c6c739..eabba68 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3997,7 +3997,7 @@ static int virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev, const virDomainDef *def, virCapsPtr caps ATTRIBUTE_UNUSED, - virDomainXMLOptionPtr xmlopt ATTRIBUTE_UNUSED) + virDomainXMLOptionPtr xmlopt) { if (dev->type == VIR_DOMAIN_DEVICE_CHR) { virDomainChrDefPtr chr = dev->data.chr; @@ -4085,6 +4085,18 @@ virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev, video->vram = VIR_ROUND_UP_POWER_OF_TWO(video->vram); } + if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) { + virDomainHostdevDefPtr hdev = dev->data.hostdev; + + if (hdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && + hdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI && + hdev->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && + virDomainHostdevAssignAddress(xmlopt, def, hdev) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Cannot assign SCSI host devices address ")); + return -1; + } + } return 0; } @@ -11860,8 +11872,8 @@ virDomainVideoDefParseXML(xmlNodePtr node, } static virDomainHostdevDefPtr -virDomainHostdevDefParseXML(virDomainXMLOptionPtr xmlopt, - const virDomainDef *vmdef, +virDomainHostdevDefParseXML(virDomainXMLOptionPtr xmlopt ATTRIBUTE_UNUSED, + const virDomainDef *vmdef ATTRIBUTE_UNUSED, xmlNodePtr node, xmlXPathContextPtr ctxt, virHashTablePtr bootHash, @@ -11922,11 +11934,11 @@ virDomainHostdevDefParseXML(virDomainXMLOptionPtr xmlopt, } break; case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: - if (def->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && - virDomainHostdevAssignAddress(xmlopt, vmdef, def) < 0) { - + if (def->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && + def->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) { virReportError(VIR_ERR_XML_ERROR, "%s", - _("SCSI host devices must have address specified")); + _("SCSI host device must use 'drive' " + "address type")); goto error; } @@ -15974,9 +15986,6 @@ virDomainDefParseXML(xmlDocPtr xml, } def->hostdevs[def->nhostdevs++] = hostdev; - - if (virDomainDefMaybeAddHostdevSCSIcontroller(def) < 0) - goto error; } VIR_FREE(nodes); -- 2.1.0

Ján Tomko

Monday, 3 August Mon, 3 Aug

8:26 a.m.

New subject: [libvirt] [PATCH v2 07/12] conf: Change when virDomainHostdevAssignAddress is called

On Wed, Jul 22, 2015 at 10:54:29AM -0400, John Ferlan wrote:

...

host device's? There's also a trailing space. Jan

John Ferlan

Wednesday, 22 July Wed, 22 Jul

9:54 a.m.

New subject: [libvirt] [PATCH v2 08/12] conf: Remove unused param from virDomainHostdevDefParseXML

Remove unused xmlopt param Signed-off-by: John Ferlan <jferlan(a)redhat.com> --- src/conf/domain_conf.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index eabba68..44ce71b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11872,8 +11872,7 @@ virDomainVideoDefParseXML(xmlNodePtr node, } static virDomainHostdevDefPtr -virDomainHostdevDefParseXML(virDomainXMLOptionPtr xmlopt ATTRIBUTE_UNUSED, - const virDomainDef *vmdef ATTRIBUTE_UNUSED, +virDomainHostdevDefParseXML(const virDomainDef *vmdef ATTRIBUTE_UNUSED, xmlNodePtr node, xmlXPathContextPtr ctxt, virHashTablePtr bootHash, @@ -12445,8 +12444,8 @@ virDomainDeviceDefParse(const char *xmlStr, goto error; break; case VIR_DOMAIN_DEVICE_HOSTDEV: - if (!(dev->data.hostdev = virDomainHostdevDefParseXML(xmlopt, def, node, - ctxt, NULL, flags))) + if (!(dev->data.hostdev = virDomainHostdevDefParseXML(def, node, ctxt, + NULL, flags))) goto error; break; case VIR_DOMAIN_DEVICE_CONTROLLER: @@ -15971,8 +15970,8 @@ virDomainDefParseXML(xmlDocPtr xml, for (i = 0; i < n; i++) { virDomainHostdevDefPtr hostdev; - hostdev = virDomainHostdevDefParseXML(xmlopt, def, nodes[i], - ctxt, bootHash, flags); + hostdev = virDomainHostdevDefParseXML(def, nodes[i], ctxt, + bootHash, flags); if (!hostdev) goto error; -- 2.1.0

Ján Tomko

Monday, 3 August Mon, 3 Aug

8:40 a.m.

New subject: [libvirt] [PATCH v2 08/12] conf: Remove unused param from virDomainHostdevDefParseXML

On Wed, Jul 22, 2015 at 10:54:30AM -0400, John Ferlan wrote:

...

Remove unused xmlopt param Signed-off-by: John Ferlan <jferlan(a)redhat.com> --- src/conf/domain_conf.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)

ACK to patches 1 to 8. Jan

John Ferlan

Wednesday, 22 July Wed, 22 Jul

9:54 a.m.

New subject: [libvirt] [PATCH v2 09/12] conf: Add SCSI hostdev check for disk drive address already in use

https://bugzilla.redhat.com/show_bug.cgi?id=1210587 (partial) If a SCSI subsystem <hostdev> element address is provided, we need to make sure the address provided doesn't conflict with an existing or libvirt generated address for a SCSI <disk> element. This will fix the issue where the domain XML provided an <address> for the <hostdev>, but not the <disk> element where the address provided ends up being the same address used for the <disk>. A <disk> address is generated using it's assigned <target> 'dev' name prior to the check/validation of the <hostdev> address value. Signed-off-by: John Ferlan <jferlan(a)redhat.com> --- src/conf/domain_conf.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 44ce71b..eba264d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11872,7 +11872,7 @@ virDomainVideoDefParseXML(xmlNodePtr node, } static virDomainHostdevDefPtr -virDomainHostdevDefParseXML(const virDomainDef *vmdef ATTRIBUTE_UNUSED, +virDomainHostdevDefParseXML(const virDomainDef *vmdef, xmlNodePtr node, xmlXPathContextPtr ctxt, virHashTablePtr bootHash, @@ -11939,6 +11939,26 @@ virDomainHostdevDefParseXML(const virDomainDef *vmdef ATTRIBUTE_UNUSED, _("SCSI host device must use 'drive' " "address type")); goto error; + } else if (def->info->type == + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) { + /* Ensure provided address doesn't conflict with existing + * scsi disk drive address + */ + virDomainDeviceDriveAddressPtr addr = &def->info->addr.drive; + if (virDomainDriveAddressIsUsedByDisk(vmdef, + VIR_DOMAIN_DISK_BUS_SCSI, + addr->controller, + addr->bus, + addr->target, + addr->unit)) { + virReportError(VIR_ERR_XML_ERROR, + _("SCSI host address controller='%u' " + "bus='%u' target='%u' unit='%u' in " + "use by a SCSI disk"), + addr->controller, addr->bus, + addr->target, addr->unit); + goto error; + } } if (virXPathBoolean("boolean(./readonly)", ctxt)) -- 2.1.0

Ján Tomko

Monday, 3 August Mon, 3 Aug

8:46 a.m.

New subject: [libvirt] [PATCH v2 09/12] conf: Add SCSI hostdev check for disk drive address already in use

On Wed, Jul 22, 2015 at 10:54:31AM -0400, John Ferlan wrote:

...

This check seems out of place in HostdevDefParse. It also does not check for conflicts with other hostdevs. Jan

...

+ virReportError(VIR_ERR_XML_ERROR, + _("SCSI host address controller='%u' " + "bus='%u' target='%u' unit='%u' in " + "use by a SCSI disk"), + addr->controller, addr->bus, + addr->target, addr->unit); + goto error; + } } if (virXPathBoolean("boolean(./readonly)", ctxt)) -- 2.1.0 -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

John Ferlan

Wednesday, 22 July Wed, 22 Jul

9:54 a.m.

New subject: [libvirt] [PATCH v2 10/12] conf: Change when virDomainDiskDefAssignAddress is called

Rather than calling virDomainDiskDefAssignAddress during the parsing of the XML, moving the setting of disk addresses into the domain/device post processing. Commit id '37588b25' which introduced VIR_DOMAIN_DEF_PARSE_DISK_SOURCE in order to avoid generating the address which wasn't required will not be affected by this as all it cared about was processing the source XML. Signed-off-by: John Ferlan <jferlan(a)redhat.com> --- src/conf/domain_conf.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index eba264d..1560823 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4077,6 +4077,10 @@ virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev, disk->dst); return -1; } + + if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && + virDomainDiskDefAssignAddress(xmlopt, disk) < 0) + return -1; } if (dev->type == VIR_DOMAIN_DEVICE_VIDEO) { @@ -7439,10 +7443,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, } if (!(flags & VIR_DOMAIN_DEF_PARSE_DISK_SOURCE)) { - if (def->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE - && virDomainDiskDefAssignAddress(xmlopt, def) < 0) - goto error; - if (virDomainDiskBackingStoreParse(ctxt, def->src) < 0) goto error; } -- 2.1.0

Ján Tomko

Monday, 3 August Mon, 3 Aug

8:49 a.m.

New subject: [libvirt] [PATCH v2 10/12] conf: Change when virDomainDiskDefAssignAddress is called

On Wed, Jul 22, 2015 at 10:54:32AM -0400, John Ferlan wrote:

...

ACK Jan

...

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index eba264d..1560823 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4077,6 +4077,10 @@ virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev, disk->dst); return -1; } + + if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && + virDomainDiskDefAssignAddress(xmlopt, disk) < 0) + return -1; } if (dev->type == VIR_DOMAIN_DEVICE_VIDEO) { @@ -7439,10 +7443,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, } if (!(flags & VIR_DOMAIN_DEF_PARSE_DISK_SOURCE)) { - if (def->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE - && virDomainDiskDefAssignAddress(xmlopt, def) < 0) - goto error; - if (virDomainDiskBackingStoreParse(ctxt, def->src) < 0) goto error; } -- 2.1.0 -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

John Ferlan

Wednesday, 22 July Wed, 22 Jul

9:54 a.m.

New subject: [libvirt] [PATCH v2 11/12] conf: Create locals for virDomainDiskDefAssignAddress

Create local controller/bus variables to be used by a future patch Signed-off-by: John Ferlan <jferlan(a)redhat.com> --- src/conf/domain_conf.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1560823..39a4cf8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5871,7 +5871,10 @@ virDomainDiskDefAssignAddress(virDomainXMLOptionPtr xmlopt, } switch (def->bus) { - case VIR_DOMAIN_DISK_BUS_SCSI: + case VIR_DOMAIN_DISK_BUS_SCSI: { + unsigned int controller; + unsigned int unit; + def->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE; if (xmlopt->config.hasWideSCSIBus) { @@ -5880,22 +5883,25 @@ virDomainDiskDefAssignAddress(virDomainXMLOptionPtr xmlopt, * Unit 7 is the SCSI controller itself. Therefore unit 7 * cannot be assigned to disks and is skipped. */ - def->info.addr.drive.controller = idx / 15; - def->info.addr.drive.bus = 0; - def->info.addr.drive.unit = idx % 15; + controller = idx / 15; + unit = idx % 15; /* Skip the SCSI controller at unit 7 */ - if (def->info.addr.drive.unit >= 7) - ++def->info.addr.drive.unit; + if (unit >= 7) + ++unit; } else { /* For a narrow SCSI bus we define the default mapping to be * 7 units per bus, 1 bus per controller, many controllers */ - def->info.addr.drive.controller = idx / 7; - def->info.addr.drive.bus = 0; - def->info.addr.drive.unit = idx % 7; + controller = idx / 7; + unit = idx % 7; } + def->info.addr.drive.controller = controller; + def->info.addr.drive.bus = 0; + def->info.addr.drive.target = 0; + def->info.addr.drive.unit = unit; break; + } case VIR_DOMAIN_DISK_BUS_IDE: /* For IDE we define the default mapping to be 2 units -- 2.1.0

John Ferlan

9:54 a.m.

New subject: [libvirt] [PATCH v2 12/12] conf: Check for hostdev conflicts when assign default disk address

https://bugzilla.redhat.com/show_bug.cgi?id=1210587 (completed) When generating the default drive address for a SCSI <disk> device, check the generated address to ensure it doesn't conflict with a SCSI <hostdev> address. The <disk> address generation algorithm uses the <target> "dev" name in order to determine which controller and unit in order to place the device. Since a SCSI <hostdev> device doesn't require a target device name, its placement on the guest SCSI address "could" conflict. For instance, if a SCSI <hostdev> exists at controller=0 unit=0 and an attempt to hotplug 'sda' into the guest made, there would be a conflict if the <hostdev> is already using /dev/sda. Signed-off-by: John Ferlan <jferlan(a)redhat.com> --- src/conf/domain_conf.c | 16 ++++++++++++++-- src/conf/domain_conf.h | 3 ++- src/qemu/qemu_command.c | 4 ++-- src/vmx/vmx.c | 22 ++++++++++++---------- src/vmx/vmx.h | 3 ++- 5 files changed, 32 insertions(+), 16 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 39a4cf8..0dac60c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4079,7 +4079,7 @@ virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev, } if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && - virDomainDiskDefAssignAddress(xmlopt, disk) < 0) + virDomainDiskDefAssignAddress(xmlopt, disk, def) < 0) return -1; } @@ -5860,7 +5860,8 @@ virDomainDiskDiffersSourceOnly(virDomainDiskDefPtr disk, int virDomainDiskDefAssignAddress(virDomainXMLOptionPtr xmlopt, - virDomainDiskDefPtr def) + virDomainDiskDefPtr def, + const virDomainDef *vmdef) { int idx = virDiskNameToIndex(def->dst); if (idx < 0) { @@ -5896,6 +5897,17 @@ virDomainDiskDefAssignAddress(virDomainXMLOptionPtr xmlopt, unit = idx % 7; } + if (virDomainDriveAddressIsUsedByHostdev(vmdef, + VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI, + controller, 0, 0, unit)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("using disk target name '%s' conflicts with " + "SCSI host device address controller='%u' " + "bus='%u' target='%u' unit='%u"), + def->dst, controller, 0, 0, unit); + return -1; + } + def->info.addr.drive.controller = controller; def->info.addr.drive.bus = 0; def->info.addr.drive.target = 0; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 0fe6b1a..bec3a9f 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2721,7 +2721,8 @@ int virDomainDiskInsert(virDomainDefPtr def, void virDomainDiskInsertPreAlloced(virDomainDefPtr def, virDomainDiskDefPtr disk); int virDomainDiskDefAssignAddress(virDomainXMLOptionPtr xmlopt, - virDomainDiskDefPtr def); + virDomainDiskDefPtr def, + const virDomainDef *vmdef); virDomainDiskDefPtr virDomainDiskRemove(virDomainDefPtr def, size_t i); diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 34a0574..a6b7686 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -11847,7 +11847,7 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt, else def->dst[2] = 'a' + idx; - if (virDomainDiskDefAssignAddress(xmlopt, def) < 0) { + if (virDomainDiskDefAssignAddress(xmlopt, def, dom) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid device name '%s'"), def->dst); virDomainDiskDefFree(def); @@ -12990,7 +12990,7 @@ qemuParseCommandLine(virCapsPtr qemuCaps, } } - if (virDomainDiskDefAssignAddress(xmlopt, disk) < 0) { + if (virDomainDiskDefAssignAddress(xmlopt, disk, def) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot assign address for device name '%s'"), disk->dst); diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index e77de62..36e2891 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -960,7 +960,9 @@ virVMXFloppyDiskNameToUnit(const char *name, int *unit) static int -virVMXVerifyDiskAddress(virDomainXMLOptionPtr xmlopt, virDomainDiskDefPtr disk) +virVMXVerifyDiskAddress(virDomainXMLOptionPtr xmlopt, + virDomainDiskDefPtr disk, + virDomainDefPtr vmdef) { virDomainDiskDef def; virDomainDeviceDriveAddressPtr drive; @@ -979,7 +981,7 @@ virVMXVerifyDiskAddress(virDomainXMLOptionPtr xmlopt, virDomainDiskDefPtr disk) def.dst = disk->dst; def.bus = disk->bus; - if (virDomainDiskDefAssignAddress(xmlopt, &def) < 0) { + if (virDomainDiskDefAssignAddress(xmlopt, &def, vmdef) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not verify disk address")); return -1; @@ -1588,7 +1590,7 @@ virVMXParseConfig(virVMXContext *ctx, if (virVMXParseDisk(ctx, xmlopt, conf, VIR_DOMAIN_DISK_DEVICE_DISK, VIR_DOMAIN_DISK_BUS_SCSI, controller, unit, - &def->disks[def->ndisks]) < 0) { + &def->disks[def->ndisks], def) < 0) { goto cleanup; } @@ -1599,7 +1601,7 @@ virVMXParseConfig(virVMXContext *ctx, if (virVMXParseDisk(ctx, xmlopt, conf, VIR_DOMAIN_DISK_DEVICE_CDROM, VIR_DOMAIN_DISK_BUS_SCSI, controller, unit, - &def->disks[def->ndisks]) < 0) { + &def->disks[def->ndisks], def) < 0) { goto cleanup; } @@ -1613,7 +1615,7 @@ virVMXParseConfig(virVMXContext *ctx, for (unit = 0; unit < 2; ++unit) { if (virVMXParseDisk(ctx, xmlopt, conf, VIR_DOMAIN_DISK_DEVICE_DISK, VIR_DOMAIN_DISK_BUS_IDE, bus, unit, - &def->disks[def->ndisks]) < 0) { + &def->disks[def->ndisks], def) < 0) { goto cleanup; } @@ -1624,7 +1626,7 @@ virVMXParseConfig(virVMXContext *ctx, if (virVMXParseDisk(ctx, xmlopt, conf, VIR_DOMAIN_DISK_DEVICE_CDROM, VIR_DOMAIN_DISK_BUS_IDE, bus, unit, - &def->disks[def->ndisks]) < 0) { + &def->disks[def->ndisks], def) < 0) { goto cleanup; } @@ -1637,7 +1639,7 @@ virVMXParseConfig(virVMXContext *ctx, for (unit = 0; unit < 2; ++unit) { if (virVMXParseDisk(ctx, xmlopt, conf, VIR_DOMAIN_DISK_DEVICE_FLOPPY, VIR_DOMAIN_DISK_BUS_FDC, 0, unit, - &def->disks[def->ndisks]) < 0) { + &def->disks[def->ndisks], def) < 0) { goto cleanup; } @@ -1925,7 +1927,7 @@ virVMXParseSCSIController(virConfPtr conf, int controller, bool *present, int virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr conf, int device, int busType, int controllerOrBus, int unit, - virDomainDiskDefPtr *def) + virDomainDiskDefPtr *def, virDomainDefPtr vmdef) { /* * device = {VIR_DOMAIN_DISK_DEVICE_DISK, @@ -2280,7 +2282,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con goto cleanup; } - if (virDomainDiskDefAssignAddress(xmlopt, *def) < 0) { + if (virDomainDiskDefAssignAddress(xmlopt, *def, vmdef) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not assign address to disk '%s'"), virDomainDiskGetSource(*def)); @@ -3189,7 +3191,7 @@ virVMXFormatConfig(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virDomainDe /* def:disks */ for (i = 0; i < def->ndisks; ++i) { - if (virVMXVerifyDiskAddress(xmlopt, def->disks[i]) < 0 || + if (virVMXVerifyDiskAddress(xmlopt, def->disks[i], def) < 0 || virVMXHandleLegacySCSIDiskDriverName(def, def->disks[i]) < 0) { goto cleanup; } diff --git a/src/vmx/vmx.h b/src/vmx/vmx.h index 6a68c8b..e986124 100644 --- a/src/vmx/vmx.h +++ b/src/vmx/vmx.h @@ -89,7 +89,8 @@ int virVMXParseSCSIController(virConfPtr conf, int controller, bool *present, int virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr conf, int device, int busType, - int controllerOrBus, int unit, virDomainDiskDefPtr *def); + int controllerOrBus, int unit, virDomainDiskDefPtr *def, + virDomainDefPtr vmdef); int virVMXParseFileSystem(virConfPtr conf, int number, virDomainFSDefPtr *def); -- 2.1.0

Ján Tomko

Monday, 3 August Mon, 3 Aug

8:57 a.m.

New subject: [libvirt] [PATCH v2 12/12] conf: Check for hostdev conflicts when assign default disk address

On Wed, Jul 22, 2015 at 10:54:34AM -0400, John Ferlan wrote:

...

This function does not really do any 'real' assignment, it just translates the target name to a fixed address. Unlike PCI address assignment, where we find unoccupied slots based on the domain defition, the domain definition is not really needed here. Moving the address check conflict to virDomainDefPostParse, after the addresses have been filled out by virDomainDeviceDefPostParse, would remove the need to change all the function prototypes and it would be a good place to check for address conflicts between disks too - after this series, two drives with the same addresses are allowed, as long as they have different target names. Jan

...

{ int idx = virDiskNameToIndex(def->dst); if (idx < 0) { @@ -5896,6 +5897,17 @@ virDomainDiskDefAssignAddress(virDomainXMLOptionPtr xmlopt, unit = idx % 7; } + if (virDomainDriveAddressIsUsedByHostdev(vmdef, + VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI, + controller, 0, 0, unit)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("using disk target name '%s' conflicts with " + "SCSI host device address controller='%u' " + "bus='%u' target='%u' unit='%u"), + def->dst, controller, 0, 0, unit); + return -1; + } + def->info.addr.drive.controller = controller; def->info.addr.drive.bus = 0; def->info.addr.drive.target = 0;

John Ferlan

Tuesday, 4 August Tue, 4 Aug

9:21 a.m.

New subject: [libvirt] [PATCH v2 12/12] conf: Check for hostdev conflicts when assign default disk address

On 08/03/2015 09:57 AM, Ján Tomko wrote:

...

On Wed, Jul 22, 2015 at 10:54:34AM -0400, John Ferlan wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1210587 (completed) > > When generating the default drive address for a SCSI <disk> device, > check the generated address to ensure it doesn't conflict with a SCSI > <hostdev> address. The <disk> address generation algorithm uses the > <target> "dev" name in order to determine which controller and unit > in order to place the device. Since a SCSI <hostdev> device doesn't > require a target device name, its placement on the guest SCSI address > "could" conflict. For instance, if a SCSI <hostdev> exists at > controller=0 unit=0 and an attempt to hotplug 'sda' into the guest > made, there would be a conflict if the <hostdev> is already using > /dev/sda. > > Signed-off-by: John Ferlan <jferlan(a)redhat.com> > --- > src/conf/domain_conf.c | 16 ++++++++++++++-- > src/conf/domain_conf.h | 3 ++- > src/qemu/qemu_command.c | 4 ++-- > src/vmx/vmx.c | 22 ++++++++++++---------- > src/vmx/vmx.h | 3 ++- > 5 files changed, 32 insertions(+), 16 deletions(-) > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 39a4cf8..0dac60c 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -4079,7 +4079,7 @@ virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev, > } > > if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && > - virDomainDiskDefAssignAddress(xmlopt, disk) < 0) > + virDomainDiskDefAssignAddress(xmlopt, disk, def) < 0) > return -1; > } > > @@ -5860,7 +5860,8 @@ virDomainDiskDiffersSourceOnly(virDomainDiskDefPtr disk, > > int > virDomainDiskDefAssignAddress(virDomainXMLOptionPtr xmlopt, > - virDomainDiskDefPtr def) > + virDomainDiskDefPtr def, > + const virDomainDef *vmdef) This function does not really do any 'real' assignment, it just translates the target name to a fixed address. Unlike PCI address assignment, where we find unoccupied slots based on the domain defition, the domain definition is not really needed here. Moving the address check conflict to virDomainDefPostParse, after the addresses have been filled out by virDomainDeviceDefPostParse, would remove the need to change all the function prototypes and it would be a good place to check for address conflicts between disks too - after this series, two drives with the same addresses are allowed, as long as they have different target names.

After looking through the code and thinking more about this - using virDomainDefPostParse won't work for the hotplug case since it's only going through virDomainDeviceDefParse and virDomainDeviceDefPostParse code in order to validate whether the address (whether provided or generated) is duplicated. Leaving the vmdef in virDomainDiskDefAssignAddress allows for the check of the generated device address based on the name to be compared against known hostdev addresses to ensure there isn't a conflict. Since the knowledge of what device type is being used is there - it just seems more natural to make the check there rather than repeating the same check in multiple callers. With respect to the other issue you note - that someone can provide duplicate drive addresses for either disk or hostdev - that's a slightly different issue, but is resolvable as well. Of course it's perhaps also true of other address types - that is I'm not sure that problem is 'drive' specific. It seems a separate patch could use the virDomainDefHasDeviceAddressIterator in some way to check all addresses for duplicates. I believe the existing patches should remain as is: Patch 9 - checks that the provided 'drive' address for a SCSI hostdev doesn't conflict with any current disk address. Since the disks would be parsed first this works to ensure there are no generated or provided address conflicts Patch 11 - just sets up to make patch 12 appear cleaner Patch 12 - checks to ensure the generated address based on name doesn't conflict with some defined SCSI hostdev address. If you have some specific suggestions for ways to handle this - I'm certainly open to reconsidering, but because of the existing hotplug paths which don't call virDomainDefPostParse I don't see it as a viable solution to the "whole" problem. John

Ján Tomko

Wednesday, 12 August Wed, 12 Aug

11:05 a.m.

New subject: [libvirt] [PATCH v2 12/12] conf: Check for hostdev conflicts when assign default disk address

On Tue, Aug 04, 2015 at 10:21:29AM -0400, John Ferlan wrote:

...

On 08/03/2015 09:57 AM, Ján Tomko wrote: > On Wed, Jul 22, 2015 at 10:54:34AM -0400, John Ferlan wrote: >> https://bugzilla.redhat.com/show_bug.cgi?id=1210587 (completed) >> >> When generating the default drive address for a SCSI <disk> device, >> check the generated address to ensure it doesn't conflict with a SCSI >> <hostdev> address. The <disk> address generation algorithm uses the >> <target> "dev" name in order to determine which controller and unit >> in order to place the device. Since a SCSI <hostdev> device doesn't >> require a target device name, its placement on the guest SCSI address >> "could" conflict. For instance, if a SCSI <hostdev> exists at >> controller=0 unit=0 and an attempt to hotplug 'sda' into the guest >> made, there would be a conflict if the <hostdev> is already using >> /dev/sda. >> >> Signed-off-by: John Ferlan <jferlan(a)redhat.com> >> --- >> src/conf/domain_conf.c | 16 ++++++++++++++-- >> src/conf/domain_conf.h | 3 ++- >> src/qemu/qemu_command.c | 4 ++-- >> src/vmx/vmx.c | 22 ++++++++++++---------- >> src/vmx/vmx.h | 3 ++- >> 5 files changed, 32 insertions(+), 16 deletions(-) >> >> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c >> index 39a4cf8..0dac60c 100644 >> --- a/src/conf/domain_conf.c >> +++ b/src/conf/domain_conf.c >> @@ -4079,7 +4079,7 @@ virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev, >> } >> >> if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && >> - virDomainDiskDefAssignAddress(xmlopt, disk) < 0) >> + virDomainDiskDefAssignAddress(xmlopt, disk, def) < 0) >> return -1; >> } >> >> @@ -5860,7 +5860,8 @@ virDomainDiskDiffersSourceOnly(virDomainDiskDefPtr disk, >> >> int >> virDomainDiskDefAssignAddress(virDomainXMLOptionPtr xmlopt, >> - virDomainDiskDefPtr def) >> + virDomainDiskDefPtr def, >> + const virDomainDef *vmdef) > > This function does not really do any 'real' assignment, it just > translates the target name to a fixed address. > Unlike PCI address assignment, where we find unoccupied slots based on > the domain defition, the domain definition is not really needed here. > > Moving the address check conflict to virDomainDefPostParse, after > the addresses have been filled out by virDomainDeviceDefPostParse, > would remove the need to change all the function prototypes and it would > be a good place to check for address conflicts between disks too - after > this series, two drives with the same addresses are allowed, as long as > they have different target names. > After looking through the code and thinking more about this - using virDomainDefPostParse won't work for the hotplug case since it's only going through virDomainDeviceDefParse and virDomainDeviceDefPostParse code in order to validate whether the address (whether provided or generated) is duplicated. Leaving the vmdef in virDomainDiskDefAssignAddress allows for the check of the generated device address based on the name to be compared against known hostdev addresses to ensure there isn't a conflict. Since the knowledge of what device type is being used is there - it just seems more natural to make the check there rather than repeating the same check in multiple callers.

Okay, auto-generating device data based on the whole domain definition makes sense in DeviceDefPostParse. Reading my reply, it seems I thought leaving that check there would not play well with checking the drive addresses against all devices, not just the devices of the "opposite" type. I still do not like using vmdef in virDomainHostdevDefParseXML.

...

With respect to the other issue you note - that someone can provide duplicate drive addresses for either disk or hostdev - that's a slightly different issue, but is resolvable as well. Of course it's perhaps also true of other address types - that is I'm not sure that problem is 'drive' specific. It seems a separate patch could use the virDomainDefHasDeviceAddressIterator in some way to check all addresses for duplicates.

The only difference I see here is that the disk address might not have been provided by user, but generated by libvirt based on the disk target. Checking for conflicts with devices of another type while leaving out devices of the same type seems incomplete.

...

I believe the existing patches should remain as is: Patch 9 - checks that the provided 'drive' address for a SCSI hostdev doesn't conflict with any current disk address. Since the disks would be parsed first this works to ensure there are no generated or provided address conflicts

I would rather see the currently unused vmdef attribute removed from virDomainHostdevDefParseXML. It seems the check would be just as effective in DeviceDefPostParse. ACK with the attribute removed and the check moved to PostParse. (If you still believe it should reamin as is, just resend it and wait for an ACK from someone else :))

...

Patch 11 - just sets up to make patch 12 appear cleaner Patch 12 - checks to ensure the generated address based on name doesn't conflict with some defined SCSI hostdev address.

ACK to these two. While they do not catch all the cases, they at least check that the address generated by libvirt does not clash with user's input. Jan

...

If you have some specific suggestions for ways to handle this - I'm certainly open to reconsidering, but because of the existing hotplug paths which don't call virDomainDefPostParse I don't see it as a viable solution to the "whole" problem. John -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

John Ferlan

3:36 p.m.

New subject: [libvirt] [PATCH v2 12/12] conf: Check for hostdev conflicts when assign default disk address

...

> After looking through the code and thinking more about this - using > virDomainDefPostParse won't work for the hotplug case since it's only > going through virDomainDeviceDefParse and virDomainDeviceDefPostParse > code in order to validate whether the address (whether provided or > generated) is duplicated. > > Leaving the vmdef in virDomainDiskDefAssignAddress allows for the check > of the generated device address based on the name to be compared against > known hostdev addresses to ensure there isn't a conflict. Since the > knowledge of what device type is being used is there - it just seems > more natural to make the check there rather than repeating the same > check in multiple callers. Okay, auto-generating device data based on the whole domain definition makes sense in DeviceDefPostParse. Reading my reply, it seems I thought leaving that check there would not play well with checking the drive addresses against all devices, not just the devices of the "opposite" type. I still do not like using vmdef in virDomainHostdevDefParseXML.

Removed, but I'll submit a follow-up patch rather than risk any wrath from pushing an unreviewed patch which essentially does this ;-)

...

> > With respect to the other issue you note - that someone can provide > duplicate drive addresses for either disk or hostdev - that's a slightly > different issue, but is resolvable as well. Of course it's perhaps also > true of other address types - that is I'm not sure that problem is > 'drive' specific. It seems a separate patch could use the > virDomainDefHasDeviceAddressIterator in some way to check all addresses > for duplicates. > The only difference I see here is that the disk address might not have been provided by user, but generated by libvirt based on the disk target. Checking for conflicts with devices of another type while leaving out devices of the same type seems incomplete.

I don't disagree, but it's a more "general" problem and started getting more complex than "just" checking disks and/or scsi subsys hostdev's whivch was the focus of this current bug/effort. I do have a local branch started which I hope to focus on shortly - perhaps after wandering through some USB patches.

...

> I believe the existing patches should remain as is: > > Patch 9 - checks that the provided 'drive' address for a SCSI hostdev > doesn't conflict with any current disk address. Since the disks would > be parsed first this works to ensure there are no generated or provided > address conflicts > I would rather see the currently unused vmdef attribute removed from virDomainHostdevDefParseXML. It seems the check would be just as effective in DeviceDefPostParse. ACK with the attribute removed and the check moved to PostParse. (If you still believe it should reamin as is, just resend it and wait for an ACK from someone else :))

Done - the check was moved to PostParse and works fine. I guess I focused more on the 'disk' problem than the fact that 'hostdev' can either be user supplied or libvirt generated based on what's already in use as opposed to causing a specific usage based solely on the target.

...

> Patch 11 - just sets up to make patch 12 appear cleaner > > Patch 12 - checks to ensure the generated address based on name doesn't > conflict with some defined SCSI hostdev address. > ACK to these two. While they do not catch all the cases, they at least check that the address generated by libvirt does not clash with user's input.

I pushed the adjusted 10, 11, and 12 and have posted a remove 'vmdef' from the virDomainHostdevDefParseXML patch. Tks - John

John Ferlan

Monday, 3 August Mon, 3 Aug

6:47 a.m.

On 07/22/2015 10:54 AM, John Ferlan wrote:

...

ping Tks, John

John Ferlan

7:30 p.m.

On 07/22/2015 10:54 AM, John Ferlan wrote:

...

I fixed the error message in patch 7 and pushed patches 1-8 and 10. Adjustments to the other patches will take a bit more time and testing - I don't think throwing away the existing patches will completely work for the hotplug cases. It's been a few weeks since I had this all fresh in my mind though. John I'll rework and post the duplicate address check

3425

days inactive

3446

days old

devel@lists.libvirt.org

Manage subscription

22 comments

2 participants

tags (0)

participants (2)

John Ferlan
Ján Tomko

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005

[libvirt] [PATCH v2 00/12] Adjust SCSI generated device address checks