Re: [PATCH phosphor-host-ipmid v2] Add get/set ipmid command support with correct DBUS property handling.

[Michael Neuling <mikey@neuling.org>]

> +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;
> +
> +    printf("IPMI SET_SYS_BOOT_OPTIONS\n");
> +    printf("IPMID set command required return bytes: %i\n", *data_len);
> +
> +    set_sys_boot_options_t *reqptr = (set_sys_boot_options_t*) request;
> +
> +    char* output_buf = (char*)malloc(NUM_RETURN_BYTES_OF_SET);

This seems to be allocating 1 byte.  Why not just allocate this
locally on the stack with:

   char output_buf;

then you can avoid the free and memory checks and a bunch of
other complexity.

I don't think you even need this buffer.

> +
> +    if (output_buf == NULL) {
> +        fprintf(stderr, "Malloc failed for set_sys_boot_options.\n");
> +        return IPMI_OUT_OF_SPACE;        
> +    }
> +
> +    if (reqptr->parameter == 5) // Parameter #5
> +    {
> +        char input_buf[NUM_INPUT_BYTES_OF_SET*2 + 1];
> +        sprintf(input_buf, "%llX", reqptr->data.data64);
> +
> +        int r = dbus_set_property(input_buf);
> +
> +        if (r < 0) {
> +            fprintf(stderr, "Dbus set property failed for set_sys_boot_options.\n");
> +            return IPMI_OUT_OF_SPACE;
> +        }
> +
> +        *data_len = NUM_RETURN_BYTES_OF_SET;
> +        // 0x0: return code OK.
> +        output_buf[0] = 0x0;
> +        memcpy(response, output_buf, *data_len);

This is a memcpy of 1 byte.  Just assign it and remove the rest of the code.
     *response = 0;

> +        free(output_buf);
> +    }
> +    else
> +    {
> +        // 0x80: parameter not supported
> +        output_buf[0] = 0x80;
> +        memcpy(response, output_buf, *data_len);

*data_len is passed in, but output_buf is only 1 byte?  Doesn't make
overflowing output_buf really easy?

Or should this be a 1 byte copy too and then we can just assign it
with something like:
     *response = 0x80;

Mikey