[PATCH ipmi-fru-parser v2] eeprom read CLI

[Patrick Williams]

On Fri, Nov 20, 2015 at 11:00:11AM -0500, OpenBMC Patches wrote:
> From: vishwa <vishwanath at in.ibm.com>
>  define __BUILD_EXE
>  $1 : $$($1_OBJS) $$(LIBS)
> -		$$(LINK.cpp) -o $$@ $$^ $(call __PKG_CONFIG,$($1_NEEDED),--libs)
> +		$$(LINK.cpp) -o $$@ $$^ $(call __PKG_CONFIG,$($1_NEEDED),--libs) 

Remove whitespace change.

>  define __BUILD_LIB
>  $1 : $$($1_OBJS)
> -		$$(LINK.cpp) -shared -o $$@ $$^ $(call __PKG_CONFIG,$($1_NEEDED),--libs)
> +		$$(LINK.cpp) -fPIC -shared -o $$@ $$^ $(call __EXTRA_LIB_RESOLV,$($1_EXTRA_LIBS)) $(call __PKG_CONFIG,$($1_NEEDED),--libs) 

This is causing $1_EXTRA_LIBS to be added as an argument directly.
   1) You need to add the $1_EXTRA_LIBS to the dependency above.
       $1 : $$($1_OBJS) $$($1_EXTRA_LIBS)
   2) You should be using -l<library> instead so that it continues to be
      dynamically linked.

> diff --git a/readeeprom.C b/readeeprom.C
> new file mode 100644
> index 0000000..27adfa1
> --- /dev/null
> +++ b/readeeprom.C
> @@ -0,0 +1,73 @@
> +#include <iostream>
> +#include <memory>
> +#include "argument.H"
> +#include "writefrudata.H"
> +
> +// FRU ID of VPD contaied in BMC accessible EEPROM
> +#define BARRELEYE_IO_BOARD	 	 64

There should not be any system-specific #define in this program.  Get it
passed as an argument if you need it.

> +//--------------------------------------------------------------------------
> +// This gets called by udev monitor soon after seeing hog plugs for EEPROMS. 
> +//--------------------------------------------------------------------------
> +int main(int argc, char **argv)
> +{
> +	int rc = 0;
> +
> +	// Handle to per process system bus
> +	sd_bus *bus_type = NULL;
> +
> +	// Read the arguments.
> +	auto cli_options = std::make_unique<ArgumentParser>(argc, argv);
> +	
> +	// Parse out each argument.
> +	auto eeprom_file = (*cli_options)["eeprom"];
> +    if (eeprom_file == ArgumentParser::empty_string)
> +    {
> +		// User has not passed in the appropriate argument value
> +        exit_with_error("eeprom data not found.", argv);
> +    }
> +
> +	auto fruid_str = (*cli_options)["fruid"];
> +    if (eeprom_file == ArgumentParser::empty_string)
> +    {
> +		// User has not passed in the appropriate argument value
> +        exit_with_error("fruid data not found.", argv);
> +    }

Please use consistent space indentation (and no tab characters).

> +	// Extract the fruid
> +	uint8_t fruid = strtol(fruid_str.c_str(), NULL, 16);
> +	if(fruid == 0)
> +	{
> +		// default to hardcoded 64
> +		fruid = BARRELEYE_IO_BOARD;
> +	}

It doesn't make much sense to me that you have it above as an error if
they don't pass it in but not an error here if they give you a string
instead of an integer.  Just make it all an error and forget about a
default that only works on one particular machine.

> -int ipmi_validate_and_update_inventory(const uint8_t fruid, const uint8_t *fru_data)
> +int ipmi_validate_and_update_inventory(const uint8_t fruid, const uint8_t *fru_data, 
> +                                       sd_bus *bus_type)
>  {
>      // Used for generic checksum calculation
>      uint8_t checksum = 0;
> @@ -249,7 +244,7 @@ int ipmi_validate_and_update_inventory(const uint8_t fruid, const uint8_t *fru_d
>      uint8_t fru_entry;
>  
>      // A generic offset locator for any FRU record.
> -    uint8_t area_offset = 0;
> +    uint16_t area_offset = 0;

Any reason to not just use size_t?

-- 
Patrick Williams
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20151123/2242ae1a/attachment.sig>