#291 ipmitool dcmi power set_limit action (BUG)

[Arvind Kumar]

This command is not working.

const struct dcmi_cmd dcmi_pwrmgmt_action_vals[] = {
{ 0x00, "No Action", "" },
{ 0x01, "Hard Power Off & Log Event to SEL", "" },
{ 0x11, "Log Event to SEL", "" },
{ 0xFF, NULL, NULL }
};

here what needs to be in 3rd field is in second field.
I think code should be
const struct dcmi_cmd dcmi_pwrmgmt_action_vals[] = {
{ 0x00, "disable", "No Action" },
{ 0x01, "power_off", "Hard Power Off & Log Event to SEL" },
{ 0x11, "sel_logging", "Log Event to SEL" },
{ 0xFF, NULL, NULL }
};

[Arvind Kumar]

Also putting one more condition here will work

if(str2val2(option, dcmi_pwrmgmt_set_usage_vals) != 0) //it should skip when option
// is action
{
if (str2uint(value, &lvalue) != 0) {
lprintf(LOG_ERR, "Given %s '%s' is invalid.",
option, value);
return (-1);
}
}

[Zdenek Styblik]

Hello,

thank you for reporting issue with ipmitool. Please, take a look at
https://sourceforge.net/p/ipmitool/bugs/144/ . I think your report is
a duplicate of this one, although previous reportee didn't suggest
solution.

Z.

[Arvind Kumar]

Zdenek Styblik Sir

So can you please check my solution.
I have tested my code and now its working.
My solution second part

if(str2val2(option, dcmi_pwrmgmt_set_usage_vals) != 0) //it should skip when option
// is action
{
if (str2uint(value, &lvalue) != 0) {
lprintf(LOG_ERR, "Given %s '%s' is invalid.",
option, value);
return (-1);
}
}

This problem is there in INTEL provided dcmitool also.

One more doubt is if that bug#144 has been created on 15 Nov 2012, why till date its not corrected.
If u feel my solution is correct please add it to 1.8.14.
I also want to contribute to ipmitool community.

[Zdenek Styblik]

One more doubt is if that bug#144 has been created on 15 Nov 2012, why till date its not corrected.

Because original reportee didn't bother to provide fix and we don't have platforms to fix this issue.

If u feel my solution is correct please add it to 1.8.14.

Please, create patch by issuing cvs diff -u lib/ipmi_dcmi.c and attach it here.

Thanks

[Arvind Kumar]

Please, verify attached patch, so it can be committed prior to 1.8.14 release

ID: 144/291 - dcmi power set_limit action command failure

"dcmi power set_limit action option " fails.
Here option are "disable", "power_off", "sel_logging".

Note-- Here the two diff/patch files differ by only one if condition at line number 35.

Thanks
Arvind Kumar

[Zdenek Styblik]

I don't think proposed patch is the correct solution to the problem in question. Also, patch alters, or changes completely, intended behaviour as described in man page - at least in my opinion.

My guess is there is a bug at line 1676:

if ( argc == 10) {
  /* Let`s initialize dcmi power parameters */
  [...]
} else {
  [...]
}

I doubt this condition is ever going to be true since 'argc' is 4 at this point with command like % dcmi power set_limit action 'No Action';. As a matter of fact, no sub-command under/after 'set_limit' works.

[Arvind Kumar]

Hi Zdenek

I have updated the ticket with patch explanation.
As per my explanation update, only 2nd and 3rd Bug patch will do the work,
and 1st, 4th, and 5th patch is to enable "No Action / disable" feature in
action parameter of set power limit.
Please review it.
Thanks & Regards
Arvind Kumar

On Sat, Feb 8, 2014 at 2:13 AM, Zdenek Styblik stybla@users.sf.net wrote:

I don't think proposed patch is the correct solution to the problem in
question. Also, patch alters, or changes completely, intended behaviour as
described in man page - at least in my opinion.

My guess is there is a bug at line 1676:

