search

mkumatag / openbmc-automation

Test OpenBMC Distribution, moved to - https://github.com/openbmc/openbmc-test-automation
Apache License 2.0
5 stars 11 forks source link

This fix is to catch command execution error in ipmi library code #67

Closed gkeishin closed 8 years ago

gkeishin commented 8 years ago

This will catch errors at command execution level and validates if it returns success or errors.

This change is Reviewable

mkumatag commented 8 years ago

I always recommend to have should and compare statements in the testcases or in the individual files instead of putting into library function. Because in future lets say if you are intentionally want to fail the command and see what really happens then you will not be able to this function because this always checks for not to have error.

Instead I would do following to fix this issue is:

Will return the stdout and error both from the Run IPMI Command function and use that check in the testcase where I'm calling( to avoid repeated should statement everywhere I will add one more wrapper keyword in the testfile which will compare all these things).

mkumatag commented 8 years ago

And even it will not give me flexibility if I wanna compare/parse error message!

Previously, mkumatag (Manjunath A Kumatagi) wrote… > I always recommend to have should and compare statements in the testcases or in the individual files instead of putting into library function. Because in future lets say if you are intentionally want to fail the command and see what really happens then you will not be able to this function because this always checks for not to have error. > > Instead I would do following to fix this issue is: > > Will return the stdout and error both from the `Run IPMI Command` function and use that check in the testcase where I'm calling( to avoid repeated should statement everywhere I will add one more wrapper keyword in the testfile which will compare all these things). >

Reviewed 1 of 1 files at r1. Review status: all files reviewed at latest revision, all discussions resolved.

_Comments from Reviewable_

gkeishin commented 8 years ago

The problem with executing commands in general is the return error message could be different and could varies for reasons like the binary not missing, command not found, wrong parameters, timeouts.. faulted... So comparing for the output is not a wise thing when we don't know what else error consoles o/p it will throw in advance...

We could have fix using the rc of the return code.. but that doesn't help to identify or locate the issue..

For example with this fix: 13:37:14.695 INFO Executing command '/tmp/ipmitool -I dbus raw 0xF1 0x8 0x05 0x80 0x00 0x00 0x00 0x00'. 13:37:16.527 INFO Command exited with return code 1. 13:37:16.528 INFO ${output} = 13:37:16.528 INFO ${stderr} = Unable to send RAW command (channel=0x0 netfn=0x31 lun=0x0 cmd=0x8 rsp=0xc1): Invalid command

we can now see with stderr and we can't now really debug what the problem is if encountered ahead...

rahulmah commented 8 years ago

@gkeishin : If we are not passing the error message, will have problem with reading error message for negative scenarios. So we should also return error message for such widely use function. Note: This implementation will have effect on many places, so make sure to get them working too.

mkumatag commented 8 years ago

I agree with @rahulmah here.

Previously, rahulmah (Rahul Maheshwari) wrote… > @gkeishin : If we are not passing the error message, will have problem with reading error message for negative scenarios. So we should also return error message for such widely use function. Note: This implementation will have effect on many places, so make sure to get them working too. >

Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

_Comments from Reviewable_

gkeishin commented 8 years ago

Just a heads up... there are 34 good cases here with this library function all.. Changing in all the places wouldn't be a good idea .

Since this will take some time to think it through to come up with a better solution to encapsulate the review comment changes and the migration of this Repo is going to happen soon.. I would want to ABORT this push and push a new one with the new changes onto the NEW Repo... Let me know if anyone of you have concerns..

rango@ubuntu:~/openbmc-automation/tests$ grep -ri "Run IPMI Command" * | wc -l 34 <-- 34 places if checks needs to be added on the caller side...

