Hello Lixiaokeng, On Thu, 2020-07-30 at 21:27 +0800, lixiaokeng wrote: > Hi. > I'm very sorry for subject mistake in first mail. > > In set_ble_device func, if blist is NULL or ble is NULL, > the vendor and product isn't free. We think it is not > reasonable that strdup(XXX) is used as set_ble_device > and store_ble functions' parameter. > > Here we call strdup() in store_ble and set_ble_device > functions and the string will be free if functions fail. > Because constant string like "sdb" will be their parameter, > char * is changed to const char *. This is base on > upstream-queue branch in openSUSE/multipath-tools. > > Here we have another question. We want to know which branch > is mainline in https://github.com/openSUSE/multipath-tools. > The master is not changed since two years ago. And we find > the ups/submit-2007 branch in mwilck/multipath-tools have > a great changes. Do you have a plan to merge it to openSUSE. > > Signed-off-by: Lixiaokeng <lixiaokeng@xxxxxxxxxx> > Signed-off-by: Zhiqiang Liu <liuzhiqiang26@xxxxxxxxxx> Thanks for the patch, it looks good. I have some minor formal issues below. Regards, Martin > > --- > libmultipath/blacklist.c | 74 +++++++++++++++++++++++--------------- > -- > libmultipath/blacklist.h | 4 +-- > tests/blacklist.c | 31 +++++++---------- > 3 files changed, 58 insertions(+), 51 deletions(-) > > diff --git a/libmultipath/blacklist.c b/libmultipath/blacklist.c > index db58ccca..04aeb426 100644 > --- a/libmultipath/blacklist.c > +++ b/libmultipath/blacklist.c > @@ -29,14 +29,19 @@ char *check_invert(char *str, bool *invert) > return str; > } > > -int store_ble(vector blist, char * str, int origin) > +int store_ble(vector blist, const char * str, int origin) Please use kernel formatting conventions for added code, i.e. "const char *str". > { > struct blentry * ble; > char *regex_str; > + char *strdup_str = NULL; > > if (!str) > return 0; > > + strdup_str = strdup(str); > + if (!strdup_str) > + return 1; > + > if (!blist) > goto out; > > @@ -45,21 +50,21 @@ int store_ble(vector blist, char * str, int > origin) > if (!ble) > goto out; > > - regex_str = check_invert(str, &ble->invert); > + regex_str = check_invert(strdup_str, &ble->invert); > if (regcomp(&ble->regex, regex_str, REG_EXTENDED|REG_NOSUB)) > goto out1; > > if (!vector_alloc_slot(blist)) > goto out1; > > - ble->str = str; > + ble->str = strdup_str; > ble->origin = origin; > vector_set_slot(blist, ble); > return 0; > out1: > FREE(ble); > out: > - FREE(str); > + FREE(strdup_str); > return 1; > } > > @@ -79,10 +84,12 @@ int alloc_ble_device(vector blist) > return 0; > } > > -int set_ble_device(vector blist, char * vendor, char * product, int > origin) > +int set_ble_device(vector blist, const char * vendor, const char * > product, int origin) > { > struct blentry_device * ble; > char *regex_str; > + char *vendor_str = NULL; > + char *product_str = NULL; > > if (!blist) > return 1; > @@ -93,31 +100,42 @@ int set_ble_device(vector blist, char * vendor, > char * product, int origin) > return 1; > > if (vendor) { > - regex_str = check_invert(vendor, &ble->vendor_invert); > + vendor_str = STRDUP(vendor); > + if (!vendor_str) > + goto out; > + > + regex_str = check_invert(vendor_str, &ble- > >vendor_invert); > if (regcomp(&ble->vendor_reg, regex_str, > REG_EXTENDED|REG_NOSUB)) { > - FREE(vendor); > - if (product) > - FREE(product); > - return 1; > + goto out; > } > - ble->vendor = vendor; > + ble->vendor = vendor_str; > } > if (product) { > - regex_str = check_invert(product, &ble- > >product_invert); > + product_str = STRDUP(product); > + if (!product_str) > + goto out1; > + > + regex_str = check_invert(product_str, &ble- > >product_invert); > if (regcomp(&ble->product_reg, regex_str, > REG_EXTENDED|REG_NOSUB)) { > - FREE(product); > - if (vendor) { > - ble->vendor = NULL; > - FREE(vendor); > - } > - return 1; > + goto out1; > } > - ble->product = product; > + ble->product = product_str; > } > ble->origin = origin; > return 0; > +out1: > + if (vendor_str) > + ble->vendor = NULL; > +out: > + if (vendor_str) > + FREE(vendor_str); > + > + if (product_str) > + FREE(product_str); These if's are superfluous; free() ignores NULL pointers. Btw, while using FREE() here isn't wrong, I tend to not use it any more. IMO it's better to simply call free(), and set the pointer to NULL explicitly when necessary. multipath-tools builtin memory allocation debugging facility (dbg_free() etc) facility is obsolete, as valgrind etc. are far more powerful. > + > + return 1; > } > > static int > @@ -178,20 +196,16 @@ setup_default_blist (struct config * conf) > { > struct blentry * ble; > struct hwentry *hwe; > - char * str; > int i; > > - str = STRDUP("!^(sd[a-z]|dasd[a-z]|nvme[0-9])"); > - if (!str) > - return 1; > - if (store_ble(conf->blist_devnode, str, ORIGIN_DEFAULT)) > + if (store_ble(conf->blist_devnode, "!^(sd[a-z]|dasd[a- > z]|nvme[0-9])" > + , ORIGIN_DEFAULT)) { > return 1; > + } > > - str = STRDUP("(SCSI_IDENT_|ID_WWN)"); > - if (!str) > - return 1; > - if (store_ble(conf->elist_property, str, ORIGIN_DEFAULT)) > + if (store_ble(conf->elist_property, "(SCSI_IDENT_|ID_WWN)", > ORIGIN_DEFAULT)) { > return 1; > + } > > vector_foreach_slot (conf->hwtable, hwe, i) { > if (hwe->bl_product) { > @@ -202,9 +216,7 @@ setup_default_blist (struct config * conf) > return 1; > ble = VECTOR_SLOT(conf->blist_device, > VECTOR_SIZE(conf- > >blist_device) - 1); > - if (set_ble_device(conf->blist_device, > - STRDUP(hwe->vendor), > - STRDUP(hwe->bl_product), > + if (set_ble_device(conf->blist_device,hwe- > >vendor,hwe->bl_product, > ORIGIN_DEFAULT)) { > FREE(ble); > vector_del_slot(conf->blist_device, > VECTOR_SIZE(conf->blist_device) - 1); > diff --git a/libmultipath/blacklist.h b/libmultipath/blacklist.h > index 4305857d..b08e0978 100644 > --- a/libmultipath/blacklist.h > +++ b/libmultipath/blacklist.h > @@ -42,8 +42,8 @@ int filter_device (vector, vector, char *, char *, > char *); > int filter_path (struct config *, struct path *); > int filter_property(struct config *, struct udev_device *, int, > const char*); > int filter_protocol(vector, vector, struct path *); > -int store_ble (vector, char *, int); > -int set_ble_device (vector, char *, char *, int); > +int store_ble (vector, const char *, int); > +int set_ble_device (vector, const char *, const char *, int); > void free_blacklist (vector); > void free_blacklist_device (vector); > void merge_blacklist(vector); > diff --git a/tests/blacklist.c b/tests/blacklist.c > index d5c40898..84a3ba2f 100644 > --- a/tests/blacklist.c > +++ b/tests/blacklist.c > @@ -94,67 +94,62 @@ static int setup(void **state) > > blist_devnode_sdb = vector_alloc(); > if (!blist_devnode_sdb || > - store_ble(blist_devnode_sdb, strdup("sdb"), ORIGIN_CONFIG)) > + store_ble(blist_devnode_sdb, "sdb", ORIGIN_CONFIG)) > return -1; > blist_devnode_sdb_inv = vector_alloc(); > if (!blist_devnode_sdb_inv || > - store_ble(blist_devnode_sdb_inv, strdup("!sdb"), > ORIGIN_CONFIG)) > + store_ble(blist_devnode_sdb_inv, "!sdb", ORIGIN_CONFIG)) > return -1; > > blist_all = vector_alloc(); > - if (!blist_all || store_ble(blist_all, strdup(".*"), > ORIGIN_CONFIG)) > + if (!blist_all || store_ble(blist_all, ".*", ORIGIN_CONFIG)) > return -1; > > blist_device_foo_bar = vector_alloc(); > if (!blist_device_foo_bar || > alloc_ble_device(blist_device_foo_bar) || > - set_ble_device(blist_device_foo_bar, strdup("foo"), > strdup("bar"), > - ORIGIN_CONFIG)) > + set_ble_device(blist_device_foo_bar, "foo", "bar", > ORIGIN_CONFIG)) > return -1; > blist_device_foo_inv_bar = vector_alloc(); > if (!blist_device_foo_inv_bar || > alloc_ble_device(blist_device_foo_inv_bar) || > - set_ble_device(blist_device_foo_inv_bar, strdup("!foo"), > - strdup("bar"), ORIGIN_CONFIG)) > + set_ble_device(blist_device_foo_inv_bar, "!foo", "bar", > ORIGIN_CONFIG)) > return -1; > blist_device_foo_bar_inv = vector_alloc(); > if (!blist_device_foo_bar_inv || > alloc_ble_device(blist_device_foo_bar_inv) || > - set_ble_device(blist_device_foo_bar_inv, strdup("foo"), > - strdup("!bar"), ORIGIN_CONFIG)) > + set_ble_device(blist_device_foo_bar_inv, "foo", "!bar", > ORIGIN_CONFIG)) > return -1; > > blist_device_all = vector_alloc(); > if (!blist_device_all || alloc_ble_device(blist_device_all) || > - set_ble_device(blist_device_all, strdup(".*"), > strdup(".*"), > - ORIGIN_CONFIG)) > + set_ble_device(blist_device_all, ".*", ".*", > ORIGIN_CONFIG)) > return -1; > > blist_wwid_xyzzy = vector_alloc(); > if (!blist_wwid_xyzzy || > - store_ble(blist_wwid_xyzzy, strdup("xyzzy"), > ORIGIN_CONFIG)) > + store_ble(blist_wwid_xyzzy, "xyzzy", ORIGIN_CONFIG)) > return -1; > blist_wwid_xyzzy_inv = vector_alloc(); > if (!blist_wwid_xyzzy_inv || > - store_ble(blist_wwid_xyzzy_inv, strdup("!xyzzy"), > ORIGIN_CONFIG)) > + store_ble(blist_wwid_xyzzy_inv, "!xyzzy", ORIGIN_CONFIG)) > return -1; > > blist_protocol_fcp = vector_alloc(); > if (!blist_protocol_fcp || > - store_ble(blist_protocol_fcp, strdup("scsi:fcp"), > ORIGIN_CONFIG)) > + store_ble(blist_protocol_fcp, "scsi:fcp", ORIGIN_CONFIG)) > return -1; > blist_protocol_fcp_inv = vector_alloc(); > if (!blist_protocol_fcp_inv || > - store_ble(blist_protocol_fcp_inv, strdup("!scsi:fcp"), > - ORIGIN_CONFIG)) > + store_ble(blist_protocol_fcp_inv, "!scsi:fcp", > ORIGIN_CONFIG)) > return -1; > > blist_property_wwn = vector_alloc(); > if (!blist_property_wwn || > - store_ble(blist_property_wwn, strdup("ID_WWN"), > ORIGIN_CONFIG)) > + store_ble(blist_property_wwn, "ID_WWN", ORIGIN_CONFIG)) > return -1; > blist_property_wwn_inv = vector_alloc(); > if (!blist_property_wwn_inv || > - store_ble(blist_property_wwn_inv, strdup("!ID_WWN"), > ORIGIN_CONFIG)) > + store_ble(blist_property_wwn_inv, "!ID_WWN", > ORIGIN_CONFIG)) > return -1; > > return 0; -- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel
