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

[peng . liang5]

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.vanass...@sandisk.com>
收件人: <peng.lia...@zte.com.cn>, 
<christophe.varo...@opensvc.com>, 
抄送:   <tang.jun...@zte.com.cn>, <zhang.ka...@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.lia...@zte.com.cn wrote:
> From: peng liang <peng.lia...@zte.com.cn>
> 
> -add missing newline to 'map|multipath $map getprstatus' reply
> -use asprintf instead of sprintf
> 
> Signed-off-by: peng liang <peng.lia...@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 
> #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.



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

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

[Bart Van Assche]

On 10/11/16 20:03, peng.lia...@zte.com.cn wrote:
> From: peng liang 
> 
> -add missing newline to 'map|multipath $map getprstatus' reply
> -use asprintf instead of sprintf
> 
> Signed-off-by: peng liang 
> ---
> 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 
> #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.

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

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

[peng . liang5]

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

Thanks


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



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

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

Signed-off-by: peng liang <peng.lia...@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 
 #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

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

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

[Bart Van Assche]

On 08/03/2016 12:09 AM, peng.lia...@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.

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

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

[peng . liang5]

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 
 #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.vanass...@sandisk.com>
收件人: <peng.lia...@zte.com.cn>, christophe varoqui 
<christophe.varo...@free.fr>, 
抄送:   <zhang.ka...@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.lia...@zte.com.cn wrote:
> From: peng liang <peng.lia...@zte.com.cn>
>
> add missing newline to 'map|multipath $map getprstatus' reply
>
> Signed-off-by: peng liang <peng.lia...@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.



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

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

[peng . liang5]

From: peng liang 

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

Signed-off-by: peng liang 
---
 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 
 #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

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

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

[Bart Van Assche]

On 08/01/2016 06:27 PM, peng.lia...@zte.com.cn wrote:

From: peng liang 

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

Signed-off-by: peng liang 
---
 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.

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