rango@ubuntu:~/openbmc-automation/tests$ grep -ri "Run IPMI Command" * test_bootpolicy.robot: Run IPMI command 0x0 0x8 0x05 0x80 0x00 0x00 0x00 0x00 test_bootpolicy.robot: Run IPMI command 0x0 0x8 0x05 0xC0 0x00 0x00 0x00 0x00 test_boot.robot: Run IPMI command 0x0 0x8 0x05 0x80 0x00 0x00 0x00 0x00 test_boot.robot: Run IPMI command 0x0 0x8 0x05 0x80 0x04 0x00 0x00 0x00 test_boot.robot: Run IPMI command 0x0 0x8 0x05 0x80 0x08 0x00 0x00 0x00 test_boot.robot: Run IPMI command 0x0 0x8 0x05 0x80 0x0C 0x00 0x00 0x00 test_boot.robot: Run IPMI command 0x0 0x8 0x05 0x80 0x14 0x00 0x00 0x00 test_boot.robot: Run IPMI command 0x0 0x8 0x05 0x80 0x18 0x00 0x00 0x00 test_sensors.robot: Run IPMI command 0x06 0x36 test_sensors.robot: Run IPMI command 0x04 0x30 ${x} 0x01 0x00 0x35 0x00 0x00 0x00 0x00 0x00 0x00 test_sensors.robot: Run IPMI command 0x04 0x30 ${x} 0xa9 0x00 0x04 0x00 0x00 0x00 0x00 0x14 0x00 test_sensors.robot: Run IPMI command 0x04 0x30 ${x} 0xa9 0x00 0x04 0x00 0x00 0x00 0x00 0x0e 0x00 test_sensors.robot: Run IPMI command 0x04 0x30 ${x} 0xa9 0x00 0x02 0x00 0x00 0x00 0x00 0x00 0x00 test_sensors.robot: Run IPMI command 0x04 0x30 ${x} 0xa9 0x00 0x01 0x00 0x00 0x00 0x00 0x20 0x00 test_sensors.robot: Run IPMI command 0x04 0x30 ${x} 0x00 0x00 0x01 0x00 0x00 0x00 0x00 0x20 0x00 test_sensors.robot: Run IPMI command 0x04 0x30 ${x} 0x00 0x00 0x04 0x00 0x00 0x00 0x00 0x20 0x00 test_sensors.robot: Run IPMI command 0x04 0x30 ${x} 0x00 0x00 0x02 0x00 0x00 0x00 0x00 0x20 0x00 test_sensors.robot: Run IPMI command 0x04 0x30 ${x} 0x00 0x00 0x01 0x00 0x00 0x00 0x00 0x20 0x00 test_sensors.robot: Run IPMI command 0x04 0x30 ${x} 0xa9 0x00 0x80 0x00 0x00 0x00 0x00 0x20 0x00 test_sensors.robot: Run IPMI command 0x04 0x30 ${x} 0xa9 0x00 0x00 0x00 0x80 0x00 0x00 0x20 0x00 test_sensors.robot: Run IPMI command 0x04 0x30 ${x} 0xa9 0xff 0x00 0x01 0x00 0x00 0x00 0x20 0x00 test_sensors.robot: Run IPMI command 0x04 0x30 ${x} 0x00 0x00 0x00 0x00 0x00 0x01 0x00 0x20 0x00 test_sensors.robot: Run IPMI command 0x04 0x30 ${x} 0xa9 0x00 0x80 0x00 0x00 0x00 0x00 0x20 0x00 test_sensors.robot: Run IPMI command 0x04 0x30 ${x} 0xa9 0x00 0x00 0x00 0x80 0x00 0x00 0x20 0x00 test_sensors.robot: Run IPMI command 0x04 0x30 ${x} 0xa9 0xff 0x00 0x01 0x00 0x00 0x00 0x20 0x00 test_sensors.robot: Run IPMI command 0x04 0x30 ${x} 0x00 0x00 0x00 0x00 0x00 0x01 0x00 0x20 0x00 test_sensors.robot: Run IPMI command 0x04 0x30 ${x} 0xa9 0x00 0x40 0x00 0x00 0x00 0x00 0x20 0x00 test_sensors.robot: Run IPMI command 0x04 0x30 ${x} 0xa9 0xff 0x00 0x00 0x40 0x00 0x00 0x20 0x00 test_sensors.robot: Run IPMI command 0x04 0x30 ${x} 0x00 0x00 0x10 0x00 0x00 0x00 0x00 0x20 0x00 test_sensors.robot: Run IPMI command 0x04 0x30 ${x} 0x00 0x00 0x00 0x00 0x10 0x00 0x00 0x20 0x00 test_sensors.robot: Run IPMI command 0x04 0x30 ${x} 0xa9 0x00 0x40 0x00 0x00 0x00 0x00 0x20 0x00 test_sensors.robot: Run IPMI command 0x04 0x30 ${x} 0x00 0x00 0x00 0x00 0x40 0x00 0x00 0x20 0x00 test_sensors.robot: Run IPMI command 0x04 0x30 ${x} 0x00 0x00 0x10 0x00 0x00 0x00 0x00 0x20 0x00 test_sensors.robot: Run IPMI command 0x04 0x30 ${x} 0x00 0x00 0x00 0x00 0x10 0x00 0x00 0x20 0x00

rahulmah commented 8 years ago

@gkeishin : Aborting is fine.

gkeishin commented 8 years ago

I don't foresee value add to catch the error for a command execution at the test code level nor any meaningful sense.. since by nature of command line execution.. whatever we execute should not fail..

Should it fail we should catch at the source where we are really executing and not propagate.. we shouldn't compare this command line execution with the REST call since the context and the nature of output are of different behaviors and results..

Having said that, Now the only other thing we could do here is... revamp the Run IPMI command into READ and WRITE commands module for IPMI... For READ return whatever o/p and for WRITE catch at the source...

Let me know whats your thoughts on it.

causten commented 8 years ago
:lgtm:
Previously, gkeishin wrote… > I don't foresee value add to catch the error for a command execution at the test code level nor any meaningful sense.. since by nature of command line execution.. whatever we execute should not fail.. > > Should it fail we should catch at the source where we are really executing and not propagate.. we shouldn't compare this command line execution with the REST call since the context and the nature of output are of different behaviors and results.. > > Having said that, Now the only other thing we could do here is... revamp the Run IPMI command into READ and WRITE commands module for IPMI... For READ return whatever o/p and for WRITE catch at the source... > > Let me know whats your thoughts on it. >

Reviewed 1 of 1 files at r1. Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from Reviewable