search

openbmc / ipmi-fru-parser

GNU General Public License v3.0
10 stars 25 forks source link

Fix 'dbus.String not found in lookup' error message #13

Closed adamliyi closed 8 years ago

adamliyi commented 8 years ago

Previously the ipmi-fru-parser will call dbus method getObjectFromId() for each fru areas to get the dbus object path of fru area, even if the fru area does not exist. Also ipmi_validate_fru_area() may be invoked multipul times before full fru data is parsed. This will generate duplicated error messages from SystemManager:, e.g: "ERROR SystemManager: dbus.String(u'INTERNAL_13') not found in lookup". In this case, "INTERNAL" area does not exists for FRU 13.

This patch tries to reduce the messages. For FRUs managed by hostboot, ipmi-fru-parser only asks for dbus object path when updating the fru data to BMC inventory.

For FRUs mananged by BMC (eeprom), ipmi-fru-parser also asks for dbus object path when setting the fru's 'present' and 'fault' status. ipmi-fru-parser needs to call getObjectFromId() to know whether a fru should be present, in order to decide the 'present' and 'fault' status. So this message will still be triggered when SystemManager does not define object path for a fru area.

This patch also sets 'present' and 'fault' properly when there is error processing a BMC managed FRU.

Signed-off-by: Yi Li adamliyi@msn.com

This change is Reviewable

vishwabmc commented 8 years ago

Review status: 0 of 1 files reviewed at latest revision, 5 unresolved discussions.

writefrudata.C, line 106 [r1] (raw file): Consider a case where opening a fru file failed, this call would still result in lookup errors since we do not have anything populated yet.

So in this case, the vector has everything like internal, chasi, board, multi and product..

Am I missing something ?

_writefrudata.C, line 159 [r1] (raw file):_ We would need like what we have for fault bit, Need Object path, fruid and the actual value.

writefrudata.C, line 558 [r1] (raw file): Why is this needed here ?. the set_present will be populated only an a success so there is nothing to undo-redo on that flash.

Similarly, the valid not needed since it wont be set to valid unless the inventory has been successfully updated.

This entire change does not seem needed.

writefrudata.C, line 607 [r1] (raw file): This is redundant here since the object creation will ensure the value is false.

_writefrudata.C, line 679 [r1] (raw file):_ Is there a reason why its moved here ?

Comments from Reviewable

adamliyi commented 8 years ago

Review status: 0 of 1 files reviewed at latest revision, 5 unresolved discussions.

writefrudata.C, line 106 [r1] (raw file): Yes. There will still be "lookup" error message in this case. And this is expected. The patch just reduce the unnecessary "lookup" error messages. As I mentioned in the commit message: "ipmi-fru-parser needs to call getObjectFromId() to know whether a fru should be present, in order to decide the 'present' and 'fault' status. So this message will still be triggered when SystemManager does not define object path for a fru area."

_writefrudata.C, line 159 [r1] (raw file):_ Yep. I can add Object path. This fix just make the "True" or "False" value looks correct, as bellow. I will update the patch: The "Present" bit: root@barreleye:~# journalctl --no-pager | grep 'Present' Apr 06 14:52:28 barreleye system_manager.py[906]: Present bit set to :[True] for fruid:[102] Apr 06 14:52:28 barreleye system_manager.py[906]: Present bit set to :[True] for fruid:[101] Apr 06 14:52:32 barreleye system_manager.py[906]: Present bit set to :[True] for fruid:[100] Apr 06 14:52:32 barreleye system_manager.py[906]: Present bit set to :[True] for fruid:[100] Apr 06 14:52:32 barreleye system_manager.py[906]: Present bit set to :[True] for fruid:[100]

writefrudata.C, line 558 [r1] (raw file): Consider the case, if an eeprom file cannot be opened for reading (i.e, fru_fp is NULL), we need to set "Present" to false. For the "valid", I just think we cannot count on default value. We need to set it explicitly. The original code works ok. This is just an enhancement.

writefrudata.C, line 607 [r1] (raw file): I think we cannot count on "default" value (fault is set to false by default) in Skeleton. It is responsibility of ipmi-fru-parser to take care of the "present" and "fault" status of FRUs reading from eeprom.

_writefrudata.C, line 679 [r1] (raw file):_ For Hostboot managed FRUs, the dbus object path is setup when updating the Inventory. These debug message will output object path, so I move it after ipmi_update_inventory().

Comments from Reviewable

vishwabmc commented 8 years ago

Review status: 0 of 1 files reviewed at latest revision, 6 unresolved discussions.

_writefrudata.C, line 106 [r1] (raw file):_ I don't think this was the intention of this story. As far as I know, we wanted to eliminate those error messages by having skeleton provide the API telling what are the different sections for a given fru id.

I am mostly certain we want to do a complete fix as these errors will appear every time the BMC is booted and hence will be a concern.

_writefrudata.C, line 349 [r1] (raw file):_ If there is no valid dbus path for a given fru id, then the path, intf name would be set to "Null" and the rc would not be < 0 so the code goes ahead and tries to use a null object path.. so it may result in undefined error messages.

I am thinking that if you force a failure somewhere, this path may crash,

_writefrudata.C, line 558 [r1] (raw file):_ The present bit is set as given by "std::ifstream so present bit is alredy set without caring the value of file pointer state..

Being present is different from failing to open the file.

Also, I am not still convinced assigning the same value could be any enhancement since the C++ object creation itself guarantees that initial value.

Since you feel that you can not count on it, are you thinking of any problems ?.

writefrudata.C, line 607 [r1] (raw file): iv_valid is guaranteed to be false when object is created per ipmi_fru constructor.

What problem scenarios are you having in mind and hence you don't want to count on default ?.

iv_valid and fault bit are different things. iv_valid is the gate keeper to set any value to 'fault'.

_writefrudata.C, line 679 [r1] (raw file):_ Okay.. But the DBUS paths for any FRU is setup during inventory update right ?

Comments from Reviewable

adamliyi commented 8 years ago

Review status: 0 of 1 files reviewed at latest revision, 6 unresolved discussions.

writefrudata.C, line 106 [r1] (raw file): Hi Vishwa, I will update the pull request - based on your comment. Thanks.

Comments from Reviewable

adamliyi commented 8 years ago

I submitted a new PR: https://github.com/openbmc/ipmi-fru-parser/pull/14 to replace this PR. This PR can be closed.