[PATCH 00/14] multipath-tools series: some cleanups and fixes checked by codedex tool

[PATCH 00/14] multipath-tools series: some cleanups and fixes checked by codedex tool

[lixiaokeng]

Hi:
    Now, we check multipath-tools codes with codedex tool. Here
are some some cleanups and fixes.

Zhiqiang Liu (7):
  multipathd: use MALLOC and check return value in cli_getprkey func
  kpartx: check return value of malloc in main func
  libmultipath: check return value of dm_mapname in sysfs_check_holders
  libmultipath: donot free *dst if REALLOC fails in merge_words
  libmultipath: check whether mp->features is NULl in assemble_map
  util/tests: use assert_non_null to ensure malloc returns non-null
    pointer
  mpathpersist: check whether malloc paramp->trnptid_list fails in
    handle_args func

lixiaokeng (7):
  multipathd: return if dm_get_major_minor failed in cli_add_map
  libmultipath: check malloc return value in print_foreign_topology
  libmultipath: use map instead of dm_task_get_name
  multipathd: check MALLOC return value in mpath_pr_event_handler_fn
  libmultipathpersist: use update_multipath_table/status in get_mpvec
  multipath: use update_multipath_table/status in check_useable_paths
  multipathpersist: delete unused variable in handle_args

 kpartx/kpartx.c                 |  4 +++
 libmpathpersist/mpath_persist.c | 11 ++-----
 libmultipath/devmapper.c        |  2 +-
 libmultipath/dmparser.c         | 18 +++++------
 libmultipath/foreign.c          |  4 +++
 libmultipath/sysfs.c            |  6 +++-
 mpathpersist/main.c             | 56 +++++++++++++++++++++++++++------
 multipath/main.c                |  9 ++----
 multipathd/cli_handlers.c       | 17 +++++++---
 multipathd/main.c               |  8 +++--
 tests/util.c                    |  2 ++
 11 files changed, 96 insertions(+), 41 deletions(-)

-- 

[PATCH 01/14] multipathd: return if dm_get_major_minor failed in, cli_add_map

[lixiaokeng]

If dm_get_major_minor failed, log with major and minor should not
be printed to avoid major and minor used before initialization.

Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Signed-off-by: Linfeilong <linfeilong@huawei.com>
---
 multipathd/cli_handlers.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 8db37961..2d297fd0 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -847,11 +847,12 @@ cli_add_map (void * v, char ** reply, int * len, void * data)
 		else {
 			sprintf(dev_path, "dm-%d", minor);
 			alias = dm_mapname(major, minor);
+			if (!alias)
+				condlog(2, "%s: mapname not found for %d:%d",
+					param, major, minor);
 		}
 		/*if there is no mapname found, we first create the device*/
 		if (!alias && !count) {
-			condlog(2, "%s: mapname not found for %d:%d",
-				param, major, minor);
 			get_refwwid(CMD_NONE, param, DEV_DEVMAP,
 				    vecs->pathvec, &refwwid);
 			if (refwwid) {
-- 

[PATCH 02/14] libmultipath: check malloc return value in, print_foreign_topology

[lixiaokeng]

We check the return value of malloc in print_foreign_topology.

Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Signed-off-by: Linfeilong <linfeilong@huawei.com>
---
 libmultipath/foreign.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/libmultipath/foreign.c b/libmultipath/foreign.c
index e8f61351..44f32d03 100644
--- a/libmultipath/foreign.c
+++ b/libmultipath/foreign.c
@@ -545,6 +545,10 @@ void print_foreign_topology(int verbosity)
 	char *buf = NULL, *tmp = NULL;

 	buf = malloc(buflen);
+	if (!buf) {
+		condlog (0, "malloc buf failed.");
+		return;
+	}
 	buf[0] = '\0';
 	while (buf != NULL) {
 		char *c = buf;
--

[PATCH 03/14] libmultipath: use map instead of dm_task_get_name

[lixiaokeng]

In dm_mapname, dm_task_get_name(dmt) has been called and the return value
has been stored in map. Use map instead of second  dm_task_get_name.

Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Signed-off-by: Linfeilong <linfeilong@huawei.com>
---
 libmultipath/devmapper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index cc2de1df..b4d77cb6 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -1315,7 +1315,7 @@ dm_mapname(int major, int minor)

 	map = dm_task_get_name(dmt);
 	if (map && strlen(map))
-		response = STRDUP((const char *)dm_task_get_name(dmt));
+		response = STRDUP((const char *)map);

 	dm_task_destroy(dmt);
 	return response;
-- 

[PATCH 04/14] multipathd: check MALLOC return value in, mpath_pr_event_handler_fn

[lixiaokeng]

In  mpath_pr_event_handler_fn, we use MALLOC instead of malloc, and check
the return value of MALLOC.

Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Signed-off-by: Linfeilong <linfeilong@huawei.com>
---
 multipathd/main.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 67e9af11..7180d3c1 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -3391,8 +3391,12 @@ void *  mpath_pr_event_handler_fn (void * pathp )
 		goto out;
 	}

-	param= malloc(sizeof(struct prout_param_descriptor));
-	memset(param, 0 , sizeof(struct prout_param_descriptor));
+	param = (struct prout_param_descriptor *)MALLOC(sizeof(struct prout_param_descriptor));
+	if (!param) {
+		ret = MPATH_PR_OTHER;
+		goto out;
+	}
+
 	param->sa_flags = mpp->sa_flags;
 	memcpy(param->sa_key, &mpp->reservation_key, 8);
 	param->num_transportid = 0;
-- 

[PATCH 05/14] multipathd: use MALLOC and check return value in, cli_getprkey func

[lixiaokeng]

In cli_getprkey func, we use MALLOC instead of malloc, and check
the return value of MALLOC.

Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
Signed-off-by: Linfeilong <linfeilong@huawei.com>
---
 multipathd/cli_handlers.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 27e4574f..d345afd3 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -1535,7 +1535,11 @@ cli_getprkey(void * v, char ** reply, int * len, void * data)
 	if (!mpp)
 		return 1;

-	*reply = malloc(26);
+	*reply = MALLOC(26);
+	if (!*reply) {
+		condlog(0, "malloc *reply failed.");
+		return 1;
+	}

 	if (!get_be64(mpp->reservation_key)) {
 		sprintf(*reply, "none\n");
-- 

[PATCH 06/14] kpartx: check return value of malloc in main func

[lixiaokeng]

In main func of kpartx.c, we should check return value of
malloc before using it.

Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
---
 kpartx/kpartx.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
index 98f6176e..2f6dea7c 100644
--- a/kpartx/kpartx.c
+++ b/kpartx/kpartx.c
@@ -384,6 +384,10 @@ main(int argc, char **argv){

 	if (delim == NULL) {
 		delim = malloc(DELIM_SIZE);
+		if (!delim) {
+			fprintf(stderr, "malloc delim failed.\n");
+			exit(1);
+		}
 		memset(delim, 0, DELIM_SIZE);
 		set_delimiter(mapname, delim);
 	}
-- 

[PATCH 07/14] libmultipath: check return value of dm_mapname in, sysfs_check_holders

[lixiaokeng]

In sysfs_check_holders func, table_name is obtained by calling
dm_mapname func, and then call dm_reassign_table for reassigning
table. However, we donnot check whether dm_mapname func returns
NULL, and then it may cause a segmentation fault in dm_task_set_name.

Here, we will check whether dm_mapname func returns NULL before
using it.

Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
---
 libmultipath/sysfs.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/libmultipath/sysfs.c b/libmultipath/sysfs.c
index 12a82d95..5390de62 100644
--- a/libmultipath/sysfs.c
+++ b/libmultipath/sysfs.c
@@ -278,7 +278,11 @@ int sysfs_check_holders(char * check_devt, char * new_devt)
 			continue;
 		}
 		table_name = dm_mapname(major, table_minor);
-
+		if (!table_name) {
+			condlog(2, "%s: mapname not found for %d:%d", check_dev,
+				major, table_minor);
+			continue;
+		}
 		condlog(0, "%s: reassign table %s old %s new %s", check_dev,
 			table_name, check_devt, new_devt);

-- 

[PATCH 08/14] libmultipath: donot free *dst if REALLOC fails in, merge_words

[lixiaokeng]

In merge_words func, if REALLOC() fails, the input *dst will
be freed. If so, mpp->hwhandler| mpp->features|mpp->selector
may be set to NULL after calling merge_words func in
disassemble_map func. This may cause accessing freed memory
problem.

Here, we donot free *dst if REALLOC() fails in merge_words func.

Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
---
 libmultipath/dmparser.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
index c1031616..482e9d0e 100644
--- a/libmultipath/dmparser.c
+++ b/libmultipath/dmparser.c
@@ -26,13 +26,12 @@ merge_words(char **dst, const char *word)

 	dstlen = strlen(*dst);
 	len = dstlen + strlen(word) + 2;
-	*dst = REALLOC(*dst, len);
+	p = REALLOC(*dst, len);

-	if (!*dst) {
-		free(p);
+	if (!p)
 		return 1;
-	}

+	*dst = p;
 	p = *dst + dstlen;
 	*p = ' ';
 	++p;
-- 

[PATCH 09/14] libmultipath: check whether mp->features is NULl in, assemble_map

[lixiaokeng]

In assemble_map func, if add_feature fails and mp->features is
default value (NULL), STRDUP(mp->features) will cause a seg-fault.
In addition, f = STRDUP(mp->features) is just used for APPEND().
We can directly pass mp->features to APPEND().

Here, we firstly check whether mp->features is null.

Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
---
 libmultipath/dmparser.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
index 482e9d0e..12182bf5 100644
--- a/libmultipath/dmparser.c
+++ b/libmultipath/dmparser.c
@@ -65,7 +65,7 @@ assemble_map (struct multipath * mp, char * params, int len)
 	int i, j;
 	int minio;
 	int nr_priority_groups, initial_pg_nr;
-	char * p, * f;
+	char * p;
 	const char *const end = params + len;
 	char no_path_retry[] = "queue_if_no_path";
 	char retain_hwhandler[] = "retain_attached_hw_handler";
@@ -86,9 +86,12 @@ assemble_map (struct multipath * mp, char * params, int len)
 	    get_linux_version_code() < KERNEL_VERSION(4, 3, 0))
 		add_feature(&mp->features, retain_hwhandler);

-	f = STRDUP(mp->features);
+	if (!mp->features) {
+		condlog(0, "mp->features is still NULL.");
+		goto err;
+	}

-	APPEND(p, end, "%s %s %i %i", f, mp->hwhandler, nr_priority_groups,
+	APPEND(p, end, "%s %s %i %i", mp->features, mp->hwhandler, nr_priority_groups,
 	       initial_pg_nr);

 	vector_foreach_slot (mp->pg, pgp, i) {
@@ -110,12 +113,10 @@ assemble_map (struct multipath * mp, char * params, int len)
 		}
 	}

-	FREE(f);
 	condlog(4, "%s: assembled map [%s]", mp->alias, params);
 	return 0;

 err:
-	FREE(f);
 	return 1;
 }

-- 

[PATCH 10/14] util/tests: use assert_non_null to ensure malloc, returns non-null pointer

[lixiaokeng]

In tests/util.c, we should use assert_non_null to ensure
malloc() returns non-null pointer in both test_strlcpy_5
and test_strlcpy_6 func.

Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
Signed-off-by: Linfeilong <linfeilong@huawei.com>
---
 tests/util.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/util.c b/tests/util.c
index 16774dff..455eeee5 100644
--- a/tests/util.c
+++ b/tests/util.c
@@ -523,6 +523,7 @@ static void test_strlcpy_5(void **state)
 	const int sz = sizeof(src_str);

 	tst = malloc(sz);
+	assert_non_null(tst);
 	memset(tst, 'f', sizeof(src_str));

 	rc = strlcpy(tst, src_str, sz);
@@ -540,6 +541,7 @@ static void test_strlcpy_6(void **state)
 	const int sz = sizeof(src_str);

 	tst = malloc(sz + 2);
+	assert_non_null(tst);
 	memset(tst, 'f', sz + 2);

 	rc = strlcpy(tst, src_str, sz + 2);
-- 

[PATCH 11/14] mpathpersist: check whether malloc paramp->trnptid_list, fails in handle_args func

[lixiaokeng]

In handle_args func, we donot check whether malloc paramp and
each paramp->trnptid_list[j] fails before using them, it may
cause access NULL pointer.

Here, we add alloc_prout_param_descriptor to allocate and init
paramp, and we add free_prout_param_descriptor to free paramp
and each paramp->trnptid_list[j].

Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
Signed-off-by: Linfeilong <linfeilong@huawei.com>
---
 mpathpersist/main.c | 55 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 46 insertions(+), 9 deletions(-)

diff --git a/mpathpersist/main.c b/mpathpersist/main.c
index 28bfe410..f20c902c 100644
--- a/mpathpersist/main.c
+++ b/mpathpersist/main.c
@@ -153,6 +153,39 @@ static int do_batch_file(const char *batch_fn)
 	return ret;
 }

