Hi Flavio,
+static const ipmi_complcmd_dissect ipmi_complcmd_array[] = {
You seem to cover a lot more completion codes than anyone would have
expected. That's great! Thanks.
+ { 0x00, 0x08, 0x80, "Parameter not supported" },
+ { 0x00, 0x08, 0x81, "Attempt to set in progress value" },
+ { 0x00, 0x08, 0x82, "Attempt to write read-only value" },
+ { 0x00, 0x09, 0x80, "Parameter not supported" },
<snip>
+ guint8 netfnr = netfn & 0xFE;
It seems you are masking out the least significant bit of the network
function because that bit accounts for request vs. response network
functions?? Then using that subsequent value for comparisons against
the 'ipmi_complcmd_dissect' table?
I believe that completion codes are only going to be sent with response
messages, so I don't believe there is any need to do this. Perhaps you
could just use response network functions in the 'ipmi_complcmd_dissect'
table and forget about masking out the least significant bit?
Just a thought,
Al
On Wed, 2007-06-20 at 14:26 -0300, Flavio Leitner wrote:
>
> Hi Al Chu,
>
> What you think about this patch?
> Thanks for your review and suggestions.
>
> -Flavio
>
> On Tue, May 29, 2007 at 01:56:45PM -0700, Al Chu wrote:
> > Hi Flavio,
> >
> > (please, keep myself on CC as I'm not subscribed)
> >
> > A few thoughts on your patch:
> >
> > 1) I think you should move the original:
> >
> > /* added by lane */
> > { 0x81, "cannot execute command, SEL erase in progress" },
> >
> > from ipmi_ccode_vals[] into a new array and add command specific error
> > support into get_ccode_cmd_text().
> >
> > 2) I believe your patch works for this particular instance, b/c in 0x39
> > is only used by the get session challenge command. However (if you take
> > a look in table 44-11), there are many "commands" that use the same hex
> > "cmd" code. (i.e. both get device id and get chassis status and get set
> > lan configuration parameters use cmd == 0x01.) So get_ccode_cmd_text()
> > should key off both the cmd and the network function in order to
> > determine which command specific error code string array to use.
> >
> > 3)
> >
> > The coding requirements and semantics for wireshark are largely unknown
> > to me, so I apologize if what I say below is faux pas for wireshark.
> >
> > If a command specific error code is returned but doesn't have an command
> > specific array of strings supported, it should instead say "Command
> > Specific Error" instead of just "Unknown".
> >
> > Al
> >
> > --
> > Albert Chu
> > chu11@xxxxxxxx
> > 925-422-5311
> > Computer Scientist
> > High Performance Systems Division
> > Lawrence Livermore National Laboratory
>
--
Albert Chu
chu11@xxxxxxxx
925-422-5311
Computer Scientist
High Performance Systems Division
Lawrence Livermore National Laboratory