<font face="Default Sans Serif,Verdana,Arial,Helvetica,sans-serif" size="2"> <span><br>Vishwa, attached are the changes needed to make your code compile and not hit link issues when we run it on the test system.  Looks like you still need to fix some things from Patricks review but this will take care of the makefile issues.<br><br>Chris Austen<br>POWER Systems Enablement Manager <br>(512) 286-5184 (T/L: 363-5184)</span><br><br><font color="#990099">-----"openbmc" <<a target="_blank" href="mailto:openbmc-bounces+austenc=us.ibm.com@lists.ozlabs.org">openbmc-bounces+austenc=us.ibm.com@lists.ozlabs.org</a>> wrote: -----</font><div class="iNotesHistory" style="padding-left:5px;"><div style="padding-right:0px;padding-left:5px;border-left:solid black 2px;">To: OpenBMC Patches <<a target="_blank" href="mailto:openbmc-patches@stwcx.xyz">openbmc-patches@stwcx.xyz</a>><br>From: Patrick Williams <patrick@stwcx.xyz><br>Sent by: "openbmc" <openbmc-bounces+austenc=us.ibm.com@lists.ozlabs.org><br>Date: 11/23/2015 04:14PM<br>Cc: <a target="_blank" href="mailto:openbmc@lists.ozlabs.org">openbmc@lists.ozlabs.org</a><br>Subject: Re: [PATCH ipmi-fru-parser v2] eeprom read CLI<br><br><div><font size="2" face="Courier New,Courier,monospace">On Fri, Nov 20, 2015 at 11:00:11AM -0500, OpenBMC Patches wrote:<br>> From: vishwa <<a target="_blank" href="mailto:vishwanath@in.ibm.com">vishwanath@in.ibm.com</a>><br>>  define __BUILD_EXE<br>>  $1 : $$($1_OBJS) $$(LIBS)<br>> -                $$(LINK.cpp) -o $$@ $$^ $(call __PKG_CONFIG,$($1_NEEDED),--libs)<br>> +                $$(LINK.cpp) -o $$@ $$^ $(call __PKG_CONFIG,$($1_NEEDED),--libs) <br><br>Remove whitespace change.<br><br>>  define __BUILD_LIB<br>>  $1 : $$($1_OBJS)<br>> -                $$(LINK.cpp) -shared -o $$@ $$^ $(call __PKG_CONFIG,$($1_NEEDED),--libs)<br>> +                $$(LINK.cpp) -fPIC -shared -o $$@ $$^ $(call __EXTRA_LIB_RESOLV,$($1_EXTRA_LIBS)) $(call __PKG_CONFIG,$($1_NEEDED),--libs) <br><br>This is causing $1_EXTRA_LIBS to be added as an argument directly.<br>   1) You need to add the $1_EXTRA_LIBS to the dependency above.<br>       $1 : $$($1_OBJS) $$($1_EXTRA_LIBS)<br>   2) You should be using -l<library> instead so that it continues to be<br>      dynamically linked.<br><br>> diff --git a/readeeprom.C b/readeeprom.C<br>> new file mode 100644<br>> index 0000000..27adfa1<br>> --- /dev/null<br>> +++ b/readeeprom.C<br>> @@ -0,0 +1,73 @@<br>> +#include <iostream><br>> +#include <memory><br>> +#include "argument.H"<br>> +#include "writefrudata.H"<br>> +<br>> +// FRU ID of VPD contaied in BMC accessible EEPROM<br>> +#define BARRELEYE_IO_BOARD                  64<br><br>There should not be any system-specific #define in this program.  Get it<br>passed as an argument if you need it.<br><br>> +//--------------------------------------------------------------------------<br>> +// This gets called by udev monitor soon after seeing hog plugs for EEPROMS. <br>> +//--------------------------------------------------------------------------<br>> +int main(int argc, char **argv)<br>> +{<br>> +        int rc = 0;<br>> +<br>> +        // Handle to per process system bus<br>> +        sd_bus *bus_type = NULL;<br>> +<br>> +        // Read the arguments.<br>> +        auto cli_options = std::make_unique<ArgumentParser>(argc, argv);<br>> +        <br>> +        // Parse out each argument.<br>> +        auto eeprom_file = (*cli_options)["eeprom"];<br>> +    if (eeprom_file == ArgumentParser::empty_string)<br>> +    {<br>> +                // User has not passed in the appropriate argument value<br>> +        exit_with_error("eeprom data not found.", argv);<br>> +    }<br>> +<br>> +        auto fruid_str = (*cli_options)["fruid"];<br>> +    if (eeprom_file == ArgumentParser::empty_string)<br>> +    {<br>> +                // User has not passed in the appropriate argument value<br>> +        exit_with_error("fruid data not found.", argv);<br>> +    }<br><br>Please use consistent space indentation (and no tab characters).<br><br>> +        // Extract the fruid<br>> +        uint8_t fruid = strtol(fruid_str.c_str(), NULL, 16);<br>> +        if(fruid == 0)<br>> +        {<br>> +                // default to hardcoded 64<br>> +                fruid = BARRELEYE_IO_BOARD;<br>> +        }<br><br>It doesn't make much sense to me that you have it above as an error if<br>they don't pass it in but not an error here if they give you a string<br>instead of an integer.  Just make it all an error and forget about a<br>default that only works on one particular machine.<br><br>> -int ipmi_validate_and_update_inventory(const uint8_t fruid, const uint8_t *fru_data)<br>> +int ipmi_validate_and_update_inventory(const uint8_t fruid, const uint8_t *fru_data, <br>> +                                       sd_bus *bus_type)<br>>  {<br>>      // Used for generic checksum calculation<br>>      uint8_t checksum = 0;<br>> @@ -249,7 +244,7 @@ int ipmi_validate_and_update_inventory(const uint8_t fruid, const uint8_t *fru_d<br>>      uint8_t fru_entry;<br>>  <br>>      // A generic offset locator for any FRU record.<br>> -    uint8_t area_offset = 0;<br>> +    uint16_t area_offset = 0;<br><br>Any reason to not just use size_t?<br><br>-- <br>Patrick Williams<br></font></div><div><font size="2" face="Courier New,Courier,monospace">_______________________________________________<br>openbmc mailing list<br><a target="_blank" href="mailto:openbmc@lists.ozlabs.org">openbmc@lists.ozlabs.org</a><br><a href="https://lists.ozlabs.org/listinfo/openbmc">https://lists.ozlabs.org/listinfo/openbmc</a><br></font></div></openbmc-bounces+austenc=us.ibm.com@lists.ozlabs.org></patrick@stwcx.xyz></div><br><br>[attachment "signature.asc" removed by Chris Austen/Austin/IBM]</div></font><BR>