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

[Albert Chu]

Hey Levi,

Looks pretty good.  Some minor nit-picks.

1)

+          q = buf;

I think this line is unnecessary???

2)

ipmipower manpage for describing the new possible hex input for -k and -K.

3)

ipmipower_config.c: In function `ipmipower_config_cmdline_parse':
ipmipower_config.c:387: warning: suggest parentheses around assignment
used as truth value
ipmipower_config.c:407: warning: suggest parentheses around assignment
used as truth value
ipmipower_config.c: In function `_cb_k_g':
ipmipower_config.c:756: warning: suggest parentheses around assignment
used as truth value

I think it's saying for parens around:

rv = parse_kg(conf->k_g, IPMI_MAX_K_G_LENGTH, kg)

4)

+  if (rv = parse_kg(conf->k_g, IPMI_MAX_K_G_LENGTH, data->string) != 0)

I think this should be < 0?

5)

+          if (p[i+1] == '\0')

+      if (k_g[i] == 0)
+          if (k_g[i] == 0)

Could we use '\0' everywhere for consistency.

6)

+          if (rv = parse_kg(conf->k_g, IPMI_MAX_K_G_LENGTH, optarg) < 0)
+            {
+              err_exit("Command Line Error: Invalid K_g");
+            }
+          else
+            {
+              if (rv > 0)
+                conf->k_g_configured = IPMIPOWER_TRUE;
+            }

How about collapsing this into:

if ((rv ...))
   err_exit("blah");
if (rv > 0)
   conf->k_g_ blah blah ...

Otherwise I think it looks good.

Al


> This version should address the issues from the review of the previous
patch.  I've tested this one a bit more and it seems pretty solid to me.
If you like it, I'll carry on making these changes in the other
> 2.0-aware utilities, starting with ipmiconsole.
>
>               --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