Hi, > -----Original Message----- > From: Alan Ott [mailto:alan@xxxxxxxxxxx] > Sent: Tuesday, December 31, 2013 4:32 AM > To: Krzysztof Opasiak; mporter@xxxxxxxxxx; linux- > usb@xxxxxxxxxxxxxxx > Cc: Andrzej Pietrasiewicz; Piotr Bereza; Karol Lewandowski; > Aleksander Zdyb; Stanislaw Wadas; myungjoo.ham@xxxxxxxxxxx; > ty317.kim@xxxxxxxxxxx > Subject: Re: [PATCH v2 06/12] libusbg: Add error handling to > gadget_read_string. > > On 12/19/2013 06:29 AM, Krzysztof Opasiak wrote: > > Add error handling when gadget_read_buf return NULL. > > If read of string fails, the string should be set as empty. > > It's typical to put () at the end of function names in log > messages. > Also s/return/returns/ > > "Add error handling when gadget_read_buf() returns NULL. > If read of string fails, the string should be set as empty." Thank you for your remarks. I will fix it in next version. > > > > > Signed-off-by: Krzysztof Opasiak <k.opasiak@xxxxxxxxxxx> > > --- > > Changes since v1: > > - Remove additional check of p variable > > > > src/gadget.c | 23 +++++++++++++++++------ > > 1 file changed, 17 insertions(+), 6 deletions(-) > > > > diff --git a/src/gadget.c b/src/gadget.c > > index f613c3e..d225af0 100644 > > --- a/src/gadget.c > > +++ b/src/gadget.c > > @@ -121,11 +121,18 @@ static int gadget_read_int(char *path, char > *name, char *file, int base) > > > > static void gadget_read_string(char *path, char *name, char > *file, char *buf) > > { > > - char *p; > > + char *p = NULL; > > Initializer isn't necessary. I'm almost sure that compiler will optimize that, so there performance is not an issue but such code is safer for future modifications. > > > + > > + p = gadget_read_buf(path, name, file, buf); > > + //check whether read was successful > > Make /* */ (assuming Matt wants kernel style). Ok. I will fix all occurrences. > > > + if (p != NULL) { > > + if ((p = strchr(buf, '\n')) != NULL) > > + *p = '\0'; > > + } else { > > + //Set this as empty string > > /* */ > > > + *buf = '\0'; > > + } > > > > - gadget_read_buf(path, name, file, buf); > > - if ((p = strchr(buf, '\n')) != NULL) > > - *p = '\0'; > > } > > > > static void gadget_write_buf(char *path, char *name, char > *file, char *buf) > > @@ -185,10 +192,14 @@ static void > gadget_parse_function_attrs(struct function *f) > > case F_RNDIS: > > gadget_read_string(f->path, f->name, "dev_addr", > str_addr); > > addr = ether_aton(str_addr); > > maybe ether_aton_r() here. There are also some other pieces of code which should be changed to make this library thread safe. I have this in my mind but I would like to do this in separate commit. > > > - memcpy(&f->attr.net.dev_addr, addr, 6); > > + if (addr) > > + memcpy(&f->attr.net.dev_addr, addr, sizeof(struct > ether_addr)); > > + > > gadget_read_string(f->path, f->name, "host_addr", > str_addr); > > addr = ether_aton(str_addr); > > ether_aton_r() > > > - memcpy(&f->attr.net.host_addr, addr, 6); > > + if(addr) > > + memcpy(&f->attr.net.host_addr, addr, > sizeof(struct ether_addr)); > > + > > gadget_read_string(f->path, f->name, "ifname", f- > >attr.net.ifname); > > f->attr.net.qmult = gadget_read_dec(f->path, f->name, > "qmult"); > > break; -- Krzysztof Opasiak Samsung R&D Institute Poland Samsung Electronics k.opasiak@xxxxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
