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

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

[lixiaokeng]

Patches 01, 02, 04, 05, 06, 09, 11, 12, 14 have some changes

Patches 01, 02, 04, 05, 11, 12, 14: change commit message
Patches 06: move xmalloc before main
Patches 09: check strdup result in setup_map, add message
"mp->features must not be NULL" in assemble_map

Zhiqiang Liu (7):
  multipathd: check return value of malloc in cli_getprkey func
  kpartx: use xmalloc to instead 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 mpp->features is NUll in setup_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: initialize major and minor in cli_add_map
  libmultipath: change malloc to calloc 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                 | 36 +++++++++---------
 libmpathpersist/mpath_persist.c | 15 +++-----
 libmultipath/configure.c        |  5 +++
 libmultipath/devmapper.c        |  2 +-
 libmultipath/dmparser.c         | 18 ++++-----
 libmultipath/foreign.c          |  4 +-
 libmultipath/sysfs.c            |  6 ++-
 mpathpersist/main.c             | 66 +++++++++++++++++++++++++--------
 multipath/main.c                |  9 ++---
 multipathd/cli_handlers.c       |  4 +-
 multipathd/main.c               |  8 ++--
 tests/util.c                    |  2 +
 12 files changed, 106 insertions(+), 69 deletions(-)

--

[PATCH V4 01/14] multipathd: initialize major and minor 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.

