[Ipmitool-devel] cxoem commands for ipmitool

Post by Jeffrey Bastian
Hello,
Calxeda has added some 'cxoem' extensions to ipmitool to manage their
http://sources.calxeda.com/gitweb/?p=ipmitool.git
I have been working on adding their patches to the Fedora ipmitool
https://bugzilla.redhat.com/show_bug.cgi?id=918296
I have gone through Calxeda's commits and extracted only those that
touch the ipmi_cxoem.{c,h} files. They have other commits that modify
fru, ekanalyzer, and more, but I'll save those for later and focus on
the cxoem extension first.
Attached is a patch that applies to the latest code in CVS.
I've also modified get_lan_param_select() from lib/ipmi_lanp.c as I
described in my email last Friday.
(And I've also added a patch to remove the serial plugin since autotools
complains about a missing file src/plugins/serial/Makefile.in, but this
is just to get it to build.)
Jeff

Hello Jeff,

just to make sure; and you want attached patches accepted and
committed to upstream CVS, right?

Thanks,
Z.

Post by Jeffrey Bastian
------------------------------------------------------------------------------
See everything from the browser to the database with AppDynamics
Get end-to-end visibility with application monitoring from AppDynamics
Isolate bottlenecks and diagnose root cause in seconds.
Start your free trial of AppDynamics Pro today!
http://pubads.g.doubleclick.net/gampad/clk?id=48808831&iu=/4140/ostg.clktrk
_______________________________________________
Ipmitool-devel mailing list
https://lists.sourceforge.net/lists/listinfo/ipmitool-devel

Jeffrey Bastian

2013-07-23 15:53:57 UTC

Post by Zdenek Styblik
just to make sure; and you want attached patches accepted and
committed to upstream CVS, right?

Hello Zdenek,

Yes, if the patches are acceptable, I'd like to see them added to
upstream CVS.

(Currently I am building my own version of ipmitool with cxoem
extensions for Fedora, but if the patches are in upstream CVS, then the
official Fedora packages will include cxoem and I can stop maintaining
private builds.)

Thanks!
Jeff

Zdenek Styblik

2013-07-23 17:48:29 UTC

On Tue, Jul 23, 2013 at 5:53 PM, Jeffrey Bastian <***@redhat.com> wrote:
[...]

Post by Jeffrey Bastian
Yes, if the patches are acceptable, I'd like to see them added to
upstream CVS.

Ok, I wasn't really sure and I'm sorry to say, but they aren't.

Funny, because that's not what you have attached.
I wanted to say adding 'cxoem' shouldn't be a big deal, but I see
couple issues already. Note: I was just reading through and I got
bored in a half after scrolling through couple screens of defines and
structs in the middle of the code.

1] code formatting - minor
2] C++ comments - ehm, say minor as well
3] int i = 0; // this is iterator <<< personally, I find this style of
comments a bit annoying
4] a lot of defs and structures and what not. I'm wondering whether
all of this stuff wouldn't be happier in header file of sorts
5] strtol() - major issue, no way. Replace with appropriate str2*
functions from 'lib/helper.c'. The same goes for all strto*() and
ato*().
6] functions with 9 arguments - really? I'm not saying it has to
change, but it definitely caught my eye. And I wonder whether there
isn't a better way to do it. Perhaps some sort of pointer to struct?
It seemed to me like these functions take pretty much the same
arguments.

Post by Jeffrey Bastian
They have other commits that modify
fru, ekanalyzer, and more, but I'll save those for later and focus on
the cxoem extension first.

Yep, and these changes were rejected almost 5 months ago.

Post by Jeffrey Bastian
I've also modified get_lan_param_select() from lib/ipmi_lanp.c as I
described in my email last Friday.

Your patch is incorrect. I see nothing wrong with code in question.

Post by Jeffrey Bastian
(And I've also added a patch to remove the serial plugin since autotools
complains about a missing file src/plugins/serial/Makefile.in, but this
is just to get it to build.)

You really weren't serious about this one, were you???!!!

My $0.02 USD.

Take it easy,
Z.

Jeffrey Bastian

2013-07-23 20:25:18 UTC

Post by Jeffrey Bastian
I have gone through Calxeda's commits and extracted only those that
touch the ipmi_cxoem.{c,h} files.

Funny, because that's not what you have attached.

I should clarify: the commits that created ipmi_cxoem.{c,h} also
modified other files like ipmi_lanp.{c,h}. For example,
http://sources.calxeda.com/gitweb/?p=ipmitool.git;a=commit;h=671b435

I just excluded the commits that focused solely on other code like fru,
ekanalyzer, sel, etc.

Post by Zdenek Styblik
I wanted to say adding 'cxoem' shouldn't be a big deal, but I see
couple issues already.

Thanks for the review. I'll pass the comments along back to the
developers at Calxeda.