+static struct prout_param_descriptor *
+alloc_prout_param_descriptor(int num_transportid)
+{
+	struct prout_param_descriptor *paramp;
+
+	if (num_transportid < 0 || num_transportid > MPATH_MX_TIDS)
+		return NULL;
+
+	paramp= malloc(sizeof(struct prout_param_descriptor) +
+				(sizeof(struct transportid *) * num_transportid));
+
+	if (!paramp)
+		return NULL;
+
+	paramp->num_transportid = num_transportid;
+	memset(paramp, 0, sizeof(struct prout_param_descriptor) +
+			(sizeof(struct transportid *) * num_transportid));
+	return paramp;
+}
+
+static void free_prout_param_descriptor(struct prout_param_descriptor *paramp)
+{
+	int i;
+	if (!paramp)
+		return;
+
+	for (i = 0; i < paramp->num_transportid; i++)
+		free(paramp->trnptid_list[i]);
+
+	free(paramp);
+	paramp = NULL;
+}
+
 static int handle_args(int argc, char * argv[], int nline)
 {
 	int c;
@@ -525,9 +558,12 @@ static int handle_args(int argc, char * argv[], int nline)
 		int j;
 		struct prout_param_descriptor *paramp;

-		paramp= malloc(sizeof(struct prout_param_descriptor) + (sizeof(struct transportid *)*(MPATH_MX_TIDS )));
-
-		memset(paramp, 0, sizeof(struct prout_param_descriptor) + (sizeof(struct transportid *)*(MPATH_MX_TIDS)));
+		paramp = alloc_prout_param_descriptor(num_transport);
+		if (!paramp) {
+			fprintf(stderr, "malloc paramp failed\n");
+			ret = MPATH_PR_OTHER;
+			goto out_fd;
+		}

 		for (j = 7; j >= 0; --j) {
 			paramp->key[j] = (param_rk & 0xff);
@@ -551,6 +587,12 @@ static int handle_args(int argc, char * argv[], int nline)
 			for (j = 0 ; j < num_transport; j++)
 			{
 				paramp->trnptid_list[j] = (struct transportid *)malloc(sizeof(struct transportid));
+				if (!paramp->trnptid_list[j]) {
+					fprintf(stderr, "malloc paramp->trnptid_list[%d] failed.\n", j);
+					ret = MPATH_PR_OTHER;
+					free_prout_param_descriptor(paramp);
+					goto out_fd;
+				}
 				memcpy(paramp->trnptid_list[j], &transportids[j],sizeof(struct transportid));
 			}
 		}
@@ -558,12 +600,7 @@ static int handle_args(int argc, char * argv[], int nline)
 		/* PROUT commands other than 'register and move' */
 		ret = __mpath_persistent_reserve_out (fd, prout_sa, 0, prout_type,
 				paramp, noisy);
-		for (j = 0 ; j < num_transport; j++)
-		{
-			tmp = paramp->trnptid_list[j];
-			free(tmp);
-		}
-		free(paramp);
+		free_prout_param_descriptor(paramp);
 	}

 	if (ret != MPATH_PR_SUCCESS)
-- 

[PATCH 12/14] libmultipathpersist: use update_multipath_table/status, in get_mpvec

[lixiaokeng]

The return values of dm_get_map, disassemble_map in get_mpvec
were not checked. Use update_multipath_table/status to instead
of them.

Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Signed-off-by: Linfeilong <linfeilong@huawei.com>
---
 libmpathpersist/mpath_persist.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c
