[libvirt] [PATCH 0/5] Additional SCSI generated device address checks
https://bugzilla.redhat.com/show_bug.cgi?id=1210587 These patches will resolve a couple issues with generation of the <address type='drive' .../> for a SCSI <disk> and <hostdev>. The <disk> generation algorithm 'assumes' that when presented with <target dev='sda'.../> that it can use controller=0 and unit=0 since sda would conceivably be the first device; however, a <hostdev> could attempt to assign itself to that address and it doesn't have a target device name, so it bypasses the virDomainDiskDefDstDuplicates checks that would normally 'catch' two <disk>'s attempting to use the same name. Likewise, if a <hostdev> occupies an <address> and we attempt to hotplug a <disk> without providing an address, the address generation could attempt to place the disk on the already existing host device. John Ferlan (5): conf: Enforce SCSI hostdev address type conf: Add 'bus' and 'target' to SCSI address conflict checks conf: Add SCSI hostdev check for disk drive address already in use conf: Refactor virDomainDiskDefParseXML to pass vmdef conf: Check for hostdev conflicts when assign default disk address docs/formatdomain.html.in | 4 +- src/conf/domain_conf.c | 115 ++++++++++++++++++++++++++++++++-------------- 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, 101 insertions(+), 50 deletions(-) -- 2.1.0
If a SCSI subsystem <hostdev> element is provided with an <address>, then enforce that the address type is 'drive'. If not provided, a 'drive' element was created by virDomainHostdevAssignAddress which uses VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE. Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatdomain.html.in | 4 ++-- src/conf/domain_conf.c | 6 ++++++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 95d8c45..0475527 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3343,8 +3343,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 is 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 e592adf..ce5093d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11752,6 +11752,12 @@ virDomainHostdevDefParseXML(virDomainXMLOptionPtr xmlopt, virReportError(VIR_ERR_XML_ERROR, "%s", _("SCSI host devices must have address specified")); goto error; + } else if (def->info->type != + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("SCSI host device must use 'drive' " + "address type")); + goto error; } if (virXPathBoolean("boolean(./readonly)", ctxt)) -- 2.1.0
On 06/22/2015 05:05 PM, John Ferlan wrote:
If a SCSI subsystem <hostdev> element is provided with an <address>, then enforce that the address type is 'drive'. If not provided, a 'drive' element was created by virDomainHostdevAssignAddress which uses VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatdomain.html.in | 4 ++-- src/conf/domain_conf.c | 6 ++++++ 2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 95d8c45..0475527 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3343,8 +3343,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 is used. <a href="#elementsAddress">See above</a> for
s/is/must be/ ?
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 e592adf..ce5093d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11752,6 +11752,12 @@ virDomainHostdevDefParseXML(virDomainXMLOptionPtr xmlopt, virReportError(VIR_ERR_XML_ERROR, "%s", _("SCSI host devices must have address specified")); goto error; + } else if (def->info->type != + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("SCSI host device must use 'drive' " + "address type")); + goto error; }
if (virXPathBoolean("boolean(./readonly)", ctxt))
On Mon, Jun 22, 2015 at 05:05:03PM -0400, John Ferlan wrote:
If a SCSI subsystem <hostdev> element is provided with an <address>, then enforce that the address type is 'drive'. If not provided, a 'drive' element was created by virDomainHostdevAssignAddress which uses VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatdomain.html.in | 4 ++-- src/conf/domain_conf.c | 6 ++++++ 2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 95d8c45..0475527 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3343,8 +3343,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 is 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 e592adf..ce5093d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11752,6 +11752,12 @@ virDomainHostdevDefParseXML(virDomainXMLOptionPtr xmlopt, virReportError(VIR_ERR_XML_ERROR, "%s", _("SCSI host devices must have address specified")); goto error;
The body of this condition is dead code. virDomainHostdevAssignAddress only returns -1 if the subsys type is not SCSI, which we checked in the switch above. Jan
+ } else if (def->info->type != + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("SCSI host device must use 'drive' " + "address type")); + goto error; }
if (virXPathBoolean("boolean(./readonly)", ctxt)) -- 2.1.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 07/15/2015 11:06 AM, Ján Tomko wrote:
On Mon, Jun 22, 2015 at 05:05:03PM -0400, John Ferlan wrote:
If a SCSI subsystem <hostdev> element is provided with an <address>, then enforce that the address type is 'drive'. If not provided, a 'drive' element was created by virDomainHostdevAssignAddress which uses VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatdomain.html.in | 4 ++-- src/conf/domain_conf.c | 6 ++++++ 2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 95d8c45..0475527 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3343,8 +3343,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 is 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 e592adf..ce5093d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11752,6 +11752,12 @@ virDomainHostdevDefParseXML(virDomainXMLOptionPtr xmlopt, virReportError(VIR_ERR_XML_ERROR, "%s", _("SCSI host devices must have address specified")); goto error;
The body of this condition is dead code. virDomainHostdevAssignAddress only returns -1 if the subsys type is not SCSI, which we checked in the switch above.
Jan
Would you like to a see a patch that removes : if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) return -1; from virDomainHostdevAssignAddress since it's superfluous? I'm still "sitting on" this series since there was some overlap with your patch: http://www.redhat.com/archives/libvir-list/2015-June/msg00887.html Although your patch moved the decision making into runtime rather than during parse, which is mostly why I'm hesitant for at least this particular patch. Of course the decision here builds on what I did with patch 3 using the else clause for handling the situation where someone does provide a drive address to make sure there's no conflict. John
On Wed, Jul 15, 2015 at 04:05:06PM -0400, John Ferlan wrote:
On 07/15/2015 11:06 AM, Ján Tomko wrote:
On Mon, Jun 22, 2015 at 05:05:03PM -0400, John Ferlan wrote:
If a SCSI subsystem <hostdev> element is provided with an <address>, then enforce that the address type is 'drive'. If not provided, a 'drive' element was created by virDomainHostdevAssignAddress which uses VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatdomain.html.in | 4 ++-- src/conf/domain_conf.c | 6 ++++++ 2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 95d8c45..0475527 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3343,8 +3343,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 is 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 e592adf..ce5093d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11752,6 +11752,12 @@ virDomainHostdevDefParseXML(virDomainXMLOptionPtr xmlopt, virReportError(VIR_ERR_XML_ERROR, "%s", _("SCSI host devices must have address specified")); goto error;
The body of this condition is dead code. virDomainHostdevAssignAddress only returns -1 if the subsys type is not SCSI, which we checked in the switch above.
Jan
Would you like to a see a patch that removes :
if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) return -1;
from virDomainHostdevAssignAddress since it's superfluous?
Yes.
I'm still "sitting on" this series since there was some overlap with your patch:
http://www.redhat.com/archives/libvir-list/2015-June/msg00887.html
Although your patch moved the decision making into runtime rather than during parse, which is mostly why I'm hesitant for at least this particular patch.
My patch is the reason I started reviewing this one - I figured if we already check the PCI address there, we could check other types too. Jan
Of course the decision here builds on what I did with patch 3 using the else clause for handling the situation where someone does provide a drive address to make sure there's no conflict.
John
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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@redhat.com> --- src/conf/domain_conf.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ce5093d..3205c43 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5183,6 +5183,8 @@ static bool virDomainDriveAddressIsUsedByDisk(const virDomainDef *def, virDomainDiskBus type, unsigned int controller, + unsigned int bus, + unsigned int target, unsigned int unit) { virDomainDiskDefPtr disk; @@ -5197,8 +5199,8 @@ 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; } @@ -5212,6 +5214,8 @@ static bool virDomainDriveAddressIsUsedByHostdev(const virDomainDef *def, virDomainHostdevSubsysType type, unsigned int controller, + unsigned int bus, + unsigned int target, unsigned int unit) { virDomainHostdevDefPtr hostdev; @@ -5225,8 +5229,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; } @@ -5236,6 +5240,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 @@ -5245,9 +5251,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; @@ -5262,7 +5269,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
On Mon, Jun 22, 2015 at 05:05:04PM -0400, John Ferlan wrote:
Modify virDomainDriveAddressIsUsedBy{Disk|Hostdev} and virDomainSCSIDriveAddressIsUsed to take 'bus' and 'target' parameters. Will be used by future patches for more complete address conflict checks
Are all of the address components equal? I mean, if w.x.y.z is occupied, then changing just one of those gets an address that might be available. (I am thinking about PCI where some devices require a whole slot to themselves).
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-)
ACK Jan
On 07/16/2015 08:31 AM, Ján Tomko wrote:
On Mon, Jun 22, 2015 at 05:05:04PM -0400, John Ferlan wrote:
Modify virDomainDriveAddressIsUsedBy{Disk|Hostdev} and virDomainSCSIDriveAddressIsUsed to take 'bus' and 'target' parameters. Will be used by future patches for more complete address conflict checks
Are all of the address components equal? I mean, if w.x.y.z is occupied, then changing just one of those gets an address that might be available.
(I am thinking about PCI where some devices require a whole slot to themselves).
Don't believe that's logical/possible in SCSI - that is I'm not aware of any device that takes up the entire slot. As I understand the SCSI addresses bus and target can validly be non zero.Physical target assignment rules indicate to assign in ascending order according to physical location and units are component of each target. So they can be non zero. Physical target assignment rules indicate to assign in ascending order according to physical location and units are component of each target. So they can be non zero. Of course I've forgotten what exactly caused me to add bus & target to the comparison, but I think it was due to the check added in patch3 to ensure the provided <hostdev> address didn't conflict with some existing disk address. While our default rules stipulate using bus = target = 0, nothing stops someone from using "target=1". And without bus & target being included in the comparison a <disk> address using <address type='drive' controller='0' bus='0' target='1' unit='0'/> it may appear to conflict with a <hostdev> using : <address type='drive' controller='0' bus='0' target='0' unit='0'/> Once patch3 was added. I do note that the comments to both functions still had $controller:0:0:$unit and adjusted those as well to be $controller:$bus:$target:$unit John
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-)
ACK
Jan
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@redhat.com> --- src/conf/domain_conf.c | 31 ++++++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3205c43..e02cd49 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11754,18 +11754,39 @@ 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) { + if (virDomainHostdevAssignAddress(xmlopt, vmdef, def) < 0) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("SCSI host devices must have address specified")); - goto error; + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Failed to assign SCSI host " + "device address")); + goto error; + } } else if (def->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) { virReportError(VIR_ERR_XML_ERROR, "%s", _("SCSI host device must use 'drive' " "address type")); goto error; + } else { + /* 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
Rather than passing the def->seclabels and def->nseclabels, refactor the API to pass the entire domain definition. This will be used in a future patch as well. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e02cd49..6259d4a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6390,8 +6390,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, xmlNodePtr node, xmlXPathContextPtr ctxt, virHashTablePtr bootHash, - virSecurityLabelDefPtr* vmSeclabels, - int nvmSeclabels, + const virDomainDef *vmdef, unsigned int flags) { virDomainDiskDefPtr def; @@ -6930,8 +6929,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, ctxt->node = sourceNode; if (virSecurityDeviceLabelDefParseXML(&def->src->seclabels, &def->src->nseclabels, - vmSeclabels, - nvmSeclabels, + vmdef->seclabels, + vmdef->nseclabels, ctxt, flags) < 0) goto error; @@ -12256,9 +12255,7 @@ virDomainDeviceDefParse(const char *xmlStr, switch ((virDomainDeviceType) dev->type) { case VIR_DOMAIN_DEVICE_DISK: if (!(dev->data.disk = virDomainDiskDefParseXML(xmlopt, node, ctxt, - NULL, def->seclabels, - def->nseclabels, - flags))) + NULL, def, flags))) goto error; break; case VIR_DOMAIN_DEVICE_LEASE: @@ -12400,9 +12397,7 @@ virDomainDiskDefSourceParse(const char *xmlStr, flags |= VIR_DOMAIN_DEF_PARSE_DISK_SOURCE; if (!(disk = virDomainDiskDefParseXML(xmlopt, node, ctxt, - NULL, def->seclabels, - def->nseclabels, - flags))) + NULL, def, flags))) goto cleanup; ret = disk->src; @@ -15418,8 +15413,7 @@ virDomainDefParseXML(xmlDocPtr xml, nodes[i], ctxt, bootHash, - def->seclabels, - def->nseclabels, + def, flags); if (!disk) goto error; -- 2.1.0
On Mon, Jun 22, 2015 at 05:05:06PM -0400, John Ferlan wrote:
Rather than passing the def->seclabels and def->nseclabels, refactor the API to pass the entire domain definition. This will be used in a future patch as well.
I think it would be nicer to separate XML parsing (which would just record what was in the XML) and auto-generating missing configuration (like generating drive addresses or checking for conflicts). Jan
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e02cd49..6259d4a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6390,8 +6390,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, xmlNodePtr node, xmlXPathContextPtr ctxt, virHashTablePtr bootHash, - virSecurityLabelDefPtr* vmSeclabels, - int nvmSeclabels, + const virDomainDef *vmdef, unsigned int flags) { virDomainDiskDefPtr def; @@ -6930,8 +6929,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, ctxt->node = sourceNode; if (virSecurityDeviceLabelDefParseXML(&def->src->seclabels, &def->src->nseclabels, - vmSeclabels, - nvmSeclabels, + vmdef->seclabels, + vmdef->nseclabels, ctxt, flags) < 0) goto error; @@ -12256,9 +12255,7 @@ virDomainDeviceDefParse(const char *xmlStr, switch ((virDomainDeviceType) dev->type) { case VIR_DOMAIN_DEVICE_DISK: if (!(dev->data.disk = virDomainDiskDefParseXML(xmlopt, node, ctxt, - NULL, def->seclabels, - def->nseclabels, - flags))) + NULL, def, flags))) goto error; break; case VIR_DOMAIN_DEVICE_LEASE: @@ -12400,9 +12397,7 @@ virDomainDiskDefSourceParse(const char *xmlStr,
flags |= VIR_DOMAIN_DEF_PARSE_DISK_SOURCE; if (!(disk = virDomainDiskDefParseXML(xmlopt, node, ctxt, - NULL, def->seclabels, - def->nseclabels, - flags))) + NULL, def, flags))) goto cleanup;
ret = disk->src; @@ -15418,8 +15413,7 @@ virDomainDefParseXML(xmlDocPtr xml, nodes[i], ctxt, bootHash, - def->seclabels, - def->nseclabels, + def, flags); if (!disk) goto error; -- 2.1.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 07/16/2015 08:03 AM, Ján Tomko wrote:
On Mon, Jun 22, 2015 at 05:05:06PM -0400, John Ferlan wrote:
Rather than passing the def->seclabels and def->nseclabels, refactor the API to pass the entire domain definition. This will be used in a future patch as well.
I think it would be nicer to separate XML parsing (which would just record what was in the XML) and auto-generating missing configuration (like generating drive addresses or checking for conflicts).
Jan
Are you advocating removing virDomainDiskDefAssignAddress() from virDomainDiskDefParseXML() and relying on qemuParseCommandLineDisk() and qemuParseCommandLine()? Following around all the existing "assumptions" especially as they relate to hotplug and even perhaps blockcopy could be an adventure. Not to say that I don't disagree with the concept separating parse from autofill; however, that seems outside the scope of this particular issue John
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e02cd49..6259d4a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6390,8 +6390,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, xmlNodePtr node, xmlXPathContextPtr ctxt, virHashTablePtr bootHash, - virSecurityLabelDefPtr* vmSeclabels, - int nvmSeclabels, + const virDomainDef *vmdef, unsigned int flags) { virDomainDiskDefPtr def; @@ -6930,8 +6929,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, ctxt->node = sourceNode; if (virSecurityDeviceLabelDefParseXML(&def->src->seclabels, &def->src->nseclabels, - vmSeclabels, - nvmSeclabels, + vmdef->seclabels, + vmdef->nseclabels, ctxt, flags) < 0) goto error; @@ -12256,9 +12255,7 @@ virDomainDeviceDefParse(const char *xmlStr, switch ((virDomainDeviceType) dev->type) { case VIR_DOMAIN_DEVICE_DISK: if (!(dev->data.disk = virDomainDiskDefParseXML(xmlopt, node, ctxt, - NULL, def->seclabels, - def->nseclabels, - flags))) + NULL, def, flags))) goto error; break; case VIR_DOMAIN_DEVICE_LEASE: @@ -12400,9 +12397,7 @@ virDomainDiskDefSourceParse(const char *xmlStr,
flags |= VIR_DOMAIN_DEF_PARSE_DISK_SOURCE; if (!(disk = virDomainDiskDefParseXML(xmlopt, node, ctxt, - NULL, def->seclabels, - def->nseclabels, - flags))) + NULL, def, flags))) goto cleanup;
ret = disk->src; @@ -15418,8 +15413,7 @@ virDomainDefParseXML(xmlDocPtr xml, nodes[i], ctxt, bootHash, - def->seclabels, - def->nseclabels, + def, flags); if (!disk) goto error; -- 2.1.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Jul 16, 2015 at 11:24:33AM -0400, John Ferlan wrote:
On 07/16/2015 08:03 AM, Ján Tomko wrote:
On Mon, Jun 22, 2015 at 05:05:06PM -0400, John Ferlan wrote:
Rather than passing the def->seclabels and def->nseclabels, refactor the API to pass the entire domain definition. This will be used in a future patch as well.
I think it would be nicer to separate XML parsing (which would just record what was in the XML) and auto-generating missing configuration (like generating drive addresses or checking for conflicts).
Jan
Are you advocating removing virDomainDiskDefAssignAddress() from virDomainDiskDefParseXML() and relying on qemuParseCommandLineDisk() and qemuParseCommandLine()?
Nobody should rely on qemuParseCommandLine, it is known to be unreliable. :)
Following around all the existing "assumptions" especially as they relate to hotplug and even perhaps blockcopy could be an adventure. Not to say that I don't disagree with the concept separating parse from autofill; however, that seems outside the scope of this particular issue
I meant moving the autofill logic to virDomainDefPostParseInternal and virDomainDeviceDefPostParseInternal. The latter is also called by the hotplug code. So the only assumptions possibly broken would be during parsing of other devices - between disks/hostdev parsing and calling the PostParse functions. It seems the only affected function is virDomainDefMaybeAddHostdevSCSIcontroller which can be moved to virDomainDefAddImplicitControllers. Jan
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@redhat.com> --- src/conf/domain_conf.c | 40 +++++++++++++++++++++++++++++----------- 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, 47 insertions(+), 25 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6259d4a..0999e86 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5672,7 +5672,8 @@ virDomainDiskFindByBusAndDst(virDomainDefPtr def, int virDomainDiskDefAssignAddress(virDomainXMLOptionPtr xmlopt, - virDomainDiskDefPtr def) + virDomainDiskDefPtr def, + const virDomainDef *vmdef) { int idx = virDiskNameToIndex(def->dst); if (idx < 0) { @@ -5683,7 +5684,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) { @@ -5692,22 +5696,36 @@ 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; + } + + if (virDomainDriveAddressIsUsedByHostdev(vmdef, + VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI, + controller, 0, 0, unit)) { + virReportError(VIR_ERR_XML_ERROR, + _("address generated using disk target name '%s' " + "may conflict 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; + def->info.addr.drive.unit = unit; break; + } case VIR_DOMAIN_DISK_BUS_IDE: /* For IDE we define the default mapping to be 2 units @@ -7261,7 +7279,7 @@ 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) + && virDomainDiskDefAssignAddress(xmlopt, def, vmdef) < 0) goto error; if (virDomainDiskBackingStoreParse(ctxt, def->src) < 0) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index c96a6e4..5b1c838 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2717,7 +2717,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 5444638..11e8e16 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -11784,7 +11784,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); @@ -12927,7 +12927,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 aede2ad..d031398 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
On Mon, Jun 22, 2015 at 05:05:07PM -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@redhat.com> --- src/conf/domain_conf.c | 40 +++++++++++++++++++++++++++++----------- 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, 47 insertions(+), 25 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6259d4a..0999e86 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5672,7 +5672,8 @@ virDomainDiskFindByBusAndDst(virDomainDefPtr def,
int virDomainDiskDefAssignAddress(virDomainXMLOptionPtr xmlopt, - virDomainDiskDefPtr def) + virDomainDiskDefPtr def, + const virDomainDef *vmdef) { int idx = virDiskNameToIndex(def->dst); if (idx < 0) { @@ -5683,7 +5684,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) { @@ -5692,22 +5696,36 @@ 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; + } +
This mixes non-functional and functional changes.
+ if (virDomainDriveAddressIsUsedByHostdev(vmdef, + VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI, + controller, 0, 0, unit)) { + virReportError(VIR_ERR_XML_ERROR, + _("address generated using disk target name '%s' " + "may conflict with SCSI host device address " + "controller='%u' bus='%u' target='%u' unit='%u"), + def->dst, controller, 0, 0, unit);
'may conflict' sounds ambiguous. Either it does conflict and we should reject it, or it does not and there is no error. And this seems like a problem of libvirt's address generation algorithm, not an XML error. Jan
On 07/16/2015 08:23 AM, Ján Tomko wrote:
On Mon, Jun 22, 2015 at 05:05:07PM -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@redhat.com> --- src/conf/domain_conf.c | 40 +++++++++++++++++++++++++++++----------- 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, 47 insertions(+), 25 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6259d4a..0999e86 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5672,7 +5672,8 @@ virDomainDiskFindByBusAndDst(virDomainDefPtr def,
int virDomainDiskDefAssignAddress(virDomainXMLOptionPtr xmlopt, - virDomainDiskDefPtr def) + virDomainDiskDefPtr def, + const virDomainDef *vmdef) { int idx = virDiskNameToIndex(def->dst); if (idx < 0) { @@ -5683,7 +5684,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) { @@ -5692,22 +5696,36 @@ 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; + } +
This mixes non-functional and functional changes.
OK - I can separate out out just the non-functional into its own patch. Do you want me to send it out again or trust that I know what I'm doing?
+ if (virDomainDriveAddressIsUsedByHostdev(vmdef, + VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI, + controller, 0, 0, unit)) { + virReportError(VIR_ERR_XML_ERROR, + _("address generated using disk target name '%s' " + "may conflict with SCSI host device address " + "controller='%u' bus='%u' target='%u' unit='%u"), + def->dst, controller, 0, 0, unit);
'may conflict' sounds ambiguous. Either it does conflict and we should reject it, or it does not and there is no error.
And this seems like a problem of libvirt's address generation algorithm, not an XML error.
Jan
Right - it does conflict... The issue is that the 'idx' is generated as a result of the target device name ("dst"). The assumption being "sda" would be controller = unit = 0, 'sdb' would be ctlr = 0, unit = 1, 'sdc' would be ctlr = 0, unit = 2, etc. However, for hostdev's there is no such "dst" logic - it's a straight assignment. When the guest boots, whatever is assigned to "c = 0, u = 0" is assigned to 'sda' (regardless of whether it's a hostdev or disk). So if the hostdev which had it's address provided in the XML, but the disk which didn't *and* is using "<target dev='sda'.../>", then there's a possible conflict because "sda" resolves to "c = 0, b = 0". The error message I guess should thus state : "using disk target name '%s' conflicts with SCSI host device address ..." Would you still call that an XML error or perhaps CONFIG_UNSUPPORTED John
On Thu, Jul 16, 2015 at 11:57:52AM -0400, John Ferlan wrote:
On 07/16/2015 08:23 AM, Ján Tomko wrote:
On Mon, Jun 22, 2015 at 05:05:07PM -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@redhat.com> --- src/conf/domain_conf.c | 40 +++++++++++++++++++++++++++++----------- 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, 47 insertions(+), 25 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6259d4a..0999e86 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5672,7 +5672,8 @@ virDomainDiskFindByBusAndDst(virDomainDefPtr def,
int virDomainDiskDefAssignAddress(virDomainXMLOptionPtr xmlopt, - virDomainDiskDefPtr def) + virDomainDiskDefPtr def, + const virDomainDef *vmdef) { int idx = virDiskNameToIndex(def->dst); if (idx < 0) { @@ -5683,7 +5684,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) { @@ -5692,22 +5696,36 @@ 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; + } +
This mixes non-functional and functional changes.
OK - I can separate out out just the non-functional into its own patch. Do you want me to send it out again or trust that I know what I'm doing?
Both. I did not give it an 'ACK with that split out' because I expected another series with the address generation done after all is parsed.
+ if (virDomainDriveAddressIsUsedByHostdev(vmdef, + VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI, + controller, 0, 0, unit)) { + virReportError(VIR_ERR_XML_ERROR, + _("address generated using disk target name '%s' " + "may conflict with SCSI host device address " + "controller='%u' bus='%u' target='%u' unit='%u"), + def->dst, controller, 0, 0, unit);
The disks are parsed before hostdevs, so this will only catch conflicts on hotplug.
'may conflict' sounds ambiguous. Either it does conflict and we should reject it, or it does not and there is no error.
And this seems like a problem of libvirt's address generation algorithm, not an XML error.
Jan
Right - it does conflict...
The issue is that the 'idx' is generated as a result of the target device name ("dst"). The assumption being "sda" would be controller = unit = 0, 'sdb' would be ctlr = 0, unit = 1, 'sdc' would be ctlr = 0, unit = 2, etc.
However, for hostdev's there is no such "dst" logic - it's a straight assignment.
When the guest boots, whatever is assigned to "c = 0, u = 0" is assigned to 'sda' (regardless of whether it's a hostdev or disk).
So if the hostdev which had it's address provided in the XML, but the disk which didn't *and* is using "<target dev='sda'.../>", then there's a possible conflict because "sda" resolves to "c = 0, b = 0". The error message I guess should thus state :
"using disk target name '%s' conflicts with SCSI host device address ..."
Would you still call that an XML error or perhaps CONFIG_UNSUPPORTED
Oh, so the targets are not really guaranteed. I guess that could be considered an XML error, even if it is caused by our interpretation of the (mandatory) target attribute. Jan
On 06/22/2015 05:05 PM, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1210587
These patches will resolve a couple issues with generation of the <address type='drive' .../> for a SCSI <disk> and <hostdev>.
The <disk> generation algorithm 'assumes' that when presented with <target dev='sda'.../> that it can use controller=0 and unit=0 since sda would conceivably be the first device; however, a <hostdev> could attempt to assign itself to that address and it doesn't have a target device name, so it bypasses the virDomainDiskDefDstDuplicates checks that would normally 'catch' two <disk>'s attempting to use the same name.
Likewise, if a <hostdev> occupies an <address> and we attempt to hotplug a <disk> without providing an address, the address generation could attempt to place the disk on the already existing host device.
John Ferlan (5): conf: Enforce SCSI hostdev address type conf: Add 'bus' and 'target' to SCSI address conflict checks conf: Add SCSI hostdev check for disk drive address already in use conf: Refactor virDomainDiskDefParseXML to pass vmdef conf: Check for hostdev conflicts when assign default disk address
docs/formatdomain.html.in | 4 +- src/conf/domain_conf.c | 115 ++++++++++++++++++++++++++++++++-------------- 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, 101 insertions(+), 50 deletions(-)
For what it's worth, other than the one doc change suggestion, the rest looks (and works) fine to me. Reviewed-by: Eric Farman <farman@linux.vnet.ibm.com> - Eric
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@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 5a9a88d..39f86a2 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
participants (3)
-
Eric Farman -
John Ferlan -
Ján Tomko