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

[Martin Wilck]

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.

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.

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