Here, we set major and minor to -1 at begining

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 8db37961..e9698704 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -820,7 +820,7 @@ cli_add_map (void * v, char ** reply, int * len, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 	char * param = get_keyparam(v, MAP);
-	int major, minor;
+	int major = -1, minor = -1;
 	char dev_path[PATH_SIZE];
 	char *refwwid, *alias = NULL;
 	int rc, count = 0;
-- 

[PATCH V4 02/14] libmultipath: change malloc to calloc in print_foreign_topology

[lixiaokeng]

We chanege malloc to calloc.

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, 2 insertions(+), 2 deletions(-)

diff --git a/libmultipath/foreign.c b/libmultipath/foreign.c
index e8f61351..fce19347 100644
--- a/libmultipath/foreign.c
+++ b/libmultipath/foreign.c
@@ -544,8 +544,8 @@ void print_foreign_topology(int verbosity)
 	int buflen = MAX_LINE_LEN * MAX_LINES;
 	char *buf = NULL, *tmp = NULL;

-	buf = malloc(buflen);
-	buf[0] = '\0';
+	buf = calloc(1, buflen);
+
 	while (buf != NULL) {
 		char *c = buf;

-- 

[PATCH V4 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. And we delete seting ret when jump to out

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, 4 insertions(+), 4 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 67e9af11..f1264459 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -3365,7 +3365,6 @@ void *  mpath_pr_event_handler_fn (void * pathp )
 	if (resp->prin_descriptor.prin_readkeys.additional_length == 0 )
 	{
 		condlog(1, "%s: No key found. Device may not be registered.", pp->dev);
-		ret = MPATH_PR_SUCCESS;
 		goto out;
 	}
 	condlog(2, "Multipath  reservation_key: 0x%" PRIx64 " ",
@@ -3387,12 +3386,13 @@ void *  mpath_pr_event_handler_fn (void * pathp )
 	{
 		condlog(0, "%s: Either device not registered or ", pp->dev);
 		condlog(0, "host is not authorised for registration. Skip path");
-		ret = MPATH_PR_OTHER;
 		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)
+		goto out;
+
 	param->sa_flags = mpp->sa_flags;
 	memcpy(param->sa_key, &mpp->reservation_key, 8);
 	param->num_transportid = 0;
-- 

[PATCH V4 05/14] multipathd: check return value of malloc in cli_getprkey func

[lixiaokeng]

In cli_getprkey func, we 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 | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index e9698704..235e2a2e 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -1531,6 +1531,8 @@ cli_getprkey(void * v, char ** reply, int * len, void * data)
 		return 1;

 	*reply = malloc(26);
+	if (!*reply)
+		return 1;

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

[PATCH V4 06/14] kpartx: use xmalloc to instead of malloc in main func

[lixiaokeng]

In main func of kpartx.c, we should check return value of
malloc before using it. So we use xmalloc to instead of
malloc.

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

diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
index 98f6176e..4a0aae93 100644
--- a/kpartx/kpartx.c
+++ b/kpartx/kpartx.c
@@ -209,6 +209,23 @@ check_uuid(char *uuid, char *part_uuid, char **err_msg) {
 	return 0;
 }

+static void *
+xmalloc (size_t size) {
+	void *t;
+
+	if (size == 0)
+		return NULL;
+
+	t = malloc (size);
+
+	if (t == NULL) {
+		fprintf(stderr, "Out of memory\n");
+		exit(1);
+	}
+
+	return t;
+}
+
 int
 main(int argc, char **argv){
 	int i, j, m, n, op, off, arg, c, d, ro=0;
@@ -383,7 +400,7 @@ main(int argc, char **argv){
 		mapname = device + off;

 	if (delim == NULL) {
-		delim = malloc(DELIM_SIZE);
+		delim = xmalloc(DELIM_SIZE);
 		memset(delim, 0, DELIM_SIZE);
 		set_delimiter(mapname, delim);
 	}
@@ -670,23 +687,6 @@ end:
 	return r;
 }

-void *
-xmalloc (size_t size) {
-	void *t;
-
-	if (size == 0)
-		return NULL;
-
-	t = malloc (size);
-
-	if (t == NULL) {
-		fprintf(stderr, "Out of memory\n");
-		exit(1);
-	}
-
-	return t;
-}
-
 /*
  * sseek: seek to specified sector
  */
-- 

[PATCH V4 09/14] libmultipath: check whether mpp->features is NUll in setup_map

[lixiaokeng]

In assemble_map func, f = STRDUP(mp->features) is just used
for APPEND(). We can directly pass mp->features to APPEND().
The mpp->features, hwhandler and selector got form strdup
should be check after select* function.

Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
---
 libmultipath/configure.c |  5 +++++
 libmultipath/dmparser.c  | 11 ++++-------
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 5bc65fd3..5d5d9415 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -355,6 +355,11 @@ int setup_map(struct multipath *mpp, char *params, int params_size,
 	select_ghost_delay(conf, mpp);
 	select_flush_on_last_del(conf, mpp);

+	if (!mpp->features || !mpp->hwhandler || !mpp->selector) {
+		condlog(0, "%s: map select failed", mpp->alias);
+		return 1;
+	}
+
 	sysfs_set_scsi_tmo(mpp, conf->checkint);
 	marginal_pathgroups = conf->marginal_pathgroups;
 	pthread_cleanup_pop(1);
diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
index 482e9d0e..685918da 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,10 +86,9 @@ 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);
-
-	APPEND(p, end, "%s %s %i %i", f, mp->hwhandler, nr_priority_groups,
-	       initial_pg_nr);
+	/* mp->features must not be NULL */
+	APPEND(p, end, "%s %s %i %i", mp->features, mp->hwhandler,
+		nr_priority_groups, initial_pg_nr);

 	vector_foreach_slot (mp->pg, pgp, i) {
 		pgp = VECTOR_SLOT(mp->pg, i);
@@ -110,12 +109,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 V4 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].

We change num_transport to num_transportids to combine them.

Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
---
 mpathpersist/main.c | 65 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 50 insertions(+), 15 deletions(-)

diff --git a/mpathpersist/main.c b/mpathpersist/main.c
index 28bfe410..da67c15c 100644
--- a/mpathpersist/main.c
+++ b/mpathpersist/main.c
@@ -153,6 +153,38 @@ 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);
+}
+
 static int handle_args(int argc, char * argv[], int nline)
 {
 	int c;
@@ -177,7 +209,6 @@ static int handle_args(int argc, char * argv[], int nline)
 	int prin = 1;
 	int prin_sa = -1;
 	int prout_sa = -1;
-	int num_transport =0;
 	char *batch_fn = NULL;
 	void *resp = NULL;
 	struct transportid * tmp;
@@ -334,13 +365,13 @@ static int handle_args(int argc, char * argv[], int nline)
 				break;

 			case 'X':
-				if (0 != construct_transportid(optarg, transportids, num_transport)) {
+				if (0 != construct_transportid(optarg, transportids, num_transportids)) {
 					fprintf(stderr, "bad argument to '--transport-id'\n");
 					ret = MPATH_PR_SYNTAX_ERROR;
 					goto out;
 				}

-				++num_transport;
+				++num_transportids;
 				break;

 			case 'l':
@@ -525,9 +556,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_transportids);
+		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);
@@ -544,13 +578,19 @@ static int handle_args(int argc, char * argv[], int nline)
 		if (param_aptpl)
 			paramp->sa_flags |= MPATH_F_APTPL_MASK;

-		if (num_transport)
+		if (num_transportids)
 		{
 			paramp->sa_flags |= MPATH_F_SPEC_I_PT_MASK;
-			paramp->num_transportid = num_transport;
-			for (j = 0 ; j < num_transport; j++)
+			paramp->num_transportid = num_transportids;
+			for (j = 0 ; j < num_transportids; 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 +598,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 V4 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. If these function fail, call remove_map

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 | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c
index e7256049..1f9817ed 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,14 +340,12 @@ 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) {
+			condlog(1, "error parsing map %s", mpp->wwid);
+			remove_map(mpp, pathvec, curmp, PURGE_VEC);
+			i--;
+		}
 	}
 	return MPATH_PR_SUCCESS ;
 }
-- 

[PATCH V4 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 da67c15c..85453ac2 100644
--- a/mpathpersist/main.c
+++ b/mpathpersist/main.c
@@ -211,7 +211,6 @@ static int handle_args(int argc, char * argv[], int nline)
 	int prout_sa = -1;
 	char *batch_fn = NULL;
 	void *resp = NULL;
-	struct transportid * tmp;

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

-- 

Re: [PATCH V4 01/14] multipathd: initialize major and minor in cli_add_map

[Martin Wilck]

On Thu, 2020-09-10 at 18:46 +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.
> 
> Here, we set major and minor to -1 at begining
> 
> Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: Linfeilong <linfeilong@huawei.com>

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

Re: [PATCH V4 02/14] libmultipath: change malloc to calloc in print_foreign_topology

[Martin Wilck]

On Thu, 2020-09-10 at 18:47 +0800, lixiaokeng wrote:
> We chanege malloc to calloc.
> 
> Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: Linfeilong <linfeilong@huawei.com>

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

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

[Martin Wilck]

On Thu, 2020-09-10 at 18:48 +0800, lixiaokeng wrote:
> In  mpath_pr_event_handler_fn, we use MALLOC instead of malloc, and
> check
> the return value of MALLOC. And we delete seting ret when jump to out
> 
> Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: Linfeilong <linfeilong@huawei.com>

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

Re: [PATCH V4 05/14] multipathd: check return value of malloc in cli_getprkey func

[Martin Wilck]

On Thu, 2020-09-10 at 18:49 +0800, lixiaokeng wrote:
> In cli_getprkey func, we 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>

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

Re: [PATCH V4 06/14] kpartx: use xmalloc to instead of malloc in main func

[Martin Wilck]

On Thu, 2020-09-10 at 18:50 +0800, lixiaokeng wrote:
> In main func of kpartx.c, we should check return value of
> malloc before using it. So we use xmalloc to instead of
> malloc.
> 
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>

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

Re: [PATCH V4 09/14] libmultipath: check whether mpp->features is NUll in setup_map

[Martin Wilck]

On Thu, 2020-09-10 at 18:51 +0800, lixiaokeng wrote:
> In assemble_map func, f = STRDUP(mp->features) is just used
> for APPEND(). We can directly pass mp->features to APPEND().
> The mpp->features, hwhandler and selector got form strdup
> should be check after select* function.
> 
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>

Thanks for submitting the new version. Looking at your patch, I realize
that my previous suggestion wasn't perfect. 

> ---
>  libmultipath/configure.c |  5 +++++
>  libmultipath/dmparser.c  | 11 ++++-------
>  2 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index 5bc65fd3..5d5d9415 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -355,6 +355,11 @@ int setup_map(struct multipath *mpp, char
> *params, int params_size,
>  	select_ghost_delay(conf, mpp);
>  	select_flush_on_last_del(conf, mpp);
> 
> +	if (!mpp->features || !mpp->hwhandler || !mpp->selector) {
> +		condlog(0, "%s: map select failed", mpp->alias);
> +		return 1;
> +	}
> +

We have a new failure mode for setup_map() here. While this is a good
thing in principle, there are issues. 

 1) by returning, we skip the rest of the initialization steps for the
map. Thus paths and pathgroups may be set up wrongly after setup_map().
 2) Not every caller deletes the map from the mpvec after setup_map()
fails. For some callers like resize_map() or reload_map(), that's
actually not possible.

1) is a minor problem because no caller calls domap() after setup_map()
failure, afaics. But 2) is a problem, because the broken, partially
initialized struct multipath will remain in our data structures, and my
assumption that features etc. are always valid will be violated.
2) is not a regression of this patch though, it has always been the
case.

