<div class="socmaildefaultfont" dir="ltr" style="font-family:Arial;font-size:10.5pt" ><div dir="ltr" >Hey Cyril,</div>
<div dir="ltr" > </div>
<div dir="ltr" >For the whitespace introduced and the coding style issue, I will try to propose a fix to it, hopefully with minimum coding style changes.</div>
<div dir="ltr" > </div>
<div dir="ltr" >For other issues, we can continue discuss it but I believe we can firstly accept this patch and then revisit them if there is really concerns for them.</div>
<div dir="ltr" > </div>
<div class="socmaildefaultfont" dir="ltr" style="font-family:Arial;font-size:10.5pt" ><div class="socmaildefaultfont" dir="ltr" style="font-family:Arial;font-size:10.5pt" ><div dir="ltr" >GOU, Peng Fei (苟鹏飞), Ph.D.</div>
<div dir="ltr" >OpenPower Team.</div>
<div dir="ltr" >+86-21-609-28631</div></div></div>
<div dir="ltr" > </div>
<div dir="ltr" > </div>
<blockquote data-history-content-modified="1" dir="ltr" style="border-left:solid #aaaaaa 2px; margin-left:5px; padding-left:5px; direction:ltr; margin-right:0px" >----- Original message -----<br>From: Cyril Bur <cyrilbur@gmail.com><br>To: Peng Fei BG Gou/China/IBM@IBMCN<br>Cc: openbmc@lists.ozlabs.org, openbmc-patches@stwcx.xyz<br>Subject: Re: [PATCH phosphor-host-ipmid v4] Add get/set boot option ipmid command support with correct DBUS property handling.<br>Date: Wed, Jan 6, 2016 3:12 PM<br> 
<div><font face="Default Monospace,Courier New,Courier,monospace" size="2" >On Wed, 6 Jan 2016 06:13:06 +0000<br>"Peng Fei BG Gou" <shgoupf@cn.ibm.com> wrote:<br><br>> Hey Cyril,<br>>  <br>> Thanks for your detailed review on this patch.<br>>  <br><br>Hi Peng,<br><br>> Just want to answer (most of) your questions in batch:<br>> 1) This patch intended to add the functionality of ipmi set/get boot option, with dbus property and object mapper used, so I believe all of the contents in this patch should be considered as a whole. We shouldn't split them into multiple patches.<br><br>The set/get boot option is fine as one patch (or two... not too fussed really) I<br>agree.<br><br>You do still have whitespace (and other) changes which touch code not related<br>to the boot option work, notably in ipmi_chassis_power_control() and<br>ipmi_chassis_control()<br><br>> 2) atoi is an alternative, but I don't see there is any reason why to use it since sscanf/sprintf works fine for now. I don't believe there is too much performance concern there.<br><br>I'm not saying sscanf won't work, I completely agree that it will. I do<br>disagree that there isn't a performance concern, if I'm not mistaken this code<br>will be running on an ASPEED2400 which is a single core ARM chip running at<br>400MHz. We have breathing space and we don't need to write absolutely<br>everything to save all the possible cycles but lets not waste the little<br>breathing space we do have.<br><br>> 3) For those sd_bus method call with signatures ("s", "v", "s{ass}", etc.), I believe the current way I use it is valid since I have tested them already.<br><br>Again, I'm not saying it won't work but I'm trying to understand why the<br>complexity in these signatures, it doesn't seem like it's being used and<br>pointless complexity doesn't help anyone.<br><br>However, if there is a good reason for these complicated signatures I'm all for<br>it, if you could document them in<br><a href="https://github.com/openbmc/docs/blob/master/dbus-interfaces.md" target="_blank" >https://github.com/openbmc/docs/blob/master/dbus-interfaces.md</a> that would be<br>fantastic.<br><br>Thanks.<br><br>> 4) For the commented out error handling, I believe it's just a work around, but it did works according to my test. So I believe it's OK for this patch to have it and we should revisit it later.<br>> 5) For the way how return code is handled, I believe it is working fine.<br>> 6) For the coding style stuff, such as spaces between function names and the bracket, I don't think it's a major issue. Maybe it's a minor issue that they are not consistent with some piece of code elsewhere. It can be fixed with my auto style tool very easily.<br>>  <br><br>It isn't a major issue at all but this project already has enough code style<br>issues as it is, I don't see why patches should be merged that introduce more<br>style problems.<br><br>Perhaps you should run your auto style tool on your patch(es) before submission?<br><br>> In a sum, thanks for your effort on reviewing this, but I believe most of your current concerns either are invalid or are only minor issues.<br>>  <br>> GOU, Peng Fei (苟鹏飞), Ph.D.<br>> OpenPower Team.<br>> +86-21-609-28631<br>>  <br>>  <br>> ----- Original message -----<br>> From: Cyril Bur <cyrilbur@gmail.com><br>> To: OpenBMC Patches <openbmc-patches@stwcx.xyz>, Peng Fei BG Gou/China/IBM@IBMCN<br>> Cc: openbmc@lists.ozlabs.org<br>> Subject: Re: [PATCH phosphor-host-ipmid v4] Add get/set boot option ipmid command support with correct DBUS property handling.<br>> Date: Wed, Jan 6, 2016 1:10 PM<br>>  <br>> On Mon,  4 Jan 2016 19:10:26 -0600<br>> OpenBMC Patches <openbmc-patches@stwcx.xyz> wrote:<br>><br>> > From: shgoupf <shgoupf@cn.ibm.com><br>> ><br>><br>> Hi again,<br>><br>> > 1) Add support for IPMI get/set boot option command.<br>> >     a) boot options stored in dbus property.<br>> >     b) IPMI get boot option command get boot option from the dbus<br>> >     property.<br>> >     c) IPMI set boot option coomand set boot option to the dbus<br>> >     property.<br>> > 2) Two methods to handle the dbus property set and get:<br>> >     a) dbus_set_property()<br>> >     b) dbus_get_property()<br>> > 3) The property is stored as a 10 character strings which representsd 5-byte information.<br>> > 4) ipmid set method is registered and implemented since petitboot will use it to clear the boot options.<br>> > 5) Get service name via object mapper<br>> >     a) The connection name is got via objectmapper.<br>> >     b) The method used to get the connection name is object_mapper_get_connection().<br>> >     c) dbus_get_property/dbus_set_property will get the connection name via the above method instead of hard coding.<br>> > 6) Error code are properly handled.<br>> > 7) Use sprinf/sscanf for int/string conversion.<br>> >     a) Instead of reinventing the wheel by defining methods converting int to string, use sprintf/sscanf should be a more clean and robust way.<br>><br>> Plus 1 for not reinventing the wheel but perhaps consider using the best tool<br>> for the job, sscanf can and does do what you want however atoi, atol, atoll,<br>> strtol, strtoll, strtoq functions are all designed for this exact purpose. They<br>> will perform much better than sscanf, also the str* versions give great control<br>> and error detection capabilities.<br>><br>><br>> > 8) Fix issues addressed by the community.<br>> >     a) change malloc heap to stack local variable.<br>> >     b) Fix multi line comment style from "//" to "/**/".<br>> >     c) Add defines for return codes.<br>> >     d) Add more comments.<br>><br>> It looks like you're adding more and more into this same patch. A single patch<br>> should be one clearly defined piece of work. So you're implementing some new<br>> functionality, that should be its own patch. You're also adding some error<br>> handling paths - also, its own patch. You've changed some whitespace (some of<br>> which was a bad thing) but the good whitespace changes should also be their own<br>> patch.<br>><br>> >  chassishandler.C | 492 +++++++++++++++++++++++++++++++++++++++++++++----------<br>> >  chassishandler.h |  18 ++<br>> >  2 files changed, 423 insertions(+), 87 deletions(-)<br>> ><br>> > diff --git a/chassishandler.C b/chassishandler.C<br>> > index 1389db9..4d941e8 100644<br>> > --- a/chassishandler.C<br>> > +++ b/chassishandler.C<br>> > @@ -5,11 +5,235 @@<br>> >  #include <stdint.h><br>> >  <br>> >  // OpenBMC Chassis Manager dbus framework<br>> > -const char  *chassis_bus_name      =  "org.openbmc.control.Chassis";<br>> > -const char  *chassis_object_name   =  "/org/openbmc/control/chassis0";<br>> > -const char  *chassis_intf_name     =  "org.openbmc.control.Chassis";<br>> > +const char * chassis_bus_name      =  "org.openbmc.control.Chassis";<br>> > +const char * chassis_object_name   =  "/org/openbmc/control/chassis0";<br>> > +const char * chassis_intf_name     =  "org.openbmc.control.Chassis";<br>> >  <br>><br>> Not necessary, if anything you should remove the double space but what you've<br>> done doesn't look good.<br>><br>> > -void register_netfn_chassis_functions() __attribute__((constructor));<br>> > +// Host settings in dbus<br>> > +// Service name should be referenced by connection name got via object mapper<br>> > +const char * settings_object_name  =  "/org/openbmc/settings/host0";<br>> > +const char * settings_intf_name    =  "org.freedesktop.DBus.Properties";<br>> > +<br>> > +const char * objmapper_service_name =  "org.openbmc.objectmapper";<br>> > +const char * objmapper_object_name  =  "/org/openbmc/objectmapper/objectmapper";<br>> > +const char * objmapper_intf_name    =  "org.openbmc.objectmapper.ObjectMapper";<br>> > +<br>><br>> We probably want to try to keep a consistent style. I looked at ipmid.C and<br>> that file isn't consistent but it seems to be mostly "type *name;" and that's<br>> the most common way everywhere else with C code.<br>><br>> > +int object_mapper_get_connection (char ** buf, const char * obj_path)<br>> > +{<br>> > +    sd_bus_error error = SD_BUS_ERROR_NULL;<br>> > +    sd_bus_message * m = NULL;<br>> > +    sd_bus * bus = NULL;<br>> > +    char * temp_buf = NULL, *intf = NULL;<br>> > +    size_t buf_size = 0;<br>> > +    int r;<br>> > +<br>> > +    // Open the system bus where most system services are provided.<br>> > +    r = sd_bus_open_system (&bus);<br>> > +<br>><br>> As an added stylistic note, again for consistency, ipmid.C, you should avoid<br>> having a space between the function name and the parameters<br>><br>> > +    if (r < 0) {<br>> > +        fprintf (stderr, "Failed to connect to system bus: %s\n", strerror (-r));<br>> > +        goto finish;<br>> > +    }<br>> > +<br>> > +    /*<br>> > +     * Bus, service, object path, interface and method are provided to call<br>> > +     * the method.<br>> > +     * Signatures and input arguments are provided by the arguments at the<br>> > +     * end.<br>> > +     */<br>> > +    r = sd_bus_call_method (bus,<br>> > +                            objmapper_service_name,                      /* service to contact */<br>> > +                            objmapper_object_name,                       /* object path */<br>> > +                            objmapper_intf_name,                         /* interface name */<br>> > +                            "GetObject",                                 /* method name */<br>> > +                            &error,                                      /* object to return error in */<br>> > +                            &m,                                          /* return message on success */<br>> > +                            "s",                                         /* input signature */<br>> > +                            obj_path                                     /* first argument */<br>> > +                           );<br>> > +<br>> > +    if (r < 0) {<br>> > +        fprintf (stderr, "Failed to issue method call: %s\n", error.message);<br>> > +        goto finish;<br>> > +    }<br>> > +<br>> > +    // Get the key, aka, the connection name<br>> > +    sd_bus_message_read (m, "a{sas}", 1, &temp_buf, 1, &intf);<br>> > +<br>><br>> This seems very roundabout, could you explain this method call. What are you<br>> expecting to receive? From the looks of what you're using it for wouldn't it<br>> make more sense to just sd_bus_message_read a "s"?<br>><br>> > +    /*<br>> > +     * TODO: check the return code. Currently for no reason the message<br>> > +     * parsing of object mapper is always complaining about<br>> > +     * "Device or resource busy", but the result seems OK for now. Need<br>> > +     * further checks.<br>><br>> If sd_bus_message_read returns a negative value, something definitely went<br>> wrong, I highly highly doubt you can use the result if it turns a negative.<br>><br>> > +     * TODO: The following code is preserved in the comments so that it can be<br>> > +     * resumed after the problem aforementioned is resolved.<br>> > +     *r = sd_bus_message_read(m, "a{sas}", 1, &temp_buf, 1, &intf);<br>> > +     *if (r < 0) {<br>> > +     *    fprintf(stderr, "Failed to parse response message: %s\n", strerror(-r));<br>> > +     *    goto finish;<br>> > +     *}<br>><br>> The code is in git, it is saved there, delete it now and when someone wants to<br>> add it back they can look in the git history for what was deleted. Having said<br>> that, the error check should be there, if you're hitting it, you should figure<br>> out why.<br>><br>> > +     */<br>> > +<br>> > +    buf_size = strlen (temp_buf) + 1;<br>> > +    printf ("IPMID connection name: %s\n", temp_buf);<br>> > +    *buf = (char *)malloc (buf_size);<br>> > +<br>> > +    if (*buf == NULL) {<br>> > +        fprintf (stderr, "Malloc failed for get_sys_boot_options");<br>> > +        r = -1;<br>> > +        goto finish;<br>> > +    }<br>> > +<br>><br>> I don't mind using malloc but these are strings no? Couldn't strndup be used,<br>> might make life easier? Not a big concern.<br>><br>> > +    memcpy (*buf, temp_buf, buf_size);<br>> > +<br>> > +finish:<br>> > +    sd_bus_error_free (&error);<br>> > +    sd_bus_message_unref (m);<br>> > +    sd_bus_unref (bus);<br>> > +<br>><br>> Spaces between name and params<br>><br>> > +    return r;<br>> > +}<br>> > +<br>> > +int dbus_get_property (char * buf)<br>> > +{<br>> > +    sd_bus_error error = SD_BUS_ERROR_NULL;<br>> > +    sd_bus_message * m = NULL;<br>> > +    sd_bus * bus = NULL;<br>> > +    char * temp_buf = NULL;<br>> > +    uint8_t * get_value = NULL;<br>> > +    char * connection = NULL;<br>> > +    int r, i;<br>> > +<br>> > +    r = object_mapper_get_connection (&connection, settings_object_name);<br>> > +<br>> > +    if (r < 0) {<br>> > +        fprintf (stderr, "Failed to get connection, return value: %d.\n", r);<br>> > +        goto finish;<br>> > +    }<br>> > +<br>> > +    printf ("connection: %s\n", connection);<br>> > +<br>> > +    // Open the system bus where most system services are provided.<br>> > +    r = sd_bus_open_system (&bus);<br>> > +<br>> > +    if (r < 0) {<br>> > +        fprintf (stderr, "Failed to connect to system bus: %s\n", strerror (-r));<br>> > +        goto finish;<br>> > +    }<br>> > +<br>> > +    /*<br>> > +     * Bus, service, object path, interface and method are provided to call<br>> > +     * the method.<br>> > +     * Signatures and input arguments are provided by the arguments at the<br>> > +     * end.<br>> > +     */<br>> > +    r = sd_bus_call_method (bus,<br>> > +                            connection,                                 /* service to contact */<br>> > +                            settings_object_name,                       /* object path */<br>> > +                            settings_intf_name,                         /* interface name */<br>> > +                            "Get",                                      /* method name */<br>> > +                            &error,                                     /* object to return error in */<br>> > +                            &m,                                         /* return message on success */<br>> > +                            "ss",                                       /* input signature */<br>> > +                            settings_intf_name,                         /* first argument */<br>> > +                            "boot_flags");                              /* second argument */<br>> > +<br>> > +    if (r < 0) {<br>> > +        fprintf (stderr, "Failed to issue method call: %s\n", error.message);<br>> > +        goto finish;<br>> > +    }<br>> > +<br>> > +    /*<br>> > +     * The output should be parsed exactly the same as the output formatting<br>> > +     * specified.<br>> > +     */<br>> > +    r = sd_bus_message_read (m, "v", "s", &temp_buf);<br>> > +<br>><br>> Why not just read a "s"?<br>><br>> > +    if (r < 0) {<br>> > +        fprintf (stderr, "Failed to parse response message: %s\n", strerror (-r));<br>> > +        goto finish;<br>> > +    }<br>> > +<br>> > +    printf ("IPMID boot option property get: {%s}.\n", (char *) temp_buf);<br>> > +<br>> > +    /*<br>> > +     * To represent a hex in string, e.g., "A0A0", which represents two bytes<br>> > +     * in the hex, but requires 5 bytes to store it as string, i.e., 4<br>> > +     * characters to store the "A0A0" and 1 additional space for "\0".<br>> > +     * Thereby we have 2 * <number of bytes> + 1 for the string buffer.<br>> > +     */<br>><br>> So why don't you have #define NUM_RETURN_BYTES_OF_GET_USED 11 ?<br>><br>> This question is out of scope of this patch but why are we passing around<br>> numbers as strings? dbus handles other types...<br>><br>> > +    memcpy (buf, temp_buf, 2 * NUM_RETURN_BYTES_OF_GET_USED + 1);<br>> > +<br>> > +finish:<br>> > +    sd_bus_error_free (&error);<br>> > +    sd_bus_message_unref (m);<br>> > +    sd_bus_unref (bus);<br>> > +    free (connection);<br>> > +<br>> > +    return r;<br>> > +}<br>> > +<br>> > +int dbus_set_property (const char * buf)<br>> > +{<br>> > +    sd_bus_error error = SD_BUS_ERROR_NULL;<br>> > +    sd_bus_message * m = NULL;<br>> > +    sd_bus * bus = NULL;<br>> > +    char * connection = NULL;<br>> > +    int r;<br>> > +<br>> > +    r = object_mapper_get_connection (&connection, settings_object_name);<br>> > +<br>> > +    if (r < 0) {<br>> > +        fprintf (stderr, "Failed to get connection, return value: %d.\n", r);<br>> > +        goto finish;<br>> > +    }<br>> > +<br>> > +    printf ("connection: %s\n", connection);<br>> > +<br>> > +    // Open the system bus where most system services are provided.<br>> > +    r = sd_bus_open_system (&bus);<br>> > +<br>> > +    if (r < 0) {<br>> > +        fprintf (stderr, "Failed to connect to system bus: %s\n", strerror (-r));<br>> > +        goto finish;<br>> > +    }<br>> > +<br>> > +    /*<br>> > +     * Bus, service, object path, interface and method are provided to call<br>> > +     * the method.<br>> > +     * Signatures and input arguments are provided by the arguments at the<br>> > +     * end.<br>> > +     */<br>> > +    r = sd_bus_call_method (bus,<br>> > +                            connection,                                 /* service to contact */<br>> > +                            settings_object_name,                       /* object path */<br>> > +                            settings_intf_name,                         /* interface name */<br>> > +                            "Set",                                      /* method name */<br>> > +                            &error,                                     /* object to return error in */<br>> > +                            &m,                                         /* return message on success */<br>> > +                            "ssv",                                      /* input signature */<br>> > +                            settings_intf_name,                         /* first argument */<br>> > +                            "boot_flags",                               /* second argument */<br>> > +                            "s",                                        /* third argument */<br>> > +                            buf);                                       /* fourth argument */<br>> > +<br>><br>> Are we documenting these interfaces at all, I don't understand why "ssv" and<br>> then a hardcoded "s", why not "sss"?<br>><br>> > +    if (r < 0) {<br>> > +        fprintf (stderr, "Failed to issue method call: %s\n", error.message);<br>> > +        goto finish;<br>> > +    }<br>> > +<br>> > +    printf ("IPMID boot option property set: {%s}.\n", buf);<br>> > +<br>> > +finish:<br>> > +    sd_bus_error_free (&error);<br>> > +    sd_bus_message_unref (m);<br>> > +    sd_bus_unref (bus);<br>> > +    free (connection);<br>> > +<br>> > + &nbsp;  return r;<br>> > +}<br>> > +<br>> > +void register_netfn_chassis_functions () __attribute__ ((constructor));<br>> >  <br>> >  struct get_sys_boot_options_t {<br>> >      uint8_t parameter;<br>> > @@ -17,11 +241,28 @@ struct get_sys_boot_options_t {<br>> >      uint8_t block;<br>> >  }  __attribute__ ((packed));<br>> >  <br>> > -ipmi_ret_t ipmi_chassis_wildcard(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> > -                              ipmi_request_t request, ipmi_response_t response,<br>> > -                              ipmi_data_len_t data_len, ipmi_context_t context)<br>> > +struct set_sys_boot_options_t {<br>> > +    uint8_t parameter;<br>> > +    union {<br>> > +        struct {<br>> > +            uint8_t d1;<br>> > +            uint8_t d2;<br>> > +            uint8_t d3;<br>> > +            uint8_t d4;<br>> > +            uint8_t d5;<br>> > +            uint8_t d6;<br>> > +            uint8_t d7;<br>> > +            uint8_t d8;<br>> > +        } data8;<br>> > +        uint64_t data64;<br>> > +    } data;<br>> > +}  __attribute__ ((packed));<br>> > +<br>> > +ipmi_ret_t ipmi_chassis_wildcard (ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> > +                                  ipmi_request_t request, ipmi_response_t response,<br>> > +                                  ipmi_data_len_t data_len, ipmi_context_t context)<br>> >  {<br>> > -    printf("Handling CHASSIS WILDCARD Netfn:[0x%X], Cmd:[0x%X]\n",netfn, cmd);<br>> > +    printf ("Handling CHASSIS WILDCARD Netfn:[0x%X], Cmd:[0x%X]\n", netfn, cmd);<br>><br>> Be careful with changes like this, you didn't change this code...<br>><br>> >      // Status code.<br>> >      ipmi_ret_t rc = IPMI_CC_OK;<br>> >      *data_len = 0;<br>> > @@ -31,116 +272,193 @@ ipmi_ret_t ipmi_chassis_wildcard(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> >  //------------------------------------------------------------<br>> >  // Calls into Chassis Control Dbus object to do the power off<br>> >  //------------------------------------------------------------<br>> > -int ipmi_chassis_power_control(const char *method)<br>> > +int ipmi_chassis_power_control (const char * method)<br>> >  {<br>> > - // sd_bus error<br>> > - int rc = 0;<br>> > +    // sd_bus error<br>> > +    int rc = 0;<br>> >  <br>> >      // SD Bus error report mechanism.<br>> >      sd_bus_error bus_error = SD_BUS_ERROR_NULL;<br>> >  <br>> > - // Response from the call. Although there is no response for this call,<br>> > - // obligated to mention this to make compiler happy.<br>> > - sd_bus_message *response = NULL;<br>> > -<br>> > - // Gets a hook onto either a SYSTEM or SESSION bus<br>> > - sd_bus *bus_type = ipmid_get_sd_bus_connection();<br>> > -<br>> > - rc = sd_bus_call_method(bus_type,         // On the System Bus<br>> > - chassis_bus_name,        // Service to contact<br>> > - chassis_object_name,     // Object path<br>> > - chassis_intf_name,       // Interface name<br>> > - method,       // Method to be called<br>> > - &bus_error,       // object to return error<br>> > - &response, // Response buffer if any<br>> > - NULL); // No input arguments<br>> > - if(rc < 0)<br>> > - {<br>> > - fprintf(stderr,"ERROR initiating Power Off:[%s]\n",bus_error.message);<br>> > - }<br>> > - else<br>> > - {<br>> > - printf("Chassis Power Off initiated successfully\n");<br>> > - }<br>> > -<br>> > -    sd_bus_error_free(&bus_error);<br>> > -    sd_bus_message_unref(response);<br>> > -<br>> > - return rc;<br>> > +    /*<br>> > +     * Response from the call. Although there is no response for this call,<br>> > +     * obligated to mention this to make compiler happy.<br>> > +     */<br>> > +    sd_bus_message * response = NULL;<br>> > +<br>> > +    // Gets a hook onto either a SYSTEM or SESSION bus<br>> > +    sd_bus * bus_type = ipmid_get_sd_bus_connection ();<br>> > +<br>> > +    rc = sd_bus_call_method (bus_type,                /* On the System Bus*/<br>> > +                             chassis_bus_name,        /* Service to contact*/<br>> > +                             chassis_object_name,     /* Object path*/<br>> > +                             chassis_intf_name,       /* Interface name*/<br>> > +                             method,                  /* Method to be called*/<br>> > +                             &bus_error,              /* object to return error*/<br>> > +                             &response,               /* Response buffer if any*/<br>> > +                             NULL);                   /* No input arguments*/<br>> > +<br>> > +    if (rc < 0) {<br>> > +        fprintf (stderr, "ERROR initiating Power Off:[%s]\n", bus_error.message);<br>> > +    } else {<br>> > +        printf ("Chassis Power Off initiated successfully\n");<br>> > +    }<br>> > +<br>> > +    sd_bus_error_free (&bus_error);<br>> > +    sd_bus_message_unref (response);<br>> > +<br>> > +    return rc;<br>><br>> Cleanups are great but please send them as a separate patch, this helps people<br>> a lot when reviewing patches, if it is just a cleanup then people can focus on<br>> checking that you didn't change behaviour OR if you're sending a functional<br>> change patch people can focus on checking that you're achieving what you set<br>> out to do.<br>><br>> >  }<br>> >  <br>> >  <br>> >  //----------------------------------------------------------------------<br>> >  // Chassis Control commands<br>> >  //----------------------------------------------------------------------<br>> > -ipmi_ret_t ipmi_chassis_control(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> > -                        ipmi_request_t request, ipmi_response_t response,<br>> > -                        ipmi_data_len_t data_len, ipmi_context_t context)<br>> > +ipmi_ret_t ipmi_chassis_control (ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> > +                                 ipmi_request_t request, ipmi_response_t response,<br>> > +                     &nbsp;           ipmi_data_len_t data_len, ipmi_context_t context)<br>> >  {<br>> > - // Error from power off.<br>> > - int rc = 0;<br>> > +    // Error from power off.<br>> > +    int rc = 0;<br>> >  <br>> > - // No response for this command.<br>> > +    // No response for this command.<br>> >      *data_len = 0;<br>> >  <br>> > - // Catch the actual operaton by peeking into request buffer<br>> > - uint8_t chassis_ctrl_cmd = *(uint8_t *)request;<br>> > - printf("Chassis Control Command: Operation:[0x%X]\n",chassis_ctrl_cmd);<br>> > -<br>> > - switch(chassis_ctrl_cmd)<br>> > - {<br>> > - case CMD_POWER_OFF:<br>> > - rc = ipmi_chassis_power_control("powerOff");<br>> > - break;<br>> > - case CMD_HARD_RESET:<br>> > - rc = ipmi_chassis_power_control("reboot");<br>> > - break;<br>> > - default:<br>> > - {<br>> > - fprintf(stderr, "Invalid Chassis Control command:[0x%X] received\n",chassis_ctrl_cmd);<br>> > - rc = -1;<br>> > - }<br>> > - }<br>> > -<br>> > - return ( (rc < 0) ? IPMI_CC_INVALID : IPMI_CC_OK);<br>> > +    // Catch the actual operaton by peeking into request buffer<br>> > +    uint8_t chassis_ctrl_cmd = * (uint8_t *)request;<br>> > +    printf ("Chassis Control Command: Operation:[0x%X]\n", chassis_ctrl_cmd);<br>> > +<br>> > +    switch (chassis_ctrl_cmd) {<br>> > +    case CMD_POWER_OFF:<br>> > +        rc = ipmi_chassis_power_control ("powerOff");<br>> > +        break;<br>> > +<br>> > +    case CMD_HARD_RESET:<br>> > +        rc = ipmi_chassis_power_control ("reboot");<br>> > +        break;<br>> > +<br>> > +    default: {<br>> > +        fprintf (stderr, "Invalid Chassis Control command:[0x%X] received\n", chassis_ctrl_cmd);<br>> > +        rc = -1;<br>> > +    }<br>> > +    }<br>> > +<br>> > +    return ((rc < 0) ? IPMI_CC_INVALID : IPMI_CC_OK);<br>> >  }<br>> >  <br>> > -ipmi_ret_t ipmi_chassis_get_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> > -                              ipmi_request_t request, ipmi_response_t response,<br>> > -                              ipmi_data_len_t data_len, ipmi_context_t context)<br>> > +ipmi_ret_t ipmi_chassis_get_sys_boot_options (ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> > +        ipmi_request_t request, ipmi_response_t response,<br>> > +        ipmi_data_len_t data_len, ipmi_context_t context)<br>> >  {<br>> >      ipmi_ret_t rc = IPMI_CC_OK;<br>> >      *data_len = 0;<br>> >  <br>> > -    printf("IPMI GET_SYS_BOOT_OPTIONS\n");<br>> > +    printf ("IPMI GET_SYS_BOOT_OPTIONS\n");<br>> > +<br>> > +    get_sys_boot_options_t * reqptr = (get_sys_boot_options_t *) request;<br>> > +<br>> > +    /*<br>> > +     * To represent a hex in string, e.g., "A0A0", which represents two bytes<br>> > +     * in the hex, but requires 5 bytes to store it as string, i.e., 4<br>> > +     * characters to store the "A0A0" and 1 additional space for "\0".<br>> > +     * Thereby we have 2 * <number of bytes> + 1 for the string buffer.<br>> > +     */<br>> > +    char buf[NUM_RETURN_BYTES_OF_GET * 2 + 1] = {0};<br>><br>> Once again, why not #define NUM_RETURN_BYTES_OF_GET 11<br>><br>> >  <br>> > -    get_sys_boot_options_t *reqptr = (get_sys_boot_options_t*) request;<br>> > +    /*<br>> > +     * Parameter #5 means boot flags. Please refer to 28.13 of ipmi doc.<br>> > +     * This is the only parameter used by petitboot.<br>> > +     */<br>> > +    if (reqptr->parameter == 5) {<br>> > +        int r = dbus_get_property (buf);<br>> >  <br>> > -    // TODO Return default values to OPAL until dbus interface is available<br>> > +        if (r < 0) {<br>> > +            fprintf (stderr, "Dbus get property failed for get_sys_boot_options.\n");<br>> > +            return IPMI_OUT_OF_SPACE;<br>> > +        }<br>> >  <br>> > -    if (reqptr->parameter == 5) // Parameter #5<br>> > -    {<br>> > -        uint8_t buf[] = {0x1,0x5,80,0,0,0,0};<br>> > -        *data_len = sizeof(buf);<br>> > -        memcpy(response, &buf, *data_len);<br>> > +        uint64_t return_value;<br>> > +        sscanf (buf, "%llX", &return_value);<br>><br>> See my comment at the top about the best tool for the job.<br>><br>> > +<br>> > +        *data_len = NUM_RETURN_BYTES_OF_GET;<br>> > +        /*<br>> > +         * TODO: last 3 bytes<br>> > +         *(NUM_RETURN_BYTES_OF_GET - NUM_RETURN_BYTES_OF_GET_USED) is meanlingless<br>> > +         */<br>> > +        memcpy (response, (uint8_t *) (&return_value), *data_len);<br>><br>> So this interests me, does whoever is going to be reading response know to read<br>> it as a uint64_t on success but as an array of chars on failure, also, rather<br>> than #defineing, wouldn't:<br>><br>> *data_len = sizeof(return_value);<br>><br>> be better?<br>><br>><br>> I've done some looking around in ipmid.C and I don't think what you've done<br>> will work,<br>> <a href="https://github.com/openbmc/phosphor-host-ipmid/blob/master/ipmid.C#L276" target="_blank" >https://github.com/openbmc/phosphor-host-ipmid/blob/master/ipmid.C#L276</a><br>> appears to use the first byte of response as the completion code but you've<br>> just written whatever the first byte of your uint64_t is into response[0]...<br>><br>> Also, 28.13 in the IPMI spec doesn't seem to match up with whats going on here,<br>> am I missing something?<br>><br>> > +    } else {<br>> > +        *data_len = NUM_RETURN_BYTES_OF_GET;<br>> > +        // Parameter not supported<br>> > +        buf[0] = IPMI_CC_PARM_NOT_SUPPORTED;<br>> > +        memcpy (response, buf, *data_len);<br>> > +        fprintf (stderr, "Unsupported parameter 0x%x\n", reqptr->parameter);<br>> > +        return IPMI_CC_PARM_NOT_SUPPORTED;<br>> >      }<br>> > -    else<br>> > -    {<br>> > -        fprintf(stderr, "Unsupported parameter 0x%x\n", reqptr->parameter);<br>> > -        return IPMI_CC_PARM_NOT_SUPPORTED;        <br>> > +<br>> > +    return rc;<br>> > +}<br>> > +<br>> > +ipmi_ret_t ipmi_chassis_set_sys_boot_options (ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> > +        ipmi_request_t request, ipmi_response_t response,<br>> > +        ipmi_data_len_t data_len, ipmi_context_t context)<br>> > +{<br>> > +    ipmi_ret_t rc = IPMI_CC_OK;<br>> > +<br>> > +    printf ("IPMI SET_SYS_BOOT_OPTIONS\n");<br>> > +    printf ("IPMID set command required return bytes: %i\n", *data_len);<br>> > +<br>> > +    set_sys_boot_options_t * reqptr = (set_sys_boot_options_t *) request;<br>> > +<br>> > +    char output_buf[NUM_RETURN_BYTES_OF_SET] = {0};<br>> > +<br>> > +    /*<br>> > +     * Parameter #5 means boot flags. Please refer to 28.13 of ipmi doc.<br>> > +     * This is the only parameter used by petitboot.<br>> > +     */<br>> > +    if (reqptr->parameter == 5) {<br>> > +        /*<br>> > +         * To represent a hex in string, e.g., "A0A0", which represents two bytes<br>> > +         * in the hex, but requires 5 bytes to store it as string, i.e., 4<br>> > +         * characters to store the "A0A0" and 1 additional space for "\0".<br>> > +         * Thereby we have 2 * <number of bytes> + 1 for the string buffer.<br>> > +         */<br>> > +        char input_buf[NUM_INPUT_BYTES_OF_SET * 2 + 1];<br>> > +        sprintf (input_buf, "%llX", reqptr->data.data64);<br>> > +<br>> > +        int r = dbus_set_property (input_buf);<br>> > +<br>> > +        if (r < 0) {<br>> > +            fprintf (stderr, "Dbus set property failed for set_sys_boot_options.\n");<br>> > +            return IPMI_OUT_OF_SPACE;<br>> > +        }<br>> > +<br>> > +        *data_len = NUM_RETURN_BYTES_OF_SET;<br>> > +        // Return code OK.<br>> > +        output_buf[0] = IPMI_OK;<br>> > +        memcpy (response, output_buf, *data_len);<br>> > +    } else {<br>> > +        // Parameter not supported<br>> > +        output_buf[0] = IPMI_CC_PARM_NOT_SUPPORTED;<br>> > +        memcpy (response, output_buf, *data_len);<br>> > +        fprintf (stderr, "Unsupported parameter 0x%x\n", reqptr->parameter);<br>> > +        return IPMI_CC_PARM_NOT_SUPPORTED;<br>><br>> As far as I can see you're simply using output_buf to assign the first byte of<br>> response to either IPMI_OK or IPMI_CC_PARM_NOT_SUPPORTED and you're using<br>> memcpy.<br>><br>> Wouldn't it be nicer to<br>> memset(response, 0, *data_len);<br>> (or not even memset since NUM_RETURN_BYTES_OF_SET is 1) and<br>> *response = IPMI_OK;<br>> or<br>> *response = IPMI_CC_PARM_NOT_SUPPORTED;<br>> and avoid having output_buf at all?<br>><br>> Which makes me wonder why response is actually a void *, is this a good idea?<br>><br>> >      }<br>> >  <br>> >      return rc;<br>> >  }<br>> >  <br>> > -void register_netfn_chassis_functions()<br>> > +void register_netfn_chassis_functions ()<br>> >  {<br>> > -    printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_CHASSIS, IPMI_CMD_WILDCARD);<br>> > -    ipmi_register_callback(NETFUN_CHASSIS, IPMI_CMD_WILDCARD, NULL, ipmi_chassis_wildcard);<br>> > +    printf ("Registering NetFn:[0x%X], Cmd:[0x%X]\n", NETFUN_CHASSIS, IPMI_CMD_WILDCARD);<br>> > +    ipmi_register_callback (NETFUN_CHASSIS, IPMI_CMD_WILDCARD, NULL, ipmi_chassis_wildcard);<br>> >  <br>> > -    printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_CHASSIS, IPMI_CMD_GET_SYS_BOOT_OPTIONS);<br>> > -    ipmi_register_callback(NETFUN_CHASSIS, IPMI_CMD_GET_SYS_BOOT_OPTIONS, NULL, ipmi_chassis_get_sys_boot_options);<br>> > +    printf ("Registering NetFn:[0x%X], Cmd:[0x%X]\n", NETFUN_CHASSIS, IPMI_CMD_GET_SYS_BOOT_OPTIONS);<br>> > +    ipmi_register_callback (NETFUN_CHASSIS, IPMI_CMD_GET_SYS_BOOT_OPTIONS, NULL, ipmi_chassis_get_sys_boot_options);<br>> >  <br>> > -    printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_CHASSIS, IPMI_CMD_CHASSIS_CONTROL);<br>> > -    ipmi_register_callback(NETFUN_CHASSIS, IPMI_CMD_CHASSIS_CONTROL, NULL, ipmi_chassis_control);<br>> > +    printf ("Registering NetFn:[0x%X], Cmd:[0x%X]\n", NETFUN_CHASSIS, IPMI_CMD_SET_SYS_BOOT_OPTIONS);<br>> > +    ipmi_register_callback (NETFUN_CHASSIS, IPMI_CMD_SET_SYS_BOOT_OPTIONS, NULL, ipmi_chassis_set_sys_boot_options);<br>> > +<br>> > +    printf ("Registering NetFn:[0x%X], Cmd:[0x%X]\n", NETFUN_CHASSIS, IPMI_CMD_CHASSIS_CONTROL);<br>> > +    ipmi_register_callback (NETFUN_CHASSIS, IPMI_CMD_CHASSIS_CONTROL, NULL, ipmi_chassis_control);<br>> >  }<br>> > +<br>> > diff --git a/chassishandler.h b/chassishandler.h<br>> > index 1a26411..a310741 100644<br>> > --- a/chassishandler.h<br>> > +++ b/chassishandler.h<br>> > @@ -3,21 +3,39 @@<br>> >  <br>> >  #include <stdint.h><br>> >  <br>> > +// TODO: Petitboot requires 8 bytes of response<br>> > +// however only 5 of them are used. The remaining<br>> > +// 3 bytes are not used in petitboot and the value<br>> > +// of them are all zero.<br>> > +#define NUM_RETURN_BYTES_OF_GET 8<br>> > +#define NUM_RETURN_BYTES_OF_GET_USED 5<br>> > +#define NUM_RETURN_BYTES_OF_SET 1<br>> > +#define NUM_INPUT_BYTES_OF_SET 5<br>> > +<br>> >  // IPMI commands for Chassis net functions.<br>> >  enum ipmi_netfn_app_cmds<br>> >  {<br>> >   // Chassis Control<br>> >   IPMI_CMD_CHASSIS_CONTROL  = 0x02,<br>> >      // Get capability bits<br>> > +    IPMI_CMD_SET_SYS_BOOT_OPTIONS = 0x08,<br>> >      IPMI_CMD_GET_SYS_BOOT_OPTIONS = 0x09,<br>> >  };<br>> >  <br>> >  // Command specific completion codes<br>> >  enum ipmi_chassis_return_codes<br>> >  {<br>> > +    IPMI_OK = 0x0,<br>> >      IPMI_CC_PARM_NOT_SUPPORTED = 0x80,<br>> >  };<br>> >  <br>> > +// Generic completion codes,<br>> > +// see IPMI doc section 5.2<br>> > +enum ipmi_generic_return_codes<br>> > +{<br>> > +    IPMI_OUT_OF_SPACE = 0xC4,<br>> > +};<br>> > +<br>> >  // Various Chassis operations under a single command.<br>> >  enum ipmi_chassis_control_cmds : uint8_t<br>> >  {<br>>  <br>>  <br>><br>> </font><br> </div></blockquote>
<div dir="ltr" > </div></div><BR>