[libvirt] [PATCH 3/4] storage_conf: Fix the scsi_host.name comparison

Ján Tomko jtomko at redhat.com
Mon Oct 6 12:45:44 UTC 2014


On 10/06/2014 01:03 PM, John Ferlan wrote:
> 
> 
> On 10/03/2014 09:20 AM, Ján Tomko wrote:
>> On 09/30/2014 11:35 PM, John Ferlan wrote:
>>> Since the 'scsi_host#' concept was introduced in commit id '9f781da6' it
>>> is possible to provide either "name='host#'" or "name='scsi_host#'" to
>>> define/name the scsi_host adapter source address.  The concept being
>>> that 'name#' is/was legacy and 'scsi_host#' is what's seen in the
>>> 'virsh nodedev-list [--cap scsi_host]' output.  However, in doing the
>>> comparison for a to be defined scsi_host, a latent bug allows the same
>>> source to be defined twice, e.g. "name='host5'" is not distiguished
>>> from "name='scsi_host5'", although in reality they eventually become
>>> the same thing in commit id 'c1f63a9b' with the introduction of the
>>> getHostNumber() API.
>>>
>>> So this change will ensure that no one can use 'host5' and 'scsi_host5'
>>> (or whatever #) to resolve to the same source adapter when going through
>>> the define the source adapter processing and checking for duplicates. Doing
>>> so will now result in an error (assuming that an existing pool is using
>>> 'host5' and the to be defined pool tries 'scsi_host5'):
>>>
>>> $ virsh pool-define scsi-pool-use-scsi_host.xml
>>> error: Failed to define pool from scsi-pool-use-scsi_host.xml
>>> error: operation failed: Storage source conflict with pool: 'scsi-pool-use-host
>>> $
>>>
>>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>>> ---
>>>  src/conf/storage_conf.c | 25 +++++++++++++++++++++++--
>>>  1 file changed, 23 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
>>> index ac41307..74267bd 100644
>>> --- a/src/conf/storage_conf.c
>>> +++ b/src/conf/storage_conf.c
>>> @@ -2063,6 +2063,27 @@ virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools,
>>>  }
>>>  
>>>  static bool
>>> +matchSCSIAdapterName(const char *pool_name,
>>> +                     const char *def_name)
>>> +{
>>> +    /* Names can be either "scsi_host#" or just "host#", where
>>> +     * "host#" is the back-compat format, but both equate to
>>> +     * the same source adapter.  First check if both pool and def
>>> +     * are using same format (easier) - if so, then compare
>>> +     */
>>> +    if ((STRPREFIX(pool_name, "scsi_") && STRPREFIX(def_name, "scsi")) ||
>>> +        (STRPREFIX(pool_name, "host") && STRPREFIX(def_name, "host")))
>>> +        return STREQ(pool_name, def_name);
>>> +
>>> +    /* If pool uses "scsi_host#" and def uses "host#", deal with that here */
>>> +    if (STRPREFIX(pool_name, "scsi_"))
>>> +        return STREQ(&pool_name[5], def_name);
>>> +
>>> +    /* Otherwise we have pool with "host#" and def with "scsi_host#" */
>>> +    return STREQ(pool_name, &def_name[5]);
>>> +}
>>
>> fc_host prefix is not handled here, but getHostNumber will allow it. Maybe the
>> checks should be shared? (as long as we don't error out on unknown prefixes,
>> since we didn't validate the adapter name in the past).
>>
> 
> Not clear what kind of sharing would be expected (perhaps it's my code
> myopia)...  

Calling getHostNumber on both pool_name and def_name and comparing the result,
or splitting out the part skipping the prefixes into something in util/virscsi.c.

> The previous (and current to this patch) code does validate
> the name - at least to the degree that the incoming name isn't already
> in use or the name that the incoming definition would resolve to in the
> case of parentaddr. It is broken - which is what this set of patches
> looks to resolve.

Currently, we don't resolve the parentaddr, just compare it to other addresses.

> 
> If you go back to patch 1/4 - you will see for a "type='scsi_host'" pool
> we'd previously either simply match the incoming name against name of
> the pool (assuming the the incoming def had a name instead of
> parentaddr) or we'd match the parentaddr (assuming that if the current
> pool def was using a parentaddr that the incoming def would be too).
> 
> All this patch does is ensure that someone cannot provide "name='host3'"
> for one pool while providing "name='scsi_host3'" for another pool for
> the Create/Define (or vice versa). There is no bug on this - I just
> noted this while working on the code.
> 
> matchSCSIAdapter[Name|Parent] is called during the Create/Define pool
> processing to ensure we don't allow user defined duplicate names of
> existing pools for SCSI_HOST pools only (eg, type='scsi_host' instead of
> type='fc_host'). A FC_HOST pool would disallow matches for the unique
> wwnn/wwpn pairs.  Yes it does use "parent='scsi_host#'" as a name, but
> that's only to find the scsi_host# defined - see
> http://wiki.libvirt.org/page/NPIV_in_libvirt during the start or refresh
> processing (in getHostNumber).
> 
> The getHostNumber is used by the scsi pool driver during the Check or
> Refresh processing in order to fetch which user provided name that was
> created/defined for use in finding the on disk
> /sys/class/scsi_host/host# directory in order to find either the fc_host
> or scsi_host data.  The fc_host processing has/uses a
> "parent='scsi_host#'" in order to define the vHBA with the the vport
> (wwnn/wwpn).  I'm not even sure at this point if fc_host could be a
> proper prefix, but that's a different issue.
> 

I haven't actually tried it, but from looking at the code, for a storage pool
with VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST name='fc_host3' would also
be duplicate with name='host3' and name='scsi_host3'. The name is not
validated on definition and the fc_host prefix will be stripped (just as
scsi_host or host) in getHostNumber.

Jan

> I can rename this set of functions to something like
> matchSCSIHostAdapterType[Name|Parent] if it's clearer, but I guess I'm
> missing the synergy.
> 
> 
> 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: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20141006/5cb0858b/attachment-0001.sig>


More information about the libvir-list mailing list