Hey Levi, And one more comment:
7) I think you missed several uses of conf->k_g in ipmipower_powercmd.c. Al > Hey Levi, > >> I decided to go with the hex by default, strings prefixed with s: >> method. > > Thinking about this a bit. I think the hex mode should be the non- > default. The reason is that atleast a few other apps (most notably > Conman) already assume string input by default. Some others might have > scripts and such. Would it be difficult to convert? Doesn't look like > it'll be too bad a change in the code. People that want to input hex > (lets say via Conman) they can pass a string prefixed by '0x'.??? > >> I left out the -'s in the hex mode, but could add them in later >> if wanted. I put the kg parser and output formatter in >> common/src/ipmi-common.c, but I'm not sure if that's the best place for >> them. > > Seems fine. > > Fundamentally, the patch looks fine. A number of comments are below. > (all visual inspection, not tested/tried.) > > 1) > > +parse_kg(unsigned char *outbuf, unsigned char *inbuf) > +format_kg(unsigned char *outbuf, unsigned char *k_g) > > Could we add some null checks and some buffer-length input parameters + > checks? > > 2) > > + p = buf; > + errno = 0; > + outbuf[j] = strtoul(buf, &p, 16); > + if (errno) > > Shouldn't this be: > > errno = 0; > outbuf[j] = strtoul(buf, &p, 16); > if (errno || (p != buf + 2)) > > ?? > > 3) > > + for (i = 0; i < IPMI_MAX_K_G_LENGTH; i++) > + { > + if (k_g[i] == 0) > + { > + foundnull = 1; > + continue; > + } > + if (!(isgraph(k_g[i]) || k_g[i] == ' ') || foundnull) > + { > + printable = 0; > + break; > + } > + } > > I think we want: > > if (!(isgraph(k_g[i]) || k_g[i] == ' ') > || (foundnull && k_g[i] != '\0') > > b/c if everything before the null is good, and we found a null, and > everything after the null is still null, it should still be printable? > > 4) > > + rv = parse_kg(conf->k_g, argv[1]); > + > + if (rv != 0) > + cbuf_printf(ttyout, "k_g invalid length\n"); > > I believe this error can be caused by invalid length or invalid > formatting? So maybe just "k_g invalid"? > > 5) > > Related to #1 and #4, multiple errors are possible w/ parse_kg(). Do we > want to perhaps have parse_kg() return multiple possible errors? Off > the top of my head, -1 on fatal error, 0 == no data copied, > formatting/length issue, > 0 length of data copied/parsed?? > > Just an idea. > > 6) > > One general comment about all the ipmipower changes. the "k_g_set" > parameter is a flag actually used to indicate k_g was set on the command > line, so it shouldn't be loaded from the config file (if it is set in > the config file). How about we create a new flag variable like > "k_g_configured"? And set it appropriately in > ipmipower_config_cmdline_parse() and _cb_k_g(). > > Seeing your confusion on this flag indicates to me that I perhaps named > this flag improperly. I should perhaps change it to "set_on_cmdline" or > something. > > Thanks, > Al > >> This patch is against the 0.3.0-stable branch. >> >> --Levi >> >> _______________________________________________ >> Freeipmi-devel mailing list >> Freeipmi-devel@gnu.org >> http://lists.gnu.org/mailman/listinfo/freeipmi-devel > -- > Albert Chu > [EMAIL PROTECTED] > 925-422-5311 > Computer Scientist > High Performance Systems Division > Lawrence Livermore National Laboratory > > > _______________________________________________ > Freeipmi-devel mailing list > Freeipmi-devel@gnu.org > http://lists.gnu.org/mailman/listinfo/freeipmi-devel > -- Albert Chu [EMAIL PROTECTED] 925-422-5311 Computer Scientist High Performance Systems Division Lawrence Livermore National Laboratory _______________________________________________ Freeipmi-devel mailing list Freeipmi-devel@gnu.org http://lists.gnu.org/mailman/listinfo/freeipmi-devel