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.