[libvirt] [PATCH v2 3/3] storage_conf: Resolve libvirtd crash matching scsi_host

[Ján Tomko]

On 10/29/2014 01:49 PM, John Ferlan wrote:
> 
> 
> On 10/29/2014 07:31 AM, Ján Tomko wrote:
>> On 10/06/2014 11:49 PM, John Ferlan wrote:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1146837
>>>
>>> Resolve a crash in libvirtd resulting from commit id 'a4bd62ad' (1.0.6)
>>> which added parentaddr and unique_id to allow unique identification of
>>> a scsi_host, but assumed that all the pool entries and the incoming
>>> definition would be similarly defined. If the existing pool uses the
>>> 'name' attribute and an incoming pool is using the parentaddr/unique_id,
>>> then the code will attempt to compare the existing name string against
>>> the incoming name string which doesn't exist (is NULL) and results in
>>> a core (STREQ).
>>>
>>> Conversely, if the existing pool used the parentaddr/unique_id and the
>>> to be defined pool used the name, then the comparison would be against
>>> the parentaddr, but since the incoming pool doesn't have one - that would
>>> leave the comparison against a parentaddr of all 0's and a unique_id of 0,
>>> which will always comparison to fail. This means someone could define the
>>> same source adapter for two pools
>>>
>>> In order to resolve this, adjust the code to get the 'host#' to be used
>>> by the storage scsi backend in order to check/start the pool and make sure
>>> the incoming definition doesn't match any of the existing pool defs.
>>>
>>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>>> ---
>>>  src/conf/storage_conf.c | 69 ++++++++++++++++++++++++++++++-------------------
>>>  1 file changed, 42 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
>>> index 36696a4..19c452b 100644
>>> --- a/src/conf/storage_conf.c
>>> +++ b/src/conf/storage_conf.c
>>> @@ -2062,26 +2062,37 @@ virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools,
>>>      return ret;
>>>  }
>>>  
>>> -static bool
>>> -matchSCSIAdapterParent(virStoragePoolObjPtr pool,
>>> -                       virStoragePoolDefPtr def)
>>> +static int
>>> +getSCSIHostNumber(virStoragePoolSourceAdapter adapter,
>>> +                  unsigned int *hostnum)
>>>  {
>>> -    virDevicePCIAddressPtr pooladdr =
>>> -        &pool->def->source.adapter.data.scsi_host.parentaddr;
>>> -    virDevicePCIAddressPtr defaddr =
>>> -        &def->source.adapter.data.scsi_host.parentaddr;
>>> -    int pool_unique_id =
>>> -        pool->def->source.adapter.data.scsi_host.unique_id;
>>> -    int def_unique_id =
>>> -        def->source.adapter.data.scsi_host.unique_id;
>>> -    if (pooladdr->domain == defaddr->domain &&
>>> -        pooladdr->bus == defaddr->bus &&
>>> -        pooladdr->slot == defaddr->slot &&
>>> -        pooladdr->function == defaddr->function &&
>>> -        pool_unique_id == def_unique_id) {
>>> -        return true;
>>> -    }
>>> -    return false;
>>> +    int ret = -1;
>>> +    unsigned int num;
>>> +    char *name = NULL;
>>> +
>>> +    if (adapter.data.scsi_host.has_parent) {
>>> +        virDevicePCIAddress addr = adapter.data.scsi_host.parentaddr;
>>> +        unsigned int unique_id = adapter.data.scsi_host.unique_id;
>>> +
>>> +        if (!(name = virGetSCSIHostNameByParentaddr(addr.domain,
>>> +                                                    addr.bus,
>>> +                                                    addr.slot,
>>> +                                                    addr.function,
>>> +                                                    unique_id)))
>>> +            goto cleanup;
>>
>> This still has the same problem v1 does:
>> https://www.redhat.com/archives/libvir-list/2014-October/msg00117.html
>>
>> A valid pool definition for an existing device can be refused because another
>> definition, the one we already accepted, is invalid. I think this is strange
>> behavior and I would rather allow duplicit pools if the user went through the
>> trouble of bypassing our checks.
>>
> 
> I think I'm missing your point. The finding of duplicate sources for
> storage pools and disallowing them to be defined (or created) has been
> around since commit id '5a1f27287".
> 
> I'm not sure what you meant by your first sentence - perhaps an example
> would help me especially in the accepted, but invalid condition.

Perhaps 'invalid' wasn't the best choice - the definition itself is valid, but
it doesn't refer to an existing device so the pool cannot be started up.

It's possible to define a pool using parentaddr (even on a host with no SCSI,
as the address is only used on pool startup, not definition):
    <adapter type='scsi_host'>
      <parentaddr unique_id='1'>
        <address domain='0x0000' bus='0x00' slot='0x1f' function='0x0'/>
      </parentaddr>
    </adapter>

After that, defining a pool with scsi_host source fails. Both via name:
<adapter type='scsi_host' name='scsi_host13'/>
and via parenaddr:
    <adapter type='scsi_host'>
      <parentaddr unique_id='1'>
        <address domain='0x0000' bus='0x00' slot='0x10' function='0x0'/>
      </parentaddr>
    </adapter>
because of the first, already accepted definition:
error: Failed to define pool from scsi2.xml
error: XML error: Failed to find scsi_host using PCI '0000:00:1f.0' and
unique_id='1'

Jan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20141029/72e6d3be/attachment-0001.sig>