I'm not yet decided how to deal with this. Perhaps setup_map()
shouldn't free features, hwhandler, and selector until the new values
have been successfully obtained.

(Actually, what's the point in running through *all* select_xyz()
functions just for a map resize?)

Ben?

Regards
Martin






>  	sysfs_set_scsi_tmo(mpp, conf->checkint);
>  	marginal_pathgroups = conf->marginal_pathgroups;
>  	pthread_cleanup_pop(1);
> diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
> index 482e9d0e..685918da 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,10 +86,9 @@ 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);
> -
> -	APPEND(p, end, "%s %s %i %i", f, mp->hwhandler,
> nr_priority_groups,
> -	       initial_pg_nr);
> +	/* mp->features must not be NULL */
> +	APPEND(p, end, "%s %s %i %i", mp->features, mp->hwhandler,
> +		nr_priority_groups, initial_pg_nr);
> 
>  	vector_foreach_slot (mp->pg, pgp, i) {
>  		pgp = VECTOR_SLOT(mp->pg, i);
> @@ -110,12 +109,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 V4 11/14] mpathpersist: check whether malloc paramp->trnptid_list fails in handle_args func

[Martin Wilck]

On Thu, 2020-09-10 at 18:52 +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].
> 
> We change num_transport to num_transportids to combine them.
> 
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
> ---
>  mpathpersist/main.c | 65 ++++++++++++++++++++++++++++++++++---------
> --
>  1 file changed, 50 insertions(+), 15 deletions(-)
> 
> diff --git a/mpathpersist/main.c b/mpathpersist/main.c
> index 28bfe410..da67c15c 100644
> --- a/mpathpersist/main.c
> +++ b/mpathpersist/main.c
> @@ -153,6 +153,38 @@ 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));

