freeipmi-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Al Chu
Subject: Re: [Freeipmi-devel] Patch for hex k_g keys in ipmipower
Date: Wed, 25 Apr 2007 18:28:18 -0700

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]