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

[John Ferlan]

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?

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