Re: [PATCH] multipathd: fix issue in 'map $map getprstatus' reply

Re: [PATCH] multipathd: fix issue in 'map $map getprstatus' reply

[peng.liang5]

[-- Attachment #1.1: Type: text/plain, Size: 1803 bytes --]

Please have a review for this patch, hope for your comments.

Thanks


发件人:         peng.liang5@zte.com.cn
收件人:         christophe varoqui <christophe.varoqui@free.fr>, 
抄送:   zhang.kai16@zte.com.cn, dm-devel@redhat.com, peng liang 
<peng.liang5@zte.com.cn>
日期:   2016-08-04 15:31
主题:   [dm-devel] [PATCH] multipathd: fix issue in 'map $map getprstatus' 
reply
发件人: dm-devel-bounces@redhat.com



From: peng liang <peng.liang5@zte.com.cn>

-add missing newline to 'map|multipath $map getprstatus' reply
-use asprintf instead of sprintf

Signed-off-by: peng liang <peng.liang5@zte.com.cn>
---
 multipathd/cli_handlers.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 8ff4362..16445ea 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -1,6 +1,9 @@
 /*
  * Copyright (c) 2005 Christophe Varoqui
  */
+#define _GNU_SOURCE
+
+#include <stdio.h>
 #include "checkers.h"
 #include "memory.h"
 #include "vector.h"
@@ -1285,14 +1288,9 @@ cli_getprstatus (void * v, char ** reply, int * 
len, void * data)
 
                 condlog(3, "%s: prflag = %u", param, (unsigned 
int)mpp->prflag);
 
-                *reply =(char *)malloc(2);
-                *len = 2;
-                memset(*reply,0,2);
-
-
-                sprintf(*reply,"%d",mpp->prflag);
-                (*reply)[1]='\0';
-
+                *len = asprintf(reply, "%d\n", mpp->prflag);
+                if (*len < 0)
+                                return 1;
 
                 condlog(3, "%s: reply = %s", param, *reply);
 
-- 
2.8.1.windows.1

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


[-- Attachment #1.2: Type: text/html, Size: 3658 bytes --]

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

Re: [PATCH] multipathd: fix issue in 'map $map getprstatus' reply

[Bart Van Assche]

On 10/11/16 20:03, peng.liang5@zte.com.cn wrote:
> From: peng liang <peng.liang5@zte.com.cn>
> 
> -add missing newline to 'map|multipath $map getprstatus' reply
> -use asprintf instead of sprintf
> 
> Signed-off-by: peng liang <peng.liang5@zte.com.cn>
> ---
> multipathd/cli_handlers.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> index 8ff4362..16445ea 100644
> --- a/multipathd/cli_handlers.c
> +++ b/multipathd/cli_handlers.c
> @@ -1,6 +1,9 @@
> /*
>  * Copyright (c) 2005 Christophe Varoqui
>  */
> +#define _GNU_SOURCE
> +
> +#include <stdio.h>
> #include "checkers.h"
> #include "memory.h"
> #include "vector.h"
> @@ -1285,14 +1288,9 @@ cli_getprstatus (void * v, char ** reply, int *
> len, void * data)
> 
>                  condlog(3, "%s: prflag = %u", param, (unsigned int)mpp->prflag);
> 
> -                 *reply =(char *)malloc(2);
> -                 *len = 2;
> -                 memset(*reply,0,2);
> -
> -
> -                 sprintf(*reply,"%d",mpp->prflag);
> -                 (*reply)[1]='\0';
> -
> +                 *len = asprintf(reply, "%d\n", mpp->prflag);
> +                 if (*len < 0)
> +                                  return 1;
> 
>                  condlog(3, "%s: reply = %s", param, *reply);

Hello Peng,

Sorry but returning 1 looks somewhat inconsistent to me. This function
is called indirectly by uxsock_trigger() and that function expects that
cli_getprstatus() either returns a positive error code (E...) or a
negative error code. Please change this patch such that ENOMEM is
returned instead of 1 if asprintf() fails.

Thanks,

Bart.

Re: [PATCH] multipathd: fix issue in 'map $map getprstatus' reply

[peng.liang5]

[-- Attachment #1.1: Type: text/plain, Size: 2590 bytes --]

Hello Bart,

Thanks for your attention. I don't think it is necessary to do that.
Whatever returning 1 or ENOMEM it will reply "fail\n" and set the 
returning to 1.

The executed code in uxsock_trigger as follows.
        if (r > 0) {
                if (r == ETIMEDOUT)
                        *reply = STRDUP("timeout\n");
                else
                        *reply = STRDUP("fail\n");
                *len = strlen(*reply) + 1;
                r = 1;
        } 




发件人:         Bart Van Assche <bart.vanassche@sandisk.com>
收件人:         <peng.liang5@zte.com.cn>, 
<christophe.varoqui@opensvc.com>, 
抄送:   <tang.junhui@zte.com.cn>, <zhang.kai16@zte.com.cn>, 
<dm-devel@redhat.com>
日期:   2016/10/12 22:44
主题:   Re: [dm-devel] [PATCH] multipathd: fix issue in 'map $map 
getprstatus' reply



On 10/11/16 20:03, peng.liang5@zte.com.cn wrote:
> From: peng liang <peng.liang5@zte.com.cn>
> 
> -add missing newline to 'map|multipath $map getprstatus' reply
> -use asprintf instead of sprintf
> 
> Signed-off-by: peng liang <peng.liang5@zte.com.cn>
> ---
> multipathd/cli_handlers.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> index 8ff4362..16445ea 100644
> --- a/multipathd/cli_handlers.c
> +++ b/multipathd/cli_handlers.c
> @@ -1,6 +1,9 @@
> /*
>  * Copyright (c) 2005 Christophe Varoqui
>  */
> +#define _GNU_SOURCE
> +
> +#include <stdio.h>
> #include "checkers.h"
> #include "memory.h"
> #include "vector.h"
> @@ -1285,14 +1288,9 @@ cli_getprstatus (void * v, char ** reply, int *
> len, void * data)
> 
>                  condlog(3, "%s: prflag = %u", param, (unsigned 
int)mpp->prflag);
> 
> -                 *reply =(char *)malloc(2);
> -                 *len = 2;
> -                 memset(*reply,0,2);
> -
> -
> -                 sprintf(*reply,"%d",mpp->prflag);
> -                 (*reply)[1]='\0';
> -
> +                 *len = asprintf(reply, "%d\n", mpp->prflag);
> +                 if (*len < 0)
> +                                  return 1;
> 
>                  condlog(3, "%s: reply = %s", param, *reply);

Hello Peng,

Sorry but returning 1 looks somewhat inconsistent to me. This function
is called indirectly by uxsock_trigger() and that function expects that
cli_getprstatus() either returns a positive error code (E...) or a
negative error code. Please change this patch such that ENOMEM is
returned instead of 1 if asprintf() fails.

Thanks,

Bart.




[-- Attachment #1.2: Type: text/html, Size: 5119 bytes --]

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

Re: [PATCH] multipathd: fix issue in 'map $map getprstatus' reply

[Bart Van Assche]

On 10/12/2016 11:30 PM, peng.liang5@zte.com.cn wrote:
> Thanks for your attention. I don't think it is necessary to do that.
> Whatever returning 1 or ENOMEM it will reply "fail\n" and set the
> returning to 1.
>
> The executed code in uxsock_trigger as follows.
>         if (r > 0) {
>                 if (r == ETIMEDOUT)
>                         *reply = STRDUP("timeout\n");
>                 else
>                         *reply = STRDUP("fail\n");
>                 *len = strlen(*reply) + 1;
>                 r = 1;
>         }

Hello Peng,

Anyone who wants to verify your patch has to look up the numeric value 
of ETIMEDOUT to see whether or not the value of that constant is equal 
to one. So using "return 1" makes the source code harder to read than 
"return ENOMEM". Additionally, it is inconsistent from a stylistic point 
of view that the caller compares the return value with an E... error 
code and that the called function returns a numeric constant. So I would 
appreciate it very much if you would change "return 1" into "return 
ENOMEM" or any other symbolic E* error code of your preference.

Bart.

Re: [PATCH] multipathd: fix issue in 'map $map getprstatus' reply

[peng.liang5]

[-- Attachment #1.1: Type: text/plain, Size: 1934 bytes --]

Hello Bart,

I can understand your concern. It may be some other error not just memory 
error
if asprintf() fails, so returning ENOMEM is not accurate.
Additionally, you still need to verify the numeric value of ETIMEDOUT 
because 
cli_getprstatus() returning 1 in the other code as follows.
        if (!mpp)
                return 1;
And the other called functions also returns 1 if error occurs, such as 
cli_suspend,
cli_resume.

Thanks 





发件人:         Bart Van Assche <bart.vanassche@sandisk.com>
收件人:         <peng.liang5@zte.com.cn>, 
抄送:   <dm-devel@redhat.com>, <tang.junhui@zte.com.cn>, 
<zhang.kai16@zte.com.cn>
日期:   2016/10/14 06:44
主题:   Re: [dm-devel] [PATCH] multipathd: fix issue in 'map $map 
getprstatus' reply



On 10/12/2016 11:30 PM, peng.liang5@zte.com.cn wrote:
> Thanks for your attention. I don't think it is necessary to do that.
> Whatever returning 1 or ENOMEM it will reply "fail\n" and set the
> returning to 1.
>
> The executed code in uxsock_trigger as follows.
>         if (r > 0) {
>                 if (r == ETIMEDOUT)
>                         *reply = STRDUP("timeout\n");
>                 else
>                         *reply = STRDUP("fail\n");
>                 *len = strlen(*reply) + 1;
>                 r = 1;
>         }

Hello Peng,

Anyone who wants to verify your patch has to look up the numeric value 
of ETIMEDOUT to see whether or not the value of that constant is equal 
to one. So using "return 1" makes the source code harder to read than 
"return ENOMEM". Additionally, it is inconsistent from a stylistic point 
of view that the caller compares the return value with an E... error 
code and that the called function returns a numeric constant. So I would 
appreciate it very much if you would change "return 1" into "return 
ENOMEM" or any other symbolic E* error code of your preference.

Bart.



[-- Attachment #1.2: Type: text/html, Size: 3683 bytes --]

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

[PATCH] multipathd: fix issue in 'map $map getprstatus' reply

[peng.liang5]

From: peng liang <peng.liang5@zte.com.cn>

-add missing newline to 'map|multipath $map getprstatus' reply
-use asprintf instead of sprintf

Signed-off-by: peng liang <peng.liang5@zte.com.cn>
---
 multipathd/cli_handlers.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 8ff4362..16445ea 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -1,6 +1,9 @@
 /*
  * Copyright (c) 2005 Christophe Varoqui
  */
+#define _GNU_SOURCE
+
+#include <stdio.h>
 #include "checkers.h"
 #include "memory.h"
 #include "vector.h"
@@ -1285,14 +1288,9 @@ cli_getprstatus (void * v, char ** reply, int * len, void * data)
 
 	condlog(3, "%s: prflag = %u", param, (unsigned int)mpp->prflag);
 
-	*reply =(char *)malloc(2);
-	*len = 2;
-	memset(*reply,0,2);
-
-
-	sprintf(*reply,"%d",mpp->prflag);
-	(*reply)[1]='\0';
-
+	*len = asprintf(reply, "%d\n", mpp->prflag);
+	if (*len < 0)
+		return 1;
 
 	condlog(3, "%s: reply = %s", param, *reply);
 
-- 
2.8.1.windows.1

Re: [PATCH] multipathd: fix issue in 'map $map getprstatus' reply

[Bart Van Assche]

On 08/03/2016 12:09 AM, peng.liang5@zte.com.cn wrote:
> +	*len = 3;
> +	asprintf(reply, "%d\n", mpp->prflag);

Hello Peng,

Please use the return value of asprintf() to compute *len instead of 
assigning a hard-coded constant to *len.

Thanks,

Bart.

[PATCH] multipathd: fix issue in 'map $map getprstatus' reply

[peng.liang5]

From: peng liang <peng.liang5@zte.com.cn>

-add missing newline to 'map|multipath $map getprstatus' reply
-use asprintf instead of sprintf

Signed-off-by: peng liang <peng.liang5@zte.com.cn>
---
 multipathd/cli_handlers.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 8ff4362..cec40eb 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -1,6 +1,9 @@
 /*
  * Copyright (c) 2005 Christophe Varoqui
  */
+#define _GNU_SOURCE
+
+#include <stdio.h>
 #include "checkers.h"
 #include "memory.h"
 #include "vector.h"
@@ -1285,14 +1288,8 @@ cli_getprstatus (void * v, char ** reply, int * len, void * data)
 
 	condlog(3, "%s: prflag = %u", param, (unsigned int)mpp->prflag);
 
-	*reply =(char *)malloc(2);
-	*len = 2;
-	memset(*reply,0,2);
-
-
-	sprintf(*reply,"%d",mpp->prflag);
-	(*reply)[1]='\0';
-
+	*len = 3;
+	asprintf(reply, "%d\n", mpp->prflag);
 
 	condlog(3, "%s: reply = %s", param, *reply);
 
-- 
2.8.1.windows.1

Re: [PATCH] multipathd: fix issue in 'map $map getprstatus' reply

[peng.liang5]

[-- Attachment #1.1: Type: text/plain, Size: 2504 bytes --]

Hi Bart,

Thanks for your advice!
I have updated the solution as follows. 

diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 0b04504..cec40eb 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -1,6 +1,9 @@
 /*
  * Copyright (c) 2005 Christophe Varoqui
  */
+#define _GNU_SOURCE
+
+#include <stdio.h>
 #include "checkers.h"
 #include "memory.h"
 #include "vector.h"
@@ -1285,14 +1288,8 @@ cli_getprstatus (void * v, char ** reply, int * 
len, void * data)

        condlog(3, "%s: prflag = %u", param, (unsigned int)mpp->prflag);

-       *reply =(char *)malloc(3);
        *len = 3;
-       memset(*reply,0,3);
-
-
-       sprintf(*reply,"%d\n",mpp->prflag);
-       (*reply)[2]='\0';
-
+       asprintf(reply, "%d\n", mpp->prflag);

        condlog(3, "%s: reply = %s", param, *reply); 




发件人:         Bart Van Assche <bart.vanassche@sandisk.com>
收件人:         <peng.liang5@zte.com.cn>, christophe varoqui 
<christophe.varoqui@free.fr>, 
抄送:   <zhang.kai16@zte.com.cn>, <dm-devel@redhat.com>
日期:   2016/08/02 23:20
主题:   Re: [dm-devel] [PATCH] multipathd: fix issue in 'map $map 
getprstatus' reply



On 08/01/2016 06:27 PM, peng.liang5@zte.com.cn wrote:
> From: peng liang <peng.liang5@zte.com.cn>
>
> add missing newline to 'map|multipath $map getprstatus' reply
>
> Signed-off-by: peng liang <peng.liang5@zte.com.cn>
> ---
>  multipathd/cli_handlers.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> index 8ff4362..0b04504 100644
> --- a/multipathd/cli_handlers.c
> +++ b/multipathd/cli_handlers.c
> @@ -1285,13 +1285,13 @@ cli_getprstatus (void * v, char ** reply, int * 
len, void * data)
>
>                condlog(3, "%s: prflag = %u", param, (unsigned 
int)mpp->prflag);
>
> -              *reply =(char *)malloc(2);
> -              *len = 2;
> -              memset(*reply,0,2);
> +              *reply =(char *)malloc(3);
> +              *len = 3;
> +              memset(*reply,0,3);
>
>
> -              sprintf(*reply,"%d",mpp->prflag);
> -              (*reply)[1]='\0';
> +              sprintf(*reply,"%d\n",mpp->prflag);
> +              (*reply)[2]='\0';

Hello Peng,

Please use asprintf() instead of malloc() + memset() + sprintf(). See 
also 
https://www.gnu.org/software/libc/manual/html_node/Dynamic-Output.html

Thanks,

Bart.




[-- Attachment #1.2: Type: text/html, Size: 6135 bytes --]

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

Re: [PATCH] multipathd: fix issue in 'map $map getprstatus' reply

[Bart Van Assche]

On 08/01/2016 06:27 PM, peng.liang5@zte.com.cn wrote:
> From: peng liang <peng.liang5@zte.com.cn>
>
> add missing newline to 'map|multipath $map getprstatus' reply
>
> Signed-off-by: peng liang <peng.liang5@zte.com.cn>
> ---
>  multipathd/cli_handlers.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> index 8ff4362..0b04504 100644
> --- a/multipathd/cli_handlers.c
> +++ b/multipathd/cli_handlers.c
> @@ -1285,13 +1285,13 @@ cli_getprstatus (void * v, char ** reply, int * len, void * data)
>
>  	condlog(3, "%s: prflag = %u", param, (unsigned int)mpp->prflag);
>
> -	*reply =(char *)malloc(2);
> -	*len = 2;
> -	memset(*reply,0,2);
> +	*reply =(char *)malloc(3);
> +	*len = 3;
> +	memset(*reply,0,3);
>
>
> -	sprintf(*reply,"%d",mpp->prflag);
> -	(*reply)[1]='\0';
> +	sprintf(*reply,"%d\n",mpp->prflag);
> +	(*reply)[2]='\0';

Hello Peng,

Please use asprintf() instead of malloc() + memset() + sprintf(). See 
also https://www.gnu.org/software/libc/manual/html_node/Dynamic-Output.html

Thanks,

Bart.

[PATCH] multipathd: fix issue in 'map $map getprstatus' reply

[peng.liang5]

From: peng liang <peng.liang5@zte.com.cn>

add missing newline to 'map|multipath $map getprstatus' reply

Signed-off-by: peng liang <peng.liang5@zte.com.cn>
---
 multipathd/cli_handlers.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 8ff4362..0b04504 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -1285,13 +1285,13 @@ cli_getprstatus (void * v, char ** reply, int * len, void * data)
 
 	condlog(3, "%s: prflag = %u", param, (unsigned int)mpp->prflag);
 
-	*reply =(char *)malloc(2);
-	*len = 2;
-	memset(*reply,0,2);
+	*reply =(char *)malloc(3);
+	*len = 3;
+	memset(*reply,0,3);
 
 
-	sprintf(*reply,"%d",mpp->prflag);
-	(*reply)[1]='\0';
+	sprintf(*reply,"%d\n",mpp->prflag);
+	(*reply)[2]='\0';
 
 
 	condlog(3, "%s: reply = %s", param, *reply);
-- 
2.8.1.windows.1