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@xxxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
>
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list