index e7256049..046fda21 100644
--- a/libmpathpersist/mpath_persist.c
+++ b/libmpathpersist/mpath_persist.c
@@ -323,7 +323,6 @@ get_mpvec (vector curmp, vector pathvec, char * refwwid)
 {
 	int i;
 	struct multipath *mpp;
-	char params[PARAMS_SIZE], status[PARAMS_SIZE];

 	vector_foreach_slot (curmp, mpp, i){
 		/*
@@ -341,13 +340,9 @@ get_mpvec (vector curmp, vector pathvec, char * refwwid)
 		if (refwwid && strncmp (mpp->alias, refwwid, WWID_SIZE - 1))
 			continue;

-		dm_get_map(mpp->alias, &mpp->size, params);
-		condlog(3, "params = %s", params);
-		dm_get_status(mpp->alias, status);
-		condlog(3, "status = %s", status);
-		disassemble_map (pathvec, params, mpp);
-		update_pathvec_from_dm(pathvec, mpp, DI_CHECKER);
-		disassemble_status (status, mpp);
+		if (update_multipath_table(mpp, pathvec, DI_CHECKER) != DMP_OK ||
+		    update_multipath_status(mpp) != DMP_OK)
+			continue;

 	}
 	return MPATH_PR_SUCCESS ;
-- 

[PATCH 13/14] multipath: use update_multipath_table/status in, check_useable_paths

[lixiaokeng]

The return values of dm_get_map, disassemble_map,dm_get_status
and disassemble_status in check_usable_paths were not checked.
Use update_multipath_table/status to instead of them.

Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Signed-off-by: Linfeilong <linfeilong@huawei.com>
---
 multipath/main.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/multipath/main.c b/multipath/main.c
index d227e0b3..9e920d89 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -251,7 +251,6 @@ static int check_usable_paths(struct config *conf,
 	struct path *pp;
 	char *mapname;
 	vector pathvec = NULL;
-	char params[PARAMS_SIZE], status[PARAMS_SIZE];
 	dev_t devt;
 	int r = 1, i, j;

@@ -285,11 +284,9 @@ static int check_usable_paths(struct config *conf,
 	if (mpp == NULL)
 		goto free;

-	dm_get_map(mpp->alias, &mpp->size, params);
-	dm_get_status(mpp->alias, status);
-	disassemble_map(pathvec, params, mpp);
-	update_pathvec_from_dm(pathvec, mpp, 0);
-	disassemble_status(status, mpp);
+	if (update_multipath_table(mpp, pathvec, 0) != DMP_OK ||
+		    update_multipath_status(mpp) != DMP_OK)
+		    goto free;

 	vector_foreach_slot (mpp->pg, pg, i) {
 		vector_foreach_slot (pg->paths, pp, j) {
-- 

[PATCH 14/14] multipathpersist: delete unused variable in handle_args

[lixiaokeng]

In handle_args, the tmp isn't used. We delete it.

Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Signed-off-by: Linfeilong <linfeilong@huawei.com>
---
 mpathpersist/main.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/mpathpersist/main.c b/mpathpersist/main.c
index f20c902c..ccf0024e 100644
--- a/mpathpersist/main.c
+++ b/mpathpersist/main.c
@@ -213,7 +213,6 @@ static int handle_args(int argc, char * argv[], int nline)
 	int num_transport =0;
 	char *batch_fn = NULL;
 	void *resp = NULL;
-	struct transportid * tmp;

 	memset(transportids, 0, MPATH_MX_TIDS * sizeof(struct transportid));

-- 

Re: [PATCH 01/14] multipathd: return if dm_get_major_minor failed in, cli_add_map

[Martin Wilck]

On Wed, 2020-09-02 at 15:15 +0800, lixiaokeng wrote:
> If dm_get_major_minor failed, log with major and minor should not
> be printed to avoid major and minor used before initialization.
> 
> Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: Linfeilong <linfeilong@huawei.com>
> ---
>  multipathd/cli_handlers.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> index 8db37961..2d297fd0 100644
> --- a/multipathd/cli_handlers.c
> +++ b/multipathd/cli_handlers.c
> @@ -847,11 +847,12 @@ cli_add_map (void * v, char ** reply, int *
> len, void * data)

Why not just quit the "do" loop in the error case
for dm_get_major_minor()?

Martin

>  		else {
>  			sprintf(dev_path, "dm-%d", minor);
>  			alias = dm_mapname(major, minor);
> +			if (!alias)
> +				condlog(2, "%s: mapname not found for
> %d:%d",
> +					param, major, minor);
>  		}
>  		/*if there is no mapname found, we first create the
> device*/
>  		if (!alias && !count) {
> -			condlog(2, "%s: mapname not found for %d:%d",
> -				param, major, minor);
>  			get_refwwid(CMD_NONE, param, DEV_DEVMAP,
>  				    vecs->pathvec, &refwwid);
>  			if (refwwid) {

Re: [PATCH 02/14] libmultipath: check malloc return value in, print_foreign_topology

[Martin Wilck]

On Wed, 2020-09-02 at 15:16 +0800, lixiaokeng wrote:
> We check the return value of malloc in print_foreign_topology.
> 
> Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: Linfeilong <linfeilong@huawei.com>
> ---
>  libmultipath/foreign.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/libmultipath/foreign.c b/libmultipath/foreign.c
> index e8f61351..44f32d03 100644
> --- a/libmultipath/foreign.c
> +++ b/libmultipath/foreign.c
> @@ -545,6 +545,10 @@ void print_foreign_topology(int verbosity)
>  	char *buf = NULL, *tmp = NULL;
> 
>  	buf = malloc(buflen);
> +	if (!buf) {
> +		condlog (0, "malloc buf failed.");
> +		return;
> +	}

Just replace the malloc() by calloc() please, and remove the buf[0]
initialization. No need for an error message.

Martin


>  	buf[0] = '\0';
>  	while (buf != NULL) {
>  		char *c = buf;
> --
> 

Re: [PATCH 03/14] libmultipath: use map instead of dm_task_get_name

[Martin Wilck]

On Wed, 2020-09-02 at 15:17 +0800, lixiaokeng wrote:
> In dm_mapname, dm_task_get_name(dmt) has been called and the return
> value
> has been stored in map. Use map instead of second  dm_task_get_name.
> 
> Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: Linfeilong <linfeilong@huawei.com>
> ---
>  libmultipath/devmapper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index cc2de1df..b4d77cb6 100644
> --- a/libmultipath/devmapper.c
> +++ b/libmultipath/devmapper.c
> @@ -1315,7 +1315,7 @@ dm_mapname(int major, int minor)
> 
>  	map = dm_task_get_name(dmt);
>  	if (map && strlen(map))
> -		response = STRDUP((const char *)dm_task_get_name(dmt));
> +		response = STRDUP((const char *)map);

dm_task_get_name() is not an expensive call. Anyway:

Reviewed-by: Martin Wilck <mwilck@suse.com>

> 
>  	dm_task_destroy(dmt);
>  	return response;

Re: [PATCH 04/14] multipathd: check MALLOC return value in, mpath_pr_event_handler_fn

[Martin Wilck]

On Wed, 2020-09-02 at 15:17 +0800, lixiaokeng wrote:
> In  mpath_pr_event_handler_fn, we use MALLOC instead of malloc, and
> check
> the return value of MALLOC.
> 
> Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: Linfeilong <linfeilong@huawei.com>
> ---
>  multipathd/main.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 67e9af11..7180d3c1 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -3391,8 +3391,12 @@ void *  mpath_pr_event_handler_fn (void *
> pathp )
>  		goto out;
>  	}
> 
> -	param= malloc(sizeof(struct prout_param_descriptor));
> -	memset(param, 0 , sizeof(struct prout_param_descriptor));
> +	param = (struct prout_param_descriptor *)MALLOC(sizeof(struct
> prout_param_descriptor));
> +	if (!param) {
> +		ret = MPATH_PR_OTHER;
> +		goto out;
> +	}

No need to set the local variable "ret" when you jump to "out". I can
see this is done elsewhere in this function, too. It has been like that
since day 1.

Regards,
Martin

Re: [PATCH 05/14] multipathd: use MALLOC and check return value in, cli_getprkey func

[Martin Wilck]

On Wed, 2020-09-02 at 15:18 +0800, lixiaokeng wrote:
> In cli_getprkey func, we use MALLOC instead of malloc, and check
> the return value of MALLOC.
> 
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
> Signed-off-by: Linfeilong <linfeilong@huawei.com>
> ---
>  multipathd/cli_handlers.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> index 27e4574f..d345afd3 100644
> --- a/multipathd/cli_handlers.c
> +++ b/multipathd/cli_handlers.c
> @@ -1535,7 +1535,11 @@ cli_getprkey(void * v, char ** reply, int *
> len, void * data)
>  	if (!mpp)
>  		return 1;
> 
> -	*reply = malloc(26);
> +	*reply = MALLOC(26);
> +	if (!*reply) {
> +		condlog(0, "malloc *reply failed.");
> +		return 1;
> +	}

MALLOC is not necessary (*reply isn't left uninialized), nor is the
error message.

Regards,
Martin

Re: [PATCH 06/14] kpartx: check return value of malloc in main func

[Martin Wilck]

On Wed, 2020-09-02 at 15:19 +0800, lixiaokeng wrote:
> In main func of kpartx.c, we should check return value of
> malloc before using it.
> 
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
> ---
>  kpartx/kpartx.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
> index 98f6176e..2f6dea7c 100644
> --- a/kpartx/kpartx.c
> +++ b/kpartx/kpartx.c
> @@ -384,6 +384,10 @@ main(int argc, char **argv){
> 
>  	if (delim == NULL) {
>  		delim = malloc(DELIM_SIZE);
> +		if (!delim) {
> +			fprintf(stderr, "malloc delim failed.\n");
> +			exit(1);
> +		}
>  		memset(delim, 0, DELIM_SIZE);
>  		set_delimiter(mapname, delim);
>  	}

Please have a look at kpartx' xmalloc() function.

Regards,
Martin

Re: [PATCH 00/14] multipath-tools series: some cleanups and fixes checked by codedex tool

[Martin Wilck]

[-- Attachment #1: Type: text/plain, Size: 567 bytes --]

Hello Lixiaokeng,

On Wed, 2020-09-02 at 14:40 +0800, lixiaokeng wrote:
> Hi:
>     Now, we check multipath-tools codes with codedex tool. Here
> are some some cleanups and fixes.

Thank you. However I'm going to nack all patches that add error
messages after unsuccesful memory allocations. Such messages are
unhelpful most of the time, and increase the code size without a true
benefit. I've actually considered to get rid of all these, and replace
them by a log_oom() macro.

See an untested prototype attached, to better understand what I mean.

Regards
Martin



[-- Attachment #2: 0001-libmultipath-prototype-implementation-of-log_oom.patch --]
[-- Type: text/x-patch, Size: 1547 bytes --]

From fbbca2c5076a489ee4ae643d6d9199ca5085be95 Mon Sep 17 00:00:00 2001
From: Martin Wilck <mwilck@suse.com>
Date: Thu, 3 Sep 2020 22:03:22 +0200
Subject: [PATCH] libmultipath: prototype implementation of log_oom()

Rationale: with VERBOSE_OOM_LOGGING, we log the part of the code
where OOM occured, with minimal runtime effort (no string formatting).
With lots of log_oom() invocations, our binary will increase by many
static strings. Without VERBOSE_OOM_LOGGING, we just print a minimal
error message, which will be enough most of the time.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/debug.c | 7 +++++++
 libmultipath/debug.h | 8 ++++++++
 2 files changed, 15 insertions(+)

diff --git a/libmultipath/debug.c b/libmultipath/debug.c
index 4128cb9..9062581 100644
--- a/libmultipath/debug.c
+++ b/libmultipath/debug.c
@@ -48,3 +48,10 @@ void dlog (int sink, int prio, const char * fmt, ...)
 	}
 	va_end(ap);
 }
+
+#ifndef VERBOSE_OOM_LOGGING
+void log_oom(void)
+{
+	condlog(0, "Out of memory");
+}
+#endif
diff --git a/libmultipath/debug.h b/libmultipath/debug.h
index c6120c1..f61ecb6 100644
--- a/libmultipath/debug.h
+++ b/libmultipath/debug.h
@@ -11,3 +11,11 @@ extern int logsink;
 
 #define condlog(prio, fmt, args...) \
 	dlog(logsink, prio, fmt "\n", ##args)
+
+#ifdef VERBOSE_OOM_LOGGING
+#define __log_oom(file, line) condlog(0, "Out of memory in " file ":" #line)
+#define _log_oom(file, line) __log_oom(file, line)
+#define log_oom() _log_oom(__FILE__, __LINE__)
+#else
+void log_oom(void);
+#endif
-- 
2.28.0


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 00/14] multipath-tools series: some cleanups and fixes checked by codedex tool

[lixiaokeng]

Hello Martin,
   Thanks for your reviews. I will remove the error messages and send
them again.

Regards
Lixiaokeng

On 2020/9/4 4:08, Martin Wilck wrote:
> Hello Lixiaokeng,
> 
> On Wed, 2020-09-02 at 14:40 +0800, lixiaokeng wrote:
>> Hi:
>>     Now, we check multipath-tools codes with codedex tool. Here
>> are some some cleanups and fixes.
> 
> Thank you. However I'm going to nack all patches that add error
> messages after unsuccesful memory allocations. Such messages are
> unhelpful most of the time, and increase the code size without a true
> benefit. I've actually considered to get rid of all these, and replace
> them by a log_oom() macro.
> 
> See an untested prototype attached, to better understand what I mean.
> 
> Regards
> Martin
> 
> 

Re: [PATCH 01/14] multipathd: return if dm_get_major_minor failed in, cli_add_map

[lixiaokeng]

On 2020/9/4 1:26, Martin Wilck wrote:
> 
> Why not just quit the "do" loop in the error case
> for dm_get_major_minor()?
> 
> Martin

If dm_get_major failed at first time, it will be executed again
for some reason I don't know in the original code. Quiting the
"do" loop in the error case for dm_get_major_minor() is against
the twice attempt rule.

Lixiaokeng
> 
>>  		else {
>>  			sprintf(dev_path, "dm-%d", minor);
>>  			alias = dm_mapname(major, minor);
>> +			if (!alias)
>> +				condlog(2, "%s: mapname not found for
>> %d:%d",
>> +					param, major, minor);
>>  		}
>>  		/*if there is no mapname found, we first create the
>> device*/
>>  		if (!alias && !count) {
>> -			condlog(2, "%s: mapname not found for %d:%d",
>> -				param, major, minor);
>>  			get_refwwid(CMD_NONE, param, DEV_DEVMAP,
>>  				    vecs->pathvec, &refwwid);
>>  			if (refwwid) {
> 
> 
> 
> .
> 

Re: [PATCH 01/14] multipathd: return if dm_get_major_minor failed in, cli_add_map

[Martin Wilck]

On Fri, 2020-09-04 at 10:48 +0800, lixiaokeng wrote:
> 
> On 2020/9/4 1:26, Martin Wilck wrote:
> > Why not just quit the "do" loop in the error case
> > for dm_get_major_minor()?
> > 
> > Martin
> 
> If dm_get_major failed at first time, it will be executed again
> for some reason I don't know in the original code. Quiting the
> "do" loop in the error case for dm_get_major_minor() is against
> the twice attempt rule.

Right. 

Then, to solve the problem you're concerned about, it should be
sufficient to initialize both major and minor to -1, or simply refrain
from printing them in the log message in the first place.

At a closer look, the logic of the function is flawed; at least
if get_refwwid() doesn't return a usable refwwid, there's no point in
trying again.

Thanks,
Martin


> 
> Lixiaokeng
> > >  		else {
> > >  			sprintf(dev_path, "dm-%d", minor);
> > >  			alias = dm_mapname(major, minor);
> > > +			if (!alias)
> > > +				condlog(2, "%s: mapname not found for
> > > %d:%d",
> > > +					param, major, minor);
> > >  		}
> > >  		/*if there is no mapname found, we first create the
> > > device*/
> > >  		if (!alias && !count) {
> > > -			condlog(2, "%s: mapname not found for %d:%d",
> > > -				param, major, minor);
> > >  			get_refwwid(CMD_NONE, param, DEV_DEVMAP,
> > >  				    vecs->pathvec, &refwwid);
> > >  			if (refwwid) {
> > 
> > 
> > .
> > 

Re: [PATCH 05/14] multipathd: use MALLOC and check return value in, cli_getprkey func

[Benjamin Marzinski]

On Thu, Sep 03, 2020 at 09:13:04PM +0200, Martin Wilck wrote:
> On Wed, 2020-09-02 at 15:18 +0800, lixiaokeng wrote:
> > In cli_getprkey func, we use MALLOC instead of malloc, and check
> > the return value of MALLOC.
> > 
> > Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> > Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
> > Signed-off-by: Linfeilong <linfeilong@huawei.com>
> > ---
> >  multipathd/cli_handlers.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> > index 27e4574f..d345afd3 100644
> > --- a/multipathd/cli_handlers.c
> > +++ b/multipathd/cli_handlers.c
> > @@ -1535,7 +1535,11 @@ cli_getprkey(void * v, char ** reply, int *
> > len, void * data)
> >  	if (!mpp)
> >  		return 1;
> > 
> > -	*reply = malloc(26);
> > +	*reply = MALLOC(26);
> > +	if (!*reply) {
> > +		condlog(0, "malloc *reply failed.");
> > +		return 1;
> > +	}
> 
> MALLOC is not necessary (*reply isn't left uninialized), nor is the
> error message.

What's you objection to the error message? Admittedly there is basically
no chance that malloc(26) would ever actually fail. But when things
fail, having error messages so that we can debug them faster is helpful.

If your objection is that malloc checks are mostly just there for good
form, and so those error messages won't actually help in practice, I
agree. But as a general rule, I think we should print error messages on
things that are unambiguoulsy errors.

-Ben

> 
> Regards,
> Martin
> 

Re: [PATCH 00/14] multipath-tools series: some cleanups and fixes checked by codedex tool

[Benjamin Marzinski]

On Thu, Sep 03, 2020 at 10:08:53PM +0200, Martin Wilck wrote:
> Hello Lixiaokeng,
> 
> On Wed, 2020-09-02 at 14:40 +0800, lixiaokeng wrote:
> > Hi:
> >     Now, we check multipath-tools codes with codedex tool. Here
> > are some some cleanups and fixes.
> 
> Thank you. However I'm going to nack all patches that add error
> messages after unsuccesful memory allocations. Such messages are
> unhelpful most of the time, and increase the code size without a true
> benefit. I've actually considered to get rid of all these, and replace
> them by a log_oom() macro.

O.k. This answers my question from patch 0005. I'm fine with this.

As a side note: man, those are some ugly preprocessor hoops you need to
jump through to stringify __LINE__.

-Ben

> 
> See an untested prototype attached, to better understand what I mean.
> 
> Regards
> Martin
> 
> 

> From fbbca2c5076a489ee4ae643d6d9199ca5085be95 Mon Sep 17 00:00:00 2001
> From: Martin Wilck <mwilck@suse.com>
> Date: Thu, 3 Sep 2020 22:03:22 +0200
> Subject: [PATCH] libmultipath: prototype implementation of log_oom()
> 
> Rationale: with VERBOSE_OOM_LOGGING, we log the part of the code
> where OOM occured, with minimal runtime effort (no string formatting).
> With lots of log_oom() invocations, our binary will increase by many
> static strings. Without VERBOSE_OOM_LOGGING, we just print a minimal
> error message, which will be enough most of the time.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/debug.c | 7 +++++++
>  libmultipath/debug.h | 8 ++++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/libmultipath/debug.c b/libmultipath/debug.c
> index 4128cb9..9062581 100644
> --- a/libmultipath/debug.c
> +++ b/libmultipath/debug.c
> @@ -48,3 +48,10 @@ void dlog (int sink, int prio, const char * fmt, ...)
>  	}
>  	va_end(ap);
>  }
> +
> +#ifndef VERBOSE_OOM_LOGGING
> +void log_oom(void)
> +{
> +	condlog(0, "Out of memory");
> +}
> +#endif
> diff --git a/libmultipath/debug.h b/libmultipath/debug.h
> index c6120c1..f61ecb6 100644
> --- a/libmultipath/debug.h
> +++ b/libmultipath/debug.h
> @@ -11,3 +11,11 @@ extern int logsink;
>  
>  #define condlog(prio, fmt, args...) \
>  	dlog(logsink, prio, fmt "\n", ##args)
> +
> +#ifdef VERBOSE_OOM_LOGGING
> +#define __log_oom(file, line) condlog(0, "Out of memory in " file ":" #line)
> +#define _log_oom(file, line) __log_oom(file, line)
> +#define log_oom() _log_oom(__FILE__, __LINE__)
> +#else
> +void log_oom(void);
> +#endif
> -- 
> 2.28.0
> 

Re: [PATCH 07/14] libmultipath: check return value of dm_mapname in, sysfs_check_holders

[Benjamin Marzinski]

On Wed, Sep 02, 2020 at 03:19:51PM +0800, lixiaokeng wrote:
> In sysfs_check_holders func, table_name is obtained by calling
> dm_mapname func, and then call dm_reassign_table for reassigning
> table. However, we donnot check whether dm_mapname func returns
> NULL, and then it may cause a segmentation fault in dm_task_set_name.
> 
> Here, we will check whether dm_mapname func returns NULL before
> using it.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> 
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
> ---
>  libmultipath/sysfs.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/libmultipath/sysfs.c b/libmultipath/sysfs.c
> index 12a82d95..5390de62 100644
> --- a/libmultipath/sysfs.c
> +++ b/libmultipath/sysfs.c
> @@ -278,7 +278,11 @@ int sysfs_check_holders(char * check_devt, char * new_devt)
>  			continue;
>  		}
>  		table_name = dm_mapname(major, table_minor);
> -
> +		if (!table_name) {
> +			condlog(2, "%s: mapname not found for %d:%d", check_dev,
> +				major, table_minor);
> +			continue;
> +		}
>  		condlog(0, "%s: reassign table %s old %s new %s", check_dev,
>  			table_name, check_devt, new_devt);
> 
> -- 

Re: [PATCH 08/14] libmultipath: donot free *dst if REALLOC fails in, merge_words

[Benjamin Marzinski]

On Wed, Sep 02, 2020 at 03:20:29PM +0800, lixiaokeng wrote:
> In merge_words func, if REALLOC() fails, the input *dst will
> be freed. If so, mpp->hwhandler| mpp->features|mpp->selector
> may be set to NULL after calling merge_words func in
> disassemble_map func. This may cause accessing freed memory
> problem.
> 

I'm not sure that this is the right way to fix the issue you're seeing.
If merge_words() frees mpp->hwhandler| mpp->features|mpp->selector, it
also sets them to NULL.  I don't see any place in disassemble_map()
where these would be accessed if merge_words() freed them.  Even with
this fix, there are still cases where disassemble_map() will return 1,
with these members set to NULL. If there is something that is
dereferencing them without checking if they're NULL, we should fix that.
But simply making them include partial information doesn't seem like it
fixes anything.

Am I missing something here?

-Ben

> Here, we donot free *dst if REALLOC() fails in merge_words func.
> 
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
> ---
>  libmultipath/dmparser.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
> index c1031616..482e9d0e 100644
> --- a/libmultipath/dmparser.c
> +++ b/libmultipath/dmparser.c
> @@ -26,13 +26,12 @@ merge_words(char **dst, const char *word)
> 
>  	dstlen = strlen(*dst);
>  	len = dstlen + strlen(word) + 2;
> -	*dst = REALLOC(*dst, len);
> +	p = REALLOC(*dst, len);
> 
> -	if (!*dst) {
> -		free(p);
> +	if (!p)
>  		return 1;
> -	}
> 
> +	*dst = p;
>  	p = *dst + dstlen;
>  	*p = ' ';
>  	++p;
> -- 

Re: [PATCH 05/14] multipathd: use MALLOC and check return value in, cli_getprkey func

[Martin Wilck]

On Fri, 2020-09-04 at 13:25 -0500, Benjamin Marzinski wrote:
> On Thu, Sep 03, 2020 at 09:13:04PM +0200, Martin Wilck wrote:
> > On Wed, 2020-09-02 at 15:18 +0800, lixiaokeng wrote:
> > > In cli_getprkey func, we use MALLOC instead of malloc, and check
> > > the return value of MALLOC.
> > > 
> > > Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> > > Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
> > > Signed-off-by: Linfeilong <linfeilong@huawei.com>
> > > ---
> > >  multipathd/cli_handlers.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/multipathd/cli_handlers.c
> > > b/multipathd/cli_handlers.c
> > > index 27e4574f..d345afd3 100644
> > > --- a/multipathd/cli_handlers.c
> > > +++ b/multipathd/cli_handlers.c
> > > @@ -1535,7 +1535,11 @@ cli_getprkey(void * v, char ** reply, int
> > > *
> > > len, void * data)
> > >  	if (!mpp)
> > >  		return 1;
> > > 
> > > -	*reply = malloc(26);
> > > +	*reply = MALLOC(26);
> > > +	if (!*reply) {
> > > +		condlog(0, "malloc *reply failed.");
> > > +		return 1;
> > > +	}
> > 
> > MALLOC is not necessary (*reply isn't left uninialized), nor is the
> > error message.
> 
> What's you objection to the error message? Admittedly there is
> basically
> no chance that malloc(26) would ever actually fail. But when things
> fail, having error messages so that we can debug them faster is
> helpful.
> 
> If your objection is that malloc checks are mostly just there for
> good
> form, and so those error messages won't actually help in practice, I
> agree. But as a general rule, I think we should print error messages
> on
> things that are unambiguoulsy errors.

See my reply to 00/14. I'd like to standardize and streamline "out of
memory" error messages, rather than hand-coding them in every
procedure. I think that in 99% of cases, if multipathd crashes or
errors out due to OOM, the information in which function OOM occured
first will not be helpful. Even if we had a major memory leak, its
unlikely that such error messages would help us find it.

Do you disagree?

Martin

Re: [PATCH 00/14] multipath-tools series: some cleanups and fixes checked by codedex tool

[Martin Wilck]

On Fri, 2020-09-04 at 15:00 -0500, Benjamin Marzinski wrote:
> On Thu, Sep 03, 2020 at 10:08:53PM +0200, Martin Wilck wrote:
> > Hello Lixiaokeng,
> > 
> > On Wed, 2020-09-02 at 14:40 +0800, lixiaokeng wrote:
> > > Hi:
> > >     Now, we check multipath-tools codes with codedex tool. Here
> > > are some some cleanups and fixes.
> > 
> > Thank you. However I'm going to nack all patches that add error
> > messages after unsuccesful memory allocations. Such messages are
> > unhelpful most of the time, and increase the code size without a
> > true
> > benefit. I've actually considered to get rid of all these, and
> > replace
> > them by a log_oom() macro.
> 
> O.k. This answers my question from patch 0005. I'm fine with this.

Great, thanks.

> As a side note: man, those are some ugly preprocessor hoops you need
> to
> jump through to stringify __LINE__.

Yeah, it took me some attempts to get it right. 
It's actually documented:
https://gcc.gnu.org/onlinedocs/gcc-4.8.5/cpp/Stringification.html

Thanks,
Martin

Re: [PATCH 09/14] libmultipath: check whether mp->features is NULl in, assemble_map

[Benjamin Marzinski]

On Wed, Sep 02, 2020 at 03:21:15PM +0800, lixiaokeng wrote:
> In assemble_map func, if add_feature fails and mp->features is
> default value (NULL), STRDUP(mp->features) will cause a seg-fault.
> In addition, f = STRDUP(mp->features) is just used for APPEND().
> We can directly pass mp->features to APPEND().
> 
> Here, we firstly check whether mp->features is null.
> 
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
> ---
>  libmultipath/dmparser.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
> index 482e9d0e..12182bf5 100644
> --- a/libmultipath/dmparser.c
> +++ b/libmultipath/dmparser.c
> @@ -65,7 +65,7 @@ assemble_map (struct multipath * mp, char * params, int len)
>  	int i, j;
>  	int minio;
>  	int nr_priority_groups, initial_pg_nr;
> -	char * p, * f;
> +	char * p;
>  	const char *const end = params + len;
>  	char no_path_retry[] = "queue_if_no_path";
>  	char retain_hwhandler[] = "retain_attached_hw_handler";
> @@ -86,9 +86,12 @@ assemble_map (struct multipath * mp, char * params, int len)
>  	    get_linux_version_code() < KERNEL_VERSION(4, 3, 0))
>  		add_feature(&mp->features, retain_hwhandler);
> 
> -	f = STRDUP(mp->features);

clearly strdup()ing without checking if mp->features NULL is incorrect.
However, I'm not sure that we need to fail if mp->features is NULL.
instead, int the APPEND call, we could use the gcc ternary operator
extension

(mp->features)?: "0"

to use "0" if mp->features is NULL.

Also, have you seen this actually occur?  Or is this just a theoretical
issue that you've found from reading the code.  It seems like
setup_map() will always call select_features() before calling
assemble_map(), which should mean that mp->features will always be set
in this case. Perhaps I'm missing something here.

-Ben

> +	if (!mp->features) {
> +		condlog(0, "mp->features is still NULL.");
> +		goto err;
> +	}
> 
> -	APPEND(p, end, "%s %s %i %i", f, mp->hwhandler, nr_priority_groups,
> +	APPEND(p, end, "%s %s %i %i", mp->features, mp->hwhandler, nr_priority_groups,
>  	       initial_pg_nr);
> 
>  	vector_foreach_slot (mp->pg, pgp, i) {
> @@ -110,12 +113,10 @@ assemble_map (struct multipath * mp, char * params, int len)
>  		}
>  	}
> 
> -	FREE(f);
>  	condlog(4, "%s: assembled map [%s]", mp->alias, params);
>  	return 0;
> 
>  err:
> -	FREE(f);
>  	return 1;
>  }
> 
> -- 

Re: [PATCH 10/14] util/tests: use assert_non_null to ensure malloc, returns non-null pointer

[Benjamin Marzinski]

On Wed, Sep 02, 2020 at 03:22:37PM +0800, lixiaokeng wrote:
> In tests/util.c, we should use assert_non_null to ensure
> malloc() returns non-null pointer in both test_strlcpy_5
> and test_strlcpy_6 func.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
> Signed-off-by: Linfeilong <linfeilong@huawei.com>
> ---
>  tests/util.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tests/util.c b/tests/util.c
> index 16774dff..455eeee5 100644
> --- a/tests/util.c
> +++ b/tests/util.c
> @@ -523,6 +523,7 @@ static void test_strlcpy_5(void **state)
>  	const int sz = sizeof(src_str);
> 
>  	tst = malloc(sz);
> +	assert_non_null(tst);
>  	memset(tst, 'f', sizeof(src_str));
> 
>  	rc = strlcpy(tst, src_str, sz);
> @@ -540,6 +541,7 @@ static void test_strlcpy_6(void **state)
>  	const int sz = sizeof(src_str);
> 
>  	tst = malloc(sz + 2);
> +	assert_non_null(tst);
>  	memset(tst, 'f', sz + 2);
> 
>  	rc = strlcpy(tst, src_str, sz + 2);
> -- 

Re: [PATCH 11/14] mpathpersist: check whether malloc paramp->trnptid_list, fails in handle_args func

[Benjamin Marzinski]

On Wed, Sep 02, 2020 at 03:23:42PM +0800, lixiaokeng wrote:
> In handle_args func, we donot check whether malloc paramp and
> each paramp->trnptid_list[j] fails before using them, it may
> cause access NULL pointer.
> 
> Here, we add alloc_prout_param_descriptor to allocate and init
> paramp, and we add free_prout_param_descriptor to free paramp
> and each paramp->trnptid_list[j].

This looks mostly fine to me. I have some minor nitpicks. But they
don't actually effect the correctness of the patch.

-Ben

> 
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
> Signed-off-by: Linfeilong <linfeilong@huawei.com>
> ---
>  mpathpersist/main.c | 55 +++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 46 insertions(+), 9 deletions(-)
> 
> diff --git a/mpathpersist/main.c b/mpathpersist/main.c
> index 28bfe410..f20c902c 100644
> --- a/mpathpersist/main.c
> +++ b/mpathpersist/main.c
> @@ -153,6 +153,39 @@ static int do_batch_file(const char *batch_fn)
>  	return ret;
>  }
> 
> +static struct prout_param_descriptor *
> +alloc_prout_param_descriptor(int num_transportid)
> +{
> +	struct prout_param_descriptor *paramp;
> +
> +	if (num_transportid < 0 || num_transportid > MPATH_MX_TIDS)
> +		return NULL;
> +
> +	paramp= malloc(sizeof(struct prout_param_descriptor) +
> +				(sizeof(struct transportid *) * num_transportid));
> +
> +	if (!paramp)
> +		return NULL;
> +
> +	paramp->num_transportid = num_transportid;
> +	memset(paramp, 0, sizeof(struct prout_param_descriptor) +
> +			(sizeof(struct transportid *) * num_transportid));
> +	return paramp;
> +}
> +
> +static void free_prout_param_descriptor(struct prout_param_descriptor *paramp)
> +{
> +	int i;
> +	if (!paramp)
> +		return;
> +
> +	for (i = 0; i < paramp->num_transportid; i++)
> +		free(paramp->trnptid_list[i]);
> +
> +	free(paramp);

Setting paramp to NULL here doesn't actually do anything.

> +	paramp = NULL;
> +}
> +
>  static int handle_args(int argc, char * argv[], int nline)
>  {
>  	int c;
> @@ -525,9 +558,12 @@ static int handle_args(int argc, char * argv[], int nline)
>  		int j;
>  		struct prout_param_descriptor *paramp;
> 
> -		paramp= malloc(sizeof(struct prout_param_descriptor) + (sizeof(struct transportid *)*(MPATH_MX_TIDS )));
> -
> -		memset(paramp, 0, sizeof(struct prout_param_descriptor) + (sizeof(struct transportid *)*(MPATH_MX_TIDS)));

When looking at your patch, I noticed that this function has both
num_transport and num_transportids, but only num_transport is used,
while num_transportids is reported back to the user.  I realize that
this is only tangentially related to your patch, but if you could
combine these two varaiables, that would be helpful.

> +		paramp = alloc_prout_param_descriptor(num_transport);
> +		if (!paramp) {
> +			fprintf(stderr, "malloc paramp failed\n");
> +			ret = MPATH_PR_OTHER;
> +			goto out_fd;
> +		}
> 
>  		for (j = 7; j >= 0; --j) {
>  			paramp->key[j] = (param_rk & 0xff);
> @@ -551,6 +587,12 @@ static int handle_args(int argc, char * argv[], int nline)
>  			for (j = 0 ; j < num_transport; j++)
>  			{
>  				paramp->trnptid_list[j] = (struct transportid *)malloc(sizeof(struct transportid));
> +				if (!paramp->trnptid_list[j]) {
> +					fprintf(stderr, "malloc paramp->trnptid_list[%d] failed.\n", j);
> +					ret = MPATH_PR_OTHER;
> +					free_prout_param_descriptor(paramp);
> +					goto out_fd;
> +				}
>  				memcpy(paramp->trnptid_list[j], &transportids[j],sizeof(struct transportid));
>  			}
>  		}
> @@ -558,12 +600,7 @@ static int handle_args(int argc, char * argv[], int nline)
>  		/* PROUT commands other than 'register and move' */
>  		ret = __mpath_persistent_reserve_out (fd, prout_sa, 0, prout_type,
>  				paramp, noisy);
> -		for (j = 0 ; j < num_transport; j++)
> -		{
> -			tmp = paramp->trnptid_list[j];
> -			free(tmp);
> -		}
> -		free(paramp);
> +		free_prout_param_descriptor(paramp);
>  	}
> 
>  	if (ret != MPATH_PR_SUCCESS)
> -- 

Re: [PATCH 12/14] libmultipathpersist: use update_multipath_table/status, in get_mpvec

[Benjamin Marzinski]

On Wed, Sep 02, 2020 at 03:24:33PM +0800, lixiaokeng wrote:
> The return values of dm_get_map, disassemble_map in get_mpvec
> were not checked. Use update_multipath_table/status to instead
> of them.
> 

Looks mostly good. I agree that we should be checking the results of
getting the raw data before we try to disassemble it. But, there's not
really any point to calling continue as the last operation of a loop.
Perhaps

if (update_multipath_table(mpp, pathvec, DI_CHECKER) == DMP_OK)
	update_multipath_status(mpp);

makes more sense.

> Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: Linfeilong <linfeilong@huawei.com>
> ---
>  libmpathpersist/mpath_persist.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c
> index e7256049..046fda21 100644
> --- a/libmpathpersist/mpath_persist.c
> +++ b/libmpathpersist/mpath_persist.c
> @@ -323,7 +323,6 @@ get_mpvec (vector curmp, vector pathvec, char * refwwid)
>  {
>  	int i;
>  	struct multipath *mpp;
> -	char params[PARAMS_SIZE], status[PARAMS_SIZE];
> 
>  	vector_foreach_slot (curmp, mpp, i){
>  		/*
> @@ -341,13 +340,9 @@ get_mpvec (vector curmp, vector pathvec, char * refwwid)
>  		if (refwwid && strncmp (mpp->alias, refwwid, WWID_SIZE - 1))
>  			continue;
> 
> -		dm_get_map(mpp->alias, &mpp->size, params);
> -		condlog(3, "params = %s", params);
> -		dm_get_status(mpp->alias, status);
> -		condlog(3, "status = %s", status);
> -		disassemble_map (pathvec, params, mpp);
> -		update_pathvec_from_dm(pathvec, mpp, DI_CHECKER);
> -		disassemble_status (status, mpp);
> +		if (update_multipath_table(mpp, pathvec, DI_CHECKER) != DMP_OK ||
> +		    update_multipath_status(mpp) != DMP_OK)
> +			continue;
> 
>  	}
>  	return MPATH_PR_SUCCESS ;
> -- 

Re: [PATCH 13/14] multipath: use update_multipath_table/status in, check_useable_paths

[Benjamin Marzinski]

On Wed, Sep 02, 2020 at 03:25:28PM +0800, lixiaokeng wrote:
> The return values of dm_get_map, disassemble_map,dm_get_status
> and disassemble_status in check_usable_paths were not checked.
> Use update_multipath_table/status to instead of them.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: Linfeilong <linfeilong@huawei.com>
> ---
>  multipath/main.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/multipath/main.c b/multipath/main.c
> index d227e0b3..9e920d89 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -251,7 +251,6 @@ static int check_usable_paths(struct config *conf,
>  	struct path *pp;
>  	char *mapname;
>  	vector pathvec = NULL;
> -	char params[PARAMS_SIZE], status[PARAMS_SIZE];
>  	dev_t devt;
>  	int r = 1, i, j;
> 
> @@ -285,11 +284,9 @@ static int check_usable_paths(struct config *conf,
>  	if (mpp == NULL)
>  		goto free;
> 
> -	dm_get_map(mpp->alias, &mpp->size, params);
> -	dm_get_status(mpp->alias, status);
> -	disassemble_map(pathvec, params, mpp);
> -	update_pathvec_from_dm(pathvec, mpp, 0);
> -	disassemble_status(status, mpp);
> +	if (update_multipath_table(mpp, pathvec, 0) != DMP_OK ||
> +		    update_multipath_status(mpp) != DMP_OK)
> +		    goto free;
> 
>  	vector_foreach_slot (mpp->pg, pg, i) {
>  		vector_foreach_slot (pg->paths, pp, j) {
> -- 

Re: [PATCH 14/14] multipathpersist: delete unused variable in handle_args

[Benjamin Marzinski]

On Wed, Sep 02, 2020 at 03:26:29PM +0800, lixiaokeng wrote:
> In handle_args, the tmp isn't used. We delete it.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: Linfeilong <linfeilong@huawei.com>
> ---
>  mpathpersist/main.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/mpathpersist/main.c b/mpathpersist/main.c
> index f20c902c..ccf0024e 100644
> --- a/mpathpersist/main.c
> +++ b/mpathpersist/main.c
> @@ -213,7 +213,6 @@ static int handle_args(int argc, char * argv[], int nline)
>  	int num_transport =0;
>  	char *batch_fn = NULL;
>  	void *resp = NULL;
> -	struct transportid * tmp;
> 
>  	memset(transportids, 0, MPATH_MX_TIDS * sizeof(struct transportid));
> 
> -- 

Re: [PATCH 08/14] libmultipath: donot free *dst if REALLOC fails in, merge_words

[lixiaokeng]

Hi Ben:
  I think you are right. We should not keep them with partial information.
Thanks for your review.

-Lixiaokeng

On 2020/9/5 5:11, Benjamin Marzinski wrote:
> On Wed, Sep 02, 2020 at 03:20:29PM +0800, lixiaokeng wrote:
>> In merge_words func, if REALLOC() fails, the input *dst will
>> be freed. If so, mpp->hwhandler| mpp->features|mpp->selector
>> may be set to NULL after calling merge_words func in
>> disassemble_map func. This may cause accessing freed memory
>> problem.
>>
> 
> I'm not sure that this is the right way to fix the issue you're seeing.
> If merge_words() frees mpp->hwhandler| mpp->features|mpp->selector, it
> also sets them to NULL.  I don't see any place in disassemble_map()
> where these would be accessed if merge_words() freed them.  Even with
> this fix, there are still cases where disassemble_map() will return 1,
> with these members set to NULL. If there is something that is
> dereferencing them without checking if they're NULL, we should fix that.
> But simply making them include partial information doesn't seem like it
> fixes anything.
> 
> Am I missing something here?
> 
> -Ben
> 
>> Here, we donot free *dst if REALLOC() fails in merge_words func.
>>
>> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
>> Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
>> ---
>>  libmultipath/dmparser.c | 7 +++----
>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
>> index c1031616..482e9d0e 100644
>> --- a/libmultipath/dmparser.c
>> +++ b/libmultipath/dmparser.c
>> @@ -26,13 +26,12 @@ merge_words(char **dst, const char *word)
>>
>>  	dstlen = strlen(*dst);
>>  	len = dstlen + strlen(word) + 2;
>> -	*dst = REALLOC(*dst, len);
>> +	p = REALLOC(*dst, len);
>>
>> -	if (!*dst) {
>> -		free(p);
>> +	if (!p)
>>  		return 1;
>> -	}
>>
>> +	*dst = p;
>>  	p = *dst + dstlen;
>>  	*p = ' ';
>>  	++p;
>> -- 
> 
> 
> .
> 

Re: [PATCH 09/14] libmultipath: check whether mp->features is NULl in, assemble_map

[lixiaokeng]

>> @@ -86,9 +86,12 @@ assemble_map (struct multipath * mp, char * params, int len)
>>  	    get_linux_version_code() < KERNEL_VERSION(4, 3, 0))
>>  		add_feature(&mp->features, retain_hwhandler);
>>
>> -	f = STRDUP(mp->features);
> 
> clearly strdup()ing without checking if mp->features NULL is incorrect.
> However, I'm not sure that we need to fail if mp->features is NULL.
> instead, int the APPEND call, we could use the gcc ternary operator
> extension
> 
> (mp->features)?: "0"
> 
> to use "0" if mp->features is NULL.
> 
> Also, have you seen this actually occur?  Or is this just a theoretical
> issue that you've found from reading the code.  It seems like
> setup_map() will always call select_features() before calling
> assemble_map(), which should mean that mp->features will always be set
> in this case. Perhaps I'm missing something here.
> 
> -Ben
> 
Hi Ben,
  This just a theoretical issue and I did not see it. But it's not necessary
to call strdup. In your opinion, need multipath be checked?  I will make new
patch with your suggestion.

-Lixiaokeng
>> +	if (!mp->features) {
>> +		condlog(0, "mp->features is still NULL.");
>> +		goto err;
>> +	}

Re: [PATCH 11/14] mpathpersist: check whether malloc paramp->trnptid_list, fails in handle_args func

[lixiaokeng]

>>
>> -		paramp= malloc(sizeof(struct prout_param_descriptor) + (sizeof(struct transportid *)*(MPATH_MX_TIDS )));
>> -
>> -		memset(paramp, 0, sizeof(struct prout_param_descriptor) + (sizeof(struct transportid *)*(MPATH_MX_TIDS)));
> 
> When looking at your patch, I noticed that this function has both
> num_transport and num_transportids, but only num_transport is used,
> while num_transportids is reported back to the user.  I realize that
> this is only tangentially related to your patch, but if you could
> combine these two varaiables, that would be helpful.
> 

Hi Ben:
  Thanks for you great advice. I will make patch v2 following your
advice.

-Lixiaokeng

Re: [PATCH 12/14] libmultipathpersist: use update_multipath_table/status, in get_mpvec

[lixiaokeng]

> Looks mostly good. I agree that we should be checking the results of
> getting the raw data before we try to disassemble it. But, there's not
> really any point to calling continue as the last operation of a loop.
> Perhaps
> 
> if (update_multipath_table(mpp, pathvec, DI_CHECKER) == DMP_OK)
> 	update_multipath_status(mpp);
> 
> makes more sense.
> 
Hi Ben，
  Thanks for your review. I will change it in patch v2.

-Lixiaokeng


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 12/14] libmultipathpersist: use update_multipath_table/status, in get_mpvec

[Martin Wilck]

On Fri, 2020-09-04 at 19:05 -0500, Benjamin Marzinski wrote:
> On Wed, Sep 02, 2020 at 03:24:33PM +0800, lixiaokeng wrote:
> > The return values of dm_get_map, disassemble_map in get_mpvec
> > were not checked. Use update_multipath_table/status to instead
> > of them.
> > 
> 
> Looks mostly good. I agree that we should be checking the results of
> getting the raw data before we try to disassemble it. But, there's
> not
> really any point to calling continue as the last operation of a loop.
> Perhaps
> 
> if (update_multipath_table(mpp, pathvec, DI_CHECKER) == DMP_OK)
> 	update_multipath_status(mpp);
> 
> makes more sense.

I was thinking about this before, and wondered whether we should call
remove_map() here if we encounter an error. Looking at it again and
comparing with get_dm_mpvec(), I think we should.

Regards
Martin

Re: [PATCH 09/14] libmultipath: check whether mp->features is NULl in, assemble_map

[Benjamin Marzinski]

On Mon, Sep 07, 2020 at 08:21:28PM +0800, lixiaokeng wrote:
> 
> >> @@ -86,9 +86,12 @@ assemble_map (struct multipath * mp, char * params, int len)
> >>  	    get_linux_version_code() < KERNEL_VERSION(4, 3, 0))
> >>  		add_feature(&mp->features, retain_hwhandler);
> >>
> >> -	f = STRDUP(mp->features);
> > 
> > clearly strdup()ing without checking if mp->features NULL is incorrect.
> > However, I'm not sure that we need to fail if mp->features is NULL.
> > instead, int the APPEND call, we could use the gcc ternary operator
> > extension
> > 
> > (mp->features)?: "0"
> > 
> > to use "0" if mp->features is NULL.
> > 
> > Also, have you seen this actually occur?  Or is this just a theoretical
> > issue that you've found from reading the code.  It seems like
> > setup_map() will always call select_features() before calling
> > assemble_map(), which should mean that mp->features will always be set
> > in this case. Perhaps I'm missing something here.
> > 
> > -Ben
> > 
> Hi Ben,
>   This just a theoretical issue and I did not see it. But it's not necessary
> to call strdup. In your opinion, need multipath be checked?  I will make new
> patch with your suggestion.

Since we don't believe it's possible for mp->features (or mp->hwhandler)
to be set to NULL here, it makes sense to print an error if it is NULL.
So, I guess my suggestion would be to print an error message if
mp->features or mp->hwhandler are NULL, but to assemble the map anyway,
using the default value of "0" if they are NULL. That's how
assemble_map() currently handles failures in add_feature().
add_feature() will print an error, but assemble_map() will go ahead with
assembling the map.

I'm willing to be convinced that there is a better solution, however.

-Ben
 
> -Lixiaokeng
> >> +	if (!mp->features) {
> >> +		condlog(0, "mp->features is still NULL.");
> >> +		goto err;
> >> +	}
> 
> 

Re: [PATCH 09/14] libmultipath: check whether mp->features is NULl in, assemble_map

[Martin Wilck]

On Tue, 2020-09-08 at 10:45 -0500, Benjamin Marzinski wrote:
> On Mon, Sep 07, 2020 at 08:21:28PM +0800, lixiaokeng wrote:
> > > > @@ -86,9 +86,12 @@ assemble_map (struct multipath * mp, char *
> > > > params, int len)
> > > >  	    get_linux_version_code() < KERNEL_VERSION(4, 3, 0))
> > > >  		add_feature(&mp->features, retain_hwhandler);
> > > > 
> > > > -	f = STRDUP(mp->features);
> > > 
> > > clearly strdup()ing without checking if mp->features NULL is
> > > incorrect.
> > > However, I'm not sure that we need to fail if mp->features is
> > > NULL.
> > > instead, int the APPEND call, we could use the gcc ternary
> > > operator
> > > extension
> > > 
> > > (mp->features)?: "0"
> > > 
> > > to use "0" if mp->features is NULL.
> > > 
> > > Also, have you seen this actually occur?  Or is this just a
> > > theoretical
> > > issue that you've found from reading the code.  It seems like
> > > setup_map() will always call select_features() before calling
> > > assemble_map(), which should mean that mp->features will always
> > > be set
> > > in this case. Perhaps I'm missing something here.
> > > 
> > > -Ben
> > > 
> > Hi Ben,
> >   This just a theoretical issue and I did not see it. But it's not
> > necessary
> > to call strdup. In your opinion, need multipath be checked?  I will
> > make new
> > patch with your suggestion.
> 
> Since we don't believe it's possible for mp->features (or mp-
> >hwhandler)
> to be set to NULL here, it makes sense to print an error if it is
> NULL.
> So, I guess my suggestion would be to print an error message if
> mp->features or mp->hwhandler are NULL, but to assemble the map
> anyway,
> using the default value of "0" if they are NULL. That's how
> assemble_map() currently handles failures in add_feature().
> add_feature() will print an error, but assemble_map() will go ahead
> with
> assembling the map.
> 
> I'm willing to be convinced that there is a better solution, however.

What about this:

assemble_map() is only called from setup_map(), which sets mp->features 
in select_features(). So what we should do is check for NULL after
select_features(), or have that function return an error code if strdup
fails, and bail out early in setup_map() in that case.

Then we simply need to add a comment in assemble_map() saying that 
mp->features must be non-null when this function is called.

As I said earlier, I'm of course not against checking function
parameters, but here we should fail to setup a "struct multipath" in
the first place in setup map(), rather than returning an incompletely
initialized one. If we handle it this way, we don't need to check the
fields of struct multipath over and over again. Similar arguments hold
for other structs.

Of course this kind of assumption needs to be better documented in the
code.

Regards
Martin

Re: [PATCH 09/14] libmultipath: check whether mp->features is NULl in, assemble_map

[Benjamin Marzinski]

On Tue, Sep 08, 2020 at 06:35:45PM +0200, Martin Wilck wrote:
> On Tue, 2020-09-08 at 10:45 -0500, Benjamin Marzinski wrote:
> > On Mon, Sep 07, 2020 at 08:21:28PM +0800, lixiaokeng wrote:
> > > > > @@ -86,9 +86,12 @@ assemble_map (struct multipath * mp, char *
> > > > > params, int len)
> > > > >  	    get_linux_version_code() < KERNEL_VERSION(4, 3, 0))
> > > > >  		add_feature(&mp->features, retain_hwhandler);
> > > > > 
> > > > > -	f = STRDUP(mp->features);
> > > > 
> > > > clearly strdup()ing without checking if mp->features NULL is
> > > > incorrect.
> > > > However, I'm not sure that we need to fail if mp->features is
> > > > NULL.
> > > > instead, int the APPEND call, we could use the gcc ternary
> > > > operator
> > > > extension
> > > > 
> > > > (mp->features)?: "0"
> > > > 
> > > > to use "0" if mp->features is NULL.
> > > > 
> > > > Also, have you seen this actually occur?  Or is this just a
> > > > theoretical
> > > > issue that you've found from reading the code.  It seems like
> > > > setup_map() will always call select_features() before calling
> > > > assemble_map(), which should mean that mp->features will always
> > > > be set
> > > > in this case. Perhaps I'm missing something here.
> > > > 
> > > > -Ben
> > > > 
> > > Hi Ben,
> > >   This just a theoretical issue and I did not see it. But it's not
> > > necessary
> > > to call strdup. In your opinion, need multipath be checked?  I will
> > > make new
> > > patch with your suggestion.
> > 
> > Since we don't believe it's possible for mp->features (or mp-
> > >hwhandler)
> > to be set to NULL here, it makes sense to print an error if it is
> > NULL.
> > So, I guess my suggestion would be to print an error message if
> > mp->features or mp->hwhandler are NULL, but to assemble the map
> > anyway,
> > using the default value of "0" if they are NULL. That's how
> > assemble_map() currently handles failures in add_feature().
> > add_feature() will print an error, but assemble_map() will go ahead
> > with
> > assembling the map.
> > 
> > I'm willing to be convinced that there is a better solution, however.
> 
> What about this:
> 
> assemble_map() is only called from setup_map(), which sets mp->features 
> in select_features(). So what we should do is check for NULL after
> select_features(), or have that function return an error code if strdup
> fails, and bail out early in setup_map() in that case.
> 
> Then we simply need to add a comment in assemble_map() saying that 
> mp->features must be non-null when this function is called.
> 
> As I said earlier, I'm of course not against checking function
> parameters, but here we should fail to setup a "struct multipath" in
> the first place in setup map(), rather than returning an incompletely
> initialized one. If we handle it this way, we don't need to check the
> fields of struct multipath over and over again. Similar arguments hold
> for other structs.
> 
> Of course this kind of assumption needs to be better documented in the
> code.

I'm fine with that.

-Ben

> Regards
> Martin
> 

Re: [PATCH 09/14] libmultipath: check whether mp->features is NULl in, assemble_map

[lixiaokeng]

On 2020/9/9 0:35, Martin Wilck wrote:
> On Tue, 2020-09-08 at 10:45 -0500, Benjamin Marzinski wrote:
>> On Mon, Sep 07, 2020 at 08:21:28PM +0800, lixiaokeng wrote:
>>>>> @@ -86,9 +86,12 @@ assemble_map (struct multipath * mp, char *
>>>>> params, int len)
>>>>>  	    get_linux_version_code() < KERNEL_VERSION(4, 3, 0))
>>>>>  		add_feature(&mp->features, retain_hwhandler);
>>>>>
>>>>> -	f = STRDUP(mp->features);
>>>>
>>>> clearly strdup()ing without checking if mp->features NULL is
>>>> incorrect.
>>>> However, I'm not sure that we need to fail if mp->features is
>>>> NULL.
>>>> instead, int the APPEND call, we could use the gcc ternary
>>>> operator
>>>> extension
>>>>
>>>> (mp->features)?: "0"
>>>>
>>>> to use "0" if mp->features is NULL.
>>>>
>>>> Also, have you seen this actually occur?  Or is this just a
>>>> theoretical
>>>> issue that you've found from reading the code.  It seems like
>>>> setup_map() will always call select_features() before calling
>>>> assemble_map(), which should mean that mp->features will always
>>>> be set
>>>> in this case. Perhaps I'm missing something here.
>>>>
>>>> -Ben
>>>>
>>> Hi Ben,
>>>   This just a theoretical issue and I did not see it. But it's not
>>> necessary
>>> to call strdup. In your opinion, need multipath be checked?  I will
>>> make new
>>> patch with your suggestion.
>>
>> Since we don't believe it's possible for mp->features (or mp-
>>> hwhandler)
>> to be set to NULL here, it makes sense to print an error if it is
>> NULL.
>> So, I guess my suggestion would be to print an error message if
>> mp->features or mp->hwhandler are NULL, but to assemble the map
>> anyway,
>> using the default value of "0" if they are NULL. That's how
>> assemble_map() currently handles failures in add_feature().
>> add_feature() will print an error, but assemble_map() will go ahead
>> with
>> assembling the map.
>>
>> I'm willing to be convinced that there is a better solution, however.
> 
> What about this:
> 
> assemble_map() is only called from setup_map(), which sets mp->features 
> in select_features(). So what we should do is check for NULL after
> select_features(), or have that function return an error code if strdup
> fails, and bail out early in setup_map() in that case.
> 
> Then we simply need to add a comment in assemble_map() saying that 
> mp->features must be non-null when this function is called.
> 
> As I said earlier, I'm of course not against checking function
> parameters, but here we should fail to setup a "struct multipath" in
> the first place in setup map(), rather than returning an incompletely
> initialized one. If we handle it this way, we don't need to check the
> fields of struct multipath over and over again. Similar arguments hold
> for other structs.
> 
> Of course this kind of assumption needs to be better documented in the
> code.
> 
> Regards
> Martin
> 
> 
Hi Martin,
   If I don't misunderstand, we check feature after select_features and
return 1 if feature is NULL in setup_map. We delete strdup and add
message "mp->features must be non-null" in assemble_map. Like this:
---
select_features(conf, mpp);
if (!mpp->featrues)
	return 1;

---
/* mp->feature must not be NULL */
APPEND(p, end, "%s %s %i %i", mp->features, mp->hwhandler
---

Regards
Lixiaokeng

Re: [PATCH 09/14] libmultipath: check whether mp->features is NULl in, assemble_map

[Martin Wilck]

On Wed, 2020-09-09 at 11:18 +0800, lixiaokeng wrote:
> 
> On 2020/9/9 0:35, Martin Wilck wrote:
> > 
> > What about this:
> > 
> > assemble_map() is only called from setup_map(), which sets mp-
> > >features 
> > in select_features(). So what we should do is check for NULL after
> > select_features(), or have that function return an error code if
> > strdup
> > fails, and bail out early in setup_map() in that case.
> > 
> > Then we simply need to add a comment in assemble_map() saying that 
> > mp->features must be non-null when this function is called.
> > 
> > As I said earlier, I'm of course not against checking function
> > parameters, but here we should fail to setup a "struct multipath"
> > in
> > the first place in setup map(), rather than returning an
> > incompletely
> > initialized one. If we handle it this way, we don't need to check
> > the
> > fields of struct multipath over and over again. Similar arguments
> > hold
> > for other structs.
> > 
> > Of course this kind of assumption needs to be better documented in
> > the
> > code.
> > 
> > Regards
> > Martin
> > 
> > 
> Hi Martin,
>    If I don't misunderstand, we check feature after select_features
> and
> return 1 if feature is NULL in setup_map. We delete strdup and add
> message "mp->features must be non-null" in assemble_map. Like this:
> ---
> select_features(conf, mpp);
> if (!mpp->featrues)
> 	return 1;
> 
> ---
> /* mp->feature must not be NULL */
> APPEND(p, end, "%s %s %i %i", mp->features, mp->hwhandler
> ---

Yes. I guess error handling in setup_map() needs a closer look, too.

Martin