Re: [libvirt] [PATCH 1/5] conf: Enforce SCSI hostdev address type

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(a)redhat.com&gt; >> --- >> 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 -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list