On Tue, Mar 12, 2013 at 11:12 AM, Ales Ledvinka <aledv...@redhat.com> wrote: > Correct me if I am overlooking something, but the current 192+ codes are > unreachable, right? >
It's seem so, yes. > It's annotated as the 1.1 initial revision but I do not see it used nowdays > anywhere in the code (macro name/number code). > The 192 in hpfwm does not seems to be related. *shrug* > > So what would be acceptable strategy for the cxoem patch merging? Is it > simply replacing the conflicting parameters and extend the lanp as it is now > or adding the oem "infrastructure"? To be honest, I'd like to see the latter. If it's not today, it's going to be tomorrow. Just because some values aren't used and can be replaced doesn't truly solve the problem, does it? My $0.02 USD. Best regards, Zdenek > > ----- Original Message ----- > From: "Zdenek Styblik" <zdenek.styb...@gmail.com> > To: "Ales Ledvinka" <aledv...@redhat.com> > Cc: "Jim Mank" <jm...@hp.com>, "ipmitool-devel" > <ipmitool-devel@lists.sourceforge.net> > Sent: Tuesday, March 12, 2013 9:25:05 AM > Subject: Re: Code Review - ID: 3600926 - 'lib/ipmi_lanp.c' - check LAN param > > 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
