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

[Ján Tomko]

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 at 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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150716/f5aa698f/attachment-0001.sig>