| Message ID | OF841ED337.71FB1AB0-ON4825804A.000FE81D-4825804A.0010D15C@zte.com.cn (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
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. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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.
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
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. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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.
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
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;