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

[Levi Pearson]

On Wed, 2007-04-25 at 18:28 -0700, Al Chu wrote:
> 
> 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'.???

Shouldn't be very difficult to change this.

> 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?

Will do on the null checks.  I figured buffer-length input parameters
were fairly pointless since both buffer sizes depend on
IPMI_MAX_K_G_LENGTH, but I guess it assures the caller will indeed think
about the sizes of the buffers they're passing.  I'll add those too.


> 
> 2)

> errno = 0;
> outbuf[j] = strtoul(buf, &p, 16);
> if (errno || (p != buf + 2))

Right, forgot to check that one.


> 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')

If the null check is true, there's a continue, which skips the second
check entirely.  So whenever the second check is run, we know k_g[i] !=
'\0', so we only need to check if there was a null found before this
character.

> 4)
> I believe this error can be caused by invalid length or invalid
> formatting?  So maybe just "k_g invalid"?
> 

That's correct.  I changed the other length messages to generic invalid
messages, but somehow that one slipped through.

> 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)
> 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().
> 

Will do on this one.


>7)
>
>I think you missed several uses of conf->k_g in ipmipower_powercmd.c.
>

I got those, but apparently I forgot to hit save on that particular
buffer.


Hopefully I'll have all these changes incorporated sometime later today.
Thanks for the review.

                --Levi