[dm-devel] [PATCH 0/5] multipath-tools series： some codeclean and add prflag to path

[dm-devel] [PATCH 0/5] multipath-tools series： some codeclean and add prflag to path

[lixiaokeng]

lixiaokeng (5):
  Fix potential null pointer dereference
  remove unnecessary memset
  remove unnecessary free
  Match FREE and MALLOC/STRDUP/REALLOC
  add prflag to path

 kpartx/dasd.c                            |  5 +++--
 kpartx/kpartx.c                          |  7 ++++---
 libmpathpersist/mpath_persist.c          |  2 +-
 libmultipath/blacklist.c                 |  6 +++---
 libmultipath/checkers/emc_clariion.c     |  2 +-
 libmultipath/config.c                    |  4 ++--
 libmultipath/configure.c                 |  4 ++--
 libmultipath/discovery.c                 |  8 +++++---
 libmultipath/dmparser.c                  |  2 +-
 libmultipath/foreign/nvme.c              |  4 +++-
 libmultipath/log.c                       |  1 -
 libmultipath/parser.c                    |  2 +-
 libmultipath/prioritizers/weightedpath.c |  3 +--
 libmultipath/structs.c                   |  3 ++-
 libmultipath/structs.h                   | 12 ++++++++++++
 libmultipath/util.c                      | 10 +++++++++-
 multipathd/cli_handlers.c                | 15 ++++++++++-----
 multipathd/main.c                        |  9 ++++++---
 multipathd/waiter.c                      |  1 -
 19 files changed, 66 insertions(+), 34 deletions(-)

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

[dm-devel] [PATCH 1/5] Fix potential null pointer dereference

[lixiaokeng]

udev_device_* may return NULL, check it.

Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
---
 libmultipath/discovery.c    |  8 +++++---
 libmultipath/foreign/nvme.c |  4 +++-
 libmultipath/util.c         | 10 +++++++++-
 3 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index f25fe9e3..48f3d8b2 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -388,8 +388,10 @@ sysfs_get_tgt_nodename(struct path *pp, char *node)
 		if (value && !strcmp(value, "usb")) {
 			pp->sg_id.proto_id = SCSI_PROTOCOL_USB;
 			tgtname = udev_device_get_sysname(tgtdev);
-			strlcpy(node, tgtname, NODE_NAME_SIZE);
-			return 0;
+			if (!tgtname) {
+				strlcpy(node, tgtname, NODE_NAME_SIZE);
+				return 0;
+			}
 		}
 		tgtdev = udev_device_get_parent(tgtdev);
 	}
