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

On 10/06/2014 01:03 PM, John Ferlan wrote:

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