Here you overwrite paramp->num_transportid, which you just set.
Otherwise, the patch looks ok.

Regards,
Martin

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

[Martin Wilck]

On Thu, 2020-09-10 at 18:53 +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. If these function fail, call remove_map
> 
> Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: Linfeilong <linfeilong@huawei.com>

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

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

[Martin Wilck]

On Thu, 2020-09-10 at 18:52 +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].
> 
> We change num_transport to num_transportids to combine them.
> 
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
> ---
>  mpathpersist/main.c | 65 ++++++++++++++++++++++++++++++++++---------
> --
>  1 file changed, 50 insertions(+), 15 deletions(-)
> 
> diff --git a/mpathpersist/main.c b/mpathpersist/main.c
> index 28bfe410..da67c15c 100644
> --- a/mpathpersist/main.c
> +++ b/mpathpersist/main.c
> @@ -153,6 +153,38 @@ 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]);

This causes a compilation error. Didn't you compile-test?

main.c: In function ‘free_prout_param_descriptor’:
main.c:182:16: error: comparison of integer expressions of different
signedness: ‘int’ and ‘uint32_t’ {aka ‘unsigned int’} [-Werror=si]
  182 |  for (i = 0; i < paramp->num_transportid; i++)
      |                ^
cc1: all warnings being treated as errors