@@ -803,7 +805,7 @@ sysfs_set_nexus_loss_tmo(struct multipath *mpp, struct path *pp)
 	     parent = udev_device_get_parent(parent)) {
 		const char *ed = udev_device_get_sysname(parent);

-		if (!strncmp(ed, ed_str, sizeof(ed_str) - 1)) {
+		if (ed && !strncmp(ed, ed_str, sizeof(ed_str) - 1)) {
 			end_dev_id = ed;
 			break;
 		}
diff --git a/libmultipath/foreign/nvme.c b/libmultipath/foreign/nvme.c
index d40c0869..f778410a 100644
--- a/libmultipath/foreign/nvme.c
+++ b/libmultipath/foreign/nvme.c
@@ -184,7 +184,9 @@ static int snprint_nvme_map(const struct gen_multipath *gmp,
 							      "firmware_rev"));
 	case 'r':
 		val = udev_device_get_sysattr_value(nvm->udev, "ro");
-		if (val[0] == 1)
+		if (!val)
+			return -1;
+		else if (val[0] == 1)
 			return append_strbuf_str(buff, "ro");
 		else
 			return append_strbuf_str(buff, "rw");
diff --git a/libmultipath/util.c b/libmultipath/util.c
index ea858409..3d036e19 100644
--- a/libmultipath/util.c
+++ b/libmultipath/util.c
@@ -168,6 +168,7 @@ size_t strlcat(char * restrict dst, const char * restrict src, size_t size)
 int devt2devname(char *devname, int devname_len, const char *devt)
 {
 	struct udev_device *u_dev;
+	const char * dev_name;
 	int r;

 	if (!devname || !devname_len || !devt)
@@ -178,7 +179,14 @@ int devt2devname(char *devname, int devname_len, const char *devt)
 		condlog(0, "\"%s\": invalid major/minor numbers, not found in sysfs", devt);
 		return 1;
 	}
-	r = strlcpy(devname, udev_device_get_sysname(u_dev), devname_len);
+
+	dev_name = udev_device_get_sysname(u_dev);
+	if (!dev_name) {
+		condlog(2, "\"%s\": fail to get sysname\n", devt);
+		udev_device_unref(u_dev);
+		return 1;
+	}
+	r = strlcpy(devname, dev_name, devname_len);
 	udev_device_unref(u_dev);

 	return !(r < devname_len);
-- 

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

Re: [dm-devel] [PATCH 1/5] Fix potential null pointer dereference

[Benjamin Marzinski]

On Tue, Nov 16, 2021 at 09:59:14PM +0800, lixiaokeng wrote:
> udev_device_* may return NULL, check it.
> 
> Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
> ---
>  libmultipath/discovery.c    |  8 +++++---
>  libmultipath/foreign/nvme.c |  4 +++-
>  libmultipath/util.c         | 10 +++++++++-
>  3 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index f25fe9e3..48f3d8b2 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -388,8 +388,10 @@ sysfs_get_tgt_nodename(struct path *pp, char *node)
>  		if (value && !strcmp(value, "usb")) {
>  			pp->sg_id.proto_id = SCSI_PROTOCOL_USB;
>  			tgtname = udev_device_get_sysname(tgtdev);
> -			strlcpy(node, tgtname, NODE_NAME_SIZE);
> -			return 0;
> +			if (!tgtname) {

I assume that you mean "if (tgtname)"

> +				strlcpy(node, tgtname, NODE_NAME_SIZE);
> +				return 0;
> +			}
>  		}
>  		tgtdev = udev_device_get_parent(tgtdev);
>  	}
> @@ -803,7 +805,7 @@ sysfs_set_nexus_loss_tmo(struct multipath *mpp, struct path *pp)
>  	     parent = udev_device_get_parent(parent)) {
>  		const char *ed = udev_device_get_sysname(parent);
> 
> -		if (!strncmp(ed, ed_str, sizeof(ed_str) - 1)) {
> +		if (ed && !strncmp(ed, ed_str, sizeof(ed_str) - 1)) {
>  			end_dev_id = ed;
>  			break;
>  		}
> diff --git a/libmultipath/foreign/nvme.c b/libmultipath/foreign/nvme.c
> index d40c0869..f778410a 100644
> --- a/libmultipath/foreign/nvme.c
> +++ b/libmultipath/foreign/nvme.c
> @@ -184,7 +184,9 @@ static int snprint_nvme_map(const struct gen_multipath *gmp,
>  							      "firmware_rev"));
>  	case 'r':
>  		val = udev_device_get_sysattr_value(nvm->udev, "ro");
> -		if (val[0] == 1)
> +		if (!val)
> +			return -1;

sprint_ro() returns "undef" for a similar case. Perhaps we should do
that here as well.

-Ben

> +		else if (val[0] == 1)
>  			return append_strbuf_str(buff, "ro");
>  		else
>  			return append_strbuf_str(buff, "rw");
> diff --git a/libmultipath/util.c b/libmultipath/util.c
> index ea858409..3d036e19 100644
> --- a/libmultipath/util.c
> +++ b/libmultipath/util.c
> @@ -168,6 +168,7 @@ size_t strlcat(char * restrict dst, const char * restrict src, size_t size)
>  int devt2devname(char *devname, int devname_len, const char *devt)
>  {
>  	struct udev_device *u_dev;
> +	const char * dev_name;
>  	int r;
> 
>  	if (!devname || !devname_len || !devt)
> @@ -178,7 +179,14 @@ int devt2devname(char *devname, int devname_len, const char *devt)
>  		condlog(0, "\"%s\": invalid major/minor numbers, not found in sysfs", devt);
>  		return 1;
>  	}
> -	r = strlcpy(devname, udev_device_get_sysname(u_dev), devname_len);
> +
> +	dev_name = udev_device_get_sysname(u_dev);
> +	if (!dev_name) {
> +		condlog(2, "\"%s\": fail to get sysname\n", devt);
> +		udev_device_unref(u_dev);
> +		return 1;
> +	}
> +	r = strlcpy(devname, dev_name, devname_len);
>  	udev_device_unref(u_dev);
> 
>  	return !(r < devname_len);
> -- 

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

Re: [dm-devel] [PATCH 1/5] Fix potential null pointer dereference

[lixiaokeng]

>> @@ -388,8 +388,10 @@ sysfs_get_tgt_nodename(struct path *pp, char *node)
>>  		if (value && !strcmp(value, "usb")) {
>>  			pp->sg_id.proto_id = SCSI_PROTOCOL_USB;
>>  			tgtname = udev_device_get_sysname(tgtdev);
>> -			strlcpy(node, tgtname, NODE_NAME_SIZE);
>> -			return 0;
>> +			if (!tgtname) {
> 
> I assume that you mean "if (tgtname)"
> 

Thanks for your correction. I'm will correct it.

>> +				strlcpy(node, tgtname, NODE_NAME_SIZE);
>> +				return 0;
>> +			}
>>  		}
>>  		tgtdev = udev_device_get_parent(tgtdev);
>>  	}
>> diff --git a/libmultipath/foreign/nvme.c b/libmultipath/foreign/nvme.c
>> index d40c0869..f778410a 100644
>> --- a/libmultipath/foreign/nvme.c
>> +++ b/libmultipath/foreign/nvme.c
>> @@ -184,7 +184,9 @@ static int snprint_nvme_map(const struct gen_multipath *gmp,
>>  							      "firmware_rev"));
>>  	case 'r':
>>  		val = udev_device_get_sysattr_value(nvm->udev, "ro");
>> -		if (val[0] == 1)
>> +		if (!val)
>> +			return -1;
> 
> sprint_ro() returns "undef" for a similar case. Perhaps we should do
> that here as well.
> 

I'll do it here and send v2 patch. Thanks again.


Regards,
Lixiaokeng

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

[dm-devel] [PATCH 2/5] remove unnecessary memset

[lixiaokeng]

MALLOC will set memory zero. memset is unnecessary.
Remove it.

Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
---
 libmultipath/log.c  | 1 -
 multipathd/waiter.c | 1 -
 2 files changed, 2 deletions(-)

diff --git a/libmultipath/log.c b/libmultipath/log.c
index 10fa32cd..f41efb5b 100644
--- a/libmultipath/log.c
+++ b/libmultipath/log.c
@@ -57,7 +57,6 @@ static int logarea_init (int size)
 		FREE(la);
 		return 1;
 	}
-	memset(la->start, 0, size);

 	la->empty = 1;
 	la->end = la->start + size;
diff --git a/multipathd/waiter.c b/multipathd/waiter.c
index bbe6c2a1..f74b395a 100644
--- a/multipathd/waiter.c
+++ b/multipathd/waiter.c
@@ -33,7 +33,6 @@ static struct event_thread *alloc_waiter (void)
 	struct event_thread *wp;

 	wp = (struct event_thread *)MALLOC(sizeof(struct event_thread));
-	memset(wp, 0, sizeof(struct event_thread));

 	return wp;
 }
--

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

Re: [dm-devel] [PATCH 2/5] remove unnecessary memset

[Benjamin Marzinski]

