On Tue, Jul 23, 2013 at 5:53 PM, Jeffrey Bastian <jbast...@redhat.com> wrote: [...] > 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. On Mon, Jul 22, 2013 at 10:33 PM, Jeffrey Bastian <jbast...@redhat.com> wrote: > Hello, > > Calxeda has added some 'cxoem' extensions to ipmitool to manage their > ARM servers: > http://sources.calxeda.com/gitweb/?p=ipmitool.git > > I have been working on adding their patches to the Fedora ipmitool > package: > 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. 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. > 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. > 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. > (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. ------------------------------------------------------------------------------ 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 Ipmitool-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
