On Mon, Mar 11, 2013 at 6:32 PM, Ales Ledvinka <aledv...@redhat.com> wrote: > Hello, > > I would ACK it, partially due to backwards compatibility. Rework proposal > below.
I'm sorry, but what? Have you actually thought about the issue you've reported and what the patch does? > Should not it be checking only <0, 192) and leave the rest to the OEM code? I don't think so. And what OEM code? I don't see any OEM code there. > > Although I expect this place to be changing soon. Do you now? Can you tell me when, how and who is going to re-write it? > The rest is more of a know this part a bit more then really a review. ? > > I see the parameters are identified by ipmi_lanp.h enum. > This place is being modified also by one oem patch pending on my list. > https://bugzilla.redhat.com/attachment.cgi?id=705662 > hunk @@ -79,10 +79,17 @@ > To be precise about what the patch does is, it adds more parameters. Sadly though, these are in conflict with the current ones. I'd say, tough cookie. > These are wire protocol id numbers, right? Spec table 23-2 parameter selector. > And the 192 and above OEM reserved, Table 23-4. > Yes, that's the one, the table. > My question really is about the OEM parameters 192 and above > with conflicting OEM overlap. Since the code does not seem to be prepared > to handle that unless I am overlooking something. > Yes, code doesn't seem to be ready. But you're free to write a patch which makes it ready. And they're only in conflict with each other, but they're just fine as a freedom of implementation by OEM. > If so then the 192+ commands are split according to spec: The OEM is > identified > according to the Manufacturer ID field returned by the Get Device ID command. > Yep, that's correct. You should get the OEM ID and the code should do the magic based on this ID. But the magic isn't there. Best regards, Zdenek > > ----- Original Message ----- > From: "Zdenek Styblik" <zdenek.styb...@gmail.com> > To: "ipmitool-devel" <ipmitool-devel@lists.sourceforge.net> > Cc: "Jim Mank" <jm...@hp.com>, "Ales Ledvinka" <aledv...@redhat.com> > Sent: Friday, March 8, 2013 9:45:05 PM > Subject: Code Review - ID: 3600926 - 'lib/ipmi_lanp.c' - check LAN param > > Hello all, > > attached is a diff of proposed change to 'lib/ipmi_lanp.c'. This bug > has been reported by Ales Ledvinka a the problem is this ``if (p == > NULL)'' condition is never met, because struct is on heap, if I > understood and writing it correctly. > Comments? Ideas? I've tested it on small snippet code and it seems to > work just fine. > It's possible such change will be required at more places in > 'lib/ipmi_lanp.c'. > > Thanks, > Z. ------------------------------------------------------------------------------ Symantec Endpoint Protection 12 positioned as A LEADER in The Forrester Wave(TM): Endpoint Security, Q1 2013 and "remains a good choice" in the endpoint security space. For insight on selecting the right partner to tackle endpoint security challenges, access the full report. http://p.sf.net/sfu/symantec-dev2dev _______________________________________________ Ipmitool-devel mailing list Ipmitool-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