On Tue, Nov 16, 2021 at 09:59:41PM +0800, lixiaokeng wrote:
> MALLOC will set memory zero. memset is unnecessary.
> Remove it.
> 
> Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmultipath/log.c  | 1 -
>  multipathd/waiter.c | 1 -
>  2 files changed, 2 deletions(-)
> 
> diff --git a/libmultipath/log.c b/libmultipath/log.c
> index 10fa32cd..f41efb5b 100644
> --- a/libmultipath/log.c
> +++ b/libmultipath/log.c
> @@ -57,7 +57,6 @@ static int logarea_init (int size)
>  		FREE(la);
>  		return 1;
>  	}
> -	memset(la->start, 0, size);
> 
>  	la->empty = 1;
>  	la->end = la->start + size;
> diff --git a/multipathd/waiter.c b/multipathd/waiter.c
> index bbe6c2a1..f74b395a 100644
> --- a/multipathd/waiter.c
> +++ b/multipathd/waiter.c
> @@ -33,7 +33,6 @@ static struct event_thread *alloc_waiter (void)
>  	struct event_thread *wp;
> 
>  	wp = (struct event_thread *)MALLOC(sizeof(struct event_thread));
> -	memset(wp, 0, sizeof(struct event_thread));
> 
>  	return wp;
>  }
> --

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

[dm-devel] [PATCH 3/5] remove unnecessary free

[lixiaokeng]

arg will be free by cleanup_charp. FREE(args)
is unnecessary before return. Remove it.

Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
---
 libmultipath/prioritizers/weightedpath.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/libmultipath/prioritizers/weightedpath.c b/libmultipath/prioritizers/weightedpath.c
index ea03fc3d..32d1cf0c 100644
--- a/libmultipath/prioritizers/weightedpath.c
+++ b/libmultipath/prioritizers/weightedpath.c
@@ -75,13 +75,12 @@ int prio_path_weight(struct path *pp, char *prio_args)
 	if (!prio_args)
 		return priority;

-	arg = temp = STRDUP(prio_args);
+	arg = temp = strdup(prio_args);

 	regex = get_next_string(&temp, split_char);

 	/* Return default priority if the argument is not parseable */
 	if (!regex) {
-		FREE(arg);
 		return priority;
 	}

-- 

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

Re: [dm-devel] [PATCH 3/5] remove unnecessary free

[Benjamin Marzinski]

On Tue, Nov 16, 2021 at 10:00:09PM +0800, lixiaokeng wrote:
> arg will be free by cleanup_charp. FREE(args)
> is unnecessary before return. Remove it.
> 
> Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmultipath/prioritizers/weightedpath.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/libmultipath/prioritizers/weightedpath.c b/libmultipath/prioritizers/weightedpath.c
> index ea03fc3d..32d1cf0c 100644
> --- a/libmultipath/prioritizers/weightedpath.c
> +++ b/libmultipath/prioritizers/weightedpath.c
> @@ -75,13 +75,12 @@ int prio_path_weight(struct path *pp, char *prio_args)
>  	if (!prio_args)
>  		return priority;
> 
> -	arg = temp = STRDUP(prio_args);
> +	arg = temp = strdup(prio_args);
> 
>  	regex = get_next_string(&temp, split_char);
> 
>  	/* Return default priority if the argument is not parseable */
>  	if (!regex) {
> -		FREE(arg);
>  		return priority;
>  	}
> 
> -- 

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

[dm-devel] [PATCH 4/5] Match FREE and MALLOC/STRDUP/REALLOC

[lixiaokeng]

In _DEBUG_ mode, MALLOC/STRDUP/REALLOC and FREE will record
the memory usage. Match them.

Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
---
 kpartx/dasd.c                        | 5 +++--
 kpartx/kpartx.c                      | 7 ++++---
 libmultipath/blacklist.c             | 6 +++---
 libmultipath/checkers/emc_clariion.c | 2 +-
 libmultipath/config.c                | 4 ++--
 libmultipath/configure.c             | 4 ++--
 libmultipath/dmparser.c              | 2 +-
 libmultipath/parser.c                | 2 +-
 libmultipath/structs.c               | 2 +-
 multipathd/main.c                    | 2 +-
 10 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/kpartx/dasd.c b/kpartx/dasd.c
index f0398645..14744048 100644
--- a/kpartx/dasd.c
+++ b/kpartx/dasd.c
@@ -40,6 +40,7 @@
 #include "kpartx.h"
 #include "byteorder.h"
 #include "dasd.h"
+#include "memory.h"

 unsigned long long sectors512(unsigned long long sectors, int blocksize)
 {
@@ -100,10 +101,10 @@ read_dasd_pt(int fd, __attribute__((unused)) struct slice all,
 		 * Get the first target and operate on that instead.
 		 */
 		if (!(dev = dm_get_first_dep(devname))) {
-			free(devname);
+			FREE(devname);
 			return -1;
 		}
-		free(devname);
+		FREE(devname);

 		if ((unsigned int)major(dev) != 94) {
 			/* Not a DASD */
diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
index 7bc64543..5e59063d 100644
--- a/kpartx/kpartx.c
+++ b/kpartx/kpartx.c
@@ -39,6 +39,7 @@
 #include "lopart.h"
 #include "kpartx.h"
 #include "version.h"
+#include "memory.h"

 #define SIZE(a) (sizeof(a)/sizeof((a)[0]))

@@ -177,7 +178,7 @@ get_hotplug_device(void)

 	/* Dirname + mapname + \0 */
 	if (!(device = (char *)malloc(sizeof(char) * (off + len + 1)))) {
-		free(mapname);
+		FREE(mapname);
 		return NULL;
 	}

@@ -187,10 +188,10 @@ get_hotplug_device(void)

 	if (strlen(device) != (off + len)) {
 		free(device);
-		free(mapname);
+		FREE(mapname);
 		return NULL;
 	}
-	free(mapname);
+	FREE(mapname);
 	return device;
 }

diff --git a/libmultipath/blacklist.c b/libmultipath/blacklist.c
index 4e315c97..573df152 100644
--- a/libmultipath/blacklist.c
+++ b/libmultipath/blacklist.c
@@ -39,7 +39,7 @@ int store_ble(vector blist, const char *str, int origin)
 	if (!str)
 		return 0;

-	strdup_str = strdup(str);
+	strdup_str = STRDUP(str);
 	if (!strdup_str)
 		return 1;

@@ -134,8 +134,8 @@ out1:
 		ble->vendor = NULL;
 	}
 out:
-	free(vendor_str);
-	free(product_str);
+	FREE(vendor_str);
+	FREE(product_str);
 	return 1;
 }

