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

[Chris Austen]

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.

Chris Austen
POWER Systems Enablement Manager 
(512) 286-5184 (T/L: 363-5184)

-----"openbmc" <openbmc-bounces+austenc=us.ibm.com at lists.ozlabs.org> wrote: -----
To: OpenBMC Patches <openbmc-patches at stwcx.xyz>
From: Patrick Williams 
Sent by: "openbmc" 
Date: 11/23/2015 04:14PM
Cc: openbmc at lists.ozlabs.org
Subject: Re: [PATCH ipmi-fru-parser v2] eeprom read CLI

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
_______________________________________________
openbmc mailing list
openbmc at lists.ozlabs.org
https://lists.ozlabs.org/listinfo/openbmc


[attachment "signature.asc" removed by Chris Austen/Austin/IBM]

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20151124/9e69a5cf/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: eeprom_updates.patch
Type: application/octet-stream
Size: 2514 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20151124/9e69a5cf/attachment.obj>