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@xxxxxxxxxx> > > --- > > 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@xxxxxxxx>, Tel. +49 (0)911 74053 2107 > SUSE > Software Solutions Germany GmbH > HRB 36809 (AG Nürnberg) GF: Felix > Imendörffer > -- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel