* [dm-devel] [PATCH 0/5] multipath-tools series: some codeclean and add prflag to path
@ 2021-11-16 13:58 lixiaokeng
2021-11-16 13:59 ` [dm-devel] [PATCH 1/5] Fix potential null pointer dereference lixiaokeng
` (4 more replies)
0 siblings, 5 replies; 17+ messages in thread
From: lixiaokeng @ 2021-11-16 13:58 UTC (permalink / raw)
To: Christophe Varoqui, Martin Wilck, Benjamin Marzinski,
dm-devel mailing list
Cc: linfeilong, liuzhiqiang (I)
lixiaokeng (5):
Fix potential null pointer dereference
remove unnecessary memset
remove unnecessary free
Match FREE and MALLOC/STRDUP/REALLOC
add prflag to path
kpartx/dasd.c | 5 +++--
kpartx/kpartx.c | 7 ++++---
libmpathpersist/mpath_persist.c | 2 +-
libmultipath/blacklist.c | 6 +++---
libmultipath/checkers/emc_clariion.c | 2 +-
libmultipath/config.c | 4 ++--
libmultipath/configure.c | 4 ++--
libmultipath/discovery.c | 8 +++++---
libmultipath/dmparser.c | 2 +-
libmultipath/foreign/nvme.c | 4 +++-
libmultipath/log.c | 1 -
libmultipath/parser.c | 2 +-
libmultipath/prioritizers/weightedpath.c | 3 +--
libmultipath/structs.c | 3 ++-
libmultipath/structs.h | 12 ++++++++++++
libmultipath/util.c | 10 +++++++++-
multipathd/cli_handlers.c | 15 ++++++++++-----
multipathd/main.c | 9 ++++++---
multipathd/waiter.c | 1 -
19 files changed, 66 insertions(+), 34 deletions(-)
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* [dm-devel] [PATCH 1/5] Fix potential null pointer dereference
2021-11-16 13:58 [dm-devel] [PATCH 0/5] multipath-tools series: some codeclean and add prflag to path lixiaokeng
@ 2021-11-16 13:59 ` lixiaokeng
2021-11-17 18:05 ` Benjamin Marzinski
2021-11-16 13:59 ` [dm-devel] [PATCH 2/5] remove unnecessary memset lixiaokeng
` (3 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: lixiaokeng @ 2021-11-16 13:59 UTC (permalink / raw)
To: Christophe Varoqui, Martin Wilck, Benjamin Marzinski,
dm-devel mailing list
Cc: linfeilong, liuzhiqiang (I)
udev_device_* may return NULL, check it.
Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
---
libmultipath/discovery.c | 8 +++++---
libmultipath/foreign/nvme.c | 4 +++-
libmultipath/util.c | 10 +++++++++-
3 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index f25fe9e3..48f3d8b2 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -388,8 +388,10 @@ sysfs_get_tgt_nodename(struct path *pp, char *node)
if (value && !strcmp(value, "usb")) {
pp->sg_id.proto_id = SCSI_PROTOCOL_USB;
tgtname = udev_device_get_sysname(tgtdev);
- strlcpy(node, tgtname, NODE_NAME_SIZE);
- return 0;
+ if (!tgtname) {
+ strlcpy(node, tgtname, NODE_NAME_SIZE);
+ return 0;
+ }
}
tgtdev = udev_device_get_parent(tgtdev);
}
@@ -803,7 +805,7 @@ sysfs_set_nexus_loss_tmo(struct multipath *mpp, struct path *pp)
parent = udev_device_get_parent(parent)) {
const char *ed = udev_device_get_sysname(parent);
- if (!strncmp(ed, ed_str, sizeof(ed_str) - 1)) {
+ if (ed && !strncmp(ed, ed_str, sizeof(ed_str) - 1)) {
end_dev_id = ed;
break;
}
diff --git a/libmultipath/foreign/nvme.c b/libmultipath/foreign/nvme.c
index d40c0869..f778410a 100644
--- a/libmultipath/foreign/nvme.c
+++ b/libmultipath/foreign/nvme.c
@@ -184,7 +184,9 @@ static int snprint_nvme_map(const struct gen_multipath *gmp,
"firmware_rev"));
case 'r':
val = udev_device_get_sysattr_value(nvm->udev, "ro");
- if (val[0] == 1)
+ if (!val)
+ return -1;
+ else if (val[0] == 1)
return append_strbuf_str(buff, "ro");
else
return append_strbuf_str(buff, "rw");
diff --git a/libmultipath/util.c b/libmultipath/util.c
index ea858409..3d036e19 100644
--- a/libmultipath/util.c
+++ b/libmultipath/util.c
@@ -168,6 +168,7 @@ size_t strlcat(char * restrict dst, const char * restrict src, size_t size)
int devt2devname(char *devname, int devname_len, const char *devt)
{
struct udev_device *u_dev;
+ const char * dev_name;
int r;
if (!devname || !devname_len || !devt)
@@ -178,7 +179,14 @@ int devt2devname(char *devname, int devname_len, const char *devt)
condlog(0, "\"%s\": invalid major/minor numbers, not found in sysfs", devt);
return 1;
}
- r = strlcpy(devname, udev_device_get_sysname(u_dev), devname_len);
+
+ dev_name = udev_device_get_sysname(u_dev);
+ if (!dev_name) {
+ condlog(2, "\"%s\": fail to get sysname\n", devt);
+ udev_device_unref(u_dev);
+ return 1;
+ }
+ r = strlcpy(devname, dev_name, devname_len);
udev_device_unref(u_dev);
return !(r < devname_len);
--
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [dm-devel] [PATCH 2/5] remove unnecessary memset
2021-11-16 13:58 [dm-devel] [PATCH 0/5] multipath-tools series: some codeclean and add prflag to path lixiaokeng
2021-11-16 13:59 ` [dm-devel] [PATCH 1/5] Fix potential null pointer dereference lixiaokeng
@ 2021-11-16 13:59 ` lixiaokeng
2021-11-18 0:10 ` Benjamin Marzinski
2021-11-16 14:00 ` [dm-devel] [PATCH 3/5] remove unnecessary free lixiaokeng
` (2 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: lixiaokeng @ 2021-11-16 13:59 UTC (permalink / raw)
To: Christophe Varoqui, Martin Wilck, Benjamin Marzinski,
dm-devel mailing list
Cc: linfeilong, liuzhiqiang (I)
MALLOC will set memory zero. memset is unnecessary.
Remove it.
Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
---
libmultipath/log.c | 1 -
multipathd/waiter.c | 1 -
2 files changed, 2 deletions(-)
diff --git a/libmultipath/log.c b/libmultipath/log.c
index 10fa32cd..f41efb5b 100644
--- a/libmultipath/log.c
+++ b/libmultipath/log.c
@@ -57,7 +57,6 @@ static int logarea_init (int size)
FREE(la);
return 1;
}
- memset(la->start, 0, size);
la->empty = 1;
la->end = la->start + size;
diff --git a/multipathd/waiter.c b/multipathd/waiter.c
index bbe6c2a1..f74b395a 100644
--- a/multipathd/waiter.c
+++ b/multipathd/waiter.c
@@ -33,7 +33,6 @@ static struct event_thread *alloc_waiter (void)
struct event_thread *wp;
wp = (struct event_thread *)MALLOC(sizeof(struct event_thread));
- memset(wp, 0, sizeof(struct event_thread));
return wp;
}
--
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [dm-devel] [PATCH 3/5] remove unnecessary free
2021-11-16 13:58 [dm-devel] [PATCH 0/5] multipath-tools series: some codeclean and add prflag to path lixiaokeng
2021-11-16 13:59 ` [dm-devel] [PATCH 1/5] Fix potential null pointer dereference lixiaokeng
2021-11-16 13:59 ` [dm-devel] [PATCH 2/5] remove unnecessary memset lixiaokeng
@ 2021-11-16 14:00 ` lixiaokeng
2021-11-18 0:14 ` Benjamin Marzinski
2021-11-16 14:00 ` [dm-devel] [PATCH 4/5] Match FREE and MALLOC/STRDUP/REALLOC lixiaokeng
2021-11-16 14:01 ` [dm-devel] [PATCH 5/5] add prflag to path lixiaokeng
4 siblings, 1 reply; 17+ messages in thread
From: lixiaokeng @ 2021-11-16 14:00 UTC (permalink / raw)
To: Christophe Varoqui, Martin Wilck, Benjamin Marzinski,
dm-devel mailing list
Cc: linfeilong, liuzhiqiang (I)
arg will be free by cleanup_charp. FREE(args)
is unnecessary before return. Remove it.
Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
---
libmultipath/prioritizers/weightedpath.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/libmultipath/prioritizers/weightedpath.c b/libmultipath/prioritizers/weightedpath.c
index ea03fc3d..32d1cf0c 100644
--- a/libmultipath/prioritizers/weightedpath.c
+++ b/libmultipath/prioritizers/weightedpath.c
@@ -75,13 +75,12 @@ int prio_path_weight(struct path *pp, char *prio_args)
if (!prio_args)
return priority;
- arg = temp = STRDUP(prio_args);
+ arg = temp = strdup(prio_args);
regex = get_next_string(&temp, split_char);
/* Return default priority if the argument is not parseable */
if (!regex) {
- FREE(arg);
return priority;
}
--
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [dm-devel] [PATCH 4/5] Match FREE and MALLOC/STRDUP/REALLOC
2021-11-16 13:58 [dm-devel] [PATCH 0/5] multipath-tools series: some codeclean and add prflag to path lixiaokeng
` (2 preceding siblings ...)
2021-11-16 14:00 ` [dm-devel] [PATCH 3/5] remove unnecessary free lixiaokeng
@ 2021-11-16 14:00 ` lixiaokeng
2021-11-18 0:47 ` Benjamin Marzinski
2021-11-16 14:01 ` [dm-devel] [PATCH 5/5] add prflag to path lixiaokeng
4 siblings, 1 reply; 17+ messages in thread
From: lixiaokeng @ 2021-11-16 14:00 UTC (permalink / raw)
To: Christophe Varoqui, Martin Wilck, Benjamin Marzinski,
dm-devel mailing list
Cc: linfeilong, liuzhiqiang (I)
In _DEBUG_ mode, MALLOC/STRDUP/REALLOC and FREE will record
the memory usage. Match them.
Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
---
kpartx/dasd.c | 5 +++--
kpartx/kpartx.c | 7 ++++---
libmultipath/blacklist.c | 6 +++---
libmultipath/checkers/emc_clariion.c | 2 +-
libmultipath/config.c | 4 ++--
libmultipath/configure.c | 4 ++--
libmultipath/dmparser.c | 2 +-
libmultipath/parser.c | 2 +-
libmultipath/structs.c | 2 +-
multipathd/main.c | 2 +-
10 files changed, 19 insertions(+), 17 deletions(-)
diff --git a/kpartx/dasd.c b/kpartx/dasd.c
index f0398645..14744048 100644
--- a/kpartx/dasd.c
+++ b/kpartx/dasd.c
@@ -40,6 +40,7 @@
#include "kpartx.h"
#include "byteorder.h"
#include "dasd.h"
+#include "memory.h"
unsigned long long sectors512(unsigned long long sectors, int blocksize)
{
@@ -100,10 +101,10 @@ read_dasd_pt(int fd, __attribute__((unused)) struct slice all,
* Get the first target and operate on that instead.
*/
if (!(dev = dm_get_first_dep(devname))) {
- free(devname);
+ FREE(devname);
return -1;
}
- free(devname);
+ FREE(devname);
if ((unsigned int)major(dev) != 94) {
/* Not a DASD */
diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
index 7bc64543..5e59063d 100644
--- a/kpartx/kpartx.c
+++ b/kpartx/kpartx.c
@@ -39,6 +39,7 @@
#include "lopart.h"
#include "kpartx.h"
#include "version.h"
+#include "memory.h"
#define SIZE(a) (sizeof(a)/sizeof((a)[0]))
@@ -177,7 +178,7 @@ get_hotplug_device(void)
/* Dirname + mapname + \0 */
if (!(device = (char *)malloc(sizeof(char) * (off + len + 1)))) {
- free(mapname);
+ FREE(mapname);
return NULL;
}
@@ -187,10 +188,10 @@ get_hotplug_device(void)
if (strlen(device) != (off + len)) {
free(device);
- free(mapname);
+ FREE(mapname);
return NULL;
}
- free(mapname);
+ FREE(mapname);
return device;
}
diff --git a/libmultipath/blacklist.c b/libmultipath/blacklist.c
index 4e315c97..573df152 100644
--- a/libmultipath/blacklist.c
+++ b/libmultipath/blacklist.c
@@ -39,7 +39,7 @@ int store_ble(vector blist, const char *str, int origin)
if (!str)
return 0;
- strdup_str = strdup(str);
+ strdup_str = STRDUP(str);
if (!strdup_str)
return 1;
@@ -134,8 +134,8 @@ out1:
ble->vendor = NULL;
}
out:
- free(vendor_str);
- free(product_str);
+ FREE(vendor_str);
+ FREE(product_str);
return 1;
}
diff --git a/libmultipath/checkers/emc_clariion.c b/libmultipath/checkers/emc_clariion.c
index 5cd63aca..b3f0aded 100644
--- a/libmultipath/checkers/emc_clariion.c
+++ b/libmultipath/checkers/emc_clariion.c
@@ -128,7 +128,7 @@ int libcheck_mp_init (struct checker * c)
void libcheck_free (struct checker * c)
{
- free(c->context);
+ FREE(c->context);
}
int libcheck_check (struct checker * c)
diff --git a/libmultipath/config.c b/libmultipath/config.c
index 30046a17..667b500b 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -1051,10 +1051,10 @@ int parse_uid_attrs(char *uid_attrs, struct config *conf)
if (!tmp) {
condlog(2, "invalid record in uid_attrs: %s",
uid_attr_record);
- free(uid_attr_record);
+ FREE(uid_attr_record);
ret = 1;
} else if (!vector_alloc_slot(attrs)) {
- free(uid_attr_record);
+ FREE(uid_attr_record);
ret = 1;
} else
vector_set_slot(attrs, uid_attr_record);
diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 7edb355b..9545854b 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -738,8 +738,8 @@ void select_action (struct multipath *mpp, const struct _vector *curmp,
condlog(1, "%s: can't use alias \"%s\" used by %s, falling back to WWID",
mpp->wwid, mpp->alias, cmpp_by_name->wwid);
/* We can do this because wwid wasn't found */
- free(mpp->alias);
- mpp->alias = strdup(mpp->wwid);
+ FREE(mpp->alias);
+ mpp->alias = STRDUP(mpp->wwid);
mpp->action = ACT_CREATE;
condlog(3, "%s: set ACT_CREATE (map does not exist, name changed)",
mpp->alias);
diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
index 4ba7f339..96beeb6d 100644
--- a/libmultipath/dmparser.c
+++ b/libmultipath/dmparser.c
@@ -30,7 +30,7 @@ merge_words(char **dst, const char *word)
*dst = REALLOC(*dst, len);
if (!*dst) {
- free(p);
+ FREE(p);
return 1;
}
diff --git a/libmultipath/parser.c b/libmultipath/parser.c
index 68262d0e..f0047c4d 100644
--- a/libmultipath/parser.c
+++ b/libmultipath/parser.c
@@ -364,7 +364,7 @@ set_value(vector strvec)
for (i = 2; i < VECTOR_SIZE(strvec); i++) {
str = VECTOR_SLOT(strvec, i);
if (!str) {
- free(alloc);
+ FREE(alloc);
condlog(0, "parse error for option '%s'",
(char *)VECTOR_SLOT(strvec, 0));
return NULL;
diff --git a/libmultipath/structs.c b/libmultipath/structs.c
index 6e5a1038..e8cacb4b 100644
--- a/libmultipath/structs.c
+++ b/libmultipath/structs.c
@@ -106,7 +106,7 @@ alloc_path (void)
dm_path_to_gen(pp)->ops = &dm_gen_path_ops;
pp->hwe = vector_alloc();
if (pp->hwe == NULL) {
- free(pp);
+ FREE(pp);
return NULL;
}
}
diff --git a/multipathd/main.c b/multipathd/main.c
index 1defeaf1..82ab3ed1 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -3555,7 +3555,7 @@ void * mpath_pr_event_handler_fn (void * pathp )
goto out;
}
- param = (struct prout_param_descriptor *)MALLOC(sizeof(struct prout_param_descriptor));
+ param = (struct prout_param_descriptor *)calloc(1, sizeof(struct prout_param_descriptor));
if (!param)
goto out;
--
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [dm-devel] [PATCH 5/5] add prflag to path
2021-11-16 13:58 [dm-devel] [PATCH 0/5] multipath-tools series: some codeclean and add prflag to path lixiaokeng
` (3 preceding siblings ...)
2021-11-16 14:00 ` [dm-devel] [PATCH 4/5] Match FREE and MALLOC/STRDUP/REALLOC lixiaokeng
@ 2021-11-16 14:01 ` lixiaokeng
2021-11-18 15:36 ` Martin Wilck
2021-11-18 16:57 ` Benjamin Marzinski
4 siblings, 2 replies; 17+ messages in thread
From: lixiaokeng @ 2021-11-16 14:01 UTC (permalink / raw)
To: Christophe Varoqui, Martin Wilck, Benjamin Marzinski,
dm-devel mailing list
Cc: linfeilong, liuzhiqiang (I)
The update_map will frequently be called and there will be
unnecessary checks of reseravtion. We add prflag to path
to avoid this.
The pp->state changes from others to up or ghost, the
mpath_pr_event_handle should be called. The
mpath_pr_event_handle in ev_add_path may not be called,
so set pp->prkey PRKEY_NO when path is removed.
Fix: 4db4fa
Signed-off-by: Lixiaokeng <lixiaokeng>
---
libmpathpersist/mpath_persist.c | 2 +-
libmultipath/structs.c | 1 +
libmultipath/structs.h | 12 ++++++++++++
multipathd/cli_handlers.c | 15 ++++++++++-----
multipathd/main.c | 5 +++--
5 files changed, 27 insertions(+), 8 deletions(-)
diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c
index 803a2a28..f88a2e89 100644
--- a/libmpathpersist/mpath_persist.c
+++ b/libmpathpersist/mpath_persist.c
@@ -924,7 +924,7 @@ int update_map_pr(struct multipath *mpp)
if (isFound)
{
- mpp->prflag = 1;
+ mpp->prflag = PRFLAG_OK;
condlog(2, "%s: prflag flag set.", mpp->alias );
}
diff --git a/libmultipath/structs.c b/libmultipath/structs.c
index e8cacb4b..82dbd565 100644
--- a/libmultipath/structs.c
+++ b/libmultipath/structs.c
@@ -122,6 +122,7 @@ uninitialize_path(struct path *pp)
pp->dmstate = PSTATE_UNDEF;
pp->uid_attribute = NULL;
pp->getuid = NULL;
+ pp->prflag = PRFLAG_NO;
if (checker_selected(&pp->checker))
checker_put(&pp->checker);
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 399540e7..5b77218b 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -249,6 +249,17 @@ enum recheck_wwid_states {
RECHECK_WWID_ON = YNU_YES,
};
+/*
+ * PRFLAG_NO for path, it means reservation should be checked.
+ * PRFLAG_NO for multipath, it means mpp has no prkey.
+ * PRFLAG_OK for path, it means reservation has been checked.
+ * PRFLAG_OK for multipath, it means mpp has prkey.
+ */
+enum prflag_states {
+ PRFLAG_NO = 0,
+ PRFLAG_OK = 1,
+};
+
struct vpd_vendor_page {
int pg;
const char *name;
@@ -327,6 +338,7 @@ struct path {
/* configlet pointers */
vector hwe;
struct gen_path generic_path;
+ int prflag;
};
typedef int (pgpolicyfn) (struct multipath *, vector);
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 6d3a0ae2..8662fad7 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -1341,7 +1341,7 @@ cli_setprstatus(void * v, char ** reply, int * len, void * data)
return 1;
if (!mpp->prflag) {
- mpp->prflag = 1;
+ mpp->prflag = PRFLAG_OK;
condlog(2, "%s: prflag set", param);
}
@@ -1352,9 +1352,11 @@ cli_setprstatus(void * v, char ** reply, int * len, void * data)
int
cli_unsetprstatus(void * v, char ** reply, int * len, void * data)
{
- struct multipath * mpp;
- struct vectors * vecs = (struct vectors *)data;
- char * param = get_keyparam(v, MAP);
+ int i;
+ struct multipath *mpp;
+ struct path *pp;
+ struct vectors *vecs = (struct vectors *)data;
+ char *param = get_keyparam(v, MAP);
param = convert_dev(param, 0);
get_path_layout(vecs->pathvec, 0);
@@ -1364,7 +1366,10 @@ cli_unsetprstatus(void * v, char ** reply, int * len, void * data)
return 1;
if (mpp->prflag) {
- mpp->prflag = 0;
+ mpp->prflag = PRFLAG_NO;
+ vector_foreach_slot(mpp->paths, pp, i) {
+ pp->prflag = PRFLAG_NO;
+ }
condlog(2, "%s: prflag unset", param);
}
diff --git a/multipathd/main.c b/multipathd/main.c
index 82ab3ed1..6ef6495b 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -506,7 +506,7 @@ retry:
if (mpp->prflag) {
vector_foreach_slot(mpp->paths, pp, i) {
- if ((pp->state == PATH_UP) || (pp->state == PATH_GHOST)) {
+ if (!pp->prflag && ((pp->state == PATH_UP) || (pp->state == PATH_GHOST))) {
/* persistent reseravtion check*/
mpath_pr_event_handle(pp);
}
@@ -3570,7 +3570,8 @@ void * mpath_pr_event_handler_fn (void * pathp )
{
condlog(0,"%s: Reservation registration failed. Error: %d", pp->dev, ret);
}
- mpp->prflag = 1;
+ mpp->prflag = PRFLAG_OK;
+ pp->prflag = PRFLAG_OK;
free(param);
out:
--
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [dm-devel] [PATCH 1/5] Fix potential null pointer dereference
2021-11-16 13:59 ` [dm-devel] [PATCH 1/5] Fix potential null pointer dereference lixiaokeng
@ 2021-11-17 18:05 ` Benjamin Marzinski
2021-11-18 2:04 ` lixiaokeng
0 siblings, 1 reply; 17+ messages in thread
From: Benjamin Marzinski @ 2021-11-17 18:05 UTC (permalink / raw)
To: lixiaokeng
Cc: linfeilong, dm-devel mailing list, Martin Wilck, liuzhiqiang (I)
On Tue, Nov 16, 2021 at 09:59:14PM +0800, lixiaokeng wrote:
> udev_device_* may return NULL, check it.
>
> Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
> ---
> libmultipath/discovery.c | 8 +++++---
> libmultipath/foreign/nvme.c | 4 +++-
> libmultipath/util.c | 10 +++++++++-
> 3 files changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index f25fe9e3..48f3d8b2 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -388,8 +388,10 @@ sysfs_get_tgt_nodename(struct path *pp, char *node)
> if (value && !strcmp(value, "usb")) {
> pp->sg_id.proto_id = SCSI_PROTOCOL_USB;
> tgtname = udev_device_get_sysname(tgtdev);
> - strlcpy(node, tgtname, NODE_NAME_SIZE);
> - return 0;
> + if (!tgtname) {
I assume that you mean "if (tgtname)"
> + strlcpy(node, tgtname, NODE_NAME_SIZE);
> + return 0;
> + }
> }
> tgtdev = udev_device_get_parent(tgtdev);
> }
> @@ -803,7 +805,7 @@ sysfs_set_nexus_loss_tmo(struct multipath *mpp, struct path *pp)
> parent = udev_device_get_parent(parent)) {
> const char *ed = udev_device_get_sysname(parent);
>
> - if (!strncmp(ed, ed_str, sizeof(ed_str) - 1)) {
> + if (ed && !strncmp(ed, ed_str, sizeof(ed_str) - 1)) {
> end_dev_id = ed;
> break;
> }
> diff --git a/libmultipath/foreign/nvme.c b/libmultipath/foreign/nvme.c
> index d40c0869..f778410a 100644
> --- a/libmultipath/foreign/nvme.c
> +++ b/libmultipath/foreign/nvme.c
> @@ -184,7 +184,9 @@ static int snprint_nvme_map(const struct gen_multipath *gmp,
> "firmware_rev"));
> case 'r':
> val = udev_device_get_sysattr_value(nvm->udev, "ro");
> - if (val[0] == 1)
> + if (!val)
> + return -1;
sprint_ro() returns "undef" for a similar case. Perhaps we should do
that here as well.
-Ben
> + else if (val[0] == 1)
> return append_strbuf_str(buff, "ro");
> else
> return append_strbuf_str(buff, "rw");
> diff --git a/libmultipath/util.c b/libmultipath/util.c
> index ea858409..3d036e19 100644
> --- a/libmultipath/util.c
> +++ b/libmultipath/util.c
> @@ -168,6 +168,7 @@ size_t strlcat(char * restrict dst, const char * restrict src, size_t size)
> int devt2devname(char *devname, int devname_len, const char *devt)
> {
> struct udev_device *u_dev;
> + const char * dev_name;
> int r;
>
> if (!devname || !devname_len || !devt)
> @@ -178,7 +179,14 @@ int devt2devname(char *devname, int devname_len, const char *devt)
> condlog(0, "\"%s\": invalid major/minor numbers, not found in sysfs", devt);
> return 1;
> }
> - r = strlcpy(devname, udev_device_get_sysname(u_dev), devname_len);
> +
> + dev_name = udev_device_get_sysname(u_dev);
> + if (!dev_name) {
> + condlog(2, "\"%s\": fail to get sysname\n", devt);
> + udev_device_unref(u_dev);
> + return 1;
> + }
> + r = strlcpy(devname, dev_name, devname_len);
> udev_device_unref(u_dev);
>
> return !(r < devname_len);
> --
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [dm-devel] [PATCH 2/5] remove unnecessary memset
2021-11-16 13:59 ` [dm-devel] [PATCH 2/5] remove unnecessary memset lixiaokeng
@ 2021-11-18 0:10 ` Benjamin Marzinski
0 siblings, 0 replies; 17+ messages in thread
From: Benjamin Marzinski @ 2021-11-18 0:10 UTC (permalink / raw)
To: lixiaokeng
Cc: linfeilong, dm-devel mailing list, Martin Wilck, liuzhiqiang (I)
On Tue, Nov 16, 2021 at 09:59:41PM +0800, lixiaokeng wrote:
> MALLOC will set memory zero. memset is unnecessary.
> Remove it.
>
> Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> libmultipath/log.c | 1 -
> multipathd/waiter.c | 1 -
> 2 files changed, 2 deletions(-)
>
> diff --git a/libmultipath/log.c b/libmultipath/log.c
> index 10fa32cd..f41efb5b 100644
> --- a/libmultipath/log.c
> +++ b/libmultipath/log.c
> @@ -57,7 +57,6 @@ static int logarea_init (int size)
> FREE(la);
> return 1;
> }
> - memset(la->start, 0, size);
>
> la->empty = 1;
> la->end = la->start + size;
> diff --git a/multipathd/waiter.c b/multipathd/waiter.c
> index bbe6c2a1..f74b395a 100644
> --- a/multipathd/waiter.c
> +++ b/multipathd/waiter.c
> @@ -33,7 +33,6 @@ static struct event_thread *alloc_waiter (void)
> struct event_thread *wp;
>
> wp = (struct event_thread *)MALLOC(sizeof(struct event_thread));
> - memset(wp, 0, sizeof(struct event_thread));
>
> return wp;
> }
> --
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [dm-devel] [PATCH 3/5] remove unnecessary free
2021-11-16 14:00 ` [dm-devel] [PATCH 3/5] remove unnecessary free lixiaokeng
@ 2021-11-18 0:14 ` Benjamin Marzinski
0 siblings, 0 replies; 17+ messages in thread
From: Benjamin Marzinski @ 2021-11-18 0:14 UTC (permalink / raw)
To: lixiaokeng
Cc: linfeilong, dm-devel mailing list, Martin Wilck, liuzhiqiang (I)
On Tue, Nov 16, 2021 at 10:00:09PM +0800, lixiaokeng wrote:
> arg will be free by cleanup_charp. FREE(args)
> is unnecessary before return. Remove it.
>
> Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> libmultipath/prioritizers/weightedpath.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/libmultipath/prioritizers/weightedpath.c b/libmultipath/prioritizers/weightedpath.c
> index ea03fc3d..32d1cf0c 100644
> --- a/libmultipath/prioritizers/weightedpath.c
> +++ b/libmultipath/prioritizers/weightedpath.c
> @@ -75,13 +75,12 @@ int prio_path_weight(struct path *pp, char *prio_args)
> if (!prio_args)
> return priority;
>
> - arg = temp = STRDUP(prio_args);
> + arg = temp = strdup(prio_args);
>
> regex = get_next_string(&temp, split_char);
>
> /* Return default priority if the argument is not parseable */
> if (!regex) {
> - FREE(arg);
> return priority;
> }
>
> --
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [dm-devel] [PATCH 4/5] Match FREE and MALLOC/STRDUP/REALLOC
2021-11-16 14:00 ` [dm-devel] [PATCH 4/5] Match FREE and MALLOC/STRDUP/REALLOC lixiaokeng
@ 2021-11-18 0:47 ` Benjamin Marzinski
2021-11-18 2:46 ` lixiaokeng
2021-11-18 15:31 ` Martin Wilck
0 siblings, 2 replies; 17+ messages in thread
From: Benjamin Marzinski @ 2021-11-18 0:47 UTC (permalink / raw)
To: lixiaokeng
Cc: linfeilong, dm-devel mailing list, Martin Wilck, liuzhiqiang (I)
On Tue, Nov 16, 2021 at 10:00:53PM +0800, lixiaokeng wrote:
> In _DEBUG_ mode, MALLOC/STRDUP/REALLOC and FREE will record
> the memory usage. Match them.
This looks fine, but personally, I'd rather just have all the DEBUG
memory code removed. If people want to check memory usage, there's
always valgrind.
-Ben
>
> Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
> ---
> kpartx/dasd.c | 5 +++--
> kpartx/kpartx.c | 7 ++++---
> libmultipath/blacklist.c | 6 +++---
> libmultipath/checkers/emc_clariion.c | 2 +-
> libmultipath/config.c | 4 ++--
> libmultipath/configure.c | 4 ++--
> libmultipath/dmparser.c | 2 +-
> libmultipath/parser.c | 2 +-
> libmultipath/structs.c | 2 +-
> multipathd/main.c | 2 +-
> 10 files changed, 19 insertions(+), 17 deletions(-)
>
> diff --git a/kpartx/dasd.c b/kpartx/dasd.c
> index f0398645..14744048 100644
> --- a/kpartx/dasd.c
> +++ b/kpartx/dasd.c
> @@ -40,6 +40,7 @@
> #include "kpartx.h"
> #include "byteorder.h"
> #include "dasd.h"
> +#include "memory.h"
>
> unsigned long long sectors512(unsigned long long sectors, int blocksize)
> {
> @@ -100,10 +101,10 @@ read_dasd_pt(int fd, __attribute__((unused)) struct slice all,
> * Get the first target and operate on that instead.
> */
> if (!(dev = dm_get_first_dep(devname))) {
> - free(devname);
> + FREE(devname);
> return -1;
> }
> - free(devname);
> + FREE(devname);
>
> if ((unsigned int)major(dev) != 94) {
> /* Not a DASD */
> diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
> index 7bc64543..5e59063d 100644
> --- a/kpartx/kpartx.c
> +++ b/kpartx/kpartx.c
> @@ -39,6 +39,7 @@
> #include "lopart.h"
> #include "kpartx.h"
> #include "version.h"
> +#include "memory.h"
>
> #define SIZE(a) (sizeof(a)/sizeof((a)[0]))
>
> @@ -177,7 +178,7 @@ get_hotplug_device(void)
>
> /* Dirname + mapname + \0 */
> if (!(device = (char *)malloc(sizeof(char) * (off + len + 1)))) {
> - free(mapname);
> + FREE(mapname);
> return NULL;
> }
>
> @@ -187,10 +188,10 @@ get_hotplug_device(void)
>
> if (strlen(device) != (off + len)) {
> free(device);
> - free(mapname);
> + FREE(mapname);
> return NULL;
> }
> - free(mapname);
> + FREE(mapname);
> return device;
> }
>
> diff --git a/libmultipath/blacklist.c b/libmultipath/blacklist.c
> index 4e315c97..573df152 100644
> --- a/libmultipath/blacklist.c
> +++ b/libmultipath/blacklist.c
> @@ -39,7 +39,7 @@ int store_ble(vector blist, const char *str, int origin)
> if (!str)
> return 0;
>
> - strdup_str = strdup(str);
> + strdup_str = STRDUP(str);
> if (!strdup_str)
> return 1;
>
> @@ -134,8 +134,8 @@ out1:
> ble->vendor = NULL;
> }
> out:
> - free(vendor_str);
> - free(product_str);
> + FREE(vendor_str);
> + FREE(product_str);
> return 1;
> }
>
> diff --git a/libmultipath/checkers/emc_clariion.c b/libmultipath/checkers/emc_clariion.c
> index 5cd63aca..b3f0aded 100644
> --- a/libmultipath/checkers/emc_clariion.c
> +++ b/libmultipath/checkers/emc_clariion.c
> @@ -128,7 +128,7 @@ int libcheck_mp_init (struct checker * c)
>
> void libcheck_free (struct checker * c)
> {
> - free(c->context);
> + FREE(c->context);
> }
>
> int libcheck_check (struct checker * c)
> diff --git a/libmultipath/config.c b/libmultipath/config.c
> index 30046a17..667b500b 100644
> --- a/libmultipath/config.c
> +++ b/libmultipath/config.c
> @@ -1051,10 +1051,10 @@ int parse_uid_attrs(char *uid_attrs, struct config *conf)
> if (!tmp) {
> condlog(2, "invalid record in uid_attrs: %s",
> uid_attr_record);
> - free(uid_attr_record);
> + FREE(uid_attr_record);
> ret = 1;
> } else if (!vector_alloc_slot(attrs)) {
> - free(uid_attr_record);
> + FREE(uid_attr_record);
> ret = 1;
> } else
> vector_set_slot(attrs, uid_attr_record);
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index 7edb355b..9545854b 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -738,8 +738,8 @@ void select_action (struct multipath *mpp, const struct _vector *curmp,
> condlog(1, "%s: can't use alias \"%s\" used by %s, falling back to WWID",
> mpp->wwid, mpp->alias, cmpp_by_name->wwid);
> /* We can do this because wwid wasn't found */
> - free(mpp->alias);
> - mpp->alias = strdup(mpp->wwid);
> + FREE(mpp->alias);
> + mpp->alias = STRDUP(mpp->wwid);
> mpp->action = ACT_CREATE;
> condlog(3, "%s: set ACT_CREATE (map does not exist, name changed)",
> mpp->alias);
> diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
> index 4ba7f339..96beeb6d 100644
> --- a/libmultipath/dmparser.c
> +++ b/libmultipath/dmparser.c
> @@ -30,7 +30,7 @@ merge_words(char **dst, const char *word)
> *dst = REALLOC(*dst, len);
>
> if (!*dst) {
> - free(p);
> + FREE(p);
> return 1;
> }
>
> diff --git a/libmultipath/parser.c b/libmultipath/parser.c
> index 68262d0e..f0047c4d 100644
> --- a/libmultipath/parser.c
> +++ b/libmultipath/parser.c
> @@ -364,7 +364,7 @@ set_value(vector strvec)
> for (i = 2; i < VECTOR_SIZE(strvec); i++) {
> str = VECTOR_SLOT(strvec, i);
> if (!str) {
> - free(alloc);
> + FREE(alloc);
> condlog(0, "parse error for option '%s'",
> (char *)VECTOR_SLOT(strvec, 0));
> return NULL;
> diff --git a/libmultipath/structs.c b/libmultipath/structs.c
> index 6e5a1038..e8cacb4b 100644
> --- a/libmultipath/structs.c
> +++ b/libmultipath/structs.c
> @@ -106,7 +106,7 @@ alloc_path (void)
> dm_path_to_gen(pp)->ops = &dm_gen_path_ops;
> pp->hwe = vector_alloc();
> if (pp->hwe == NULL) {
> - free(pp);
> + FREE(pp);
> return NULL;
> }
> }
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 1defeaf1..82ab3ed1 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -3555,7 +3555,7 @@ void * mpath_pr_event_handler_fn (void * pathp )
> goto out;
> }
>
> - param = (struct prout_param_descriptor *)MALLOC(sizeof(struct prout_param_descriptor));
> + param = (struct prout_param_descriptor *)calloc(1, sizeof(struct prout_param_descriptor));
> if (!param)
> goto out;
>
> --
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [dm-devel] [PATCH 1/5] Fix potential null pointer dereference
2021-11-17 18:05 ` Benjamin Marzinski
@ 2021-11-18 2:04 ` lixiaokeng
0 siblings, 0 replies; 17+ messages in thread
From: lixiaokeng @ 2021-11-18 2:04 UTC (permalink / raw)
To: Benjamin Marzinski
Cc: linfeilong, dm-devel mailing list, Martin Wilck, liuzhiqiang (I)
>> @@ -388,8 +388,10 @@ sysfs_get_tgt_nodename(struct path *pp, char *node)
>> if (value && !strcmp(value, "usb")) {
>> pp->sg_id.proto_id = SCSI_PROTOCOL_USB;
>> tgtname = udev_device_get_sysname(tgtdev);
>> - strlcpy(node, tgtname, NODE_NAME_SIZE);
>> - return 0;
>> + if (!tgtname) {
>
> I assume that you mean "if (tgtname)"
>
Thanks for your correction. I'm will correct it.
>> + strlcpy(node, tgtname, NODE_NAME_SIZE);
>> + return 0;
>> + }
>> }
>> tgtdev = udev_device_get_parent(tgtdev);
>> }
>> diff --git a/libmultipath/foreign/nvme.c b/libmultipath/foreign/nvme.c
>> index d40c0869..f778410a 100644
>> --- a/libmultipath/foreign/nvme.c
>> +++ b/libmultipath/foreign/nvme.c
>> @@ -184,7 +184,9 @@ static int snprint_nvme_map(const struct gen_multipath *gmp,
>> "firmware_rev"));
>> case 'r':
>> val = udev_device_get_sysattr_value(nvm->udev, "ro");
>> - if (val[0] == 1)
>> + if (!val)
>> + return -1;
>
> sprint_ro() returns "undef" for a similar case. Perhaps we should do
> that here as well.
>
I'll do it here and send v2 patch. Thanks again.
Regards,
Lixiaokeng
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [dm-devel] [PATCH 4/5] Match FREE and MALLOC/STRDUP/REALLOC
2021-11-18 0:47 ` Benjamin Marzinski
@ 2021-11-18 2:46 ` lixiaokeng
2021-11-18 15:35 ` Martin Wilck
2021-11-18 15:31 ` Martin Wilck
1 sibling, 1 reply; 17+ messages in thread
From: lixiaokeng @ 2021-11-18 2:46 UTC (permalink / raw)
To: Benjamin Marzinski
Cc: linfeilong, dm-devel mailing list, Martin Wilck, liuzhiqiang (I)
On 2021/11/18 8:47, Benjamin Marzinski wrote:
> On Tue, Nov 16, 2021 at 10:00:53PM +0800, lixiaokeng wrote:
>> In _DEBUG_ mode, MALLOC/STRDUP/REALLOC and FREE will record
>> the memory usage. Match them.
>
> This looks fine, but personally, I'd rather just have all the DEBUG
> memory code removed. If people want to check memory usage, there's
> always valgrind.
>
> -Ben
>
>>
If MALLOC/STRDUP/REALLOC and FREE is unnecessary, I'll remove this patch
in this series and make a patch remove them latter. What is Martin's
opinion?
Regards
Lixiaokeng
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [dm-devel] [PATCH 4/5] Match FREE and MALLOC/STRDUP/REALLOC
2021-11-18 0:47 ` Benjamin Marzinski
2021-11-18 2:46 ` lixiaokeng
@ 2021-11-18 15:31 ` Martin Wilck
1 sibling, 0 replies; 17+ messages in thread
From: Martin Wilck @ 2021-11-18 15:31 UTC (permalink / raw)
To: Benjamin Marzinski, lixiaokeng
Cc: dm-devel, list, liuzhiqiang (I), linfeilong
On Wed, 2021-11-17 at 18:47 -0600, Benjamin Marzinski wrote:
> On Tue, Nov 16, 2021 at 10:00:53PM +0800, lixiaokeng wrote:
> > In _DEBUG_ mode, MALLOC/STRDUP/REALLOC and FREE will record
> > the memory usage. Match them.
>
> This looks fine, but personally, I'd rather just have all the DEBUG
> memory code removed. If people want to check memory usage, there's
> always valgrind.
Ack. I'd rather see the MALLOC, FREE etc removed from the code.
I wouldn't want to add any more of them.
Good to see that you have similar feelings about this.
Martin
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [dm-devel] [PATCH 4/5] Match FREE and MALLOC/STRDUP/REALLOC
2021-11-18 2:46 ` lixiaokeng
@ 2021-11-18 15:35 ` Martin Wilck
0 siblings, 0 replies; 17+ messages in thread
From: Martin Wilck @ 2021-11-18 15:35 UTC (permalink / raw)
To: lixiaokeng, Benjamin Marzinski
Cc: dm-devel, list, liuzhiqiang (I), linfeilong
On Thu, 2021-11-18 at 10:46 +0800, lixiaokeng wrote:
>
>
> On 2021/11/18 8:47, Benjamin Marzinski wrote:
> > On Tue, Nov 16, 2021 at 10:00:53PM +0800, lixiaokeng wrote:
> > > In _DEBUG_ mode, MALLOC/STRDUP/REALLOC and FREE will record
> > > the memory usage. Match them.
> >
> > This looks fine, but personally, I'd rather just have all the DEBUG
> > memory code removed. If people want to check memory usage, there's
> > always valgrind.
> >
> > -Ben
> >
> > >
>
> If MALLOC/STRDUP/REALLOC and FREE is unnecessary, I'll remove this
> patch
> in this series and make a patch remove them latter. What is Martin's
> opinion?
As you saw, I agree with Ben. If you want to make a major contribution,
prepare a patch that gets rid of all those. :-)
Just note that MALLOC() is really calloc().
The only one that that's useful in non-debug mode is FREE(), because it
sets the pointer to NULL, avoiding double-free. Therefore a patch
replacing FREE() with free() would need to assess where that's
necessary (basically always if the data structure that holds the
pointer lives on after the free() call).
Regards
Martin
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [dm-devel] [PATCH 5/5] add prflag to path
2021-11-16 14:01 ` [dm-devel] [PATCH 5/5] add prflag to path lixiaokeng
@ 2021-11-18 15:36 ` Martin Wilck
2021-11-18 16:57 ` Benjamin Marzinski
1 sibling, 0 replies; 17+ messages in thread
From: Martin Wilck @ 2021-11-18 15:36 UTC (permalink / raw)
To: lixiaokeng, Christophe Varoqui, Benjamin Marzinski,
dm-devel mailing list
Cc: linfeilong, liuzhiqiang (I)
On Tue, 2021-11-16 at 22:01 +0800, lixiaokeng wrote:
> The update_map will frequently be called and there will be
> unnecessary checks of reseravtion. We add prflag to path
> to avoid this.
>
> The pp->state changes from others to up or ghost, the
> mpath_pr_event_handle should be called. The
> mpath_pr_event_handle in ev_add_path may not be called,
> so set pp->prkey PRKEY_NO when path is removed.
>
> Fix: 4db4fa
> Signed-off-by: Lixiaokeng <lixiaokeng>
> ---
> libmpathpersist/mpath_persist.c | 2 +-
> libmultipath/structs.c | 1 +
> libmultipath/structs.h | 12 ++++++++++++
> multipathd/cli_handlers.c | 15 ++++++++++-----
> multipathd/main.c | 5 +++--
> 5 files changed, 27 insertions(+), 8 deletions(-)
>
> diff --git a/libmpathpersist/mpath_persist.c
> b/libmpathpersist/mpath_persist.c
> index 803a2a28..f88a2e89 100644
> --- a/libmpathpersist/mpath_persist.c
> +++ b/libmpathpersist/mpath_persist.c
>
> +/*
> + * PRFLAG_NO for path, it means reservation should be checked.
> + * PRFLAG_NO for multipath, it means mpp has no prkey.
> + * PRFLAG_OK for path, it means reservation has been checked.
> + * PRFLAG_OK for multipath, it means mpp has prkey.
> + */
Please don't use the same enum with different meanings in different
context.
Martin
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [dm-devel] [PATCH 5/5] add prflag to path
2021-11-16 14:01 ` [dm-devel] [PATCH 5/5] add prflag to path lixiaokeng
2021-11-18 15:36 ` Martin Wilck
@ 2021-11-18 16:57 ` Benjamin Marzinski
2021-11-19 8:41 ` lixiaokeng
1 sibling, 1 reply; 17+ messages in thread
From: Benjamin Marzinski @ 2021-11-18 16:57 UTC (permalink / raw)
To: lixiaokeng
Cc: linfeilong, dm-devel mailing list, Martin Wilck, liuzhiqiang (I)
On Tue, Nov 16, 2021 at 10:01:15PM +0800, lixiaokeng wrote:
> The update_map will frequently be called and there will be
> unnecessary checks of reseravtion. We add prflag to path
> to avoid this.
>
> The pp->state changes from others to up or ghost, the
> mpath_pr_event_handle should be called. The
> mpath_pr_event_handle in ev_add_path may not be called,
> so set pp->prkey PRKEY_NO when path is removed.
This patch kind of confuses me. You only check pp->prkey before calling
mpath_pr_event_handle() in update_map(). I get from your commit message
that you are doing this to keep from frequent, unnecessary calls. But
isn't update_map() only called when a multipath device is first created,
or when multipathd stops waiting for something that it noticed during
device creation? I don't see how this can be frequently called on a
multipath device. What am I missing?
-Ben
>
> Fix: 4db4fa
> Signed-off-by: Lixiaokeng <lixiaokeng>
> ---
> libmpathpersist/mpath_persist.c | 2 +-
> libmultipath/structs.c | 1 +
> libmultipath/structs.h | 12 ++++++++++++
> multipathd/cli_handlers.c | 15 ++++++++++-----
> multipathd/main.c | 5 +++--
> 5 files changed, 27 insertions(+), 8 deletions(-)
>
> diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c
> index 803a2a28..f88a2e89 100644
> --- a/libmpathpersist/mpath_persist.c
> +++ b/libmpathpersist/mpath_persist.c
> @@ -924,7 +924,7 @@ int update_map_pr(struct multipath *mpp)
>
> if (isFound)
> {
> - mpp->prflag = 1;
> + mpp->prflag = PRFLAG_OK;
> condlog(2, "%s: prflag flag set.", mpp->alias );
> }
>
> diff --git a/libmultipath/structs.c b/libmultipath/structs.c
> index e8cacb4b..82dbd565 100644
> --- a/libmultipath/structs.c
> +++ b/libmultipath/structs.c
> @@ -122,6 +122,7 @@ uninitialize_path(struct path *pp)
> pp->dmstate = PSTATE_UNDEF;
> pp->uid_attribute = NULL;
> pp->getuid = NULL;
> + pp->prflag = PRFLAG_NO;
>
> if (checker_selected(&pp->checker))
> checker_put(&pp->checker);
> diff --git a/libmultipath/structs.h b/libmultipath/structs.h
> index 399540e7..5b77218b 100644
> --- a/libmultipath/structs.h
> +++ b/libmultipath/structs.h
> @@ -249,6 +249,17 @@ enum recheck_wwid_states {
> RECHECK_WWID_ON = YNU_YES,
> };
>
> +/*
> + * PRFLAG_NO for path, it means reservation should be checked.
> + * PRFLAG_NO for multipath, it means mpp has no prkey.
> + * PRFLAG_OK for path, it means reservation has been checked.
> + * PRFLAG_OK for multipath, it means mpp has prkey.
> + */
> +enum prflag_states {
> + PRFLAG_NO = 0,
> + PRFLAG_OK = 1,
> +};
> +
> struct vpd_vendor_page {
> int pg;
> const char *name;
> @@ -327,6 +338,7 @@ struct path {
> /* configlet pointers */
> vector hwe;
> struct gen_path generic_path;
> + int prflag;
> };
>
> typedef int (pgpolicyfn) (struct multipath *, vector);
> diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> index 6d3a0ae2..8662fad7 100644
> --- a/multipathd/cli_handlers.c
> +++ b/multipathd/cli_handlers.c
> @@ -1341,7 +1341,7 @@ cli_setprstatus(void * v, char ** reply, int * len, void * data)
> return 1;
>
> if (!mpp->prflag) {
> - mpp->prflag = 1;
> + mpp->prflag = PRFLAG_OK;
> condlog(2, "%s: prflag set", param);
> }
>
> @@ -1352,9 +1352,11 @@ cli_setprstatus(void * v, char ** reply, int * len, void * data)
> int
> cli_unsetprstatus(void * v, char ** reply, int * len, void * data)
> {
> - struct multipath * mpp;
> - struct vectors * vecs = (struct vectors *)data;
> - char * param = get_keyparam(v, MAP);
> + int i;
> + struct multipath *mpp;
> + struct path *pp;
> + struct vectors *vecs = (struct vectors *)data;
> + char *param = get_keyparam(v, MAP);
>
> param = convert_dev(param, 0);
> get_path_layout(vecs->pathvec, 0);
> @@ -1364,7 +1366,10 @@ cli_unsetprstatus(void * v, char ** reply, int * len, void * data)
> return 1;
>
> if (mpp->prflag) {
> - mpp->prflag = 0;
> + mpp->prflag = PRFLAG_NO;
> + vector_foreach_slot(mpp->paths, pp, i) {
> + pp->prflag = PRFLAG_NO;
> + }
> condlog(2, "%s: prflag unset", param);
> }
>
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 82ab3ed1..6ef6495b 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -506,7 +506,7 @@ retry:
>
> if (mpp->prflag) {
> vector_foreach_slot(mpp->paths, pp, i) {
> - if ((pp->state == PATH_UP) || (pp->state == PATH_GHOST)) {
> + if (!pp->prflag && ((pp->state == PATH_UP) || (pp->state == PATH_GHOST))) {
> /* persistent reseravtion check*/
> mpath_pr_event_handle(pp);
> }
> @@ -3570,7 +3570,8 @@ void * mpath_pr_event_handler_fn (void * pathp )
> {
> condlog(0,"%s: Reservation registration failed. Error: %d", pp->dev, ret);
> }
> - mpp->prflag = 1;
> + mpp->prflag = PRFLAG_OK;
> + pp->prflag = PRFLAG_OK;
>
> free(param);
> out:
> --
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [dm-devel] [PATCH 5/5] add prflag to path
2021-11-18 16:57 ` Benjamin Marzinski
@ 2021-11-19 8:41 ` lixiaokeng
0 siblings, 0 replies; 17+ messages in thread
From: lixiaokeng @ 2021-11-19 8:41 UTC (permalink / raw)
To: Benjamin Marzinski
Cc: linfeilong, dm-devel mailing list, Martin Wilck, liuzhiqiang (I)
On 2021/11/19 0:57, Benjamin Marzinski wrote:
> On Tue, Nov 16, 2021 at 10:01:15PM +0800, lixiaokeng wrote:
>> The update_map will frequently be called and there will be
>> unnecessary checks of reseravtion. We add prflag to path
>> to avoid this.
>>
>> The pp->state changes from others to up or ghost, the
>> mpath_pr_event_handle should be called. The
>> mpath_pr_event_handle in ev_add_path may not be called,
>> so set pp->prkey PRKEY_NO when path is removed.
>
> This patch kind of confuses me. You only check pp->prkey before calling
> mpath_pr_event_handle() in update_map(). I get from your commit message
> that you are doing this to keep from frequent, unnecessary calls. But
> isn't update_map() only called when a multipath device is first created,
> or when multipathd stops waiting for something that it noticed during
> device creation? I don't see how this can be frequently called on a
> multipath device. What am I missing?
>
> -Ben
>
Discussed in the previous patch (multipathd: fix missing persistent
reseravtion for active path) as follows:
On Mon, 2021-09-13 at 10:32 -0500, Benjamin Marzinski wrote:
> On Mon, Sep 13, 2021 at 09:01:11AM +0200, Martin Wilck wrote:
>> Hello lixiaokeng,
>>
>> On Mon, 2021-09-13 at 10:43 +0800, lixiaokeng wrote:
>>> There are two paths(sucu as sda and adb) for one LUN. The two
>>> paths log in, but before the two uevents have been processed
>>> (for example there are many uevent), users use multipathd add
>>> path /dev/sda to cause mpatha and use mpathpersist -o -I to
>>> register prkey for mpatha. The add map uevent is after add path
>>> uevent, the the uevent(add sdb) will delay and missing persistent
>>> reseravtion check.
>>>
>>> Here, we add persistent reseravtion check in update_map() which
>>> is called ev_add_map().
>>>
>>> Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
>>
>> Thank you, this looks ok to me. Have you tested it?
>>
>> I'll wait for Ben's opinion nonetheless, because he's more
>> exprerienced
>> with this part of the code than myself.
>>
>> This said, I would like to have multipathd record which paths have
>> already registered the key, to avoid doing that repeatedly.
>>
> Other than adding this, the patch looks fine.
There may be some mistakes. I mistakenly thought update_map would
be called multiple times
In check_path, mpath_pr_event_handle is only called pp->state
changes from others to up. Some prkey changes may happen when
pp->state is down, so mpath_pr_event_handle should be called
even pp->prkey is PRKEY_OK.
As Ben said, update_map will not be called repetitively. I think
this patch should be removed.
Thank Ben and Martin.
Regards,
Lixiaokeng
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2021-11-19 8:41 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-16 13:58 [dm-devel] [PATCH 0/5] multipath-tools series: some codeclean and add prflag to path lixiaokeng
2021-11-16 13:59 ` [dm-devel] [PATCH 1/5] Fix potential null pointer dereference lixiaokeng
2021-11-17 18:05 ` Benjamin Marzinski
2021-11-18 2:04 ` lixiaokeng
2021-11-16 13:59 ` [dm-devel] [PATCH 2/5] remove unnecessary memset lixiaokeng
2021-11-18 0:10 ` Benjamin Marzinski
2021-11-16 14:00 ` [dm-devel] [PATCH 3/5] remove unnecessary free lixiaokeng
2021-11-18 0:14 ` Benjamin Marzinski
2021-11-16 14:00 ` [dm-devel] [PATCH 4/5] Match FREE and MALLOC/STRDUP/REALLOC lixiaokeng
2021-11-18 0:47 ` Benjamin Marzinski
2021-11-18 2:46 ` lixiaokeng
2021-11-18 15:35 ` Martin Wilck
2021-11-18 15:31 ` Martin Wilck
2021-11-16 14:01 ` [dm-devel] [PATCH 5/5] add prflag to path lixiaokeng
2021-11-18 15:36 ` Martin Wilck
2021-11-18 16:57 ` Benjamin Marzinski
2021-11-19 8:41 ` lixiaokeng
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.