Post by Jeffrey Bastian
I've also modified get_lan_param_select() from lib/ipmi_lanp.c as I
described in my email last Friday.

Your patch is incorrect. I see nothing wrong with code in question.

I'll add more details to the other email thread since that bug is independent
of the cxoem patches.

Post by Zdenek Styblik
You really weren't serious about this one, were you???!!!

Zdenek Styblik

2013-07-24 04:40:07 UTC

Post by Jeffrey Bastian
I should clarify: the commits that created ipmi_cxoem.{c,h} also
modified other files like ipmi_lanp.{c,h}. For example,
http://sources.calxeda.com/gitweb/?p=ipmitool.git;a=commit;h=671b435
I just excluded the commits that focused solely on other code like fru,
ekanalyzer, sel, etc.

I'm almost sure these would be fine. However, changes to
ipmi_lanp.{c,h} aren't as discussed 5 months ago, because they replace
one, resp. existing, OEM functionality with another.

Post by Zdenek Styblik
I wanted to say adding 'cxoem' shouldn't be a big deal, but I see
couple issues already.

Thanks for the review. I'll pass the comments along back to the
developers at Calxeda.

Ok.

Post by Zdenek Styblik
You really weren't serious about this one, were you???!!!

No, I'm an idiot. I forgot the -d flag when I ran 'cvs update' so I
was missing the src/plugins/serial directory. Sorry about that...
PEBKAC... (I use mostly git now so my cvs is very rusty.)

Heh, ok. But my point was that's not how you fix bugs/problems.

Z.

Ales Ledvinka

2013-07-24 14:10:06 UTC

----- Original Message -----
.snip.

Post by Zdenek Styblik
However, changes to
ipmi_lanp.{c,h} aren't as discussed 5 months ago, because they replace
one, resp. existing, OEM functionality with another.

Hello,

Were you able to locate the OEM code that uses the lan options
oem codes defined in ipmi_lanp.h ?
Since I failed to locate any code associated with that. Either
it's some dropped functionality of obsoleted hardware.
Or feature that did not made it into the repository. But that's
speculation. I don't feel like checking whole commit history.

Jeffrey Bastian

2013-07-24 17:38:24 UTC

Post by Ales Ledvinka

However, changes to ipmi_lanp.{c,h} aren't as discussed 5 months
ago, because they replace one, resp. existing, OEM functionality
with another.

Were you able to locate the OEM code that uses the lan options
oem codes defined in ipmi_lanp.h ?
Since I failed to locate any code associated with that. Either
it's some dropped functionality of obsoleted hardware.
Or feature that did not made it into the repository. But that's
speculation. I don't feel like checking whole commit history.

That was my understanding also: the cxoem patch removes some existing
lanp codes, but nothing is using those lanp codes, so no harm done.

The 4 removed lanp codes are:
IPMI_LANP_DHCP_SERVER_IP=192,
IPMI_LANP_DHCP_SERVER_MAC=193,
IPMI_LANP_DHCP_ENABLE=194,
IPMI_LANP_CHAN_ACCESS_MODE=201,

Currently the only file that mentions those codes are the definitions
themselves in ipmi_lanp.h:

$ find . -type f | xargs egrep -l \
'IPMI_LANP_(DHCP_SERVER_IP|DHCP_SERVER_MAC|DHCP_ENABLE|CHAN_ACCESS_MODE)'
./include/ipmitool/ipmi_lanp.h

Jeff

Zdenek Styblik

2013-07-26 06:56:40 UTC

Post by Ales Ledvinka

However, changes to ipmi_lanp.{c,h} aren't as discussed 5 months
ago, because they replace one, resp. existing, OEM functionality
with another.

That was my understanding also: the cxoem patch removes some existing
lanp codes, but nothing is using those lanp codes, so no harm done.
IPMI_LANP_DHCP_SERVER_IP=192,
IPMI_LANP_DHCP_SERVER_MAC=193,
IPMI_LANP_DHCP_ENABLE=194,
IPMI_LANP_CHAN_ACCESS_MODE=201,
Currently the only file that mentions those codes are the definitions
$ find . -type f | xargs egrep -l \
'IPMI_LANP_(DHCP_SERVER_IP|DHCP_SERVER_MAC|DHCP_ENABLE|CHAN_ACCESS_MODE)'
./include/ipmitool/ipmi_lanp.h
Jeff

We had this conversation 5 months ago and as far as I'm concerned,
nothing has changed.

Z.

Albert Chu

2013-07-26 15:45:41 UTC

A quick comment, there appears to be new OEM options under the 'lan'
command of ipmitool.

