[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Freeipmi-devel] Patch for hex k_g keys in ipmipower
From: |
Levi Pearson |
Subject: |
Re: [Freeipmi-devel] Patch for hex k_g keys in ipmipower |
Date: |
Thu, 26 Apr 2007 10:17:19 -0600 |
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