[PATCH phosphor-host-ipmid v3 2/6] Add get/set ipmid command support with correct DBUS property handling.

Chris Austen austenc at us.ibm.com
Thu Dec 17 15:01:09 AEDT 2015


I'm very confused with this patch.  
   
    1) sdbus already supports a get and set property so do not create your own
    
    2) there is a lot of deletes.  Feels like this needs some additional eyes.  
  
  Sent from my iPhone using IBM Verse
  
  On Dec 16, 2015, 4:46:14 PM, "Stewart Smith" <stewart at linux.vnet.ibm.com> wrote:
  
     OpenBMC Patches  writes:
   > From: shgoupf 
   >
   > 1) Two methods to handle the dbus property set and get:
   >     a) dbus_set_property()
   >     b) dbus_get_property()
   > 2) The property is stored as a 10 character strings which represents
   > 5-byte information.
   > 3) ipmid set method is registered and implemented since petitboot will
   > use it to clear the boot options.
   > ---
   >  apphandler.C       |  70 ++----------
   >  apphandler.h       |  33 +-----
   >  chassishandler.C   | 306 ++++++++++++++++++++++++++++++++++++++---------------
   >  chassishandler.h   |  20 ++--
   >  ipmid-api.h        |  13 +--
   >  ipmid.C            |  26 ++---
   >  ipmisensor.C       |  35 +-----
   >  sensorhandler.C    |  17 ++-
   >  storageaddsel.C    |  42 ++++++--
   >  storagehandler.C   |  20 +---
   >  transporthandler.C |   2 +
   >  11 files changed, 304 insertions(+), 280 deletions(-)
   >
   > diff --git a/apphandler.C b/apphandler.C
   > index 2c9ce6b..6467397 100644
   > --- a/apphandler.C
   > +++ b/apphandler.C
   > @@ -10,76 +10,20 @@ extern sd_bus *bus;
   >  
   >  void register_netfn_app_functions() __attribute__((constructor));
   >  
   > -//---------------------------------------------------------------------
   > -// Called by Host on seeing a SMS_ATN bit set. Return a hardcoded 
   > -// value of 0x2 indicating we need Host read some data.
   > -//-------------------------------------------------------------------
   > -ipmi_ret_t ipmi_app_get_msg_flags(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)
   It is unclear why this is being removed. Is it replaced by something else?
   > -//-------------------------------------------------------------------
   > -// Called by Host post response from Get_Message_Flags
   > -//-------------------------------------------------------------------
   >  ipmi_ret_t ipmi_app_read_event(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)
   >  {
   >      ipmi_ret_t rc = IPMI_CC_OK;
   > -    printf("IPMI APP READ EVENT command received\n");
   > -
   > - // TODO : For now, this is catering only to the Soft Power Off via OEM SEL
   > - //        mechanism. If we need to make this generically used for some
   > - //        other conditions, then we can take advantage of context pointer.
   > -
   > - struct oem_sel_timestamped soft_off = {0};
   > -    *data_len = sizeof(struct oem_sel_timestamped);
   > -
   > - // either id[0] -or- id[1] can be filled in. We will use id[0]
   > - soft_off.id[0]  = SEL_OEM_ID_0;
   > - soft_off.id[1]  = SEL_OEM_ID_0;
   > - soft_off.type  = SEL_RECORD_TYPE_OEM;
   > -
   > - // Following 3 bytes are from IANA Manufactre_Id field. See below
   > - soft_off.manuf_id[0]= 0x41;
   > - soft_off.manuf_id[1]= 0xA7;
   > - soft_off.manuf_id[2]= 0x00;
   > -
   > - // per IPMI spec NetFuntion for OEM
   > - soft_off.netfun  = 0x3A;
   > -
   > - // Mechanism to kick start soft shutdown.
   > - soft_off.cmd  = CMD_POWER;
   > - soft_off.data[0] = SOFT_OFF;
   > -
   > - // All '0xFF' since unused.
   > - memset(&soft_off.data[1], 0xFF, 3);
   > +    *data_len = 0;
   >  
   > -    // Pack the actual response
   > -    memcpy(response, &soft_off, *data_len);
   > +    printf("IPMI APP READ EVENT Ignoring for now\n");
   >      return rc;
   > +
   >  }
   Feels like should be a separate patch, I'm unsure as to how this is
   related to adding get/set methods.
   > diff --git a/apphandler.h b/apphandler.h
   > index aa2a55d..35a2b20 100644
   > --- a/apphandler.h
   > +++ b/apphandler.h
   > @@ -1,19 +1,6 @@
   >  #ifndef __HOST_IPMI_APP_HANDLER_H__
   >  #define __HOST_IPMI_APP_HANDLER_H__
   >  
   > -#include 
   > -
   > -// These are per skiboot ipmi-sel code
   > -
   > -// OEM_SEL type with Timestamp
   > -#define SEL_OEM_ID_0 0x55
   > -// SEL type is OEM and -not- general SEL
   > -#define SEL_RECORD_TYPE_OEM 0xC0
   > -// Minor command for soft shurdown
   > -#define SOFT_OFF 0x00
   > -// Major command for Any kind of power ops
   > -#define CMD_POWER 0x04
   > -
   not sure how the removal of these is related to this patch. Should be a
   separate one?
   >  // IPMI commands for App net functions.
   >  enum ipmi_netfn_app_cmds
   >  {
   > @@ -24,27 +11,9 @@ enum ipmi_netfn_app_cmds
   >      IPMI_CMD_RESET_WD               = 0x22,
   >      IPMI_CMD_SET_WD                 = 0x24,
   >      IPMI_CMD_SET_BMC_GLOBAL_ENABLES = 0x2E,
   > -    IPMI_CMD_GET_MSG_FLAGS          = 0x31,
   >      IPMI_CMD_READ_EVENT             = 0x35,
   >      IPMI_CMD_GET_CAP_BIT            = 0x36,
   > -};
   same here.
   >  
   > -// A Mechanism to tell host to shtudown hosts by sending this PEM SEL. Really
   > -// the only used fields by skiboot are:
   > -// id[0] / id[1] for ID_0 , ID_1
   > -// type : SEL_RECORD_TYPE_OEM as standard SELs are ignored by skiboot
   > -// cmd : CMD_POWER for power functions
   > -// data[0], specific commands.  example Soft power off. power cycle, etc.
   > -struct oem_sel_timestamped
   > -{
   > - /* SEL header */
   > - uint8_t id[2];
   > - uint8_t type;
   > - uint8_t manuf_id[3];
   > - uint8_t timestamp[4];
   > - /* OEM SEL data (6 bytes) follows */
   > - uint8_t netfun;
   > - uint8_t cmd;
   > - uint8_t data[4];
   >  };
   > +
   >  #endif
   and here.
   > diff --git a/chassishandler.C b/chassishandler.C
   > index 1389db9..7694bd5 100644
   > --- a/chassishandler.C
   > +++ b/chassishandler.C
   > @@ -4,10 +4,147 @@
   >  #include 
   >  #include 
   >  
   > -// OpenBMC Chassis Manager dbus framework
   > -const char  *chassis_bus_name      =  "org.openbmc.control.Chassis";
   > -const char  *chassis_object_name   =  "/org/openbmc/control/chassis0";
   > -const char  *chassis_intf_name     =  "org.openbmc.control.Chassis";
   > +char* uint8_to_char(uint8_t *a, size_t size)
   > +{
   > +    char* buffer;
   > +    int i;
   > +
   > +    buffer = (char*)malloc(size * 2 + 1);
   > +    if (!buffer)
   > +        return NULL;
   > +
   > +    buffer[size * 2] = 0;
   > +    for (i = 0; i < size; i++) {
   > +        uint8_t msb = (a[i] >> 4) & 0xF;
   > +        uint8_t lsb = a[i] & 0xF;
   > +        buffer[2*i] = msb > 9 ? msb + 'A' - 10 : msb + '0';
   > +        buffer[2*i + 1] = lsb > 9 ? lsb + 'A' - 10 : lsb + '0';
   > +    }
   > +
   > +    return buffer;
   > +}
   > +
   > +uint8_t* char_to_uint8(char *a, size_t size)
   > +{
   > +    uint8_t* buffer;
   > +    int i;
   > +
   > +    buffer = (uint8_t*)malloc(size);
   > +    if (!buffer)
   > +        return NULL;
   > +
   > +    for (i = 0; i < size; i++) {
   > +        uint8_t msb = (uint8_t)(a[2*i] > '9' ? a[2*i] - 'A' + 10 : a[2*i] - '0');
   > +        uint8_t lsb = (uint8_t)(a[2*i+1] > '9' ? a[2*i+1] - 'A' + 10 : a[2*i+1] - '0');
   > +        buffer[i] = ((msb << 4) | (lsb & 0xF)) & 0xFF;
   > +    }
   > +
   > +    return buffer;
   > +}
   What do these functions *do*? The names of uint8_to_char and
   char_to_uint8 make absolutely zero sense to me as a char is, in fact, 8
   bits, i.e. a uint8_t, just with a different semantic meaning.
   Is there some weird IPMI specific format of strings?
   > -//----------------------------------------------------------------------
   > -// Chassis Control commands
   > -//----------------------------------------------------------------------
   > -ipmi_ret_t ipmi_chassis_control(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)
   > -{
   > - // Error from power off.
   > - int rc = 0;
   > +    char* buf = (char*)malloc(NUM_RETURN_BYTES_OF_GET);
   >  
   > - // No response for this command.
   > -    *data_len = 0;
   > +    if (reqptr->parameter == 5) // Parameter #5
   > +    {
   > +        dbus_get_property(buf);
   > +        uint8_t* return_value = char_to_uint8(buf, NUM_RETURN_BYTES_OF_GET_USED);
   > +        *data_len = NUM_RETURN_BYTES_OF_GET;
   > +        // TODO: last 3 bytes
   > +        // (NUM_RETURN_BYTES_OF_GET - NUM_RETURN_BYTES_OF_GET_USED) is meanlingless
   > +        memcpy(response, return_value, *data_len);
   > +        free(buf);
   > +        free(return_value);
   This isn't safe if dbus_get_property fails (which it can). Indeed, if it
   does, you'll be running a whole bunch of code on unitialised data. If
   you're lucky you'll segv, if unlucky, exploitable security hole.
   > -ipmi_ret_t ipmi_chassis_get_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd, 
   > +ipmi_ret_t ipmi_chassis_set_sys_boot_options(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)
   >  {
   >      ipmi_ret_t rc = IPMI_CC_OK;
   > -    *data_len = 0;
   >  
   > -    printf("IPMI GET_SYS_BOOT_OPTIONS\n");
   > +    printf("IPMI SET_SYS_BOOT_OPTIONS\n");
   > +    printf("IPMID set command required return bytes: %i\n", *data_len);
   >  
   > -    get_sys_boot_options_t *reqptr = (get_sys_boot_options_t*) request;
   > +    set_sys_boot_options_t *reqptr = (set_sys_boot_options_t*) request;
   >  
   > -    // TODO Return default values to OPAL until dbus interface is available
   > +    char* output_buf = (char*)malloc(NUM_RETURN_BYTES_OF_SET);
   This pattern repeats itself a lot - not checking for return value of
   malloc.
   Since OpenBMC is going to be running in a relatively constrained
   resource environment, likely without swap and there's probably a good
   argument for turning memory overcommit off, you probably want to fail
   somewhat gracefully - or at the very least assert() and fail in a known
   way rather than continue ond dereference null pointers.
   > @@ -141,6 +257,28 @@ void register_netfn_chassis_functions()
   >      printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_CHASSIS, IPMI_CMD_GET_SYS_BOOT_OPTIONS);
   >      ipmi_register_callback(NETFUN_CHASSIS, IPMI_CMD_GET_SYS_BOOT_OPTIONS, NULL, ipmi_chassis_get_sys_boot_options);
   >  
   > -    printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_CHASSIS, IPMI_CMD_CHASSIS_CONTROL);
   > -    ipmi_register_callback(NETFUN_CHASSIS, IPMI_CMD_CHASSIS_CONTROL, NULL, ipmi_chassis_control);
   > +    printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_CHASSIS, IPMI_CMD_SET_SYS_BOOT_OPTIONS);
   > +    ipmi_register_callback(NETFUN_CHASSIS, IPMI_CMD_SET_SYS_BOOT_OPTIONS, NULL, ipmi_chassis_set_sys_boot_options);
   > +
   > +    // TODO: Testing for dbus property set/get and related methods.
   > +    printf("----> Start of chassis handler testing.\n");
   > +    set_sys_boot_options_t req = {0x80, 0x10, 0xA2, 0x3B, 0x45, 0x57}; 
   > +    char* set_value = uint8_to_char((uint8_t*)(&(req.data1)), 5);
   > +    dbus_set_property(set_value);
   > +    char* buf = (char*)malloc(NUM_RETURN_BYTES_OF_GET_USED * 2 + 1);
   again, not checking results of dbus_set_property or malloc.
   > +    dbus_get_property(buf);
   > +    uint8_t* get_value = char_to_uint8(buf, NUM_RETURN_BYTES_OF_GET_USED);
   > +    int i;
   > +    printf("buf: %s\n", (char*)buf);
   > +    printf("0x");
   > +    for (i = 0; i < 5; i++) {
   > +        printf("%2x", get_value[i]);
   > +    }
   > +    printf("\n");
   > +    printf("----> End of chassis handler testing.\n");
   > +    free(buf);
   > +    free(set_value);
   > +    free(get_value);
   >  }
   > +
   > +
   > diff --git a/chassishandler.h b/chassishandler.h
   > index 1a26411..80e40a8 100644
   > --- a/chassishandler.h
   > +++ b/chassishandler.h
   > @@ -1,14 +1,18 @@
   >  #ifndef __HOST_IPMI_CHASSIS_HANDLER_H__
   >  #define __HOST_IPMI_CHASSIS_HANDLER_H__
   >  
   > -#include 
   > +// TODO: Petitboot requires 8 bytes of response
   > +// however only 5 of them are used.
   Is this due to petitboto behaviour quirk or something that's
   standardized somewhere?
   what should the unused bytes be set to? 0x00? 0xFF?
   _______________________________________________
   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/20151217/487eda21/attachment-0001.html>


More information about the openbmc mailing list