#374 PICMG LED SET is too restrictive with its parameter checking.
In http://sourceforge.net/p/ipmitool/source/ci/a8f63417285e876dd9fc7fc301c555c2178c8407/ parameter checking was added to several functions, including ipmi_picmg_set_led_state(). However, in this function the parameter checking - more precisely the is_led_duration() parameter checking - is too fierce.
Checking the V3.0 PICMG specification (which I can't find online...) the duration is ignored and set to 0 if Byte 4 (LED function) is greater than or equal to 0xFC - i.e. local control or ON.
Unfortunately I have now come across at least one IPMC manufacturer who mandates that this parameter MUST be 0. I am attempting to get them to be more lenient in their parameter checking. However, it would be useful for IPMItool to allow 0 as an LED duration under these conditions and, potentially, to set the parameter to 0 under these conditions.
This affects all versions of IPMItool from V1.8.13 to V1.8.15. My current work around is to either use an older version of IPMItool, or to use "ipmitool raw".
Please let me know if a patch would be useful, and I'll try and work out how to fork/edit/push on SourceForge!
Thanks,
Robert
Related
Obsolete (git): 54ff1fc204161f159b6832b0
Obsolete (git): 55005a9a04161f6e93e0acf2
Obsolete (git): 5506cedbc4d1044f159adcbe
I'm sorry to hear that, although I'm fairly sure that checking is based on some PICMG specification.
Taking a picture of page in question with digital camera or scanning it should do the trick.
Well, one could detect OEM ID and act upon it. Or is duration 0 valid argument, like in 0 == off?
I've just found something relevant online. The comment here is word for word the part of the spec I've had quoted to me (I don't have a copy myself). http://svn.ohwr.org/white-rabbit/trunk/software/watchdog/avr_rev1/ic2sw_ipmi/ipmi.h - Search for "LED on-time".
"On-duration: LED on-time in tens of milliseconds if (1 = Byte 4 = FAh) Lamp Test time in hundreds of milliseconds if (Byte 4 = FBh). Lamp Test time value must be less than 128. Other values when Byte 4 = FBh are reserved. Otherwise, this field is ignored and shall be set to 0h."
It's the otherwise that I don't think IPMItool is respecting. It's not that 0 is a valid argument - more that, if you're setting the LED to on, the duration is irrelevant (as it's infinity) and so should be a) ignored and b) set to 0 - and this is explicit in the spec.
I'd like IPMItool to at least do a), and possibly do b) - because systems exist that mandate b).
Thanks,
Robert
I see. Feel free to create a patch for it. It would take considerable time to get my head around it as both of my jobs kicked in and this project is on (my) back burner now. So, patch review should be faster and easier for me than actually fixing it.
Thanks
Patch created. I have tested that this patch builds and fixes this issue on machines that I have access to (CentOS 6.6).
Please, can you elaborate on what your patch does and why it does it in this way?
Last edit: Zdenek Styblik 2015-03-11
In writing this I realised there was a minor flaw in my previous code. I've fixed this and will re-send the merge request after posting this. To describe my (new) code changes:
The valid LED duration values depend on the value of the LED function parameter. I therefore pass the LED function (which has already been validated/set by is_led_function()) into is_led_duration().
After validating that the duration can be parsed into a char, I then check the LED function. If the LED function is 0 (OFF) or > 0xFB (ON) I set the duration to 0. Otherwise I apply the previous validation and check that it's between (not equal to) 0 and 128.
The minor flaw in my previous merge request was that I wouldn't set the duration to 0 in all circumstances where the LED function was 0 or > 0xFB.
I will accept a change to not over-ride the LED duration in the cases where the LED is being set ON or OFF. (Which is not to say I won't accept other changes. Just that this part is a change that I consider slightly controversial, although within spec, and that I am not that fussed about!).
Thanks,
Robert
To be honest with you, I've asked on purpose. I just don't see reason why to pass led_function around. It just didn't make sense to me. Also, I guess I guess duration within <0..127> range is acceptable.
Anyway, my counter-patch is attached.
This does mean that is_led_duration() doesn't always catch invalid LED durations - but I guess you know that.
Your code doesn't ignore a duration of e.g. 128 - but I also can't see someone passing this in and expecting it to work, so I don't really mind.
One comment on your patch - it also needs to set msg_data[4] to 0 if msg_data[3] is 0 (i.e. the LED is being set OFF).
If you want to use your patch, feel free.
I'm sorry, but what do you mean? Can you elaborate?
:::C
if ((led_function == 0) || (led_function > 0xFB)) {
[...]
} else if (led_duration_ptr > 0 && led_duration_ptr <= 127) {
[...]
Was this meant as a backdoor for durations greater than 127 of whatever?
I see. True, I didn't catch that.
I'd like to keep function in question generic and leave up decisions up to caller.
Attached is updated version. As for the patch, yeah, it's preferred. If you want, apply it and create PR with it. After all, it's entirely based on your work.
Last edit: Zdenek Styblik 2015-03-11
I was just looking at your new patch, and was about to apply it, when I saw that you had added the spec snippet into your code. I then re-read the spec, and realised we've both misunderstood it and there is actually another (minor) issue as well.
Spec says (with some formatting to make boundaries clearer):
So, this has 3 parts to it.
If 0x1 <= LED function <= 0xFA, then it's the ON time in tens of milliseconds - with no restriction on value. i.e. Maximum LED on time of 255 * 10 = 2550 milliseconds (2.55 seconds).
If LED function == 0xFB, then it's a lamp test ON time in hundreds of milliseconds, with a restriction on the on value of < 128. i.e. Maximum LED on time of 127 * 100 = 12700 milliseconds (12.7 seconds).
If LED function == 0 OR LED function > 0xFB, then any value is accepted, but it should be set to 0.
If you agree with this, then both of our patches (and the existing code) have in addition limited the maximum on time of LEDs to 1.27 seconds.
With this in mind, I have another patch file attached. This has 3 changes.
1) is_led_duration() now allows 0 - including for LED lamp test (there's nothing which excludes 0 in the spec).
This actually fixes my original bug, by allowing 0 for LED ON and OFF.
2) Only check LED duration if we're doing a lamp test.
At this point I think we allow all spec allowed values - i.e. > 127 values are allowed for LED ON times.
3) If we're turning the LED OFF or ON, set the value to 0.
Sorry for the sudden switch on this,
Robert
Edited to add help text changes to diff.
Last edit: Robert Sworder 2015-03-12
As I've said, I'm quite super busy. Therefore, I was just mimicking your code. I have couple questions to new diff.
Are you aware this means user input like 'abc' will pass as well?
Yet these are limited in is_led_duration() to <0..127>. I'm a bit confused, sorry. Wouldn't it be better to move the former code out of that huge equation into separate one? And perhaps even the whole <0..127> limitation out of is_led_duration()?
Just asking.
Sorry for the delay in responding.
It's actually worse than your comments above suggest. Unless it's a lamp test we don't go through is_led_duration(), and so won't set msg_data[4] at all.
I attach a new diff. This new diff:
- Removes is_led_duration() entirely.
- Puts all the code from is_led_duration() and the fixes for this bug inline into ipmi_picmg_set_led_state().
- Alters the help text accordingly.
I don't like having all of this code inline in ipmi_picmg_set_led_state(). However, doing anything else feels very similar to my pull request from 10th March, which you rejected because you didn't like having the LED function passed into is_led_duration() - despite this being required to know what's a valid duration.
Let me know what you think of this diff. I've tested this code - it works, and does seem to stop invalid parameters.
Robert
Robert, if you're still alive, would you care to produce git patch?
Thank you,
Z.
Hello!
Thanks for getting back to me after all this time :)
FTR, I've been running with this patch (based off CentOS ipmitool RPM) since I created it.
And here's the GIT patch. Hopefully it's in the correct format.
Thanks!
Robert
Heh, sorry about that.
Cool.
It is.
Thanks,
Z.
Hi Zdenek, this is my patch proposal for the problem.
Hello Dmitry,
thank you, but we've been through this iteration already earlier on and it has been rejected. I'm quite fine with the latest patch.
Z.
Of course, if there are problems with the patch, I'd like to hear about them.
Z.
Hello Zdenek. Didn't notice that you've alredy got a patch. It looks good. Thanks.