[dm-devel] [PATCH 09/15] libmultipath: add code to get vendor specific vpd data

[Benjamin Marzinski]

On Fri, Jan 17, 2020 at 05:56:40PM +0100, Martin Wilck wrote:
> On Thu, 2020-01-16 at 20:18 -0600, Benjamin Marzinski wrote:
> > This adds the wildcard 'g' for multipath and path formatted printing,
> > which returns extra data from a device's vendor specific vpd
> > page.  The
> > specific vendor vpd page to use, and the vendor/product id to decode
> > it
> > can be set in the hwentry with vpd_vendor_pg and vpd_vendor_id. It
> > can
> > be configured in the devices section of multipath.conf with the
> > vpd_vendor parameter. Currently, the only devices that use this are
> > HPE
> > 3PAR arrays, to return the Volume Name.
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins at redhat.com>
> > ---
> >  libmultipath/config.c      |  4 ++++
> >  libmultipath/config.h      |  2 ++
> >  libmultipath/dict.c        | 34 ++++++++++++++++++++++++++++++++++
> >  libmultipath/discovery.c   | 34 +++++++++++++++++++++++++++++++++-
> >  libmultipath/hwtable.c     |  2 ++
> >  libmultipath/print.c       | 27 +++++++++++++++++++++++++++
> >  libmultipath/propsel.c     | 24 ++++++++++++++++++++++++
> >  libmultipath/propsel.h     |  2 ++
> >  libmultipath/structs.h     |  9 +++++++++
> >  multipath/multipath.conf.5 |  8 ++++++++
> >  10 files changed, 145 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libmultipath/config.c b/libmultipath/config.c
> > index 85626e96..72b8d37c 100644
> > --- a/libmultipath/config.c
> > +++ b/libmultipath/config.c
> > @@ -369,6 +369,8 @@ merge_hwe (struct hwentry * dst, struct hwentry *
> > src)
> >  	merge_num(max_sectors_kb);
> >  	merge_num(ghost_delay);
> >  	merge_num(all_tg_pt);
> > +	merge_num(vpd_vendor_pg);
> > +	merge_num(vpd_vendor_id);
> >  	merge_num(san_path_err_threshold);
> >  	merge_num(san_path_err_forget_rate);
> >  	merge_num(san_path_err_recovery_time);
> > @@ -517,6 +519,8 @@ store_hwe (vector hwtable, struct hwentry * dhwe)
> >  	hwe->detect_prio = dhwe->detect_prio;
> >  	hwe->detect_checker = dhwe->detect_checker;
> >  	hwe->ghost_delay = dhwe->ghost_delay;
> > +	hwe->vpd_vendor_pg = dhwe->vpd_vendor_pg;
> > +	hwe->vpd_vendor_id = dhwe->vpd_vendor_id;
> >  
> >  	if (dhwe->bl_product && !(hwe->bl_product = set_param_str(dhwe-
> > >bl_product)))
> >  		goto out;
> > diff --git a/libmultipath/config.h b/libmultipath/config.h
> > index e69aa07c..589146de 100644
> > --- a/libmultipath/config.h
> > +++ b/libmultipath/config.h
> > @@ -87,6 +87,8 @@ struct hwentry {
> >  	int max_sectors_kb;
> >  	int ghost_delay;
> >  	int all_tg_pt;
> > +	int vpd_vendor_pg;
> > +	int vpd_vendor_id;
> >  	char * bl_product;
> >  };
> >  
> > diff --git a/libmultipath/dict.c b/libmultipath/dict.c
> > index 2b046e1d..d6d8b79b 100644
> > --- a/libmultipath/dict.c
> > +++ b/libmultipath/dict.c
> > @@ -1366,6 +1366,39 @@ def_uxsock_timeout_handler(struct config
> > *conf, vector strvec)
> >  	return 0;
> >  }
> >  
> > +static int
> > +hw_vpd_vendor_handler(struct config *conf, vector strvec)
> > +{
> > +	int rc = 0;
> > +	char *buff;
> > +
> > +	struct hwentry * hwe = VECTOR_LAST_SLOT(conf->hwtable);
> > +	if (!hwe)
> > +		return 1;
> > +
> > +	buff = set_value(strvec);
> > +	if (!buff)
> > +		return 1;
> > +	if (strcmp(buff, "hp3par") == 0) {
> > +		hwe->vpd_vendor_pg = 0xc0;
> > +		hwe->vpd_vendor_id = VPD_VP_HP3PAR;
> > +	} else
> > +		rc = 1;
> > +	FREE(buff);
> > +	return rc;
> > +}
> > +
> > +static int
> > +snprint_hw_vpd_vendor(struct config *conf, char * buff, int len,
> > +		      const void * data)
> > +{
> > +	const struct hwentry * hwe = (const struct hwentry *)data;
> > +
> > +	if (hwe->vpd_vendor_pg == 0xc0 && hwe->vpd_vendor_id ==
> > VPD_VP_HP3PAR)
> > +		return snprintf(buff, len, "hp3par");
> > +	return 0;
> > +}
> 
> I don't understand this design. The way you set up the hwe, it would be
> logical to add "vpd_vendor_page" and "vpd_vendor_id" properties for
> device entries.

The vpd page abbreviations specify both the page to read, and the vendor
id to use to decode it, and they are more user readable. It seemed like
a much more foolproof way to have people specify this.
 
> BUT ok, that's overengineered with just one supported combination of
> page and vendor. I can understand that. But then, it seems also
> overengineered to carry around vpd_vendor_pg and vpd_vendor_id in the
> hwe. 
> 
> I'd suggest creating a hard-coded table with "vendor vpd schemes",
> 
> struct {
> 	int pg;
> 	int vendor_id;
> 	const char *name;
> } vendor_vpd_schemes[]  = { 
>     { 0xc0, VPD_VP_HP3PAR, "hp3par", }, 
>     { 0, }, 
> };
> 
> and in the hwentry, only carry around the index into this table,
> and look up the page and vendor ID there. Actually, with just that
> single user, we might as well not introduce a new device property at
> all, but simply use a separate hardcoded table with lookup values for
> vendor and product ID. I'm unsure if it's wise to make this
> configurable via multipath.conf.

Sure. I can make the hwe change. The reason why I added this to
multipath.conf is that I'm not sure what devices actually support these
vpd information pages.  So, if we miss one, or a new device comes out
that isn't in the built-in config, the user can still get this info.

> Regards,
> Martin
> 
> -- 
> Dr. Martin Wilck <mwilck at suse.com>, Tel. +49 (0)911 74053 2107
> SUSE
> Software Solutions Germany GmbH 
> HRB 36809 (AG Nürnberg) GF: Felix
> Imendörffer
>