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

[John Ferlan]

On 10/06/2014 08:45 AM, Ján Tomko wrote:
> On 10/06/2014 01:03 PM, John Ferlan wrote:

<...snip...>

>>>>  
>>>>  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.
> 

Hmm.. true...  Although similar other SCSI_HOST and FC_HOST functions
are in util/virutil.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.
> 

yeah and this makes the getHostNumber a bit more tricky - especially
with respect to virDevicePCIAddress which when added to virutil.{c,h}
created a mess

>>
>> 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.
> 

In any case, I see what you're driving at - I'm reworking the patches
and will post a new series shortly...

John

FWIW:
It seems commit id 'b52fbad1' (interesting sequence for hash id :-))
should never have allowed 'fc_host' as a value for the name property.
Oh well, what's done is done I guess.