+ lprintf(LOG_NOTICE, " tftp ipaddr <x.x.x.x> Set tftp
server IP address");
+ lprintf(LOG_NOTICE, " ntp ipaddr <x.x.x.x> Set ntp
server IP address");
+ lprintf(LOG_NOTICE, " tftp port <num> Set tftp
server UDP port num ");
+ lprintf(LOG_NOTICE, " ntp port <num> Set ntp
server UDP port num ");
<http://sources.calxeda.com/gitweb/?p=ipmitool.git;a=commit;h=671b435>

IMO, they should all be moved under the cxoem command. It appears that
these settings are done via Get/Set Lan Parameters, which is why it's in
this section. However, they are OEM specific parameters. Because it's
under the 'lan' section, your average user might believe that it is
available on all IPMI machines, rather than being an OEM specific option.

In addition, what if another vendor comes along and has an alternate OEM
tftp/ntp IP/port setting. Where will it go?

Al

Post by Jeffrey Bastian
I have gone through Calxeda's commits and extracted only those that
touch the ipmi_cxoem.{c,h} files.

Funny, because that's not what you have attached.

I should clarify: the commits that created ipmi_cxoem.{c,h} also
modified other files like ipmi_lanp.{c,h}. For example,
http://sources.calxeda.com/gitweb/?p=ipmitool.git;a=commit;h=671b435
I just excluded the commits that focused solely on other code like fru,
ekanalyzer, sel, etc.

Post by Zdenek Styblik
I wanted to say adding 'cxoem' shouldn't be a big deal, but I see
couple issues already.

Thanks for the review. I'll pass the comments along back to the
developers at Calxeda.

Post by Jeffrey Bastian
I've also modified get_lan_param_select() from lib/ipmi_lanp.c as I
described in my email last Friday.

Your patch is incorrect. I see nothing wrong with code in question.

I'll add more details to the other email thread since that bug is independent
of the cxoem patches.

Post by Zdenek Styblik
You really weren't serious about this one, were you???!!!

No, I'm an idiot. I forgot the -d flag when I ran 'cvs update' so I
was missing the src/plugins/serial directory. Sorry about that...
PEBKAC... (I use mostly git now so my cvs is very rusty.)
Thanks again!
Jeff
------------------------------------------------------------------------------
See everything from the browser to the database with AppDynamics
Get end-to-end visibility with application monitoring from AppDynamics
Isolate bottlenecks and diagnose root cause in seconds.
Start your free trial of AppDynamics Pro today!
http://pubads.g.doubleclick.net/gampad/clk?id=48808831&iu=/4140/ostg.clktrk
_______________________________________________
Ipmitool-devel mailing list
https://lists.sourceforge.net/lists/listinfo/ipmitool-devel

Hank Bruning

2013-07-26 16:20:41 UTC

Albert is correct. Infact the Calxeda OEM Lan parameters have been used for
years.
The are the same LAN parameters that can be used by the PICMG HPM.2 R1.0
(published July 2012) specification and are documented in Table 3-11 HPM.2
LAN Configuration Parameters.
It use 12 LAN parameters to control LAN bridging and redundancy for LAN
interfaces.
In other words the HPM.2 and Calxeda conflict.
Hank Bruning

Post by Albert Chu
A quick comment, there appears to be new OEM options under the 'lan'
command of ipmitool.
+ lprintf(LOG_NOTICE, " tftp ipaddr <x.x.x.x> Set tftp
server IP address");
+ lprintf(LOG_NOTICE, " ntp ipaddr <x.x.x.x> Set ntp
server IP address");
+ lprintf(LOG_NOTICE, " tftp port <num> Set tftp
server UDP port num ");
+ lprintf(LOG_NOTICE, " ntp port <num> Set ntp
server UDP port num ");
<http://sources.calxeda.com/gitweb/?p=ipmitool.git;a=commit;h=671b435>
IMO, they should all be moved under the cxoem command. It appears that
these settings are done via Get/Set Lan Parameters, which is why it's in
this section. However, they are OEM specific parameters. Because it's
under the 'lan' section, your average user might believe that it is
available on all IPMI machines, rather than being an OEM specific option.
In addition, what if another vendor comes along and has an alternate OEM
tftp/ntp IP/port setting. Where will it go?
Al

Post by Jeffrey Bastian
I have gone through Calxeda's commits and extracted only those that
touch the ipmi_cxoem.{c,h} files.

Funny, because that's not what you have attached.

I should clarify: the commits that created ipmi_cxoem.{c,h} also
modified other files like ipmi_lanp.{c,h}. For example,
http://sources.calxeda.com/gitweb/?p=ipmitool.git;a=commit;h=671b435
I just excluded the commits that focused solely on other code like fru,
ekanalyzer, sel, etc.

Post by Zdenek Styblik
I wanted to say adding 'cxoem' shouldn't be a big deal, but I see
couple issues already.

Thanks for the review. I'll pass the comments along back to the
developers at Calxeda.