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