Regards,
Martin




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

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

[Martin Wilck]

On Thu, 2020-09-10 at 18:54 +0800, lixiaokeng wrote:
> 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>

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


> ---
>  mpathpersist/main.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/mpathpersist/main.c b/mpathpersist/main.c
> index da67c15c..85453ac2 100644
> --- a/mpathpersist/main.c
> +++ b/mpathpersist/main.c
> @@ -211,7 +211,6 @@ static int handle_args(int argc, char * argv[],
> int nline)
>  	int prout_sa = -1;
>  	char *batch_fn = NULL;
>  	void *resp = NULL;
> -	struct transportid * tmp;
> 
>  	memset(transportids, 0, MPATH_MX_TIDS * sizeof(struct
> transportid));
> 

Re: [PATCH V4 09/14] libmultipath: check whether mpp->features is NUll in setup_map

[Benjamin Marzinski]

On Thu, Sep 10, 2020 at 06:48:56PM +0200, Martin Wilck wrote:
> On Thu, 2020-09-10 at 18:51 +0800, lixiaokeng wrote:
> > In assemble_map func, f = STRDUP(mp->features) is just used
> > for APPEND(). We can directly pass mp->features to APPEND().
> > The mpp->features, hwhandler and selector got form strdup
> > should be check after select* function.
> > 
> > Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> > Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
> 
> Thanks for submitting the new version. Looking at your patch, I realize
> that my previous suggestion wasn't perfect. 
> 
> > ---
> >  libmultipath/configure.c |  5 +++++
> >  libmultipath/dmparser.c  | 11 ++++-------
> >  2 files changed, 9 insertions(+), 7 deletions(-)
> > 
> > diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> > index 5bc65fd3..5d5d9415 100644
> > --- a/libmultipath/configure.c
> > +++ b/libmultipath/configure.c
> > @@ -355,6 +355,11 @@ int setup_map(struct multipath *mpp, char
> > *params, int params_size,
> >  	select_ghost_delay(conf, mpp);
> >  	select_flush_on_last_del(conf, mpp);
> > 
> > +	if (!mpp->features || !mpp->hwhandler || !mpp->selector) {
> > +		condlog(0, "%s: map select failed", mpp->alias);
> > +		return 1;
> > +	}
> > +
> 
> We have a new failure mode for setup_map() here. While this is a good
> thing in principle, there are issues. 
> 
>  1) by returning, we skip the rest of the initialization steps for the
> map. Thus paths and pathgroups may be set up wrongly after setup_map().
>  2) Not every caller deletes the map from the mpvec after setup_map()
> fails. For some callers like resize_map() or reload_map(), that's
> actually not possible.
> 
> 1) is a minor problem because no caller calls domap() after setup_map()
> failure, afaics. But 2) is a problem, because the broken, partially
> initialized struct multipath will remain in our data structures, and my
> assumption that features etc. are always valid will be violated.
> 2) is not a regression of this patch though, it has always been the
> case.
> 
> I'm not yet decided how to deal with this. Perhaps setup_map()
> shouldn't free features, hwhandler, and selector until the new values
> have been successfully obtained.
> 
> (Actually, what's the point in running through *all* select_xyz()
> functions just for a map resize?)
> 
> Ben?

I don't know of any reason why that's necessary, or why it was
originally done that way.  If we've already configured the device, and
we're not doing a reconfigure, there's not a lot to be gained by doing
configuring again. Some to the select_* functions grab stuff from sysfs,
and so could return different values, but I'm sure there is a lot of
work that could be cut out here, with no noticeable changes in behavior.

Ideally we should stick to a "do no harm" policy when updating the
device.  It seems better to have a device structure that's outdated than
one that's invalid.

But regardless of what we do in setup_map, the assemble_map() part of
this patch is correct, and as you said, the setup_map() changes don't
introduce any new problems. It seems like resolving the issue in
setup_map() should be seperate work.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.coM>
 