if ( argc == 10) {
/ Let`s initialize dcmi power parameters /
[...]} else {
[...]}

I doubt this condition is ever going to be true since 'argc' is 4 at this
point with command like % dcmi power set_limit action 'No Action';. As a
matter of fact, no sub-command under/after 'set_limit' works.

---

• [bugs:#291] http://sourceforge.net/p/ipmitool/bugs/291/ ipmitool dcmi
  power set_limit action (BUG)*

Status: open
Created: Thu Jan 16, 2014 07:29 AM UTC by Arvind Kumar
Last Updated: Sat Jan 18, 2014 06:09 PM UTC
Owner: nobody

This command is not working.

const struct dcmi_cmd dcmi_pwrmgmt_action_vals[] = {
{ 0x00, "No Action", "" },
{ 0x01, "Hard Power Off & Log Event to SEL", "" },
{ 0x11, "Log Event to SEL", "" },
{ 0xFF, NULL, NULL }
};

here what needs to be in 3rd field is in second field.
I think code should be
const struct dcmi_cmd dcmi_pwrmgmt_action_vals[] = {
{ 0x00, "disable", "No Action" },
{ 0x01, "power_off", "Hard Power Off & Log Event to SEL" },
{ 0x11, "sel_logging", "Log Event to SEL" },
{ 0xFF, NULL, NULL }
};

---

Sent from sourceforge.net because you indicated interest in
https://sourceforge.net/p/ipmitool/bugs/291/

To unsubscribe from further messages, please visit
https://sourceforge.net/auth/subscriptions/

[Arvind Kumar]

Hi Zdenek

I disagree with you because

if (argc == 10) is true when we want to set all power limit parameters i.e
ipmitool dcmi power set_limit action sel_logging limit 500 correction 90000 sample 600

Here this condition will be true and this command does not works properly because of SECOND BUG
but if we want to set any one of the parameters then this if condition fails
and else condition works where it calls the function ipmi_dcmi_pwr_slimit()

---

First lets talk of this condition if (argc == 10)
On setting action parameter in (argc==10) condtion it converts string to value based on switch (str2val2(argv[2], dcmi_pwrmgmt_action_vals)) {

FIRST BUG

here switch case for 0x00 (disable) is missing
so this patch
@@ -1696,6 +1702,10 @@

/* action */
switch (str2val2(argv[2], dcmi_pwrmgmt_action_vals)) {
+   case 0x00:
+   /* disable */
+   data[4] = 0x00;
+   break;

SECOND BUG
Here we convert string to value based on dcmi_pwrmgmt_action_vals
so this patch which solves the problem of setting power limit.
Note: But this solves problem of if(argc == 10) but when we individually sets different parameters(else block) then THIRD BUG patch is needed.
@@ -196,9 +196,9 @@

 /* power management set action commands */
 const struct dcmi_cmd dcmi_pwrmgmt_action_vals[] = {
-   { 0x00, "No Action", "" },
-   { 0x01, "Hard Power Off & Log Event to SEL", "" },
-   { 0x11, "Log Event to SEL", ""  },
+   { 0x00, "disable", "    No Action" },
+   { 0x01, "power_off", "  Hard Power Off & Log Event to SEL" },
+   { 0x11, "sel_logging", "Log Event to SEL"  },
    { 0xFF, NULL, NULL      }
 };

---

THIRD BUG

Now lets talk of else condition
Here this function is called for individual parameter i.e(action, limit, correction, sample)

So the main bug here is if you see the function ipmi_dcmi_pwr_slimit()

if (str2uint(value, &lvalue) != 0) ---> this function tries to convert string to uint which is valid in case of
(limit, correction and sample ) as it takes integer values (400, 90000, 600) respectively [For Example].
but it should not do when parameter is action as action accepts (disable, poweroff, sel_logging)

So my this patch

@@ -1456,10 +1456,12 @@

    uint32_t lvalue = 0;
    int i;
  - if (str2uint(value, &lvalue) != 0) {
  + if(str2val2(option, dcmi_pwrmgmt_set_usage_vals) != 0) {
  +     if (str2uint(value, &lvalue) != 0) {
        lprintf(LOG_ERR, "Given %s '%s' is invalid.",
                option, value);
        return (-1);
  +     }
    }

FOURTH BUG
Also when we see at ipmi_dcmi_pwr_slimit() function switch block for parameter action
switch case for 0x00 (disable) is missing
so this patch

@@ -1503,6 +1505,10 @@

    case 0x00:
    /* action */
    switch (str2val2(value, dcmi_pwrmgmt_action_vals)) {
+   case 0x00:
+   /* disable */
+   val.action = 0x00;
+   break;

---

FIFTH BUG
Last but not the least this patch is required to show at terminal this disable feature in action parameter

@@ -187,7 +187,7 @@

 /* set power limit commands */
 const struct dcmi_cmd dcmi_pwrmgmt_set_usage_vals[] = {
-   { 0x00, "action", "    sel_logging | power_off" },
+   { 0x00, "action", "    disable | power_off | sel_logging" },

---

---

So to conclude most important are second and third bug patch and rest is to make our ipmitool more efficient and user-friendly as 1st, 4th, and 5th patch is to enable "No Action / disable" feature in action parameter of set power limit.

[Arvind Kumar]

Also the reason behind not having support for action == disable (1st, 4th and 5th bug)
is that
setting action as disable(No Action) came as part of Spec Errata Markup, V1.1 and passed on to V1.5

In attached jpg, u can see that.

[Zdenek Styblik]

Arvind,

first of all, let me give you a bit of advice.
1) it's a good practice to fix one bug per one patch/diff file
2) provide description what the patch does. You did that, indeed, but it turns out the patch doesn't fix described issue, but like 5 others(?).

Now, I hate to lie and leave people hanging. So, please, take your patch to ipmitool-dev mailing list and get review there from somebody whom has knowledge of 'lib/ipmi_dcmi.c'. Obviously, I'm not the right person and I'm unwilling to make any excuses no matter how hard and horrible reality is.

I'm not only puzzled by how parsing of arguments is done in ipmi_dcmi, but I think it's rather crappy(code in general). But hey, that's my opinion and it is entirely possible I'm just wrong.

if (str2uint(value, &lvalue) != 0) ---> this function tries to convert string to uint which is valid in case of
(limit, correction and sample ) as it takes integer values (400, 90000, 600) respectively [For Example].
but it should not do when parameter is action as action accepts (disable, poweroff, sel_logging)

Perhaps, action shouldn't be send to ipmi_dcmi_pwr_slimit() in the first place. My $0.02 USD.

[Arvind Kumar]

Thanks for your advice sir.
Since I am student and for the first time doing something like this, so failed to adopt the good practice.
In future, I will follow what you have told me. I will create individual patch.

Thanks

[Zdenek Styblik]

Closing in favour of https://sourceforge.net/p/ipmitool/bugs/144/. If the issue persists, please, open a new ticket with reference to either of these two.

Changes are in Git, if you want to re-test.