Not entirely. This is separate architecture which has much in common with PICMG. Nevertheless, it differes. I uses ipmi_picmg.c as a base for my modifications.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
here are my comments regarding this patch:
1. Despite you get some points for cleaning up formatting, these points are immediately lost as not being in separate patch. Either you do more than you should or you're squashing your commits in the final patch.
Anyway, it makes code review hard. Please, more smaller commits/patches rather one big patch.
2. formatting - mix of spaces and tabs is a no-no. Also, formatting in general.
3. I'm somewhat against vita_discover(ipmi_main_intf) being called every single time for any command what so ever. I know picmg_discover(ipmi_main_intf) is already there and I can't tell you why, but I guess that's something we have to live with. However, adding more calls is not way to go, in my opinion. Or is VITA major player with hit rate 99%? Point is, if Oracle, HP, IBM, Supermicro and so on so forth do the same, then IPMI tool is going to spend years by just detecting stuff before doing stuff.
if(rsp&&!rsp->ccode){if((rsp->data[0]==0)&&((rsp->data[1]&0x0F)==PICMG_ATCA_MAJOR_VERSION||(rsp->data[1]&0x0F)==PICMG_AMC_MAJOR_VERSION)){picmg_avail=1;lprintf(LOG_INFO,"Discovered PICMG Extension %d.%d",(rsp->data[1]&0x0f),(rsp->data[1]>>4));}}elseif(rsp==NULL){lprintf(LOG_INFO,"No Response from Get PICMG Properties");}else{lprintf(LOG_INFO,"Error Response %#x from Get PICMG Properities",rsp->ccode);}
Let's simplify this:
if(rsp==NULL){/* error + return picmg_avail OR picmg_avail = 0 + error msg and I prefer early returns */}elseif(rsp->ccode!=0){/* ditto */}/* Also, it seems like a good idea to check length of data */if((rsp->data[0]==0)&&((rsp->data[1]&0x0F)==PICMG_ATCA_MAJOR_VERSION||(rsp->data[1]&0x0F)==PICMG_AMC_MAJOR_VERSION){/* or invert/split condition above and return picmg_avail = 0 for each failure */picmg_avail=1;}returnpicmg_avail;
Actually, checking of received data length might be good idea at more places.
Reading further:
msg_data[1]=strtoul(argv[0],NULL,0);/* FRU ID */
No! :-) Several places in the code.
if(rsp->ccode){printf("Get FRU Address Info:
else if here???
if(rsp->ccode){printf("Get VSO Capabilities
ditto and at more places in the code.
if(rsp->ccode){printf("returned CC code 0x%02x\n",rsp->ccode);return-1;}else{printf("FRU state policy bits have been updated\n");}return0;
This is a bit weird.
puts("\n");
puts()???
if(argc==0||(!strncmp(argv[0],"help",4))){
argc === 0 shouldn't return 0, but error.
As for help texts in ipmi_vita_main(), put those into print-out functions, please.
Also, it seems to me some of error messages go to STDOUT instead of STDERR.
And that's all for now.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
1. Despite you get some points for cleaning up formatting,
these points are immediately lost as not being in separate
patch. Either you do more than you should or you're squashing
your commits in the final patch.
Anyway, it makes code review hard. Please, more smaller
commits/patches rather one big patch.
Understood. I'll think how to proceed here. Probably, I'll come with
a separate patch for formatting changes.
2. formatting - mix of spaces and tabs is a no-no. Also,
formatting in general.
Ok.
3. I'm somewhat against vita_discover(ipmi_main_intf)
being called every single time for any command what so ever. I
know picmg_discover(ipmi_main_intf) is already there
and I can't tell you why, but I guess that's something we have
to live with. However, adding more calls is not way to go, in
my opinion. Or is VITA major player with hit rate 99%? Point
is, if Oracle, HP, IBM, Supermicro and so on so forth do the
same, then IPMI tool is going to spend years by just detecting
stuff before doing stuff.
VITA is an IPMI-based standard, much like PICMG.
IPMItool has no way to specify standard it is going to operate with.
So, the detection routine is the only choice here.
In order to make it very properly, the ipmitool infrastructure
shall be rethinked to take into account that there are
several IPMI-based standards which have some peculiar features.
I'm not totally against that, but it will require significantly more
effort than it was required to just add a detection routine.
I do not believe there will be plenty of such standards in the near
future.
No more comments below, the rest part will just be addressed.
if(rsp&&!rsp->ccode){ if((rsp->data[0]==0)&& ((rsp->data[1]&0x0F)==PICMG_ATCA_MAJOR_VERSION ||(rsp->data[1]&0x0F)==PICMG_AMC_MAJOR_VERSION)){ picmg_avail=1; lprintf(LOG_INFO,"Discovered PICMG Extension %d.%d", (rsp->data[1]&0x0f),(rsp->data[1]>>4)); } }elseif(rsp==NULL){ lprintf(LOG_INFO,"No Response from Get PICMG Properties"); }else{ lprintf(LOG_INFO,"Error Response %#x from Get PICMG Properities",rsp->ccode); }
Let's simplify this:
if(rsp==NULL){ / error + return picmg_avail OR picmg_avail = 0 + error msg and I prefer early returns / }elseif(rsp->ccode!=0){ / ditto / } / Also, it seems like a good idea to check length of data / if((rsp->data[0]==0)&& ((rsp->data[1]&0x0F)==PICMG_ATCA_MAJOR_VERSION ||(rsp->data[1]&0x0F)==PICMG_AMC_MAJOR_VERSION){ / or invert/split condition above and return picmg_avail = 0 for each failure / picmg_avail=1; } returnpicmg_avail;
Actually, checking of received data length might be good idea
at more places.
Reading further:
msg_data[1]=strtoul(argv[0],NULL,0);/ FRU ID /
No! :-) Several places in the code.
if(rsp->ccode){ printf("Get FRU Address Info:
else if here???
if(rsp->ccode){ printf("Get VSO Capabilities
ditto and at more places in the code.
if(rsp->ccode){ printf("returned CC code 0x%02x\n",rsp->ccode); return-1; }else{ printf("FRU state policy bits have been updated\n"); } return0;
This is a bit weird.
puts("\n");
puts()???
if(argc==0||(!strncmp(argv[0],"help",4))){
argc === 0 shouldn't return 0, but error.
As for help texts in ipmi_vita_main(), put those into
print-out functions, please.
Also, it seems to me some of error messages go to STDOUT
instead of STDERR.
And that's all for now.
[bugs:#320] VITA 46.11
support
Status: open
Group: version-1.8.15
Created: Tue Apr 15, 2014 12:20 PM UTC by
Dmitry Bazhenov
Last Updated: Tue Nov 11, 2014 01:10 PM UTC
Owner: nobody
The attached patch adds support for VITA 46.11 systems.
Ticket moved from /p/ipmitool/patches/94/
I can't help but wonder, is
lib/ipmi_vita.ccopy-paste oflib/ipmi_picmg.c?Not entirely. This is separate architecture which has much in common with PICMG. Nevertheless, it differes. I uses ipmi_picmg.c as a base for my modifications.
Hello, Zdenek,
Do you want me to rebase the patch against the latest trunk?
Regards,
Dmitry
Dmitry,
Please rebase the patch against the latest trunk and I'll take a look at it
Rebased to the current top of branch.
Dmitry,
here are my comments regarding this patch:
1. Despite you get some points for cleaning up formatting, these points are immediately lost as not being in separate patch. Either you do more than you should or you're squashing your commits in the final patch.
Anyway, it makes code review hard. Please, more smaller commits/patches rather one big patch.
2. formatting - mix of spaces and tabs is a no-no. Also, formatting in general.
3. I'm somewhat against vita_discover(ipmi_main_intf) being called every single time for any command what so ever. I know picmg_discover(ipmi_main_intf) is already there and I can't tell you why, but I guess that's something we have to live with. However, adding more calls is not way to go, in my opinion. Or is VITA major player with hit rate 99%? Point is, if Oracle, HP, IBM, Supermicro and so on so forth do the same, then IPMI tool is going to spend years by just detecting stuff before doing stuff.
Let's simplify this:
Actually, checking of received data length might be good idea at more places.
Reading further:
No! :-) Several places in the code.
else if here???
ditto and at more places in the code.
This is a bit weird.
puts()???
argc === 0 shouldn't return 0, but error.
As for help texts in ipmi_vita_main(), put those into print-out functions, please.
Also, it seems to me some of error messages go to STDOUT instead of STDERR.
And that's all for now.
<html>
<head>
<meta content="text/html; charset=ISO-8859-1" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Please, see my comment below.
04.12.2014 16:03, Zdenek Styblik пишет:
Understood. I'll think how to proceed here. Probably, I'll come with
a separate patch for formatting changes.
Ok.
VITA is an IPMI-based standard, much like PICMG.
IPMItool has no way to specify standard it is going to operate with.
So, the detection routine is the only choice here.
In order to make it very properly, the ipmitool infrastructure
shall be rethinked to take into account that there are
several IPMI-based standards which have some peculiar features.
I'm not totally against that, but it will require significantly more
effort than it was required to just add a detection routine.
I do not believe there will be plenty of such standards in the near
future.
No more comments below, the rest part will just be addressed.
</body>
</html>
Addressed review comments.
Still happening :\
Addressed review comments. Hope there will be no more.
I'm properly confused. Is mix of tabs and spaces intentional then?
I removed all such entries. Please, review.
I'm
... Feb 5 09:32 6-vita-support.diff cat -vt 6-vita-support.diff + default: +^Ilprintf(LOG_NOTICE, "Unknown command"); +^Icmd = VITA_CMD_HELP; +^Ishow_help = 1; +^Ibreak; + } + + if (show_help) { +^Ilprintf(LOG_NOTICE, "%s", val2str(cmd, vita_help_strings)); + } + + return rc; +}Also, number of warnings is ~ +150, although I admit this could be caused by my GCC.
Ah, understood. That's an indentation format. Will change it.
Fixed ipmi_vita.c indentation format.
Committed into git. Thanks.