[PATCH ipmi-fru-parser v4] Set Fault and Present status while handling fru
Vishwanatha Subbanna
vishwanath at in.ibm.com
Wed Jan 20 17:48:14 AEDT 2016
hello Stewart,
Thank you for reviewing. Please find my comments.
Thanks
-------------------------------------------------------------------------------------
Thanks and Regards,
Vishwanath.
Advisory Software Engineer,
Power Firmware Development,
Systems &Technology Lab,
MG2-6F-255 , Manyata Embassy Business Park,
Bangalore , KA , 560045
Ph: +91-80-46678255
E-mail: vishwanath at in.ibm.com
----------------------------------------------------------------------------------
From: Stewart Smith <stewart at linux.vnet.ibm.com>
To: OpenBMC Patches <openbmc-patches at stwcx.xyz>,
openbmc at lists.ozlabs.org
Date: 14/01/2016 03:06 am
Subject: Re: [PATCH ipmi-fru-parser v4] Set Fault and Present status
while handling fru
Sent by: "openbmc" <openbmc-bounces
+vishwanath=in.ibm.com at lists.ozlabs.org>
OpenBMC Patches <openbmc-patches at stwcx.xyz> writes:
> diff --git a/fru-area.H b/fru-area.H
> new file mode 100644
> index 0000000..90dd92b
> --- /dev/null
> +++ b/fru-area.H
> @@ -0,0 +1,153 @@
> +#ifndef __IPMI_FRU_AREA_H__
> +#define __IPMI_FRU_AREA_H__
> +
> +#include <stdint.h>
> +#include <stddef.h>
> +#include <systemd/sd-bus.h>
> +#include <string>
> +#include <vector>
> +#include <memory>
> +#include "writefrudata.H"
> +
> +class ipmi_fru;
> +typedef std::vector<std::unique_ptr<ipmi_fru>> fru_area_vec_t;
> +
> +class ipmi_fru
> +{
> + private:
> + // FRU ID
Comment is not needed, it should be obvious from member name.
> + uint8_t iv_fruid;
What does iv_ prefix mean?
iv_ is widely used C++ notation for class Instance Variable.
> +
> + // Type of the fru matching offsets in common header
> + uint8_t iv_type;
if it's a type of a limited set, should this be an enum?
Sorry, not sure I got this question. Type is actually an enum in frup.h in
ipmi-fru-parser repo.
> + // If a particular area has been marked valid / invalid
> + inline bool get_valid() const
> + {
> + return iv_valid;
> + }
why not is_valid() ?
Yeah.. sounds better name.
> --- a/readeeprom.C
> +++ b/readeeprom.C
> @@ -12,7 +12,7 @@ static void exit_with_error(const char* err, char**
argv)
> }
>
> //--------------------------------------------------------------------------
> -// This gets called by udev monitor soon after seeing hog plugs for
EEPROMS.
> +// This gets called by udev monitor soon after seeing hog plugs for
EEPROMS.
> //--------------------------------------------------------------------------
Best to do whitespace fixups in separate patch.
I understand. But do you feel I can go ahead this time and practise this
from next submissions ?. -or- do you see a need to do here ?
> int main(int argc, char **argv)
> {
> @@ -21,7 +21,7 @@ int main(int argc, char **argv)
>
> // Handle to per process system bus
> sd_bus *bus_type = NULL;
> -
> +
again, whitespace in sep patch
> // Read the arguments.
> auto cli_options = std::make_unique<ArgumentParser>(argc, argv);
>
> @@ -53,7 +53,7 @@ int main(int argc, char **argv)
>
> // Get a handle to System Bus
> rc = sd_bus_open_system(&bus_type);
> - if (rc < 0)
> + if (rc < 0)
and here
> diff --git a/strgfnhandler.C b/strgfnhandler.C
> index a00688f..eda4290 100644
> --- a/strgfnhandler.C
> +++ b/strgfnhandler.C
> @@ -11,8 +11,8 @@ sd_bus* ipmid_get_sd_bus_connection(void);
> ///-------------------------------------------------------
> // Called by IPMI netfn router for write fru data command
> //--------------------------------------------------------
> -ipmi_ret_t ipmi_storage_write_fru_data(ipmi_netfn_t netfn, ipmi_cmd_t
cmd,
> - ipmi_request_t request, ipmi_response_t
response,
> +ipmi_ret_t ipmi_storage_write_fru_data(ipmi_netfn_t netfn, ipmi_cmd_t
cmd,
> + ipmi_request_t request, ipmi_response_t
response,
> ipmi_data_len_t data_len,
> ipmi_context_t context)
and here
> {
> FILE *fp = NULL;
> @@ -38,7 +38,7 @@ ipmi_ret_t ipmi_storage_write_fru_data(ipmi_netfn_t
netfn, ipmi_cmd_t cmd,
>
> // On error there is no response data for this command.
> *data_len = 0;
> -
> +
and here
> #ifdef __IPMI__DEBUG__
> printf("IPMI WRITE-FRU-DATA for [%s] Offset = [%d] Length =
[%d]\n",
> fru_file_name, offset, len);
> @@ -59,17 +59,17 @@ ipmi_ret_t ipmi_storage_write_fru_data(ipmi_netfn_t
netfn, ipmi_cmd_t cmd,
> fclose(fp);
> return rc;
> }
> -
> +
> if(fwrite(&reqptr->data, len, 1, fp) != 1)
> {
> perror("Error:");
> fclose(fp);
> return rc;
> }
> -
> +
> fclose(fp);
> - }
> - else
> + }
> + else
and here.
>
> +//----------------------------------------------------------------
> +// Constructor
> +//----------------------------------------------------------------
> +ipmi_fru::ipmi_fru(const uint8_t fruid, const uint8_t type,
> + sd_bus *bus_type, bool bmc_fru)
The comment is redundant.
Okay.. It was more of a practise that I have followed to have atleast one
liner on what a particular function is.
Are you seeing it does not make sense ?. I saw couple examples in hostboot
ipmi code with such comments so thought its fine to use.
> +//-----------------------------------------------------
> +// Accepts a pointer to data and sets it in the object.
> +//-----------------------------------------------------
> +void ipmi_fru::set_data(const uint8_t *data, const size_t len)
> +{
> + iv_len = len;
> + iv_data = new uint8_t[len];
> + memcpy(iv_data, data, len);
> +}
Comment would do better detailing ownership of data rather than simply
explaining what the code obviously does.
Agreed.
> +//-----------------------------------------------------
> +// Sets the dbus parameters
> +//-----------------------------------------------------
> +void ipmi_fru::update_dbus_paths(const char *bus_name,
> + const char *obj_path, const char *intf_name)
> +{
> + iv_bus_name = bus_name;
> + iv_obj_path = obj_path;
> + iv_intf_name = intf_name;
> +}
why are we passing in C strings when everything else is std::string?
It's just as easy to create std::string on the way in as it is to do so
here, and if the caller is dealing with std::string, then we have more
hoops
Norm's skeleton code which I am calling to get those parameters are
returning them as char *s
> +
> +//-------------------
> +// Destructor
> +//-------------------
> +ipmi_fru::~ipmi_fru()
redundant comment.
> //------------------------------------------------
> // Takes the pointer to stream of bytes and length
> // returns the 8 bit checksum per IPMI spec.
> //-------------------------------------------------
> -unsigned char calculate_crc(unsigned char *data, int len)
> +unsigned char calculate_crc(const unsigned char *data, size_t len)
> {
> char crc = 0;
> - int byte = 0;
> + size_t byte = 0;
>
> for(byte = 0; byte < len; byte++)
> {
This should likely be a separate patch, this change seems unrelated to
the other changes.
You mean, submit a patch for changing the name ?
What is the checksum anyway? Is it CRC32? A byte at a time loop is
unlikely to be the most optimal way of doing things.
Its a standard IPMI V2.0 checksum algo.
http://www.intel.in/content/dam/www/public/us/en/documents/product-briefs/ipmi-second-gen-interface-spec-v2-rev1-1.pdf
Page #: 163 / 208
--
Stewart Smith
OPAL Architect, IBM.
_______________________________________________
openbmc mailing list
openbmc at lists.ozlabs.org
https://lists.ozlabs.org/listinfo/openbmc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20160120/1b731ac8/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: graycol.gif
Type: image/gif
Size: 105 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20160120/1b731ac8/attachment-0001.gif>
More information about the openbmc
mailing list