diff --git a/libmultipath/checkers/emc_clariion.c b/libmultipath/checkers/emc_clariion.c
index 5cd63aca..b3f0aded 100644
--- a/libmultipath/checkers/emc_clariion.c
+++ b/libmultipath/checkers/emc_clariion.c
@@ -128,7 +128,7 @@ int libcheck_mp_init (struct checker * c)

 void libcheck_free (struct checker * c)
 {
-	free(c->context);
+	FREE(c->context);
 }

 int libcheck_check (struct checker * c)
diff --git a/libmultipath/config.c b/libmultipath/config.c
index 30046a17..667b500b 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -1051,10 +1051,10 @@ int parse_uid_attrs(char *uid_attrs, struct config *conf)
 		if (!tmp) {
 			condlog(2, "invalid record in uid_attrs: %s",
 				uid_attr_record);
-			free(uid_attr_record);
+			FREE(uid_attr_record);
 			ret = 1;
 		} else if (!vector_alloc_slot(attrs)) {
-			free(uid_attr_record);
+			FREE(uid_attr_record);
 			ret = 1;
 		} else
 			vector_set_slot(attrs, uid_attr_record);
diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 7edb355b..9545854b 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -738,8 +738,8 @@ void select_action (struct multipath *mpp, const struct _vector *curmp,
 		condlog(1, "%s: can't use alias \"%s\" used by %s, falling back to WWID",
 			mpp->wwid, mpp->alias, cmpp_by_name->wwid);
 		/* We can do this because wwid wasn't found */
-		free(mpp->alias);
-		mpp->alias = strdup(mpp->wwid);
+		FREE(mpp->alias);
+		mpp->alias = STRDUP(mpp->wwid);
 		mpp->action = ACT_CREATE;
 		condlog(3, "%s: set ACT_CREATE (map does not exist, name changed)",
 			mpp->alias);
diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
index 4ba7f339..96beeb6d 100644
--- a/libmultipath/dmparser.c
+++ b/libmultipath/dmparser.c
@@ -30,7 +30,7 @@ merge_words(char **dst, const char *word)
 	*dst = REALLOC(*dst, len);

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

diff --git a/libmultipath/parser.c b/libmultipath/parser.c
index 68262d0e..f0047c4d 100644
--- a/libmultipath/parser.c
+++ b/libmultipath/parser.c
@@ -364,7 +364,7 @@ set_value(vector strvec)
 	for (i = 2; i < VECTOR_SIZE(strvec); i++) {
 		str = VECTOR_SLOT(strvec, i);
 		if (!str) {
-			free(alloc);
+			FREE(alloc);
 			condlog(0, "parse error for option '%s'",
 				(char *)VECTOR_SLOT(strvec, 0));
 			return NULL;
diff --git a/libmultipath/structs.c b/libmultipath/structs.c
index 6e5a1038..e8cacb4b 100644
--- a/libmultipath/structs.c
+++ b/libmultipath/structs.c
@@ -106,7 +106,7 @@ alloc_path (void)
 		dm_path_to_gen(pp)->ops = &dm_gen_path_ops;
 		pp->hwe = vector_alloc();
 		if (pp->hwe == NULL) {
-			free(pp);
+			FREE(pp);
 			return NULL;
 		}
 	}
diff --git a/multipathd/main.c b/multipathd/main.c
index 1defeaf1..82ab3ed1 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -3555,7 +3555,7 @@ void *  mpath_pr_event_handler_fn (void * pathp )
 		goto out;
 	}

-	param = (struct prout_param_descriptor *)MALLOC(sizeof(struct prout_param_descriptor));
+	param = (struct prout_param_descriptor *)calloc(1, sizeof(struct prout_param_descriptor));
 	if (!param)
 		goto out;

-- 

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

Re: [dm-devel] [PATCH 4/5] Match FREE and MALLOC/STRDUP/REALLOC

[Benjamin Marzinski]

On Tue, Nov 16, 2021 at 10:00:53PM +0800, lixiaokeng wrote:
> In _DEBUG_ mode, MALLOC/STRDUP/REALLOC and FREE will record
> the memory usage. Match them.

This looks fine, but personally, I'd rather just have all the DEBUG
memory code removed. If people want to check memory usage, there's
always valgrind.

-Ben

> 
> Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
> ---
>  kpartx/dasd.c                        | 5 +++--
>  kpartx/kpartx.c                      | 7 ++++---
>  libmultipath/blacklist.c             | 6 +++---
>  libmultipath/checkers/emc_clariion.c | 2 +-
>  libmultipath/config.c                | 4 ++--
>  libmultipath/configure.c             | 4 ++--
>  libmultipath/dmparser.c              | 2 +-
>  libmultipath/parser.c                | 2 +-
>  libmultipath/structs.c               | 2 +-
>  multipathd/main.c                    | 2 +-
>  10 files changed, 19 insertions(+), 17 deletions(-)
> 
> diff --git a/kpartx/dasd.c b/kpartx/dasd.c
> index f0398645..14744048 100644
> --- a/kpartx/dasd.c
> +++ b/kpartx/dasd.c
> @@ -40,6 +40,7 @@
>  #include "kpartx.h"
>  #include "byteorder.h"
>  #include "dasd.h"
> +#include "memory.h"
> 
>  unsigned long long sectors512(unsigned long long sectors, int blocksize)
>  {
> @@ -100,10 +101,10 @@ read_dasd_pt(int fd, __attribute__((unused)) struct slice all,
>  		 * Get the first target and operate on that instead.
>  		 */
>  		if (!(dev = dm_get_first_dep(devname))) {
> -			free(devname);
> +			FREE(devname);
>  			return -1;
>  		}
> -		free(devname);
> +		FREE(devname);
> 
>  		if ((unsigned int)major(dev) != 94) {
>  			/* Not a DASD */
> diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
> index 7bc64543..5e59063d 100644
> --- a/kpartx/kpartx.c
> +++ b/kpartx/kpartx.c
> @@ -39,6 +39,7 @@
>  #include "lopart.h"
>  #include "kpartx.h"
>  #include "version.h"
> +#include "memory.h"
> 
>  #define SIZE(a) (sizeof(a)/sizeof((a)[0]))
> 
> @@ -177,7 +178,7 @@ get_hotplug_device(void)
> 
>  	/* Dirname + mapname + \0 */
>  	if (!(device = (char *)malloc(sizeof(char) * (off + len + 1)))) {
> -		free(mapname);
> +		FREE(mapname);
>  		return NULL;
>  	}
> 
> @@ -187,10 +188,10 @@ get_hotplug_device(void)
> 
>  	if (strlen(device) != (off + len)) {
>  		free(device);
> -		free(mapname);
> +		FREE(mapname);
>  		return NULL;
>  	}
> -	free(mapname);
> +	FREE(mapname);
>  	return device;
>  }
> 
> diff --git a/libmultipath/blacklist.c b/libmultipath/blacklist.c
> index 4e315c97..573df152 100644
> --- a/libmultipath/blacklist.c
> +++ b/libmultipath/blacklist.c
> @@ -39,7 +39,7 @@ int store_ble(vector blist, const char *str, int origin)
>  	if (!str)
>  		return 0;
> 
> -	strdup_str = strdup(str);
> +	strdup_str = STRDUP(str);
>  	if (!strdup_str)
>  		return 1;
> 
> @@ -134,8 +134,8 @@ out1:
>  		ble->vendor = NULL;
>  	}
>  out:
> -	free(vendor_str);
> -	free(product_str);
> +	FREE(vendor_str);
> +	FREE(product_str);
>  	return 1;
>  }
> 
> diff --git a/libmultipath/checkers/emc_clariion.c b/libmultipath/checkers/emc_clariion.c
> index 5cd63aca..b3f0aded 100644
> --- a/libmultipath/checkers/emc_clariion.c
> +++ b/libmultipath/checkers/emc_clariion.c
> @@ -128,7 +128,7 @@ int libcheck_mp_init (struct checker * c)
> 
>  void libcheck_free (struct checker * c)
>  {
> -	free(c->context);
> +	FREE(c->context);
>  }
> 
>  int libcheck_check (struct checker * c)
> diff --git a/libmultipath/config.c b/libmultipath/config.c
> index 30046a17..667b500b 100644
> --- a/libmultipath/config.c
> +++ b/libmultipath/config.c
> @@ -1051,10 +1051,10 @@ int parse_uid_attrs(char *uid_attrs, struct config *conf)
>  		if (!tmp) {
>  			condlog(2, "invalid record in uid_attrs: %s",
>  				uid_attr_record);
> -			free(uid_attr_record);
> +			FREE(uid_attr_record);
>  			ret = 1;
>  		} else if (!vector_alloc_slot(attrs)) {
> -			free(uid_attr_record);
> +			FREE(uid_attr_record);
>  			ret = 1;
>  		} else
>  			vector_set_slot(attrs, uid_attr_record);
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index 7edb355b..9545854b 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -738,8 +738,8 @@ void select_action (struct multipath *mpp, const struct _vector *curmp,
>  		condlog(1, "%s: can't use alias \"%s\" used by %s, falling back to WWID",
>  			mpp->wwid, mpp->alias, cmpp_by_name->wwid);
>  		/* We can do this because wwid wasn't found */
> -		free(mpp->alias);
> -		mpp->alias = strdup(mpp->wwid);
> +		FREE(mpp->alias);
> +		mpp->alias = STRDUP(mpp->wwid);
>  		mpp->action = ACT_CREATE;
>  		condlog(3, "%s: set ACT_CREATE (map does not exist, name changed)",
>  			mpp->alias);
> diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
> index 4ba7f339..96beeb6d 100644
> --- a/libmultipath/dmparser.c
> +++ b/libmultipath/dmparser.c
> @@ -30,7 +30,7 @@ merge_words(char **dst, const char *word)
>  	*dst = REALLOC(*dst, len);
> 
>  	if (!*dst) {
> -		free(p);
> +		FREE(p);
>  		return 1;
>  	}
> 
> diff --git a/libmultipath/parser.c b/libmultipath/parser.c
> index 68262d0e..f0047c4d 100644
> --- a/libmultipath/parser.c
> +++ b/libmultipath/parser.c
> @@ -364,7 +364,7 @@ set_value(vector strvec)
>  	for (i = 2; i < VECTOR_SIZE(strvec); i++) {
>  		str = VECTOR_SLOT(strvec, i);
>  		if (!str) {
> -			free(alloc);
> +			FREE(alloc);
>  			condlog(0, "parse error for option '%s'",
>  				(char *)VECTOR_SLOT(strvec, 0));
>  			return NULL;
> diff --git a/libmultipath/structs.c b/libmultipath/structs.c
> index 6e5a1038..e8cacb4b 100644
> --- a/libmultipath/structs.c
> +++ b/libmultipath/structs.c
> @@ -106,7 +106,7 @@ alloc_path (void)
>  		dm_path_to_gen(pp)->ops = &dm_gen_path_ops;
>  		pp->hwe = vector_alloc();
>  		if (pp->hwe == NULL) {
> -			free(pp);
> +			FREE(pp);
>  			return NULL;
>  		}
>  	}
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 1defeaf1..82ab3ed1 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -3555,7 +3555,7 @@ void *  mpath_pr_event_handler_fn (void * pathp )
>  		goto out;
>  	}
> 
> -	param = (struct prout_param_descriptor *)MALLOC(sizeof(struct prout_param_descriptor));
> +	param = (struct prout_param_descriptor *)calloc(1, sizeof(struct prout_param_descriptor));
>  	if (!param)
>  		goto out;
> 
> -- 

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

Re: [dm-devel] [PATCH 4/5] Match FREE and MALLOC/STRDUP/REALLOC

[lixiaokeng]

On 2021/11/18 8:47, Benjamin Marzinski wrote:
> On Tue, Nov 16, 2021 at 10:00:53PM +0800, lixiaokeng wrote:
>> In _DEBUG_ mode, MALLOC/STRDUP/REALLOC and FREE will record
>> the memory usage. Match them.
> 
> This looks fine, but personally, I'd rather just have all the DEBUG
> memory code removed. If people want to check memory usage, there's
> always valgrind.
> 
> -Ben
> 
>>

If MALLOC/STRDUP/REALLOC and FREE is unnecessary, I'll remove this patch
in this series and make a patch remove them latter. What is Martin's
opinion?

Regards
Lixiaokeng

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

Re: [dm-devel] [PATCH 4/5] Match FREE and MALLOC/STRDUP/REALLOC

[Martin Wilck]

On Thu, 2021-11-18 at 10:46 +0800, lixiaokeng wrote:
> 
> 
> On 2021/11/18 8:47, Benjamin Marzinski wrote:
> > On Tue, Nov 16, 2021 at 10:00:53PM +0800, lixiaokeng wrote:
> > > In _DEBUG_ mode, MALLOC/STRDUP/REALLOC and FREE will record
> > > the memory usage. Match them.
> > 
> > This looks fine, but personally, I'd rather just have all the DEBUG
> > memory code removed. If people want to check memory usage, there's
> > always valgrind.
> > 
> > -Ben
> > 
> > > 
> 
> If MALLOC/STRDUP/REALLOC and FREE is unnecessary, I'll remove this
> patch
> in this series and make a patch remove them latter. What is Martin's
> opinion?

As you saw, I agree with Ben. If you want to make a major contribution,
prepare a patch that gets rid of all those. :-) 

Just note that MALLOC() is really calloc(). 

The only one that that's useful in non-debug mode is FREE(), because it
sets the pointer to NULL, avoiding double-free. Therefore a patch
replacing FREE() with free() would need to assess where that's
necessary (basically always if the data structure that holds the
pointer lives on after the free() call).

Regards
Martin


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

Re: [dm-devel] [PATCH 4/5] Match FREE and MALLOC/STRDUP/REALLOC

[Martin Wilck]

On Wed, 2021-11-17 at 18:47 -0600, Benjamin Marzinski wrote:
> On Tue, Nov 16, 2021 at 10:00:53PM +0800, lixiaokeng wrote:
> > In _DEBUG_ mode, MALLOC/STRDUP/REALLOC and FREE will record
> > the memory usage. Match them.
> 
> This looks fine, but personally, I'd rather just have all the DEBUG
> memory code removed. If people want to check memory usage, there's
> always valgrind.

Ack. I'd rather see the MALLOC, FREE etc removed from the code.
I wouldn't want to add any more of them.

Good to see that you have similar feelings about this.

Martin

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

[dm-devel] [PATCH 5/5] add prflag to path

[lixiaokeng]

The update_map will frequently be called and there will be
unnecessary checks of reseravtion. We add prflag to path
to avoid this.

The pp->state changes from others to up or ghost, the
mpath_pr_event_handle should be called. The
mpath_pr_event_handle in ev_add_path may not be called,
so set pp->prkey PRKEY_NO when path is removed.

Fix: 4db4fa
Signed-off-by: Lixiaokeng <lixiaokeng>
---
 libmpathpersist/mpath_persist.c |  2 +-
 libmultipath/structs.c          |  1 +
 libmultipath/structs.h          | 12 ++++++++++++
 multipathd/cli_handlers.c       | 15 ++++++++++-----
 multipathd/main.c               |  5 +++--
 5 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c
index 803a2a28..f88a2e89 100644
--- a/libmpathpersist/mpath_persist.c
+++ b/libmpathpersist/mpath_persist.c
@@ -924,7 +924,7 @@ int update_map_pr(struct multipath *mpp)

 	if (isFound)
 	{
-		mpp->prflag = 1;
+		mpp->prflag = PRFLAG_OK;
 		condlog(2, "%s: prflag flag set.", mpp->alias );
 	}

diff --git a/libmultipath/structs.c b/libmultipath/structs.c
index e8cacb4b..82dbd565 100644
--- a/libmultipath/structs.c
+++ b/libmultipath/structs.c
@@ -122,6 +122,7 @@ uninitialize_path(struct path *pp)
 	pp->dmstate = PSTATE_UNDEF;
 	pp->uid_attribute = NULL;
 	pp->getuid = NULL;
+	pp->prflag = PRFLAG_NO;

 	if (checker_selected(&pp->checker))
 		checker_put(&pp->checker);
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 399540e7..5b77218b 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -249,6 +249,17 @@ enum recheck_wwid_states {
 	RECHECK_WWID_ON = YNU_YES,
 };

+/*
+ * PRFLAG_NO for path, it means reservation should be checked.
+ * PRFLAG_NO for multipath, it means mpp has no prkey.
+ * PRFLAG_OK for path, it means reservation has been checked.
+ * PRFLAG_OK for multipath, it means mpp has prkey.
+ */
+enum prflag_states {
+	PRFLAG_NO = 0,
+	PRFLAG_OK = 1,
+};
+
 struct vpd_vendor_page {
 	int pg;
 	const char *name;
@@ -327,6 +338,7 @@ struct path {
 	/* configlet pointers */
 	vector hwe;
 	struct gen_path generic_path;
+	int prflag;
 };

 typedef int (pgpolicyfn) (struct multipath *, vector);
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 6d3a0ae2..8662fad7 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -1341,7 +1341,7 @@ cli_setprstatus(void * v, char ** reply, int * len, void * data)
 		return 1;

 	if (!mpp->prflag) {
-		mpp->prflag = 1;
+		mpp->prflag = PRFLAG_OK;
 		condlog(2, "%s: prflag set", param);
 	}

@@ -1352,9 +1352,11 @@ cli_setprstatus(void * v, char ** reply, int * len, void * data)
 int
 cli_unsetprstatus(void * v, char ** reply, int * len, void * data)
 {
-	struct multipath * mpp;
-	struct vectors * vecs = (struct vectors *)data;
-	char * param = get_keyparam(v, MAP);
+	int i;
+	struct multipath *mpp;
+	struct path *pp;
+	struct vectors *vecs = (struct vectors *)data;
+	char *param = get_keyparam(v, MAP);

 	param = convert_dev(param, 0);
 	get_path_layout(vecs->pathvec, 0);
@@ -1364,7 +1366,10 @@ cli_unsetprstatus(void * v, char ** reply, int * len, void * data)
 		return 1;

 	if (mpp->prflag) {
-		mpp->prflag = 0;
+		mpp->prflag = PRFLAG_NO;
+		vector_foreach_slot(mpp->paths, pp, i) {
+			pp->prflag = PRFLAG_NO;
+		}
 		condlog(2, "%s: prflag unset", param);
 	}

diff --git a/multipathd/main.c b/multipathd/main.c
index 82ab3ed1..6ef6495b 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -506,7 +506,7 @@ retry:

 	if (mpp->prflag) {
 		vector_foreach_slot(mpp->paths, pp, i) {
-			if ((pp->state == PATH_UP)  || (pp->state == PATH_GHOST)) {
+			if (!pp->prflag && ((pp->state == PATH_UP) || (pp->state == PATH_GHOST))) {
 				/* persistent reseravtion check*/
 				mpath_pr_event_handle(pp);
 			}
@@ -3570,7 +3570,8 @@ void *  mpath_pr_event_handler_fn (void * pathp )
 	{
 		condlog(0,"%s: Reservation registration failed. Error: %d", pp->dev, ret);
 	}
-	mpp->prflag = 1;
+	mpp->prflag = PRFLAG_OK;
+	pp->prflag = PRFLAG_OK;

 	free(param);
 out:
-- 

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

Re: [dm-devel] [PATCH 5/5] add prflag to path

[Martin Wilck]

On Tue, 2021-11-16 at 22:01 +0800, lixiaokeng wrote:
> The update_map will frequently be called and there will be
> unnecessary checks of reseravtion. We add prflag to path
> to avoid this.
> 
> The pp->state changes from others to up or ghost, the
> mpath_pr_event_handle should be called. The
> mpath_pr_event_handle in ev_add_path may not be called,
> so set pp->prkey PRKEY_NO when path is removed.
> 
> Fix: 4db4fa
> Signed-off-by: Lixiaokeng <lixiaokeng>
> ---
>  libmpathpersist/mpath_persist.c |  2 +-
>  libmultipath/structs.c          |  1 +
>  libmultipath/structs.h          | 12 ++++++++++++
>  multipathd/cli_handlers.c       | 15 ++++++++++-----
>  multipathd/main.c               |  5 +++--
>  5 files changed, 27 insertions(+), 8 deletions(-)
> 
> diff --git a/libmpathpersist/mpath_persist.c
> b/libmpathpersist/mpath_persist.c
> index 803a2a28..f88a2e89 100644
> --- a/libmpathpersist/mpath_persist.c
> +++ b/libmpathpersist/mpath_persist.c
> 
> +/*
> + * PRFLAG_NO for path, it means reservation should be checked.
> + * PRFLAG_NO for multipath, it means mpp has no prkey.
> + * PRFLAG_OK for path, it means reservation has been checked.
> + * PRFLAG_OK for multipath, it means mpp has prkey.
> + */

Please don't use the same enum with different meanings in different
context.

Martin


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

Re: [dm-devel] [PATCH 5/5] add prflag to path

[Benjamin Marzinski]

On Tue, Nov 16, 2021 at 10:01:15PM +0800, lixiaokeng wrote:
> The update_map will frequently be called and there will be
> unnecessary checks of reseravtion. We add prflag to path
> to avoid this.
> 
> The pp->state changes from others to up or ghost, the
> mpath_pr_event_handle should be called. The
> mpath_pr_event_handle in ev_add_path may not be called,
> so set pp->prkey PRKEY_NO when path is removed.

This patch kind of confuses me.  You only check pp->prkey before calling
mpath_pr_event_handle() in update_map(). I get from your commit message
that you are doing this to keep from frequent, unnecessary calls. But
isn't update_map() only called when a multipath device is first created,
or when multipathd stops waiting for something that it noticed during
device creation? I don't see how this can be frequently called on a
multipath device. What am I missing?

-Ben

> 
> Fix: 4db4fa
> Signed-off-by: Lixiaokeng <lixiaokeng>
> ---
>  libmpathpersist/mpath_persist.c |  2 +-
>  libmultipath/structs.c          |  1 +
>  libmultipath/structs.h          | 12 ++++++++++++
>  multipathd/cli_handlers.c       | 15 ++++++++++-----
>  multipathd/main.c               |  5 +++--
>  5 files changed, 27 insertions(+), 8 deletions(-)
> 
> diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c
> index 803a2a28..f88a2e89 100644
> --- a/libmpathpersist/mpath_persist.c
> +++ b/libmpathpersist/mpath_persist.c
> @@ -924,7 +924,7 @@ int update_map_pr(struct multipath *mpp)
> 
>  	if (isFound)
>  	{
> -		mpp->prflag = 1;
> +		mpp->prflag = PRFLAG_OK;
>  		condlog(2, "%s: prflag flag set.", mpp->alias );
>  	}
> 
> diff --git a/libmultipath/structs.c b/libmultipath/structs.c
> index e8cacb4b..82dbd565 100644
> --- a/libmultipath/structs.c
> +++ b/libmultipath/structs.c
> @@ -122,6 +122,7 @@ uninitialize_path(struct path *pp)
>  	pp->dmstate = PSTATE_UNDEF;
>  	pp->uid_attribute = NULL;
>  	pp->getuid = NULL;
> +	pp->prflag = PRFLAG_NO;
> 
>  	if (checker_selected(&pp->checker))
>  		checker_put(&pp->checker);
> diff --git a/libmultipath/structs.h b/libmultipath/structs.h
> index 399540e7..5b77218b 100644
> --- a/libmultipath/structs.h
> +++ b/libmultipath/structs.h
> @@ -249,6 +249,17 @@ enum recheck_wwid_states {
>  	RECHECK_WWID_ON = YNU_YES,
>  };
> 
> +/*
> + * PRFLAG_NO for path, it means reservation should be checked.
> + * PRFLAG_NO for multipath, it means mpp has no prkey.
> + * PRFLAG_OK for path, it means reservation has been checked.
> + * PRFLAG_OK for multipath, it means mpp has prkey.
> + */
> +enum prflag_states {
> +	PRFLAG_NO = 0,
> +	PRFLAG_OK = 1,
> +};
> +
>  struct vpd_vendor_page {
>  	int pg;
>  	const char *name;
> @@ -327,6 +338,7 @@ struct path {
>  	/* configlet pointers */
>  	vector hwe;
>  	struct gen_path generic_path;
> +	int prflag;
>  };
> 
>  typedef int (pgpolicyfn) (struct multipath *, vector);
> diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> index 6d3a0ae2..8662fad7 100644
> --- a/multipathd/cli_handlers.c
> +++ b/multipathd/cli_handlers.c
> @@ -1341,7 +1341,7 @@ cli_setprstatus(void * v, char ** reply, int * len, void * data)
>  		return 1;
> 
>  	if (!mpp->prflag) {
> -		mpp->prflag = 1;
> +		mpp->prflag = PRFLAG_OK;
>  		condlog(2, "%s: prflag set", param);
>  	}
> 
> @@ -1352,9 +1352,11 @@ cli_setprstatus(void * v, char ** reply, int * len, void * data)
>  int
>  cli_unsetprstatus(void * v, char ** reply, int * len, void * data)
>  {
> -	struct multipath * mpp;
> -	struct vectors * vecs = (struct vectors *)data;
> -	char * param = get_keyparam(v, MAP);
> +	int i;
> +	struct multipath *mpp;
> +	struct path *pp;
> +	struct vectors *vecs = (struct vectors *)data;
> +	char *param = get_keyparam(v, MAP);
> 
>  	param = convert_dev(param, 0);
>  	get_path_layout(vecs->pathvec, 0);
> @@ -1364,7 +1366,10 @@ cli_unsetprstatus(void * v, char ** reply, int * len, void * data)
>  		return 1;
> 
>  	if (mpp->prflag) {
> -		mpp->prflag = 0;
> +		mpp->prflag = PRFLAG_NO;
> +		vector_foreach_slot(mpp->paths, pp, i) {
> +			pp->prflag = PRFLAG_NO;
> +		}
>  		condlog(2, "%s: prflag unset", param);
>  	}
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 82ab3ed1..6ef6495b 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -506,7 +506,7 @@ retry:
> 
>  	if (mpp->prflag) {
>  		vector_foreach_slot(mpp->paths, pp, i) {
> -			if ((pp->state == PATH_UP)  || (pp->state == PATH_GHOST)) {
> +			if (!pp->prflag && ((pp->state == PATH_UP) || (pp->state == PATH_GHOST))) {
>  				/* persistent reseravtion check*/
>  				mpath_pr_event_handle(pp);
>  			}
> @@ -3570,7 +3570,8 @@ void *  mpath_pr_event_handler_fn (void * pathp )
>  	{
>  		condlog(0,"%s: Reservation registration failed. Error: %d", pp->dev, ret);
>  	}
> -	mpp->prflag = 1;
> +	mpp->prflag = PRFLAG_OK;
> +	pp->prflag = PRFLAG_OK;
> 
>  	free(param);
>  out:
> -- 

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

Re: [dm-devel] [PATCH 5/5] add prflag to path

[lixiaokeng]

On 2021/11/19 0:57, Benjamin Marzinski wrote:
> On Tue, Nov 16, 2021 at 10:01:15PM +0800, lixiaokeng wrote:
>> The update_map will frequently be called and there will be
>> unnecessary checks of reseravtion. We add prflag to path
>> to avoid this.
>>
>> The pp->state changes from others to up or ghost, the
>> mpath_pr_event_handle should be called. The
>> mpath_pr_event_handle in ev_add_path may not be called,
>> so set pp->prkey PRKEY_NO when path is removed.
> 
> This patch kind of confuses me.  You only check pp->prkey before calling
> mpath_pr_event_handle() in update_map(). I get from your commit message
> that you are doing this to keep from frequent, unnecessary calls. But
> isn't update_map() only called when a multipath device is first created,
> or when multipathd stops waiting for something that it noticed during
> device creation? I don't see how this can be frequently called on a
> multipath device. What am I missing?
> 
> -Ben
> 

Discussed in the previous patch (multipathd: fix missing persistent
reseravtion for active path) as follows:

On Mon, 2021-09-13 at 10:32 -0500, Benjamin Marzinski wrote:
> On Mon, Sep 13, 2021 at 09:01:11AM +0200, Martin Wilck wrote:
>> Hello lixiaokeng,
>>
>> On Mon, 2021-09-13 at 10:43 +0800, lixiaokeng wrote:
>>> There are two paths(sucu as sda and adb) for one LUN. The two
>>> paths log in, but before the two uevents have been processed
>>> (for example there are many uevent), users use multipathd add
>>> path /dev/sda to cause mpatha and use mpathpersist -o -I to
>>> register prkey for mpatha. The add map uevent is after add path
>>> uevent, the the uevent(add sdb) will delay and missing persistent
>>> reseravtion check.
>>>
>>> Here, we add persistent reseravtion check in update_map() which
>>> is called ev_add_map().
>>>
>>> Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
>>
>> Thank you, this looks ok to me. Have you tested it?
>>
>> I'll wait for Ben's opinion nonetheless, because he's more
>> exprerienced
>> with this part of the code than myself.
>>
>> This said, I would like to have multipathd record which paths have
>> already registered the key, to avoid doing that repeatedly.
>>
> Other than adding this, the patch looks fine.

There may be some mistakes. I mistakenly thought update_map would
be called multiple times

In check_path, mpath_pr_event_handle is only called pp->state
changes from others to up. Some prkey changes may happen when
pp->state is down, so mpath_pr_event_handle should be called
even pp->prkey is PRKEY_OK.

As Ben said, update_map will not be called repetitively. I think
this patch should be removed.

Thank Ben and Martin.

Regards,
Lixiaokeng

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