> Regards
> Martin
> 
> 
> 
> 
> 
> 
> >  	sysfs_set_scsi_tmo(mpp, conf->checkint);
> >  	marginal_pathgroups = conf->marginal_pathgroups;
> >  	pthread_cleanup_pop(1);
> > diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
> > index 482e9d0e..685918da 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,10 +86,9 @@ 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);
> > -
> > -	APPEND(p, end, "%s %s %i %i", f, mp->hwhandler,
> > nr_priority_groups,
> > -	       initial_pg_nr);
> > +	/* mp->features must not be NULL */
> > +	APPEND(p, end, "%s %s %i %i", mp->features, mp->hwhandler,
> > +		nr_priority_groups, initial_pg_nr);
> > 
> >  	vector_foreach_slot (mp->pg, pgp, i) {
> >  		pgp = VECTOR_SLOT(mp->pg, i);
> > @@ -110,12 +109,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 V4 09/14] libmultipath: check whether mpp->features is NUll in setup_map

[Martin Wilck]

On Thu, 2020-09-10 at 18:51 +0800, lixiaokeng wrote:
> In assemble_map func, f = STRDUP(mp->features) is just used
> for APPEND(). We can directly pass mp->features to APPEND().
> The mpp->features, hwhandler and selector got form strdup
> should be check after select* function.
> 
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
> ---
>  libmultipath/configure.c |  5 +++++
>  libmultipath/dmparser.c  | 11 ++++-------
>  2 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index 5bc65fd3..5d5d9415 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -355,6 +355,11 @@ int setup_map(struct multipath *mpp, char
> *params, int params_size,
>  	select_ghost_delay(conf, mpp);
>  	select_flush_on_last_del(conf, mpp);
> 
> +	if (!mpp->features || !mpp->hwhandler || !mpp->selector) {
> +		condlog(0, "%s: map select failed", mpp->alias);
> +		return 1;
> +	}
> +
> 
> 	sysfs_set_scsi_tmo(mpp, conf->checkint);
>  	marginal_pathgroups = conf->marginal_pathgroups;
>  	pthread_cleanup_pop(1);

Just saw this: you can't return between pthread_cleanup_push() and
pthread_cleanup_pop(). You need to move this block below the
pthread_cleanup_pop() statement.

So, Nack for this version of the patch. Sorry

Martin

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

[lixiaokeng]

On 2020/9/11 2:48, Martin Wilck wrote:
> On Thu, 2020-09-10 at 18:52 +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].
>>
>> We change num_transport to num_transportids to combine them.
>>
>> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
>> Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
>> ---
>>  mpathpersist/main.c | 65 ++++++++++++++++++++++++++++++++++---------
>> --
>>  1 file changed, 50 insertions(+), 15 deletions(-)
>>
>> diff --git a/mpathpersist/main.c b/mpathpersist/main.c
>> index 28bfe410..da67c15c 100644
>> --- a/mpathpersist/main.c
>> +++ b/mpathpersist/main.c
>> @@ -153,6 +153,38 @@ 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]);
> 
> This causes a compilation error. Didn't you compile-test?
> 
> main.c: In function ‘free_prout_param_descriptor’:
> main.c:182:16: error: comparison of integer expressions of different
> signedness: ‘int’ and ‘uint32_t’ {aka ‘unsigned int’} [-Werror=si]
>   182 |  for (i = 0; i < paramp->num_transportid; i++)
>       |                ^
> cc1: all warnings being treated as errors
> 
> Regards,
> Martin
> 
Hi Martin,
  When I compile it, I add a patch thats changes int to unsigned int.
But I don't think it is an error. It is  just a warning and becomes an
error with [-Werror]. Anyway, I will change int to unsigned it and send
it again. Thanks.

Regards
Lixiaokeng


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

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

[Martin Wilck]

Hi Lixiaokeng,

On Fri, 2020-09-11 at 09:34 +0800, lixiaokeng wrote:
> 
> Hi Martin,
>   When I compile it, I add a patch thats changes int to unsigned int.
> But I don't think it is an error. It is  just a warning and becomes
> an
> error with [-Werror]. Anyway,

What do you think, why did we add -Werror?

Disabling the warning options when working on the code is strongly
discouraged.

> > I will change int to unsigned it and send
> it again. Thanks.

Thank you.

Martin