Re: [Freeipmi-devel] Patch for hex k_g keys in ipmipower

[Albert Chu]

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
>> address@hidden
>> http://lists.gnu.org/mailman/listinfo/freeipmi-devel
> --
> Albert Chu
> address@hidden
> 925-422-5311
> Computer Scientist
> High Performance Systems Division
> Lawrence Livermore National Laboratory
>
>
> _______________________________________________
> Freeipmi-devel mailing list
> address@hidden
> http://lists.gnu.org/mailman/listinfo/freeipmi-devel
>


-- 
Albert Chu
address@hidden
925-422-5311
Computer Scientist
High Performance Systems Division
Lawrence Livermore National Laboratory