[U-Boot] [PATCH v3] usb: gadget: dynamic envstr size in cb_getvar

[Nicolas le bayon]

2017-03-10 17:52 GMT+01:00 Marek Vasut <marex at denx.de>:

> On 03/10/2017 05:31 PM, Nicolas le bayon wrote:
> > From: Nicolas Le Bayon <nlebayon at gmail.com>
> >
> > use dynamic allocation to consider all variable lengths
>
> Of what ? Where ? Why ?
>

Here is a proposal of updated label:
usb: gadget: dynamic alloc for variable name in cb_getvar

And a better description proposal:
Allocate dynamically the buffer of the variable name in cb_getvar function
This allows to treat correctly all variable name lengths, growing through
releases, and also avoids cutting their names, leading to undefined
variable for the function.


> > Signed-off-by: Nicolas Le Bayon <nlebayon at gmail.com>
> > ---
> >  drivers/usb/gadget/f_fastboot.c | 16 ++++++++++++++--
> >  1 file changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/f_fastboot.c
> > b/drivers/usb/gadget/f_fastboot.c
> > index 2160b1c..9bb3a95 100644
> > --- a/drivers/usb/gadget/f_fastboot.c
> > +++ b/drivers/usb/gadget/f_fastboot.c
> > @@ -432,9 +432,19 @@ static void cb_getvar(struct usb_ep *ep, struct
> > usb_request *req)
> >   else
> >   strcpy(response, "FAILValue not set");
> >   } else {
> > - char envstr[32];
> > + char *envstr = NULL;
>
> This var is inited by malloc() below, drop the = NULL .
>

OK


> > + unsigned int len;
> >
> > - snprintf(envstr, sizeof(envstr) - 1, "fastboot.%s", cmd);
> > + len = strlen("fastboot.") + strlen(cmd) + 1;
> > +
> > + envstr = malloc(len);
>
> The len is only used here, just do malloc(strlen ..... ));


OK I merge all this in a single line


> > + if (!envstr) {
> > + error("variable malloc error");
>
> This error message makes no sense ...
>

Is error("malloc error"); enough from your point of view?


> > + fastboot_tx_write_str("FAILvar malloc error");
>
> The indent of this whole block is wrong ...
>

Regarding previous remark on error message, I also modified it to
 fastboot_tx_write_str("FAILmalloc error");

Regarding indent, it seems to me that it fits with the rest of the
function, but I'm surely wrong, could you precise me the problem please?



> > + return;
> > + }
> > +
> > + sprintf(envstr, "fastboot.%s", cmd);
> >   s = getenv(envstr);
> >   if (s) {
> >   strncat(response, s, chars_left);
> > @@ -442,6 +452,8 @@ static void cb_getvar(struct usb_ep *ep, struct
> > usb_request *req)
> >   printf("WARNING: unknown variable: %s\n", cmd);
> >   strcpy(response, "FAILVariable not implemented");
> >   }
> > +
> > + free(envstr);
> >   }
> >   fastboot_tx_write_str(response);
> >  }
> >
>
>
> --
> Best regards,
> Marek Vasut
>

Regars
Nicolas Le Bayon