[PATCH 0/6] multipath-tools series: some cleanups and fixes

[PATCH 0/6] multipath-tools series: some cleanups and fixes

[Zhiqiang Liu]

When I learn and review the multipath-tools source code, I find
some cleanups and fixes.

repo: openSUSE/multipath-tools
repo link: https://github.com/openSUSE/multipath-tools
branch: upstream-queue

ZhiqiangLiu (6):
  checker: remove useless name check in checker_get func
  multipathd: adopt static char* arr in daemon_status
  multipathd: adopt static char* arr in sd_notify_status func
  libmultipath: check blist before calling MALLOC in alloc_ble_device
    func
  vector: add lower boundary check in vector_foreach_slot_after
  multipathd: return NULL if MALLOC fails in alloc_waiteri func

 libmultipath/blacklist.c |  8 +++++--
 libmultipath/checkers.c  |  9 ++++----
 libmultipath/vector.h    |  6 +++---
 multipathd/main.c        | 56 +++++++++++++++++++++++-------------------------
 multipathd/main.h        |  3 ++-
 multipathd/waiter.c      |  4 +++-
 6 files changed, 45 insertions(+), 41 deletions(-)

-- 
1.8.3.1

[PATCH 1/6] checker: remove useless name check in checker_get func

[Zhiqiang Liu]

In checker_get func, input name will be checked before
calling checker_class_lookup func, in which name will
also be checked.

Here, we just remove useless input name check in checker_get func.

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

diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c
index f7ddd53..ac41d64 100644
--- a/libmultipath/checkers.c
+++ b/libmultipath/checkers.c
@@ -361,11 +361,10 @@ void checker_get(const char *multipath_dir, struct checker *dst,
 	if (!dst)
 		return;

-	if (name && strlen(name)) {
-		src = checker_class_lookup(name);
-		if (!src)
-			src = add_checker_class(multipath_dir, name);
-	}
+	src = checker_class_lookup(name);
+	if (!src)
+		src = add_checker_class(multipath_dir, name);
+
 	dst->cls = src;
 	if (!src)
 		return;
-- 
1.8.3.1

[PATCH 2/6] multipathd: adopt static char* arr in daemon_status

[Zhiqiang Liu]

We adopt static char* array (demon_status_msg) in daemon_status func,
so it looks more simpler and easier to expand.

Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
---
 multipathd/main.c | 30 +++++++++++++++---------------
 multipathd/main.h |  3 ++-
 2 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 9ec6585..cab1d0d 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -153,24 +153,24 @@ static volatile sig_atomic_t exit_sig;
 static volatile sig_atomic_t reconfig_sig;
 static volatile sig_atomic_t log_reset_sig;

+static const char *demon_status_msg[DAEMON_STATUS_SIZE] = {
+	[DAEMON_INIT] = "init",
+	[DAEMON_START] = "startup",
+	[DAEMON_CONFIGURE] = "configure",
+	[DAEMON_IDLE] = "idle",
+	[DAEMON_RUNNING] = "running",
+	[DAEMON_SHUTDOWN] = "shutdown",
+};
+
 const char *
 daemon_status(void)
 {
-	switch (get_running_state()) {
-	case DAEMON_INIT:
-		return "init";
-	case DAEMON_START:
-		return "startup";
-	case DAEMON_CONFIGURE:
-		return "configure";
-	case DAEMON_IDLE:
-		return "idle";
-	case DAEMON_RUNNING:
-		return "running";
-	case DAEMON_SHUTDOWN:
-		return "shutdown";
-	}
-	return NULL;
+	enum daemon_status status = get_running_state();
+
+	if (status < DAEMON_INIT || status >= DAEMON_STATUS_SIZE)
+		return NULL;
+
+	return demon_status_msg[status];
 }

 /*
diff --git a/multipathd/main.h b/multipathd/main.h
index 5dff17e..6a5592c 100644
--- a/multipathd/main.h
+++ b/multipathd/main.h
@@ -4,12 +4,13 @@
 #define MAPGCINT 5

 enum daemon_status {
-	DAEMON_INIT,
+	DAEMON_INIT = 0,
 	DAEMON_START,
 	DAEMON_CONFIGURE,
 	DAEMON_IDLE,
 	DAEMON_RUNNING,
 	DAEMON_SHUTDOWN,
+	DAEMON_STATUS_SIZE,
 };

 struct prout_param_descriptor;
-- 
1.8.3.1

[PATCH 3/6] multipathd: adopt static char* arr in sd_notify_status func

[Zhiqiang Liu]

We adopt static char* array (sd_notify_status_msg) in
sd_notify_status func, so it looks more simpler and easier
to expand.

Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
---
 multipathd/main.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index cab1d0d..a09ccd1 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -177,23 +177,21 @@ daemon_status(void)
  * I love you too, systemd ...
  */
 #ifdef USE_SYSTEMD
+static const char *sd_notify_status_msg[DAEMON_STATUS_SIZE] = {
+	[DAEMON_INIT] = "STATUS=init",
+	[DAEMON_START] = "STATUS=startup",
+	[DAEMON_CONFIGURE] = "STATUS=configure",
+	[DAEMON_IDLE] = "STATUS=up",
+	[DAEMON_RUNNING] = "STATUS=up",
+	[DAEMON_SHUTDOWN] = "STATUS=shutdown",
+};
+
 static const char *
 sd_notify_status(enum daemon_status state)
 {
-	switch (state) {
-	case DAEMON_INIT:
-		return "STATUS=init";
-	case DAEMON_START:
-		return "STATUS=startup";
-	case DAEMON_CONFIGURE:
-		return "STATUS=configure";
-	case DAEMON_IDLE:
-	case DAEMON_RUNNING:
-		return "STATUS=up";
-	case DAEMON_SHUTDOWN:
-		return "STATUS=shutdown";
-	}
-	return NULL;
+	if (state < DAEMON_INIT || state >= DAEMON_STATUS_SIZE)
+		return NULL;
+	return sd_notify_status_msg[state];
 }

 static void do_sd_notify(enum daemon_status old_state,
-- 
1.8.3.1

[PATCH 4/6] libmultipath: check blist before calling MALLOC in alloc_ble_device func

[Zhiqiang Liu]

In alloc_ble_device func, ble is firstly allocated by calling MALLOC,
and then input blist is checked whether it is valid. If blist is not
valid, ble will be freed without using.

Here, we should check blist firstly.

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

diff --git a/libmultipath/blacklist.c b/libmultipath/blacklist.c
index db58ccc..bedcc7e 100644
--- a/libmultipath/blacklist.c
+++ b/libmultipath/blacklist.c
@@ -66,12 +66,16 @@ out:

 int alloc_ble_device(vector blist)
 {
-	struct blentry_device * ble = MALLOC(sizeof(struct blentry_device));
+	struct blentry_device *ble;

+	if (!blist)
+		return 1;
+
+	ble = MALLOC(sizeof(struct blentry_device));
 	if (!ble)
 		return 1;

-	if (!blist || !vector_alloc_slot(blist)) {
+	if (!vector_alloc_slot(blist)) {
 		FREE(ble);
 		return 1;
 	}
-- 
1.8.3.1

[PATCH 5/6] vector: add lower boundary check in vector_foreach_slot_after

[Zhiqiang Liu]

In vector_foreach_slot_after macro, i is the input var, which
may have a illegal value (i < 0). So we should add lower boundary
check in vector_foreach_slot_after macro.

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

diff --git a/libmultipath/vector.h b/libmultipath/vector.h
index 2862dc2..45dbfc1 100644
--- a/libmultipath/vector.h
+++ b/libmultipath/vector.h
@@ -38,11 +38,11 @@ typedef struct _vector *vector;
 #define VECTOR_LAST_SLOT(V)   (((V) && VECTOR_SIZE(V) > 0) ? (V)->slot[(VECTOR_SIZE(V) - 1)] : NULL)

 #define vector_foreach_slot(v,p,i) \
-	for (i = 0; (v) && (int)i < VECTOR_SIZE(v) && ((p) = (v)->slot[i]); i++)
+	for ((i) = 0; (v) && (int)(i) < VECTOR_SIZE(v) && ((p) = (v)->slot[i]); (i)++)
 #define vector_foreach_slot_after(v,p,i) \
-	for (; (v) && (int)i < VECTOR_SIZE(v) && ((p) = (v)->slot[i]); i++)
+	for (; (v) && (int)(i) < VECTOR_SIZE(v) && (int)(i) >= 0 && ((p) = (v)->slot[i]); (i)++)
 #define vector_foreach_slot_backwards(v,p,i) \
-	for (i = VECTOR_SIZE(v) - 1; (int)i >= 0 && ((p) = (v)->slot[i]); i--)
+	for ((i) = VECTOR_SIZE(v) - 1; (int)(i) >= 0 && ((p) = (v)->slot[i]); (i)--)

 #define identity(x) (x)
 /*
-- 
1.8.3.1

[PATCH 6/6] multipathd: return NULL if MALLOC fails in alloc_waiteri, func

[Zhiqiang Liu]

In alloc_waiter func, if MALLOC fails, wp is equal to NULL.
If now we call memset(wp), segmentation fault will occur.

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

diff --git a/multipathd/waiter.c b/multipathd/waiter.c
index e645766..80e6e6e 100644
--- a/multipathd/waiter.c
+++ b/multipathd/waiter.c
@@ -33,8 +33,10 @@ 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));
+	if (!wp)
+		return NULL;

+	memset(wp, 0, sizeof(struct event_thread));
 	return wp;
 }

-- 
1.8.3.1

Re: [PATCH 1/6] checker: remove useless name check in checker_get func

[Martin Wilck]

On Sun, 2020-08-16 at 09:42 +0800, Zhiqiang Liu wrote:
> In checker_get func, input name will be checked before
> calling checker_class_lookup func, in which name will
> also be checked.
> 
> Here, we just remove useless input name check in checker_get func.
> 
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
> ---
>  libmultipath/checkers.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)

Nack. Better safe than sorry. If you look at the generated assembly,
you'll see that the compiler optimizes this double check away, so
doesn't inflict runtime overhead, while it makes it easier to verify
the code.

I'd ack this if we had a solid convention in the multipath-tools code
to check parameters always (only) in the callee. But so far we don't.
I guess if we want to do that, we'd first need to agree on and
implement a common convention for return values, too.

If checker_class_lookup() was non-static and/or the code was executed
very often, things would also look different to me. But both is not the
case.

Regards,
Martin



> 
> diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c
> index f7ddd53..ac41d64 100644
> --- a/libmultipath/checkers.c
> +++ b/libmultipath/checkers.c
> @@ -361,11 +361,10 @@ void checker_get(const char *multipath_dir,
> struct checker *dst,
>  	if (!dst)
>  		return;
> 
> -	if (name && strlen(name)) {
> -		src = checker_class_lookup(name);
> -		if (!src)
> -			src = add_checker_class(multipath_dir, name);
> -	}
> +	src = checker_class_lookup(name);
> +	if (!src)
> +		src = add_checker_class(multipath_dir, name);
> +
>  	dst->cls = src;
>  	if (!src)
>  		return;

Re: [PATCH 2/6] multipathd: adopt static char* arr in daemon_status

[Martin Wilck]

On Sun, 2020-08-16 at 09:42 +0800, Zhiqiang Liu wrote:
> We adopt static char* array (demon_status_msg) in daemon_status func,
> so it looks more simpler and easier to expand.
> 
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
> ---
>  multipathd/main.c | 30 +++++++++++++++---------------
>  multipathd/main.h |  3 ++-
>  2 files changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 9ec6585..cab1d0d 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -153,24 +153,24 @@ static volatile sig_atomic_t exit_sig;
>  static volatile sig_atomic_t reconfig_sig;
>  static volatile sig_atomic_t log_reset_sig;
> 
> +static const char *demon_status_msg[DAEMON_STATUS_SIZE] = {
> +	[DAEMON_INIT] = "init",
> +	[DAEMON_START] = "startup",
> +	[DAEMON_CONFIGURE] = "configure",
> +	[DAEMON_IDLE] = "idle",
> +	[DAEMON_RUNNING] = "running",
> +	[DAEMON_SHUTDOWN] = "shutdown",
> +};
> +
>  const char *
>  daemon_status(void)
>  {
> -	switch (get_running_state()) {
> -	case DAEMON_INIT:
> -		return "init";
> -	case DAEMON_START:
> -		return "startup";
> -	case DAEMON_CONFIGURE:
> -		return "configure";
> -	case DAEMON_IDLE:
> -		return "idle";
> -	case DAEMON_RUNNING:
> -		return "running";
> -	case DAEMON_SHUTDOWN:
> -		return "shutdown";
> -	}
> -	return NULL;
> +	enum daemon_status status = get_running_state();
> +
> +	if (status < DAEMON_INIT || status >= DAEMON_STATUS_SIZE)
> +		return NULL;

I'd rather use "int" as the type of "status", because (not technically,
but logically), an "enum daemon_status" cannot be outside this range.

Martin

Re: [PATCH 3/6] multipathd: adopt static char* arr in sd_notify_status func

[Martin Wilck]

On Sun, 2020-08-16 at 09:44 +0800, Zhiqiang Liu wrote:
> We adopt static char* array (sd_notify_status_msg) in
> sd_notify_status func, so it looks more simpler and easier
> to expand.
> 
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
> ---
>  multipathd/main.c | 26 ++++++++++++--------------
>  1 file changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index cab1d0d..a09ccd1 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -177,23 +177,21 @@ daemon_status(void)
>   * I love you too, systemd ...
>   */
>  #ifdef USE_SYSTEMD
> +static const char *sd_notify_status_msg[DAEMON_STATUS_SIZE] = {
> +	[DAEMON_INIT] = "STATUS=init",
> +	[DAEMON_START] = "STATUS=startup",
> +	[DAEMON_CONFIGURE] = "STATUS=configure",
> +	[DAEMON_IDLE] = "STATUS=up",
> +	[DAEMON_RUNNING] = "STATUS=up",
> +	[DAEMON_SHUTDOWN] = "STATUS=shutdown",
> +};
> +

This repetition of "STATUS=" looks clumsy. It's not your fault, because
the current code does the same thing. But if you want to clean this up,
please create the notification string in a dynamic buffer, and use
daemon_status() for those cases where it applies.

Regards
Martin

>  static const char *
>  sd_notify_status(enum daemon_status state)
>  {
> -	switch (state) {
> -	case DAEMON_INIT:
> -		return "STATUS=init";
> -	case DAEMON_START:
> -		return "STATUS=startup";
> -	case DAEMON_CONFIGURE:
> -		return "STATUS=configure";
> -	case DAEMON_IDLE:
> -	case DAEMON_RUNNING:
> -		return "STATUS=up";
> -	case DAEMON_SHUTDOWN:
> -		return "STATUS=shutdown";
> -	}
> -	return NULL;
> +	if (state < DAEMON_INIT || state >= DAEMON_STATUS_SIZE)
> +		return NULL;
> +	return sd_notify_status_msg[state];
>  }
> 
>  static void do_sd_notify(enum daemon_status old_state,

Re: [PATCH 4/6] libmultipath: check blist before calling MALLOC in alloc_ble_device func

[Martin Wilck]

On Sun, 2020-08-16 at 09:45 +0800, Zhiqiang Liu wrote:
> In alloc_ble_device func, ble is firstly allocated by calling MALLOC,
> and then input blist is checked whether it is valid. If blist is not
> valid, ble will be freed without using.
> 
> Here, we should check blist firstly.
> 
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
> ---
>  libmultipath/blacklist.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)

This patch isn't wrong, but it fixes code which isn't buggy. It's
rather a style thing, an optimization for an extremely unlikely error
case. I agree with you in the sense that I prefer the "new" style over
the old (I generally dislike expressions that can fail, like malloc()
calls, being used as variable initializers), but I'm not sure if we
should start applying patches for cases like this. So far we've been
rather conservative with "style" patches, because they tend to make it
unnecessarily hard to track code history.

Ben, Christophe, what's your take on this matter?

Regards,
Martin


> 
> diff --git a/libmultipath/blacklist.c b/libmultipath/blacklist.c
> index db58ccc..bedcc7e 100644
> --- a/libmultipath/blacklist.c
> +++ b/libmultipath/blacklist.c
> @@ -66,12 +66,16 @@ out:
> 
>  int alloc_ble_device(vector blist)
>  {
> -	struct blentry_device * ble = MALLOC(sizeof(struct
> blentry_device));
> +	struct blentry_device *ble;
> 
> +	if (!blist)
> +		return 1;
> +
> +	ble = MALLOC(sizeof(struct blentry_device));
>  	if (!ble)
>  		return 1;
> 
> -	if (!blist || !vector_alloc_slot(blist)) {
> +	if (!vector_alloc_slot(blist)) {
>  		FREE(ble);
>  		return 1;
>  	}

Re: [PATCH 5/6] vector: add lower boundary check in vector_foreach_slot_after

[Martin Wilck]

On Sun, 2020-08-16 at 09:45 +0800, Zhiqiang Liu wrote:
> In vector_foreach_slot_after macro, i is the input var, which
> may have a illegal value (i < 0). So we should add lower boundary
> check in vector_foreach_slot_after macro.
> 
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
> ---
>  libmultipath/vector.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Perhaps we should write this instead? I'm unsure if compilers can
optimize away the repeated extra check in all cases.

#define vector_foreach_slot_after(v,p,i) \
	if ((v) && (int)(i) >= 0)        \
		for (; (int)(i) < VECTOR_SIZE(v) && ((p) = (v)->slot[i]); (i)++)

Regards,
Martin

> 
> diff --git a/libmultipath/vector.h b/libmultipath/vector.h
> index 2862dc2..45dbfc1 100644
> --- a/libmultipath/vector.h
> +++ b/libmultipath/vector.h
> @@ -38,11 +38,11 @@ typedef struct _vector *vector;
>  #define VECTOR_LAST_SLOT(V)   (((V) && VECTOR_SIZE(V) > 0) ? (V)-
> >slot[(VECTOR_SIZE(V) - 1)] : NULL)
> 
>  #define vector_foreach_slot(v,p,i) \
> -	for (i = 0; (v) && (int)i < VECTOR_SIZE(v) && ((p) = (v)-
> >slot[i]); i++)
> +	for ((i) = 0; (v) && (int)(i) < VECTOR_SIZE(v) && ((p) = (v)-
> >slot[i]); (i)++)
>  #define vector_foreach_slot_after(v,p,i) \
> -	for (; (v) && (int)i < VECTOR_SIZE(v) && ((p) = (v)->slot[i]);
> i++)
> +	for (; (v) && (int)(i) < VECTOR_SIZE(v) && (int)(i) >= 0 &&
> ((p) = (v)->slot[i]); (i)++)
>  #define vector_foreach_slot_backwards(v,p,i) \
> -	for (i = VECTOR_SIZE(v) - 1; (int)i >= 0 && ((p) = (v)-
> >slot[i]); i--)
> +	for ((i) = VECTOR_SIZE(v) - 1; (int)(i) >= 0 && ((p) = (v)-
> >slot[i]); (i)--)
> 
>  #define identity(x) (x)
>  /*

Re: [PATCH 6/6] multipathd: return NULL if MALLOC fails in alloc_waiteri, func

[Martin Wilck]

On Sun, 2020-08-16 at 09:46 +0800, Zhiqiang Liu wrote:
> In alloc_waiter func, if MALLOC fails, wp is equal to NULL.
> If now we call memset(wp), segmentation fault will occur.
> 
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
> ---
>  multipathd/waiter.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/multipathd/waiter.c b/multipathd/waiter.c
> index e645766..80e6e6e 100644
> --- a/multipathd/waiter.c
> +++ b/multipathd/waiter.c
> @@ -33,8 +33,10 @@ 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));
> +	if (!wp)
> +		return NULL;
> 
> +	memset(wp, 0, sizeof(struct event_thread));
>  	return wp;
>  }
> 

This is correct, but while you're at it, please also remove the
superfluous memset() call (counterintuitively, MALLOC(x) maps to
calloc(1, x)).

Martin

Re: [PATCH 6/6] multipathd: return NULL if MALLOC fails in alloc_waiteri, func

[Zhiqiang Liu]

On 2020/8/17 17:21, Martin Wilck wrote:
> On Sun, 2020-08-16 at 09:46 +0800, Zhiqiang Liu wrote:
>> In alloc_waiter func, if MALLOC fails, wp is equal to NULL.
>> If now we call memset(wp), segmentation fault will occur.
>>
>> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
>> Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
>> ---
>>  multipathd/waiter.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/multipathd/waiter.c b/multipathd/waiter.c
>> index e645766..80e6e6e 100644
>> --- a/multipathd/waiter.c
>> +++ b/multipathd/waiter.c
>> @@ -33,8 +33,10 @@ 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));
>> +	if (!wp)
>> +		return NULL;
>>
>> +	memset(wp, 0, sizeof(struct event_thread));
>>  	return wp;
>>  }
>>
> 
> This is correct, but while you're at it, please also remove the
> superfluous memset() call (counterintuitively, MALLOC(x) maps to
> calloc(1, x)).
> 
> Martin
> 
Thanks for your suggestion.
I will remove the memset() call in v2 patch.

Zhiqiang
> 
> 
> .
> 

Re: [PATCH 1/6] checker: remove useless name check in checker_get func

[Zhiqiang Liu]

On 2020/8/17 16:05, Martin Wilck wrote:
> On Sun, 2020-08-16 at 09:42 +0800, Zhiqiang Liu wrote:
>> In checker_get func, input name will be checked before
>> calling checker_class_lookup func, in which name will
>> also be checked.
>>
>> Here, we just remove useless input name check in checker_get func.
>>
>> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
>> Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
>> ---
>>  libmultipath/checkers.c | 9 ++++-----
>>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> Nack. Better safe than sorry. If you look at the generated assembly,
> you'll see that the compiler optimizes this double check away, so
> doesn't inflict runtime overhead, while it makes it easier to verify
> the code.
> 
> I'd ack this if we had a solid convention in the multipath-tools code
> to check parameters always (only) in the callee. But so far we don't.
> I guess if we want to do that, we'd first need to agree on and
> implement a common convention for return values, too.
> 
I agree with you.
The location of the parameter check is not uniform.
Are we dealing with it now? or add it in your TODO list?


> If checker_class_lookup() was non-static and/or the code was executed
> very often, things would also look different to me. But both is not the
> case.
> 



> Regards,
> Martin
> 
> 
> 
>>
>> diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c
>> index f7ddd53..ac41d64 100644
>> --- a/libmultipath/checkers.c
>> +++ b/libmultipath/checkers.c
>> @@ -361,11 +361,10 @@ void checker_get(const char *multipath_dir,
>> struct checker *dst,
>>  	if (!dst)
>>  		return;
>>
>> -	if (name && strlen(name)) {
>> -		src = checker_class_lookup(name);
>> -		if (!src)
>> -			src = add_checker_class(multipath_dir, name);
>> -	}
>> +	src = checker_class_lookup(name);
>> +	if (!src)
>> +		src = add_checker_class(multipath_dir, name);
>> +
>>  	dst->cls = src;
>>  	if (!src)
>>  		return;
> 
> 
> .
> 

Re: [PATCH 2/6] multipathd: adopt static char* arr in daemon_status

[Zhiqiang Liu]

On 2020/8/17 16:23, Martin Wilck wrote:
> On Sun, 2020-08-16 at 09:42 +0800, Zhiqiang Liu wrote:
>> We adopt static char* array (demon_status_msg) in daemon_status func,
>> so it looks more simpler and easier to expand.
>>
>> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
>> Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
>> ---
>>  multipathd/main.c | 30 +++++++++++++++---------------
>>  multipathd/main.h |  3 ++-
>>  2 files changed, 17 insertions(+), 16 deletions(-)
>>
>> diff --git a/multipathd/main.c b/multipathd/main.c
>> index 9ec6585..cab1d0d 100644
>> --- a/multipathd/main.c
>> +++ b/multipathd/main.c
>> @@ -153,24 +153,24 @@ static volatile sig_atomic_t exit_sig;
>>  static volatile sig_atomic_t reconfig_sig;
>>  static volatile sig_atomic_t log_reset_sig;
>>
>> +static const char *demon_status_msg[DAEMON_STATUS_SIZE] = {
>> +	[DAEMON_INIT] = "init",
>> +	[DAEMON_START] = "startup",
>> +	[DAEMON_CONFIGURE] = "configure",
>> +	[DAEMON_IDLE] = "idle",
>> +	[DAEMON_RUNNING] = "running",
>> +	[DAEMON_SHUTDOWN] = "shutdown",
>> +};
>> +
>>  const char *
>>  daemon_status(void)
>>  {
>> -	switch (get_running_state()) {
>> -	case DAEMON_INIT:
>> -		return "init";
>> -	case DAEMON_START:
>> -		return "startup";
>> -	case DAEMON_CONFIGURE:
>> -		return "configure";
>> -	case DAEMON_IDLE:
>> -		return "idle";
>> -	case DAEMON_RUNNING:
>> -		return "running";
>> -	case DAEMON_SHUTDOWN:
>> -		return "shutdown";
>> -	}
>> -	return NULL;
>> +	enum daemon_status status = get_running_state();
>> +
>> +	if (status < DAEMON_INIT || status >= DAEMON_STATUS_SIZE)
>> +		return NULL;
> 
> I'd rather use "int" as the type of "status", because (not technically,
> but logically), an "enum daemon_status" cannot be outside this range.
> 
> Martin
> 
Thanks for your suggestion.
I will use 'int' as the type of 'status' in v2 patch.

Zhiqiang Liu
> 
> 
> .
> 

Re: [PATCH 1/6] checker: remove useless name check in checker_get func

[Martin Wilck]

On Mon, 2020-08-17 at 23:12 +0800, Zhiqiang Liu wrote:
> On 2020/8/17 16:05, Martin Wilck wrote:
> > On Sun, 2020-08-16 at 09:42 +0800, Zhiqiang Liu wrote:
> > > In checker_get func, input name will be checked before
> > > calling checker_class_lookup func, in which name will
> > > also be checked.
> > > 
> > > Here, we just remove useless input name check in checker_get
> > > func.
> > > 
> > > Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> > > Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
> > > ---
> > >  libmultipath/checkers.c | 9 ++++-----
> > >  1 file changed, 4 insertions(+), 5 deletions(-)
> > 
> > Nack. Better safe than sorry. If you look at the generated
> > assembly,
> > you'll see that the compiler optimizes this double check away, so
> > doesn't inflict runtime overhead, while it makes it easier to
> > verify
> > the code.
> > 
> > I'd ack this if we had a solid convention in the multipath-tools
> > code
> > to check parameters always (only) in the callee. But so far we
> > don't.
> > I guess if we want to do that, we'd first need to agree on and
> > implement a common convention for return values, too.
> > 
> I agree with you.
> The location of the parameter check is not uniform.
> Are we dealing with it now? or add it in your TODO list?

We don't have an official todo list. I tend to handle such "style"
things incrementally - when I touch some code for some non-trivial
reason, I also add "style" changes as appropriate (look at "constify"
changes in recent multipath-tools commits, for example).

I believe that most functions in multipath-tools already check the
passed arguments (in the callee), and I believe that this is where it
should be done. Let's have an eye on this for some time to come.
Eventually we can audit the entire code to make sure all functions do
this properly, and once that's done, we can start removing the
respective checks in callers, at least some.

Does that make sense?

Cheers,
Martin






> 
> 
> > If checker_class_lookup() was non-static and/or the code was
> > executed
> > very often, things would also look different to me. But both is not
> > the
> > case.
> > 
> 
> 
> > Regards,
> > Martin
> > 
> > 
> > 
> > > diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c
> > > index f7ddd53..ac41d64 100644
> > > --- a/libmultipath/checkers.c
> > > +++ b/libmultipath/checkers.c
> > > @@ -361,11 +361,10 @@ void checker_get(const char *multipath_dir,
> > > struct checker *dst,
> > >  	if (!dst)
> > >  		return;
> > > 
> > > -	if (name && strlen(name)) {
> > > -		src = checker_class_lookup(name);
> > > -		if (!src)
> > > -			src = add_checker_class(multipath_dir, name);
> > > -	}
> > > +	src = checker_class_lookup(name);
> > > +	if (!src)
> > > +		src = add_checker_class(multipath_dir, name);
> > > +
> > >  	dst->cls = src;
> > >  	if (!src)
> > >  		return;
> > 
> > .
> > 

Re: [PATCH 3/6] multipathd: adopt static char* arr in sd_notify_status func

[Zhiqiang Liu]

On 2020/8/17 16:35, Martin Wilck wrote:
> On Sun, 2020-08-16 at 09:44 +0800, Zhiqiang Liu wrote:
>> We adopt static char* array (sd_notify_status_msg) in
>> sd_notify_status func, so it looks more simpler and easier
>> to expand.
>>
>> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
>> Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
>> ---
>>  multipathd/main.c | 26 ++++++++++++--------------
>>  1 file changed, 12 insertions(+), 14 deletions(-)
>>
>> diff --git a/multipathd/main.c b/multipathd/main.c
>> index cab1d0d..a09ccd1 100644
>> --- a/multipathd/main.c
>> +++ b/multipathd/main.c
>> @@ -177,23 +177,21 @@ daemon_status(void)
>>   * I love you too, systemd ...
>>   */
>>  #ifdef USE_SYSTEMD
>> +static const char *sd_notify_status_msg[DAEMON_STATUS_SIZE] = {
>> +	[DAEMON_INIT] = "STATUS=init",
>> +	[DAEMON_START] = "STATUS=startup",
>> +	[DAEMON_CONFIGURE] = "STATUS=configure",
>> +	[DAEMON_IDLE] = "STATUS=up",
>> +	[DAEMON_RUNNING] = "STATUS=up",
>> +	[DAEMON_SHUTDOWN] = "STATUS=shutdown",
>> +};
>> +
> 
> This repetition of "STATUS=" looks clumsy. It's not your fault, because
> the current code does the same thing. But if you want to clean this up,
> please create the notification string in a dynamic buffer, and use
> daemon_status() for those cases where it applies.
> 
> Regards
> Martin
> 
Thanks for your reply.
Besides the prefixes "STATUS=", there are also some differences between
sd_notify_status_msg[DAEMON_IDLE|DAEMON_RUNNING] and
demon_status_msg[DAEMON_IDLE|DAEMON_RUNNING].

For example,
sd_notify_status_msg[DAEMON_RUNNING] = "STATUS=up",
demon_status_msg[DAEMON_RUNNING] = "running"

So if we create the notification string in a dynamic buffer with
using daemon_status, we have to make some special judgement
on DAEMON_RUNNING and DAEMON_IDLE status. This may be why
the sd_notify_status func was created.

We can implement both solutions. Martin, which one do you prefer?


Regards
Zhiqiang Liu


>>  static const char *
>>  sd_notify_status(enum daemon_status state)
>>  {
>> -	switch (state) {
>> -	case DAEMON_INIT:
>> -		return "STATUS=init";
>> -	case DAEMON_START:
>> -		return "STATUS=startup";
>> -	case DAEMON_CONFIGURE:
>> -		return "STATUS=configure";
>> -	case DAEMON_IDLE:
>> -	case DAEMON_RUNNING:
>> -		return "STATUS=up";
>> -	case DAEMON_SHUTDOWN:
>> -		return "STATUS=shutdown";
>> -	}
>> -	return NULL;
>> +	if (state < DAEMON_INIT || state >= DAEMON_STATUS_SIZE)
>> +		return NULL;
>> +	return sd_notify_status_msg[state];
>>  }
>>
>>  static void do_sd_notify(enum daemon_status old_state,
> 
> 
> 
> .
> 

Re: [PATCH 3/6] multipathd: adopt static char* arr in sd_notify_status func

[Martin Wilck]

On Mon, 2020-08-17 at 23:33 +0800, Zhiqiang Liu wrote:
> 
> On 2020/8/17 16:35, Martin Wilck wrote:
> > On Sun, 2020-08-16 at 09:44 +0800, Zhiqiang Liu wrote:
> > > We adopt static char* array (sd_notify_status_msg) in
> > > sd_notify_status func, so it looks more simpler and easier
> > > to expand.
> > > 
> > > Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> > > Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
> > > ---
> > >  multipathd/main.c | 26 ++++++++++++--------------
> > >  1 file changed, 12 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/multipathd/main.c b/multipathd/main.c
> > > index cab1d0d..a09ccd1 100644
> > > --- a/multipathd/main.c
> > > +++ b/multipathd/main.c
> > > @@ -177,23 +177,21 @@ daemon_status(void)
> > >   * I love you too, systemd ...
> > >   */
> > >  #ifdef USE_SYSTEMD
> > > +static const char *sd_notify_status_msg[DAEMON_STATUS_SIZE] = {
> > > +	[DAEMON_INIT] = "STATUS=init",
> > > +	[DAEMON_START] = "STATUS=startup",
> > > +	[DAEMON_CONFIGURE] = "STATUS=configure",
> > > +	[DAEMON_IDLE] = "STATUS=up",
> > > +	[DAEMON_RUNNING] = "STATUS=up",
> > > +	[DAEMON_SHUTDOWN] = "STATUS=shutdown",
> > > +};
> > > +
> > 
> > This repetition of "STATUS=" looks clumsy. It's not your fault,
> > because
> > the current code does the same thing. But if you want to clean this
> > up,
> > please create the notification string in a dynamic buffer, and use
> > daemon_status() for those cases where it applies.
> > 
> > Regards
> > Martin
> > 
> Thanks for your reply.
> Besides the prefixes "STATUS=", there are also some differences
> between
> sd_notify_status_msg[DAEMON_IDLE|DAEMON_RUNNING] and
> demon_status_msg[DAEMON_IDLE|DAEMON_RUNNING].
> 
> For example,
> sd_notify_status_msg[DAEMON_RUNNING] = "STATUS=up",
> demon_status_msg[DAEMON_RUNNING] = "running"
> 
> So if we create the notification string in a dynamic buffer with
> using daemon_status, we have to make some special judgement
> on DAEMON_RUNNING and DAEMON_IDLE status. This may be why
> the sd_notify_status func was created.
> 
> We can implement both solutions. Martin, which one do you prefer?

That's what I meant with "where it applies". You treat the IDLE and
RUNNING cases first (probably in a switch statement), and use
daemon_status() for the "default" case.

The  reason we don't differentiate between "running" and "idle" in the
sd_notify() code path is that the daemon switches between these states
very often (at least once per tick) and these notifications causes lots
of dbus traffic without much value.

Regards
Martin

Re: [PATCH 1/6] checker: remove useless name check in checker_get func

[Zhiqiang Liu]

On 2020/8/17 23:25, Martin Wilck wrote:
> On Mon, 2020-08-17 at 23:12 +0800, Zhiqiang Liu wrote:
>> On 2020/8/17 16:05, Martin Wilck wrote:
>>> On Sun, 2020-08-16 at 09:42 +0800, Zhiqiang Liu wrote:
>>>> In checker_get func, input name will be checked before
>>>> calling checker_class_lookup func, in which name will
>>>> also be checked.
>>>>
>>>> Here, we just remove useless input name check in checker_get
>>>> func.
>>>>
>>>> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
>>>> Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
>>>> ---
>>>>  libmultipath/checkers.c | 9 ++++-----
>>>>  1 file changed, 4 insertions(+), 5 deletions(-)
>>>
>>> Nack. Better safe than sorry. If you look at the generated
>>> assembly,
>>> you'll see that the compiler optimizes this double check away, so
>>> doesn't inflict runtime overhead, while it makes it easier to
>>> verify
>>> the code.
>>>
>>> I'd ack this if we had a solid convention in the multipath-tools
>>> code
>>> to check parameters always (only) in the callee. But so far we
>>> don't.
>>> I guess if we want to do that, we'd first need to agree on and
>>> implement a common convention for return values, too.
>>>
>> I agree with you.
>> The location of the parameter check is not uniform.
>> Are we dealing with it now? or add it in your TODO list?
> 
> We don't have an official todo list. I tend to handle such "style"
> things incrementally - when I touch some code for some non-trivial
> reason, I also add "style" changes as appropriate (look at "constify"
> changes in recent multipath-tools commits, for example).
> 
> I believe that most functions in multipath-tools already check the
> passed arguments (in the callee), and I believe that this is where it
> should be done. Let's have an eye on this for some time to come.
> Eventually we can audit the entire code to make sure all functions do
> this properly, and once that's done, we can start removing the
> respective checks in callers, at least some.
> 
> Does that make sense?
> 
> Cheers,
> Martin
> 

Thank you for your patience.
Because I am learning and reviewing multipath-tools code,
I'll be happy to push a patch to handle such "style" things.

Regards
Zhiqiang Liu


> 
> 
> 
> 
>>
>>
>>> If checker_class_lookup() was non-static and/or the code was
>>> executed
>>> very often, things would also look different to me. But both is not
>>> the
>>> case.
>>>
>>
>>
>>> Regards,
>>> Martin
>>>
>>>
>>>
>>>> diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c
>>>> index f7ddd53..ac41d64 100644
>>>> --- a/libmultipath/checkers.c
>>>> +++ b/libmultipath/checkers.c
>>>> @@ -361,11 +361,10 @@ void checker_get(const char *multipath_dir,
>>>> struct checker *dst,
>>>>  	if (!dst)
>>>>  		return;
>>>>
>>>> -	if (name && strlen(name)) {
>>>> -		src = checker_class_lookup(name);
>>>> -		if (!src)
>>>> -			src = add_checker_class(multipath_dir, name);
>>>> -	}
>>>> +	src = checker_class_lookup(name);
>>>> +	if (!src)
>>>> +		src = add_checker_class(multipath_dir, name);
>>>> +
>>>>  	dst->cls = src;
>>>>  	if (!src)
>>>>  		return;
>>>
>>> .
>>>
> 
> 
> 
> .
> 

Re: [PATCH 3/6] multipathd: adopt static char* arr in sd_notify_status func

[Zhiqiang Liu]

On 2020/8/17 23:44, Martin Wilck wrote:
> On Mon, 2020-08-17 at 23:33 +0800, Zhiqiang Liu wrote:
>>
>> On 2020/8/17 16:35, Martin Wilck wrote:
>>> On Sun, 2020-08-16 at 09:44 +0800, Zhiqiang Liu wrote:
>>>> We adopt static char* array (sd_notify_status_msg) in
>>>> sd_notify_status func, so it looks more simpler and easier
>>>> to expand.
>>>>
>>>> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
>>>> Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
>>>> ---
>>>>  multipathd/main.c | 26 ++++++++++++--------------
>>>>  1 file changed, 12 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/multipathd/main.c b/multipathd/main.c
>>>> index cab1d0d..a09ccd1 100644
>>>> --- a/multipathd/main.c
>>>> +++ b/multipathd/main.c
>>>> @@ -177,23 +177,21 @@ daemon_status(void)
>>>>   * I love you too, systemd ...
>>>>   */
>>>>  #ifdef USE_SYSTEMD
>>>> +static const char *sd_notify_status_msg[DAEMON_STATUS_SIZE] = {
>>>> +	[DAEMON_INIT] = "STATUS=init",
>>>> +	[DAEMON_START] = "STATUS=startup",
>>>> +	[DAEMON_CONFIGURE] = "STATUS=configure",
>>>> +	[DAEMON_IDLE] = "STATUS=up",
>>>> +	[DAEMON_RUNNING] = "STATUS=up",
>>>> +	[DAEMON_SHUTDOWN] = "STATUS=shutdown",
>>>> +};
>>>> +
>>>
>>> This repetition of "STATUS=" looks clumsy. It's not your fault,
>>> because
>>> the current code does the same thing. But if you want to clean this
>>> up,
>>> please create the notification string in a dynamic buffer, and use
>>> daemon_status() for those cases where it applies.
>>>
>>> Regards
>>> Martin
>>>
>> Thanks for your reply.
>> Besides the prefixes "STATUS=", there are also some differences
>> between
>> sd_notify_status_msg[DAEMON_IDLE|DAEMON_RUNNING] and
>> demon_status_msg[DAEMON_IDLE|DAEMON_RUNNING].
>>
>> For example,
>> sd_notify_status_msg[DAEMON_RUNNING] = "STATUS=up",
>> demon_status_msg[DAEMON_RUNNING] = "running"
>>
>> So if we create the notification string in a dynamic buffer with
>> using daemon_status, we have to make some special judgement
>> on DAEMON_RUNNING and DAEMON_IDLE status. This may be why
>> the sd_notify_status func was created.
>>
>> We can implement both solutions. Martin, which one do you prefer?
> 
> That's what I meant with "where it applies". You treat the IDLE and
> RUNNING cases first (probably in a switch statement), and use
> daemon_status() for the "default" case.
> 
> The  reason we don't differentiate between "running" and "idle" in the
> sd_notify() code path is that the daemon switches between these states
> very often (at least once per tick) and these notifications causes lots
> of dbus traffic without much value.
> 
> Regards
> Martin
> 

I will do that as your suggestion in v2 patch.
Thanks again.

Regards
Zhiqiang Liu

Re: [PATCH 5/6] vector: add lower boundary check in vector_foreach_slot_after

[Zhiqiang Liu]

On 2020/8/17 17:11, Martin Wilck wrote:
> On Sun, 2020-08-16 at 09:45 +0800, Zhiqiang Liu wrote:
>> In vector_foreach_slot_after macro, i is the input var, which
>> may have a illegal value (i < 0). So we should add lower boundary
>> check in vector_foreach_slot_after macro.
>>
>> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
>> Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
>> ---
>>  libmultipath/vector.h | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> Perhaps we should write this instead? I'm unsure if compilers can
> optimize away the repeated extra check in all cases.
> 
> #define vector_foreach_slot_after(v,p,i) \
> 	if ((v) && (int)(i) >= 0)        \
> 		for (; (int)(i) < VECTOR_SIZE(v) && ((p) = (v)->slot[i]); (i)++)
> 
> Regards,
> Martin
> 
Thanks for your advice.
When I modified the vector_foreach_slot_after func, I just focused
on the basic functionality, and the performance is totally not considered.
I learn a lesson from your advice.

Thanks again.

Regards
Zhiqiang Liu

>>
>> diff --git a/libmultipath/vector.h b/libmultipath/vector.h
>> index 2862dc2..45dbfc1 100644
>> --- a/libmultipath/vector.h
>> +++ b/libmultipath/vector.h
>> @@ -38,11 +38,11 @@ typedef struct _vector *vector;
>>  #define VECTOR_LAST_SLOT(V)   (((V) && VECTOR_SIZE(V) > 0) ? (V)-
>>> slot[(VECTOR_SIZE(V) - 1)] : NULL)
>>
>>  #define vector_foreach_slot(v,p,i) \
>> -	for (i = 0; (v) && (int)i < VECTOR_SIZE(v) && ((p) = (v)-
>>> slot[i]); i++)
>> +	for ((i) = 0; (v) && (int)(i) < VECTOR_SIZE(v) && ((p) = (v)-
>>> slot[i]); (i)++)
>>  #define vector_foreach_slot_after(v,p,i) \
>> -	for (; (v) && (int)i < VECTOR_SIZE(v) && ((p) = (v)->slot[i]);
>> i++)
>> +	for (; (v) && (int)(i) < VECTOR_SIZE(v) && (int)(i) >= 0 &&
>> ((p) = (v)->slot[i]); (i)++)
>>  #define vector_foreach_slot_backwards(v,p,i) \
>> -	for (i = VECTOR_SIZE(v) - 1; (int)i >= 0 && ((p) = (v)-
>>> slot[i]); i--)
>> +	for ((i) = VECTOR_SIZE(v) - 1; (int)(i) >= 0 && ((p) = (v)-
>>> slot[i]); (i)--)
>>
>>  #define identity(x) (x)
>>  /*
> 
> 
> 
> .
> 

Re: [PATCH 3/6] multipathd: adopt static char* arr in sd_notify_status func

[Zhiqiang Liu]

On 2020/8/17 23:44, Martin Wilck wrote:
> On Mon, 2020-08-17 at 23:33 +0800, Zhiqiang Liu wrote:
>>
>> On 2020/8/17 16:35, Martin Wilck wrote:
>>> On Sun, 2020-08-16 at 09:44 +0800, Zhiqiang Liu wrote:
>>>> We adopt static char* array (sd_notify_status_msg) in
>>>> sd_notify_status func, so it looks more simpler and easier
>>>> to expand.
>>>>
>>>> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
>>>> Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
>>>> ---
>>>>  multipathd/main.c | 26 ++++++++++++--------------
>>>>  1 file changed, 12 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/multipathd/main.c b/multipathd/main.c
>>>> index cab1d0d..a09ccd1 100644
>>>> --- a/multipathd/main.c
>>>> +++ b/multipathd/main.c
>>>> @@ -177,23 +177,21 @@ daemon_status(void)
>>>>   * I love you too, systemd ...
>>>>   */
>>>>  #ifdef USE_SYSTEMD
>>>> +static const char *sd_notify_status_msg[DAEMON_STATUS_SIZE] = {
>>>> +	[DAEMON_INIT] = "STATUS=init",
>>>> +	[DAEMON_START] = "STATUS=startup",
>>>> +	[DAEMON_CONFIGURE] = "STATUS=configure",
>>>> +	[DAEMON_IDLE] = "STATUS=up",
>>>> +	[DAEMON_RUNNING] = "STATUS=up",
>>>> +	[DAEMON_SHUTDOWN] = "STATUS=shutdown",
>>>> +};
>>>> +
>>>
>>> This repetition of "STATUS=" looks clumsy. It's not your fault,
>>> because
>>> the current code does the same thing. But if you want to clean this
>>> up,
>>> please create the notification string in a dynamic buffer, and use
>>> daemon_status() for those cases where it applies.
>>>
>>> Regards
>>> Martin
>>>
>> Thanks for your reply.
>> Besides the prefixes "STATUS=", there are also some differences
>> between
>> sd_notify_status_msg[DAEMON_IDLE|DAEMON_RUNNING] and
>> demon_status_msg[DAEMON_IDLE|DAEMON_RUNNING].
>>
>> For example,
>> sd_notify_status_msg[DAEMON_RUNNING] = "STATUS=up",
>> demon_status_msg[DAEMON_RUNNING] = "running"
>>
>> So if we create the notification string in a dynamic buffer with
>> using daemon_status, we have to make some special judgement
>> on DAEMON_RUNNING and DAEMON_IDLE status. This may be why
>> the sd_notify_status func was created.
>>
>> We can implement both solutions. Martin, which one do you prefer?
> 
> That's what I meant with "where it applies". You treat the IDLE and
> RUNNING cases first (probably in a switch statement), and use
> daemon_status() for the "default" case.
> 
> The  reason we don't differentiate between "running" and "idle" in the
> sd_notify() code path is that the daemon switches between these states
> very often (at least once per tick) and these notifications causes lots
> of dbus traffic without much value.
> 
> Regards
> Martin

Thank you for your patience.
I will do that as your suggestion.

Regards
Zhiqiang Liu

Re: [PATCH 4/6] libmultipath: check blist before calling MALLOC in alloc_ble_device func

[Benjamin Marzinski]

On Mon, Aug 17, 2020 at 10:51:49AM +0200, Martin Wilck wrote:
> On Sun, 2020-08-16 at 09:45 +0800, Zhiqiang Liu wrote:
> > In alloc_ble_device func, ble is firstly allocated by calling MALLOC,
> > and then input blist is checked whether it is valid. If blist is not
> > valid, ble will be freed without using.
> > 
> > Here, we should check blist firstly.
> > 
> > Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> > Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
> > ---
> >  libmultipath/blacklist.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> This patch isn't wrong, but it fixes code which isn't buggy. It's
> rather a style thing, an optimization for an extremely unlikely error
> case. I agree with you in the sense that I prefer the "new" style over
> the old (I generally dislike expressions that can fail, like malloc()
> calls, being used as variable initializers), but I'm not sure if we
> should start applying patches for cases like this. So far we've been
> rather conservative with "style" patches, because they tend to make it
> unnecessarily hard to track code history.
> 
> Ben, Christophe, what's your take on this matter?

While I'm not really a fan of whitespace tweaking patches, I'm fine with
this. All things being equal, I really do prefer it when functions check
their arguments first, instead of doing possibly unnecessary work.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
 
> Regards,
> Martin
> 
> 
> > 
> > diff --git a/libmultipath/blacklist.c b/libmultipath/blacklist.c
> > index db58ccc..bedcc7e 100644
> > --- a/libmultipath/blacklist.c
> > +++ b/libmultipath/blacklist.c
> > @@ -66,12 +66,16 @@ out:
> > 
> >  int alloc_ble_device(vector blist)
> >  {
> > -	struct blentry_device * ble = MALLOC(sizeof(struct
> > blentry_device));
> > +	struct blentry_device *ble;
> > 
> > +	if (!blist)
> > +		return 1;
> > +
> > +	ble = MALLOC(sizeof(struct blentry_device));
> >  	if (!ble)
> >  		return 1;
> > 
> > -	if (!blist || !vector_alloc_slot(blist)) {
> > +	if (!vector_alloc_slot(blist)) {
> >  		FREE(ble);
> >  		return 1;
> >  	}
>