I argue that strcat() as interface is bad enough done once, but done in a loop, by continuously throwing away the buffer length at every call, is even evil. Anyway, I chose evil in this case, as that function could be written in Cobol and still be performant enough, but that strcat loop should not be taught at school 😀
On Thu, Nov 12, 2015 at 7:41 PM, Dan Cross <[email protected]> wrote: > On Thu, Nov 12, 2015 at 10:33 PM, 'Davide Libenzi' via Akaros < > [email protected]> wrote: > >> On Thu, Nov 12, 2015 at 7:22 PM, Dan Cross <[email protected]> wrote: >> >>> On Thu, Nov 12, 2015 at 3:02 PM, Barret Rhoden <[email protected]> >>> wrote: >>> >>>> > +static void kprof_usage_fail(void) >>>> > +{ >>>> > + static const char *ctlstring = "clear|start|stop|timer"; >>>> > + const char* const * cmds = profiler_configure_cmds(); >>>> > + size_t i, size, csize; >>>> > + char msgbuf[128]; >>>> > + >>>> > + strncpy(msgbuf, ctlstring, sizeof(msgbuf)); >>>> >>> >>> This is arguably incorrect. If one decreased the size of 'msgbuf' or >>> made ctlstring larger so that sizeof(msgbuf) <= strlen(ctlstring), this >>> this would omit the trailing NUL on 'msgbuf'. Even if that's not true, this >>> is overwriting every byte of 'msgbuf' beyond the the copy of 'ctlstring' >>> with 0s, seemingly unnecessarily. >>> >> >> Right that could be strlcpy(). Will change that. >> But, on one side you are worried about the performance (which is a >> function like this, arguably means nothing) of filling ~100 zeros, and ... >> >> >> > + for (i = 0; cmds[i]; i++) { >>>> > + csize = strlen(cmds[i]); >>>> > + if ((csize + size + 2) > sizeof(msgbuf)) >>>> > + break; >>>> > + msgbuf[size] = '|'; >>>> > + memcpy(msgbuf + size + 1, cmds[i], csize); >>>> > + size += csize + 1; >>>> > + } >>>> > + msgbuf[size] = 0; >>>> >>> >>> This loop could be replaced with, >>> >>> strlcpy(msgbuf, ctlstring, sizeof(msgbuf)); >>> for (int i = 0; i < nelem(cmds); i++) { >>> strlcat(msgbuf, "|", sizeof(msgbuf)); >>> strlcat(msgbuf, cmds[i], sizeof(msgbuf)); >>> } >>> >> >> ... here, you are suggesting to walk the string from the first byte using >> strlcat() ☺ >> > > I don't care about the performance; I just don't see the point of zeroing > the rest of the buffer. > > >> >> >> -- >> You received this message because you are subscribed to the Google Groups >> "Akaros" group. >> To unsubscribe from this group and stop receiving emails from it, send an >> email to [email protected]. >> To post to this group, send email to [email protected]. >> For more options, visit https://groups.google.com/d/optout. >> > > -- > You received this message because you are subscribed to the Google Groups > "Akaros" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to [email protected]. > To post to this group, send email to [email protected]. > For more options, visit https://groups.google.com/d/optout. > -- You received this message because you are subscribed to the Google Groups "Akaros" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. For more options, visit https://groups.google.com/d/optout.
