[v2,01/10] ipmi: remove IPMI_CHECK_CMD_LEN() macro

Message ID	1456913698-11452-2-git-send-email-clg@fr.ibm.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org> Gateway: Authorized Use Only! Violators will be prosecuted for <qemu-devel@nongnu.org> from <clg@fr.ibm.com>; Wed, 2 Mar 2016 10:15:39 -0000 Gateway: Authorized Use Only! Violators will be prosecuted; Wed, 2 Mar 2016 10:15:09 -0000 From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= <clg@fr.ibm.com> To: Corey Minyard <cminyard@mvista.com> Date: Wed, 2 Mar 2016 11:14:49 +0100 Message-Id: <1456913698-11452-2-git-send-email-clg@fr.ibm.com> In-Reply-To: <1456913698-11452-1-git-send-email-clg@fr.ibm.com> References: <1456913698-11452-1-git-send-email-clg@fr.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cc: Marcel Apfelbaum <marcel@redhat.com>, =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= <clg@fr.ibm.com>, qemu-devel@nongnu.org, "Michael S. Tsirkin" <mst@redhat.com> Subject: [Qemu-devel] [PATCH v2 01/10] ipmi: remove IPMI_CHECK_CMD_LEN() macro Precedence: list Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org

Cédric Le Goater March 2, 2016, 10:14 a.m. UTC

Most IPMI command handlers in the BMC simulator start with a call to
the macro IPMI_CHECK_CMD_LEN() which verifies that a minimal number of
arguments expected by the command are indeed available. To achieve
this task, the macro implicitly uses local variables which is
misleading in the code.

This patch adds a 'cmd_len_min' attribute to the struct IPMICmdHandler
defining the minimal number of arguments expected by the command and
moves this check in the global command handler ipmi_sim_handle_command().

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
---
 hw/ipmi/ipmi_bmc_sim.c | 137 ++++++++++++++++++++++---------------------------
 1 file changed, 62 insertions(+), 75 deletions(-)

Corey Minyard March 5, 2016, 11:41 a.m. UTC | #1

On 03/02/2016 04:14 AM, Cédric Le Goater wrote:
> Most IPMI command handlers in the BMC simulator start with a call to
> the macro IPMI_CHECK_CMD_LEN() which verifies that a minimal number of
> arguments expected by the command are indeed available. To achieve
> this task, the macro implicitly uses local variables which is
> misleading in the code.
>
> This patch adds a 'cmd_len_min' attribute to the struct IPMICmdHandler
> defining the minimal number of arguments expected by the command and
> moves this check in the global command handler ipmi_sim_handle_command().

This is much better.  One style comment inline...

Acked-by: Corey Minyard <cminyard@mvista.com>

> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
> ---
>   hw/ipmi/ipmi_bmc_sim.c | 137 ++++++++++++++++++++++---------------------------
>   1 file changed, 62 insertions(+), 75 deletions(-)
>
> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
> index 51d234aa1bf2..30b9fb48ea2d 100644
> --- a/hw/ipmi/ipmi_bmc_sim.c
> +++ b/hw/ipmi/ipmi_bmc_sim.c
> @@ -155,10 +155,15 @@ typedef struct IPMISensor {
>   typedef struct IPMIBmcSim IPMIBmcSim;
>
>   #define MAX_NETFNS 64
> -typedef void (*IPMICmdHandler)(IPMIBmcSim *s,
> -                               uint8_t *cmd, unsigned int cmd_len,
> -                               uint8_t *rsp, unsigned int *rsp_len,
> -                               unsigned int max_rsp_len);
> +
> +typedef struct IPMICmdHandler {
> +    void (*cmd_handler)(IPMIBmcSim *s,
> +                        uint8_t *cmd, unsigned int cmd_len,
> +                        uint8_t *rsp, unsigned int *rsp_len,
> +                        unsigned int max_rsp_len);
> +    unsigned int cmd_len_min;
> +} IPMICmdHandler;
> +
>   typedef struct IPMINetfn {
>       unsigned int cmd_nums;
>       const IPMICmdHandler *cmd_handlers;
> @@ -269,13 +274,6 @@ struct IPMIBmcSim {
>           rsp[(*rsp_len)++] = (b);                           \
>       } while (0)
>
> -/* Verify that the received command is a certain length. */
> -#define IPMI_CHECK_CMD_LEN(l) \
> -    if (cmd_len < l) {                                     \
> -        rsp[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID;      \
> -        return; \
> -    }
> -
>   /* Check that the reservation in the command is valid. */
>   #define IPMI_CHECK_RESERVATION(off, r) \
>       do {                                                   \
> @@ -623,14 +621,19 @@ static void ipmi_sim_handle_command(IPMIBmc *b,
>
>       /* Odd netfns are not valid, make sure the command is registered */
>       if ((netfn & 1) || !ibs->netfns[netfn / 2] ||
> -                        (cmd[1] >= ibs->netfns[netfn / 2]->cmd_nums) ||
> -                        (!ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]])) {
> +        (cmd[1] >= ibs->netfns[netfn / 2]->cmd_nums) ||
> +        (!ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]].cmd_handler)) {
>           rsp[2] = IPMI_CC_INVALID_CMD;
>           goto out;

I'm not sure of the qemu style here, but the above change makes the code 
hard to read.

The qemu style really seems to be to not have big if statements like
this, so maybe this crasy check should be moved to a separate function?

>       }
>
> -    ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]](ibs, cmd, cmd_len, rsp, rsp_len,
> -                                                max_rsp_len);
> +    if (cmd_len < ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]].cmd_len_min) {
> +        rsp[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID;
> +        goto out;
> +    }
> +
> +    ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]].cmd_handler(ibs, cmd, cmd_len,
> +                                                rsp, rsp_len, max_rsp_len);
>
>    out:
>       k->handle_rsp(s, msg_id, rsp, *rsp_len);
> @@ -737,7 +740,6 @@ static void chassis_control(IPMIBmcSim *ibs,
>       IPMIInterface *s = ibs->parent.intf;
>       IPMIInterfaceClass *k = IPMI_INTERFACE_GET_CLASS(s);
>
> -    IPMI_CHECK_CMD_LEN(3);
>       switch (cmd[2] & 0xf) {
>       case 0: /* power down */
>           rsp[2] = k->do_hw_op(s, IPMI_POWEROFF_CHASSIS, 0);
> @@ -838,7 +840,6 @@ static void set_acpi_power_state(IPMIBmcSim *ibs,
>                             uint8_t *rsp, unsigned int *rsp_len,
>                             unsigned int max_rsp_len)
>   {
> -    IPMI_CHECK_CMD_LEN(4);
>       ibs->acpi_power_state[0] = cmd[2];
>       ibs->acpi_power_state[1] = cmd[3];
>   }
> @@ -869,7 +870,6 @@ static void set_bmc_global_enables(IPMIBmcSim *ibs,
>                                      uint8_t *rsp, unsigned int *rsp_len,
>                                      unsigned int max_rsp_len)
>   {
> -    IPMI_CHECK_CMD_LEN(3);
>       set_global_enables(ibs, cmd[2]);
>   }
>
> @@ -889,7 +889,6 @@ static void clr_msg_flags(IPMIBmcSim *ibs,
>       IPMIInterface *s = ibs->parent.intf;
>       IPMIInterfaceClass *k = IPMI_INTERFACE_GET_CLASS(s);
>
> -    IPMI_CHECK_CMD_LEN(3);
>       ibs->msg_flags &= ~cmd[2];
>       k->set_atn(s, attn_set(ibs), attn_irq_enabled(ibs));
>   }
> @@ -976,15 +975,17 @@ static void send_msg(IPMIBmcSim *ibs,
>       uint8_t *buf;
>       uint8_t netfn, rqLun, rsLun, rqSeq;
>
> -    IPMI_CHECK_CMD_LEN(3);
> -
>       if (cmd[2] != 0) {
>           /* We only handle channel 0 with no options */
>           rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
>           return;
>       }
>
> -    IPMI_CHECK_CMD_LEN(10);
> +    if (cmd_len < 10) {
> +        rsp[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID;
> +        return;
> +    }
> +
>       if (cmd[3] != 0x40) {
>           /* We only emulate a MC at address 0x40. */
>           rsp[2] = 0x83; /* NAK on write */
> @@ -1092,7 +1093,6 @@ static void set_watchdog_timer(IPMIBmcSim *ibs,
>       IPMIInterfaceClass *k = IPMI_INTERFACE_GET_CLASS(s);
>       unsigned int val;
>
> -    IPMI_CHECK_CMD_LEN(8);
>       val = cmd[2] & 0x7; /* Validate use */
>       if (val == 0 || val > 5) {
>           rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
> @@ -1217,7 +1217,6 @@ static void get_sdr(IPMIBmcSim *ibs,
>       uint16_t nextrec;
>       struct ipmi_sdr_header *sdrh;
>
> -    IPMI_CHECK_CMD_LEN(8);
>       if (cmd[6]) {
>           IPMI_CHECK_RESERVATION(2, ibs->sdr.reservation);
>       }
> @@ -1271,7 +1270,6 @@ static void clear_sdr_rep(IPMIBmcSim *ibs,
>                             uint8_t *rsp, unsigned int *rsp_len,
>                             unsigned int max_rsp_len)
>   {
> -    IPMI_CHECK_CMD_LEN(8);
>       IPMI_CHECK_RESERVATION(2, ibs->sdr.reservation);
>       if (cmd[4] != 'C' || cmd[5] != 'L' || cmd[6] != 'R') {
>           rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
> @@ -1330,7 +1328,6 @@ static void get_sel_entry(IPMIBmcSim *ibs,
>   {
>       unsigned int val;
>
> -    IPMI_CHECK_CMD_LEN(8);
>       if (cmd[6]) {
>           IPMI_CHECK_RESERVATION(2, ibs->sel.reservation);
>       }
> @@ -1375,7 +1372,6 @@ static void add_sel_entry(IPMIBmcSim *ibs,
>                             uint8_t *rsp, unsigned int *rsp_len,
>                             unsigned int max_rsp_len)
>   {
> -    IPMI_CHECK_CMD_LEN(18);
>       if (sel_add_event(ibs, cmd + 2)) {
>           rsp[2] = IPMI_CC_OUT_OF_SPACE;
>           return;
> @@ -1390,7 +1386,6 @@ static void clear_sel(IPMIBmcSim *ibs,
>                         uint8_t *rsp, unsigned int *rsp_len,
>                         unsigned int max_rsp_len)
>   {
> -    IPMI_CHECK_CMD_LEN(8);
>       IPMI_CHECK_RESERVATION(2, ibs->sel.reservation);
>       if (cmd[4] != 'C' || cmd[5] != 'L' || cmd[6] != 'R') {
>           rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
> @@ -1434,7 +1429,6 @@ static void set_sel_time(IPMIBmcSim *ibs,
>       uint32_t val;
>       struct ipmi_time now;
>
> -    IPMI_CHECK_CMD_LEN(6);
>       val = cmd[2] | (cmd[3] << 8) | (cmd[4] << 16) | (cmd[5] << 24);
>       ipmi_gettime(&now);
>       ibs->sel.time_offset = now.tv_sec - ((long) val);
> @@ -1447,7 +1441,6 @@ static void set_sensor_evt_enable(IPMIBmcSim *ibs,
>   {
>       IPMISensor *sens;
>
> -    IPMI_CHECK_CMD_LEN(4);
>       if ((cmd[2] >= MAX_SENSORS) ||
>               !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
>           rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
> @@ -1499,7 +1492,6 @@ static void get_sensor_evt_enable(IPMIBmcSim *ibs,
>   {
>       IPMISensor *sens;
>
> -    IPMI_CHECK_CMD_LEN(3);
>       if ((cmd[2] >= MAX_SENSORS) ||
>           !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
>           rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
> @@ -1520,7 +1512,6 @@ static void rearm_sensor_evts(IPMIBmcSim *ibs,
>   {
>       IPMISensor *sens;
>
> -    IPMI_CHECK_CMD_LEN(4);
>       if ((cmd[2] >= MAX_SENSORS) ||
>           !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
>           rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
> @@ -1542,7 +1533,6 @@ static void get_sensor_evt_status(IPMIBmcSim *ibs,
>   {
>       IPMISensor *sens;
>
> -    IPMI_CHECK_CMD_LEN(3);
>       if ((cmd[2] >= MAX_SENSORS) ||
>           !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
>           rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
> @@ -1564,7 +1554,6 @@ static void get_sensor_reading(IPMIBmcSim *ibs,
>   {
>       IPMISensor *sens;
>
> -    IPMI_CHECK_CMD_LEN(3);
>       if ((cmd[2] >= MAX_SENSORS) ||
>               !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
>           rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
> @@ -1587,7 +1576,6 @@ static void set_sensor_type(IPMIBmcSim *ibs,
>       IPMISensor *sens;
>
>
> -    IPMI_CHECK_CMD_LEN(5);
>       if ((cmd[2] >= MAX_SENSORS) ||
>               !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
>           rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
> @@ -1606,7 +1594,6 @@ static void get_sensor_type(IPMIBmcSim *ibs,
>       IPMISensor *sens;
>
>
> -    IPMI_CHECK_CMD_LEN(3);
>       if ((cmd[2] >= MAX_SENSORS) ||
>               !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
>           rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
> @@ -1619,10 +1606,10 @@ static void get_sensor_type(IPMIBmcSim *ibs,
>
>
>   static const IPMICmdHandler chassis_cmds[] = {
> -    [IPMI_CMD_GET_CHASSIS_CAPABILITIES] = chassis_capabilities,
> -    [IPMI_CMD_GET_CHASSIS_STATUS] = chassis_status,
> -    [IPMI_CMD_CHASSIS_CONTROL] = chassis_control,
> -    [IPMI_CMD_GET_SYS_RESTART_CAUSE] = chassis_get_sys_restart_cause
> +    [IPMI_CMD_GET_CHASSIS_CAPABILITIES] = { chassis_capabilities },
> +    [IPMI_CMD_GET_CHASSIS_STATUS] = { chassis_status },
> +    [IPMI_CMD_CHASSIS_CONTROL] = { chassis_control, 3 },
> +    [IPMI_CMD_GET_SYS_RESTART_CAUSE] = { chassis_get_sys_restart_cause }
>   };
>   static const IPMINetfn chassis_netfn = {
>       .cmd_nums = ARRAY_SIZE(chassis_cmds),
> @@ -1630,13 +1617,13 @@ static const IPMINetfn chassis_netfn = {
>   };
>
>   static const IPMICmdHandler sensor_event_cmds[] = {
> -    [IPMI_CMD_SET_SENSOR_EVT_ENABLE] = set_sensor_evt_enable,
> -    [IPMI_CMD_GET_SENSOR_EVT_ENABLE] = get_sensor_evt_enable,
> -    [IPMI_CMD_REARM_SENSOR_EVTS] = rearm_sensor_evts,
> -    [IPMI_CMD_GET_SENSOR_EVT_STATUS] = get_sensor_evt_status,
> -    [IPMI_CMD_GET_SENSOR_READING] = get_sensor_reading,
> -    [IPMI_CMD_SET_SENSOR_TYPE] = set_sensor_type,
> -    [IPMI_CMD_GET_SENSOR_TYPE] = get_sensor_type,
> +    [IPMI_CMD_SET_SENSOR_EVT_ENABLE] = { set_sensor_evt_enable, 4 },
> +    [IPMI_CMD_GET_SENSOR_EVT_ENABLE] = { get_sensor_evt_enable, 3 },
> +    [IPMI_CMD_REARM_SENSOR_EVTS] = { rearm_sensor_evts, 4 },
> +    [IPMI_CMD_GET_SENSOR_EVT_STATUS] = { get_sensor_evt_status, 3 },
> +    [IPMI_CMD_GET_SENSOR_READING] = { get_sensor_reading, 3 },
> +    [IPMI_CMD_SET_SENSOR_TYPE] = { set_sensor_type, 5 },
> +    [IPMI_CMD_GET_SENSOR_TYPE] = { get_sensor_type, 3 },
>   };
>   static const IPMINetfn sensor_event_netfn = {
>       .cmd_nums = ARRAY_SIZE(sensor_event_cmds),
> @@ -1644,22 +1631,22 @@ static const IPMINetfn sensor_event_netfn = {
>   };
>
>   static const IPMICmdHandler app_cmds[] = {
> -    [IPMI_CMD_GET_DEVICE_ID] = get_device_id,
> -    [IPMI_CMD_COLD_RESET] = cold_reset,
> -    [IPMI_CMD_WARM_RESET] = warm_reset,
> -    [IPMI_CMD_SET_ACPI_POWER_STATE] = set_acpi_power_state,
> -    [IPMI_CMD_GET_ACPI_POWER_STATE] = get_acpi_power_state,
> -    [IPMI_CMD_GET_DEVICE_GUID] = get_device_guid,
> -    [IPMI_CMD_SET_BMC_GLOBAL_ENABLES] = set_bmc_global_enables,
> -    [IPMI_CMD_GET_BMC_GLOBAL_ENABLES] = get_bmc_global_enables,
> -    [IPMI_CMD_CLR_MSG_FLAGS] = clr_msg_flags,
> -    [IPMI_CMD_GET_MSG_FLAGS] = get_msg_flags,
> -    [IPMI_CMD_GET_MSG] = get_msg,
> -    [IPMI_CMD_SEND_MSG] = send_msg,
> -    [IPMI_CMD_READ_EVT_MSG_BUF] = read_evt_msg_buf,
> -    [IPMI_CMD_RESET_WATCHDOG_TIMER] = reset_watchdog_timer,
> -    [IPMI_CMD_SET_WATCHDOG_TIMER] = set_watchdog_timer,
> -    [IPMI_CMD_GET_WATCHDOG_TIMER] = get_watchdog_timer,
> +    [IPMI_CMD_GET_DEVICE_ID] = { get_device_id },
> +    [IPMI_CMD_COLD_RESET] = { cold_reset },
> +    [IPMI_CMD_WARM_RESET] = { warm_reset },
> +    [IPMI_CMD_SET_ACPI_POWER_STATE] = { set_acpi_power_state, 4 },
> +    [IPMI_CMD_GET_ACPI_POWER_STATE] = { get_acpi_power_state },
> +    [IPMI_CMD_GET_DEVICE_GUID] = { get_device_guid },
> +    [IPMI_CMD_SET_BMC_GLOBAL_ENABLES] = { set_bmc_global_enables, 3 },
> +    [IPMI_CMD_GET_BMC_GLOBAL_ENABLES] = { get_bmc_global_enables },
> +    [IPMI_CMD_CLR_MSG_FLAGS] = { clr_msg_flags, 3 },
> +    [IPMI_CMD_GET_MSG_FLAGS] = { get_msg_flags },
> +    [IPMI_CMD_GET_MSG] = { get_msg },
> +    [IPMI_CMD_SEND_MSG] = { send_msg, 3 },
> +    [IPMI_CMD_READ_EVT_MSG_BUF] = { read_evt_msg_buf },
> +    [IPMI_CMD_RESET_WATCHDOG_TIMER] = { reset_watchdog_timer },
> +    [IPMI_CMD_SET_WATCHDOG_TIMER] = { set_watchdog_timer, 8 },
> +    [IPMI_CMD_GET_WATCHDOG_TIMER] = { get_watchdog_timer },
>   };
>   static const IPMINetfn app_netfn = {
>       .cmd_nums = ARRAY_SIZE(app_cmds),
> @@ -1667,18 +1654,18 @@ static const IPMINetfn app_netfn = {
>   };
>
>   static const IPMICmdHandler storage_cmds[] = {
> -    [IPMI_CMD_GET_SDR_REP_INFO] = get_sdr_rep_info,
> -    [IPMI_CMD_RESERVE_SDR_REP] = reserve_sdr_rep,
> -    [IPMI_CMD_GET_SDR] = get_sdr,
> -    [IPMI_CMD_ADD_SDR] = add_sdr,
> -    [IPMI_CMD_CLEAR_SDR_REP] = clear_sdr_rep,
> -    [IPMI_CMD_GET_SEL_INFO] = get_sel_info,
> -    [IPMI_CMD_RESERVE_SEL] = reserve_sel,
> -    [IPMI_CMD_GET_SEL_ENTRY] = get_sel_entry,
> -    [IPMI_CMD_ADD_SEL_ENTRY] = add_sel_entry,
> -    [IPMI_CMD_CLEAR_SEL] = clear_sel,
> -    [IPMI_CMD_GET_SEL_TIME] = get_sel_time,
> -    [IPMI_CMD_SET_SEL_TIME] = set_sel_time,
> +    [IPMI_CMD_GET_SDR_REP_INFO] = { get_sdr_rep_info },
> +    [IPMI_CMD_RESERVE_SDR_REP] = { reserve_sdr_rep },
> +    [IPMI_CMD_GET_SDR] = { get_sdr, 8 },
> +    [IPMI_CMD_ADD_SDR] = { add_sdr },
> +    [IPMI_CMD_CLEAR_SDR_REP] = { clear_sdr_rep, 8 },
> +    [IPMI_CMD_GET_SEL_INFO] = { get_sel_info },
> +    [IPMI_CMD_RESERVE_SEL] = { reserve_sel },
> +    [IPMI_CMD_GET_SEL_ENTRY] = { get_sel_entry, 8 },
> +    [IPMI_CMD_ADD_SEL_ENTRY] = { add_sel_entry, 18 },
> +    [IPMI_CMD_CLEAR_SEL] = { clear_sel, 8 },
> +    [IPMI_CMD_GET_SEL_TIME] = { get_sel_time, 6 },
> +    [IPMI_CMD_SET_SEL_TIME] = { set_sel_time },
>   };
>
>   static const IPMINetfn storage_netfn = {

Cédric Le Goater March 7, 2016, 10:40 a.m. UTC | #2

On 03/05/2016 12:41 PM, Corey Minyard wrote:
> On 03/02/2016 04:14 AM, Cédric Le Goater wrote:
>> Most IPMI command handlers in the BMC simulator start with a call to
>> the macro IPMI_CHECK_CMD_LEN() which verifies that a minimal number of
>> arguments expected by the command are indeed available. To achieve
>> this task, the macro implicitly uses local variables which is
>> misleading in the code.
>>
>> This patch adds a 'cmd_len_min' attribute to the struct IPMICmdHandler
>> defining the minimal number of arguments expected by the command and
>> moves this check in the global command handler ipmi_sim_handle_command().
> 
> This is much better.  One style comment inline...
> 
> Acked-by: Corey Minyard <cminyard@mvista.com>
> 
>> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
>> ---
>>   hw/ipmi/ipmi_bmc_sim.c | 137 ++++++++++++++++++++++---------------------------
>>   1 file changed, 62 insertions(+), 75 deletions(-)
>>
>> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
>> index 51d234aa1bf2..30b9fb48ea2d 100644
>> --- a/hw/ipmi/ipmi_bmc_sim.c
>> +++ b/hw/ipmi/ipmi_bmc_sim.c
>> @@ -155,10 +155,15 @@ typedef struct IPMISensor {
>>   typedef struct IPMIBmcSim IPMIBmcSim;
>>
>>   #define MAX_NETFNS 64
>> -typedef void (*IPMICmdHandler)(IPMIBmcSim *s,
>> -                               uint8_t *cmd, unsigned int cmd_len,
>> -                               uint8_t *rsp, unsigned int *rsp_len,
>> -                               unsigned int max_rsp_len);
>> +
>> +typedef struct IPMICmdHandler {
>> +    void (*cmd_handler)(IPMIBmcSim *s,
>> +                        uint8_t *cmd, unsigned int cmd_len,
>> +                        uint8_t *rsp, unsigned int *rsp_len,
>> +                        unsigned int max_rsp_len);
>> +    unsigned int cmd_len_min;
>> +} IPMICmdHandler;
>> +
>>   typedef struct IPMINetfn {
>>       unsigned int cmd_nums;
>>       const IPMICmdHandler *cmd_handlers;
>> @@ -269,13 +274,6 @@ struct IPMIBmcSim {
>>           rsp[(*rsp_len)++] = (b);                           \
>>       } while (0)
>>
>> -/* Verify that the received command is a certain length. */
>> -#define IPMI_CHECK_CMD_LEN(l) \
>> -    if (cmd_len < l) {                                     \
>> -        rsp[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID;      \
>> -        return; \
>> -    }
>> -
>>   /* Check that the reservation in the command is valid. */
>>   #define IPMI_CHECK_RESERVATION(off, r) \
>>       do {                                                   \
>> @@ -623,14 +621,19 @@ static void ipmi_sim_handle_command(IPMIBmc *b,
>>
>>       /* Odd netfns are not valid, make sure the command is registered */
>>       if ((netfn & 1) || !ibs->netfns[netfn / 2] ||
>> -                        (cmd[1] >= ibs->netfns[netfn / 2]->cmd_nums) ||
>> -                        (!ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]])) {
>> +        (cmd[1] >= ibs->netfns[netfn / 2]->cmd_nums) ||
>> +        (!ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]].cmd_handler)) {
>>           rsp[2] = IPMI_CC_INVALID_CMD;
>>           goto out;
> 
> I'm not sure of the qemu style here, but the above change makes the code hard to read.

Cleary. I was lazy. I will try to clarify what this code does with a helper
or some intermediate variables.

Thanks,

C.

> The qemu style really seems to be to not have big if statements like
> this, so maybe this crasy check should be moved to a separate function?
>
>>       }
>>
>> -    ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]](ibs, cmd, cmd_len, rsp, rsp_len,
>> -                                                max_rsp_len);
>> +    if (cmd_len < ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]].cmd_len_min) {
>> +        rsp[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID;
>> +        goto out;
>> +    }
>> +
>> +    ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]].cmd_handler(ibs, cmd, cmd_len,
>> +                                                rsp, rsp_len, max_rsp_len);
>>
>>    out:
>>       k->handle_rsp(s, msg_id, rsp, *rsp_len);
>> @@ -737,7 +740,6 @@ static void chassis_control(IPMIBmcSim *ibs,
>>       IPMIInterface *s = ibs->parent.intf;
>>       IPMIInterfaceClass *k = IPMI_INTERFACE_GET_CLASS(s);
>>
>> -    IPMI_CHECK_CMD_LEN(3);
>>       switch (cmd[2] & 0xf) {
>>       case 0: /* power down */
>>           rsp[2] = k->do_hw_op(s, IPMI_POWEROFF_CHASSIS, 0);
>> @@ -838,7 +840,6 @@ static void set_acpi_power_state(IPMIBmcSim *ibs,
>>                             uint8_t *rsp, unsigned int *rsp_len,
>>                             unsigned int max_rsp_len)
>>   {
>> -    IPMI_CHECK_CMD_LEN(4);
>>       ibs->acpi_power_state[0] = cmd[2];
>>       ibs->acpi_power_state[1] = cmd[3];
>>   }
>> @@ -869,7 +870,6 @@ static void set_bmc_global_enables(IPMIBmcSim *ibs,
>>                                      uint8_t *rsp, unsigned int *rsp_len,
>>                                      unsigned int max_rsp_len)
>>   {
>> -    IPMI_CHECK_CMD_LEN(3);
>>       set_global_enables(ibs, cmd[2]);
>>   }
>>
>> @@ -889,7 +889,6 @@ static void clr_msg_flags(IPMIBmcSim *ibs,
>>       IPMIInterface *s = ibs->parent.intf;
>>       IPMIInterfaceClass *k = IPMI_INTERFACE_GET_CLASS(s);
>>
>> -    IPMI_CHECK_CMD_LEN(3);
>>       ibs->msg_flags &= ~cmd[2];
>>       k->set_atn(s, attn_set(ibs), attn_irq_enabled(ibs));
>>   }
>> @@ -976,15 +975,17 @@ static void send_msg(IPMIBmcSim *ibs,
>>       uint8_t *buf;
>>       uint8_t netfn, rqLun, rsLun, rqSeq;
>>
>> -    IPMI_CHECK_CMD_LEN(3);
>> -
>>       if (cmd[2] != 0) {
>>           /* We only handle channel 0 with no options */
>>           rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
>>           return;
>>       }
>>
>> -    IPMI_CHECK_CMD_LEN(10);
>> +    if (cmd_len < 10) {
>> +        rsp[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID;
>> +        return;
>> +    }
>> +
>>       if (cmd[3] != 0x40) {
>>           /* We only emulate a MC at address 0x40. */
>>           rsp[2] = 0x83; /* NAK on write */
>> @@ -1092,7 +1093,6 @@ static void set_watchdog_timer(IPMIBmcSim *ibs,
>>       IPMIInterfaceClass *k = IPMI_INTERFACE_GET_CLASS(s);
>>       unsigned int val;
>>
>> -    IPMI_CHECK_CMD_LEN(8);
>>       val = cmd[2] & 0x7; /* Validate use */
>>       if (val == 0 || val > 5) {
>>           rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
>> @@ -1217,7 +1217,6 @@ static void get_sdr(IPMIBmcSim *ibs,
>>       uint16_t nextrec;
>>       struct ipmi_sdr_header *sdrh;
>>
>> -    IPMI_CHECK_CMD_LEN(8);
>>       if (cmd[6]) {
>>           IPMI_CHECK_RESERVATION(2, ibs->sdr.reservation);
>>       }
>> @@ -1271,7 +1270,6 @@ static void clear_sdr_rep(IPMIBmcSim *ibs,
>>                             uint8_t *rsp, unsigned int *rsp_len,
>>                             unsigned int max_rsp_len)
>>   {
>> -    IPMI_CHECK_CMD_LEN(8);
>>       IPMI_CHECK_RESERVATION(2, ibs->sdr.reservation);
>>       if (cmd[4] != 'C' || cmd[5] != 'L' || cmd[6] != 'R') {
>>           rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
>> @@ -1330,7 +1328,6 @@ static void get_sel_entry(IPMIBmcSim *ibs,
>>   {
>>       unsigned int val;
>>
>> -    IPMI_CHECK_CMD_LEN(8);
>>       if (cmd[6]) {
>>           IPMI_CHECK_RESERVATION(2, ibs->sel.reservation);
>>       }
>> @@ -1375,7 +1372,6 @@ static void add_sel_entry(IPMIBmcSim *ibs,
>>                             uint8_t *rsp, unsigned int *rsp_len,
>>                             unsigned int max_rsp_len)
>>   {
>> -    IPMI_CHECK_CMD_LEN(18);
>>       if (sel_add_event(ibs, cmd + 2)) {
>>           rsp[2] = IPMI_CC_OUT_OF_SPACE;
>>           return;
>> @@ -1390,7 +1386,6 @@ static void clear_sel(IPMIBmcSim *ibs,
>>                         uint8_t *rsp, unsigned int *rsp_len,
>>                         unsigned int max_rsp_len)
>>   {
>> -    IPMI_CHECK_CMD_LEN(8);
>>       IPMI_CHECK_RESERVATION(2, ibs->sel.reservation);
>>       if (cmd[4] != 'C' || cmd[5] != 'L' || cmd[6] != 'R') {
>>           rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
>> @@ -1434,7 +1429,6 @@ static void set_sel_time(IPMIBmcSim *ibs,
>>       uint32_t val;
>>       struct ipmi_time now;
>>
>> -    IPMI_CHECK_CMD_LEN(6);
>>       val = cmd[2] | (cmd[3] << 8) | (cmd[4] << 16) | (cmd[5] << 24);
>>       ipmi_gettime(&now);
>>       ibs->sel.time_offset = now.tv_sec - ((long) val);
>> @@ -1447,7 +1441,6 @@ static void set_sensor_evt_enable(IPMIBmcSim *ibs,
>>   {
>>       IPMISensor *sens;
>>
>> -    IPMI_CHECK_CMD_LEN(4);
>>       if ((cmd[2] >= MAX_SENSORS) ||
>>               !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
>>           rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
>> @@ -1499,7 +1492,6 @@ static void get_sensor_evt_enable(IPMIBmcSim *ibs,
>>   {
>>       IPMISensor *sens;
>>
>> -    IPMI_CHECK_CMD_LEN(3);
>>       if ((cmd[2] >= MAX_SENSORS) ||
>>           !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
>>           rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
>> @@ -1520,7 +1512,6 @@ static void rearm_sensor_evts(IPMIBmcSim *ibs,
>>   {
>>       IPMISensor *sens;
>>
>> -    IPMI_CHECK_CMD_LEN(4);
>>       if ((cmd[2] >= MAX_SENSORS) ||
>>           !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
>>           rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
>> @@ -1542,7 +1533,6 @@ static void get_sensor_evt_status(IPMIBmcSim *ibs,
>>   {
>>       IPMISensor *sens;
>>
>> -    IPMI_CHECK_CMD_LEN(3);
>>       if ((cmd[2] >= MAX_SENSORS) ||
>>           !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
>>           rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
>> @@ -1564,7 +1554,6 @@ static void get_sensor_reading(IPMIBmcSim *ibs,
>>   {
>>       IPMISensor *sens;
>>
>> -    IPMI_CHECK_CMD_LEN(3);
>>       if ((cmd[2] >= MAX_SENSORS) ||
>>               !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
>>           rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
>> @@ -1587,7 +1576,6 @@ static void set_sensor_type(IPMIBmcSim *ibs,
>>       IPMISensor *sens;
>>
>>
>> -    IPMI_CHECK_CMD_LEN(5);
>>       if ((cmd[2] >= MAX_SENSORS) ||
>>               !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
>>           rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
>> @@ -1606,7 +1594,6 @@ static void get_sensor_type(IPMIBmcSim *ibs,
>>       IPMISensor *sens;
>>
>>
>> -    IPMI_CHECK_CMD_LEN(3);
>>       if ((cmd[2] >= MAX_SENSORS) ||
>>               !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
>>           rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
>> @@ -1619,10 +1606,10 @@ static void get_sensor_type(IPMIBmcSim *ibs,
>>
>>
>>   static const IPMICmdHandler chassis_cmds[] = {
>> -    [IPMI_CMD_GET_CHASSIS_CAPABILITIES] = chassis_capabilities,
>> -    [IPMI_CMD_GET_CHASSIS_STATUS] = chassis_status,
>> -    [IPMI_CMD_CHASSIS_CONTROL] = chassis_control,
>> -    [IPMI_CMD_GET_SYS_RESTART_CAUSE] = chassis_get_sys_restart_cause
>> +    [IPMI_CMD_GET_CHASSIS_CAPABILITIES] = { chassis_capabilities },
>> +    [IPMI_CMD_GET_CHASSIS_STATUS] = { chassis_status },
>> +    [IPMI_CMD_CHASSIS_CONTROL] = { chassis_control, 3 },
>> +    [IPMI_CMD_GET_SYS_RESTART_CAUSE] = { chassis_get_sys_restart_cause }
>>   };
>>   static const IPMINetfn chassis_netfn = {
>>       .cmd_nums = ARRAY_SIZE(chassis_cmds),
>> @@ -1630,13 +1617,13 @@ static const IPMINetfn chassis_netfn = {
>>   };
>>
>>   static const IPMICmdHandler sensor_event_cmds[] = {
>> -    [IPMI_CMD_SET_SENSOR_EVT_ENABLE] = set_sensor_evt_enable,
>> -    [IPMI_CMD_GET_SENSOR_EVT_ENABLE] = get_sensor_evt_enable,
>> -    [IPMI_CMD_REARM_SENSOR_EVTS] = rearm_sensor_evts,
>> -    [IPMI_CMD_GET_SENSOR_EVT_STATUS] = get_sensor_evt_status,
>> -    [IPMI_CMD_GET_SENSOR_READING] = get_sensor_reading,
>> -    [IPMI_CMD_SET_SENSOR_TYPE] = set_sensor_type,
>> -    [IPMI_CMD_GET_SENSOR_TYPE] = get_sensor_type,
>> +    [IPMI_CMD_SET_SENSOR_EVT_ENABLE] = { set_sensor_evt_enable, 4 },
>> +    [IPMI_CMD_GET_SENSOR_EVT_ENABLE] = { get_sensor_evt_enable, 3 },
>> +    [IPMI_CMD_REARM_SENSOR_EVTS] = { rearm_sensor_evts, 4 },
>> +    [IPMI_CMD_GET_SENSOR_EVT_STATUS] = { get_sensor_evt_status, 3 },
>> +    [IPMI_CMD_GET_SENSOR_READING] = { get_sensor_reading, 3 },
>> +    [IPMI_CMD_SET_SENSOR_TYPE] = { set_sensor_type, 5 },
>> +    [IPMI_CMD_GET_SENSOR_TYPE] = { get_sensor_type, 3 },
>>   };
>>   static const IPMINetfn sensor_event_netfn = {
>>       .cmd_nums = ARRAY_SIZE(sensor_event_cmds),
>> @@ -1644,22 +1631,22 @@ static const IPMINetfn sensor_event_netfn = {
>>   };
>>
>>   static const IPMICmdHandler app_cmds[] = {
>> -    [IPMI_CMD_GET_DEVICE_ID] = get_device_id,
>> -    [IPMI_CMD_COLD_RESET] = cold_reset,
>> -    [IPMI_CMD_WARM_RESET] = warm_reset,
>> -    [IPMI_CMD_SET_ACPI_POWER_STATE] = set_acpi_power_state,
>> -    [IPMI_CMD_GET_ACPI_POWER_STATE] = get_acpi_power_state,
>> -    [IPMI_CMD_GET_DEVICE_GUID] = get_device_guid,
>> -    [IPMI_CMD_SET_BMC_GLOBAL_ENABLES] = set_bmc_global_enables,
>> -    [IPMI_CMD_GET_BMC_GLOBAL_ENABLES] = get_bmc_global_enables,
>> -    [IPMI_CMD_CLR_MSG_FLAGS] = clr_msg_flags,
>> -    [IPMI_CMD_GET_MSG_FLAGS] = get_msg_flags,
>> -    [IPMI_CMD_GET_MSG] = get_msg,
>> -    [IPMI_CMD_SEND_MSG] = send_msg,
>> -    [IPMI_CMD_READ_EVT_MSG_BUF] = read_evt_msg_buf,
>> -    [IPMI_CMD_RESET_WATCHDOG_TIMER] = reset_watchdog_timer,
>> -    [IPMI_CMD_SET_WATCHDOG_TIMER] = set_watchdog_timer,
>> -    [IPMI_CMD_GET_WATCHDOG_TIMER] = get_watchdog_timer,
>> +    [IPMI_CMD_GET_DEVICE_ID] = { get_device_id },
>> +    [IPMI_CMD_COLD_RESET] = { cold_reset },
>> +    [IPMI_CMD_WARM_RESET] = { warm_reset },
>> +    [IPMI_CMD_SET_ACPI_POWER_STATE] = { set_acpi_power_state, 4 },
>> +    [IPMI_CMD_GET_ACPI_POWER_STATE] = { get_acpi_power_state },
>> +    [IPMI_CMD_GET_DEVICE_GUID] = { get_device_guid },
>> +    [IPMI_CMD_SET_BMC_GLOBAL_ENABLES] = { set_bmc_global_enables, 3 },
>> +    [IPMI_CMD_GET_BMC_GLOBAL_ENABLES] = { get_bmc_global_enables },
>> +    [IPMI_CMD_CLR_MSG_FLAGS] = { clr_msg_flags, 3 },
>> +    [IPMI_CMD_GET_MSG_FLAGS] = { get_msg_flags },
>> +    [IPMI_CMD_GET_MSG] = { get_msg },
>> +    [IPMI_CMD_SEND_MSG] = { send_msg, 3 },
>> +    [IPMI_CMD_READ_EVT_MSG_BUF] = { read_evt_msg_buf },
>> +    [IPMI_CMD_RESET_WATCHDOG_TIMER] = { reset_watchdog_timer },
>> +    [IPMI_CMD_SET_WATCHDOG_TIMER] = { set_watchdog_timer, 8 },
>> +    [IPMI_CMD_GET_WATCHDOG_TIMER] = { get_watchdog_timer },
>>   };
>>   static const IPMINetfn app_netfn = {
>>       .cmd_nums = ARRAY_SIZE(app_cmds),
>> @@ -1667,18 +1654,18 @@ static const IPMINetfn app_netfn = {
>>   };
>>
>>   static const IPMICmdHandler storage_cmds[] = {
>> -    [IPMI_CMD_GET_SDR_REP_INFO] = get_sdr_rep_info,
>> -    [IPMI_CMD_RESERVE_SDR_REP] = reserve_sdr_rep,
>> -    [IPMI_CMD_GET_SDR] = get_sdr,
>> -    [IPMI_CMD_ADD_SDR] = add_sdr,
>> -    [IPMI_CMD_CLEAR_SDR_REP] = clear_sdr_rep,
>> -    [IPMI_CMD_GET_SEL_INFO] = get_sel_info,
>> -    [IPMI_CMD_RESERVE_SEL] = reserve_sel,
>> -    [IPMI_CMD_GET_SEL_ENTRY] = get_sel_entry,
>> -    [IPMI_CMD_ADD_SEL_ENTRY] = add_sel_entry,
>> -    [IPMI_CMD_CLEAR_SEL] = clear_sel,
>> -    [IPMI_CMD_GET_SEL_TIME] = get_sel_time,
>> -    [IPMI_CMD_SET_SEL_TIME] = set_sel_time,
>> +    [IPMI_CMD_GET_SDR_REP_INFO] = { get_sdr_rep_info },
>> +    [IPMI_CMD_RESERVE_SDR_REP] = { reserve_sdr_rep },
>> +    [IPMI_CMD_GET_SDR] = { get_sdr, 8 },
>> +    [IPMI_CMD_ADD_SDR] = { add_sdr },
>> +    [IPMI_CMD_CLEAR_SDR_REP] = { clear_sdr_rep, 8 },
>> +    [IPMI_CMD_GET_SEL_INFO] = { get_sel_info },
>> +    [IPMI_CMD_RESERVE_SEL] = { reserve_sel },
>> +    [IPMI_CMD_GET_SEL_ENTRY] = { get_sel_entry, 8 },
>> +    [IPMI_CMD_ADD_SEL_ENTRY] = { add_sel_entry, 18 },
>> +    [IPMI_CMD_CLEAR_SEL] = { clear_sel, 8 },
>> +    [IPMI_CMD_GET_SEL_TIME] = { get_sel_time, 6 },
>> +    [IPMI_CMD_SET_SEL_TIME] = { set_sel_time },
>>   };
>>
>>   static const IPMINetfn storage_netfn = {
>

Cédric Le Goater March 8, 2016, 5:06 p.m. UTC | #3

On 03/07/2016 11:40 AM, Cédric Le Goater wrote:
> On 03/05/2016 12:41 PM, Corey Minyard wrote:
>> On 03/02/2016 04:14 AM, Cédric Le Goater wrote:
>>> Most IPMI command handlers in the BMC simulator start with a call to
>>> the macro IPMI_CHECK_CMD_LEN() which verifies that a minimal number of
>>> arguments expected by the command are indeed available. To achieve
>>> this task, the macro implicitly uses local variables which is
>>> misleading in the code.
>>>
>>> This patch adds a 'cmd_len_min' attribute to the struct IPMICmdHandler
>>> defining the minimal number of arguments expected by the command and
>>> moves this check in the global command handler ipmi_sim_handle_command().
>>
>> This is much better.  One style comment inline...
>>
>> Acked-by: Corey Minyard <cminyard@mvista.com>
>>
>>> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
>>> ---
>>>   hw/ipmi/ipmi_bmc_sim.c | 137 ++++++++++++++++++++++---------------------------
>>>   1 file changed, 62 insertions(+), 75 deletions(-)
>>>
>>> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
>>> index 51d234aa1bf2..30b9fb48ea2d 100644
>>> --- a/hw/ipmi/ipmi_bmc_sim.c
>>> +++ b/hw/ipmi/ipmi_bmc_sim.c
>>> @@ -155,10 +155,15 @@ typedef struct IPMISensor {
>>>   typedef struct IPMIBmcSim IPMIBmcSim;
>>>
>>>   #define MAX_NETFNS 64
>>> -typedef void (*IPMICmdHandler)(IPMIBmcSim *s,
>>> -                               uint8_t *cmd, unsigned int cmd_len,
>>> -                               uint8_t *rsp, unsigned int *rsp_len,
>>> -                               unsigned int max_rsp_len);
>>> +
>>> +typedef struct IPMICmdHandler {
>>> +    void (*cmd_handler)(IPMIBmcSim *s,
>>> +                        uint8_t *cmd, unsigned int cmd_len,
>>> +                        uint8_t *rsp, unsigned int *rsp_len,
>>> +                        unsigned int max_rsp_len);
>>> +    unsigned int cmd_len_min;
>>> +} IPMICmdHandler;
>>> +
>>>   typedef struct IPMINetfn {
>>>       unsigned int cmd_nums;
>>>       const IPMICmdHandler *cmd_handlers;
>>> @@ -269,13 +274,6 @@ struct IPMIBmcSim {
>>>           rsp[(*rsp_len)++] = (b);                           \
>>>       } while (0)
>>>
>>> -/* Verify that the received command is a certain length. */
>>> -#define IPMI_CHECK_CMD_LEN(l) \
>>> -    if (cmd_len < l) {                                     \
>>> -        rsp[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID;      \
>>> -        return; \
>>> -    }
>>> -
>>>   /* Check that the reservation in the command is valid. */
>>>   #define IPMI_CHECK_RESERVATION(off, r) \
>>>       do {                                                   \
>>> @@ -623,14 +621,19 @@ static void ipmi_sim_handle_command(IPMIBmc *b,
>>>
>>>       /* Odd netfns are not valid, make sure the command is registered */
>>>       if ((netfn & 1) || !ibs->netfns[netfn / 2] ||
>>> -                        (cmd[1] >= ibs->netfns[netfn / 2]->cmd_nums) ||
>>> -                        (!ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]])) {
>>> +        (cmd[1] >= ibs->netfns[netfn / 2]->cmd_nums) ||
>>> +        (!ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]].cmd_handler)) {
>>>           rsp[2] = IPMI_CC_INVALID_CMD;
>>>           goto out;
>>
>> I'm not sure of the qemu style here, but the above change makes the code hard to read.
> 
> Cleary. I was lazy. I will try to clarify what this code does with a helper
> or some intermediate variables.

How's that  ? 

@@ -591,6 +589,7 @@ static void ipmi_sim_handle_command(IPMI
     unsigned int rsp_len_holder = 0;
     unsigned int *rsp_len = &rsp_len_holder;
     unsigned int max_rsp_len = sizeof(rsp);
+    const IPMICmdHandler *hdl;
 
     /* Set up the response, set the low bit of NETFN. */
     /* Note that max_rsp_len must be at least 3 */
@@ -623,14 +622,23 @@ static void ipmi_sim_handle_command(IPMI
 
     /* Odd netfns are not valid, make sure the command is registered */
     if ((netfn & 1) || !ibs->netfns[netfn / 2] ||
-                        (cmd[1] >= ibs->netfns[netfn / 2]->cmd_nums) ||
-                        (!ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]])) {
+        (cmd[1] >= ibs->netfns[netfn / 2]->cmd_nums)) {
         rsp[2] = IPMI_CC_INVALID_CMD;
         goto out;
     }
 
-    ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]](ibs, cmd, cmd_len, rsp, rsp_len,
-                                                max_rsp_len);
+    hdl = &ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]];
+    if (!hdl->cmd_handler) {
+        rsp[2] = IPMI_CC_INVALID_CMD;
+        goto out;
+    }
+
+    if (cmd_len < hdl->cmd_len_min) {
+        rsp[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID;
+        goto out;
+    }
+
+    hdl->cmd_handler(ibs, cmd, cmd_len, rsp, rsp_len, max_rsp_len);
 
  out:
     k->handle_rsp(s, msg_id, rsp, *rsp_len);



The function still fits in one emacs page, so I think we are fine.

Just waiting a bit for Marcel to chime in before sending a v3.

Thanks,
 
C.

 
>> The qemu style really seems to be to not have big if statements like
>> this, so maybe this crasy check should be moved to a separate function?
>>
>>>       }
>>>
>>> -    ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]](ibs, cmd, cmd_len, rsp, rsp_len,
>>> -                                                max_rsp_len);
>>> +    if (cmd_len < ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]].cmd_len_min) {
>>> +        rsp[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID;
>>> +        goto out;
>>> +    }
>>> +
>>> +    ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]].cmd_handler(ibs, cmd, cmd_len,
>>> +                                                rsp, rsp_len, max_rsp_len);
>>>
>>>    out:
>>>       k->handle_rsp(s, msg_id, rsp, *rsp_len);
>>> @@ -737,7 +740,6 @@ static void chassis_control(IPMIBmcSim *ibs,
>>>       IPMIInterface *s = ibs->parent.intf;
>>>       IPMIInterfaceClass *k = IPMI_INTERFACE_GET_CLASS(s);
>>>
>>> -    IPMI_CHECK_CMD_LEN(3);
>>>       switch (cmd[2] & 0xf) {
>>>       case 0: /* power down */
>>>           rsp[2] = k->do_hw_op(s, IPMI_POWEROFF_CHASSIS, 0);
>>> @@ -838,7 +840,6 @@ static void set_acpi_power_state(IPMIBmcSim *ibs,
>>>                             uint8_t *rsp, unsigned int *rsp_len,
>>>                             unsigned int max_rsp_len)
>>>   {
>>> -    IPMI_CHECK_CMD_LEN(4);
>>>       ibs->acpi_power_state[0] = cmd[2];
>>>       ibs->acpi_power_state[1] = cmd[3];
>>>   }
>>> @@ -869,7 +870,6 @@ static void set_bmc_global_enables(IPMIBmcSim *ibs,
>>>                                      uint8_t *rsp, unsigned int *rsp_len,
>>>                                      unsigned int max_rsp_len)
>>>   {
>>> -    IPMI_CHECK_CMD_LEN(3);
>>>       set_global_enables(ibs, cmd[2]);
>>>   }
>>>
>>> @@ -889,7 +889,6 @@ static void clr_msg_flags(IPMIBmcSim *ibs,
>>>       IPMIInterface *s = ibs->parent.intf;
>>>       IPMIInterfaceClass *k = IPMI_INTERFACE_GET_CLASS(s);
>>>
>>> -    IPMI_CHECK_CMD_LEN(3);
>>>       ibs->msg_flags &= ~cmd[2];
>>>       k->set_atn(s, attn_set(ibs), attn_irq_enabled(ibs));
>>>   }
>>> @@ -976,15 +975,17 @@ static void send_msg(IPMIBmcSim *ibs,
>>>       uint8_t *buf;
>>>       uint8_t netfn, rqLun, rsLun, rqSeq;
>>>
>>> -    IPMI_CHECK_CMD_LEN(3);
>>> -
>>>       if (cmd[2] != 0) {
>>>           /* We only handle channel 0 with no options */
>>>           rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
>>>           return;
>>>       }
>>>
>>> -    IPMI_CHECK_CMD_LEN(10);
>>> +    if (cmd_len < 10) {
>>> +        rsp[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID;
>>> +        return;
>>> +    }
>>> +
>>>       if (cmd[3] != 0x40) {
>>>           /* We only emulate a MC at address 0x40. */
>>>           rsp[2] = 0x83; /* NAK on write */
>>> @@ -1092,7 +1093,6 @@ static void set_watchdog_timer(IPMIBmcSim *ibs,
>>>       IPMIInterfaceClass *k = IPMI_INTERFACE_GET_CLASS(s);
>>>       unsigned int val;
>>>
>>> -    IPMI_CHECK_CMD_LEN(8);
>>>       val = cmd[2] & 0x7; /* Validate use */
>>>       if (val == 0 || val > 5) {
>>>           rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
>>> @@ -1217,7 +1217,6 @@ static void get_sdr(IPMIBmcSim *ibs,
>>>       uint16_t nextrec;
>>>       struct ipmi_sdr_header *sdrh;
>>>
>>> -    IPMI_CHECK_CMD_LEN(8);
>>>       if (cmd[6]) {
>>>           IPMI_CHECK_RESERVATION(2, ibs->sdr.reservation);
>>>       }
>>> @@ -1271,7 +1270,6 @@ static void clear_sdr_rep(IPMIBmcSim *ibs,
>>>                             uint8_t *rsp, unsigned int *rsp_len,
>>>                             unsigned int max_rsp_len)
>>>   {
>>> -    IPMI_CHECK_CMD_LEN(8);
>>>       IPMI_CHECK_RESERVATION(2, ibs->sdr.reservation);
>>>       if (cmd[4] != 'C' || cmd[5] != 'L' || cmd[6] != 'R') {
>>>           rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
>>> @@ -1330,7 +1328,6 @@ static void get_sel_entry(IPMIBmcSim *ibs,
>>>   {
>>>       unsigned int val;
>>>
>>> -    IPMI_CHECK_CMD_LEN(8);
>>>       if (cmd[6]) {
>>>           IPMI_CHECK_RESERVATION(2, ibs->sel.reservation);
>>>       }
>>> @@ -1375,7 +1372,6 @@ static void add_sel_entry(IPMIBmcSim *ibs,
>>>                             uint8_t *rsp, unsigned int *rsp_len,
>>>                             unsigned int max_rsp_len)
>>>   {
>>> -    IPMI_CHECK_CMD_LEN(18);
>>>       if (sel_add_event(ibs, cmd + 2)) {
>>>           rsp[2] = IPMI_CC_OUT_OF_SPACE;
>>>           return;
>>> @@ -1390,7 +1386,6 @@ static void clear_sel(IPMIBmcSim *ibs,
>>>                         uint8_t *rsp, unsigned int *rsp_len,
>>>                         unsigned int max_rsp_len)
>>>   {
>>> -    IPMI_CHECK_CMD_LEN(8);
>>>       IPMI_CHECK_RESERVATION(2, ibs->sel.reservation);
>>>       if (cmd[4] != 'C' || cmd[5] != 'L' || cmd[6] != 'R') {
>>>           rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
>>> @@ -1434,7 +1429,6 @@ static void set_sel_time(IPMIBmcSim *ibs,
>>>       uint32_t val;
>>>       struct ipmi_time now;
>>>
>>> -    IPMI_CHECK_CMD_LEN(6);
>>>       val = cmd[2] | (cmd[3] << 8) | (cmd[4] << 16) | (cmd[5] << 24);
>>>       ipmi_gettime(&now);
>>>       ibs->sel.time_offset = now.tv_sec - ((long) val);
>>> @@ -1447,7 +1441,6 @@ static void set_sensor_evt_enable(IPMIBmcSim *ibs,
>>>   {
>>>       IPMISensor *sens;
>>>
>>> -    IPMI_CHECK_CMD_LEN(4);
>>>       if ((cmd[2] >= MAX_SENSORS) ||
>>>               !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
>>>           rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
>>> @@ -1499,7 +1492,6 @@ static void get_sensor_evt_enable(IPMIBmcSim *ibs,
>>>   {
>>>       IPMISensor *sens;
>>>
>>> -    IPMI_CHECK_CMD_LEN(3);
>>>       if ((cmd[2] >= MAX_SENSORS) ||
>>>           !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
>>>           rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
>>> @@ -1520,7 +1512,6 @@ static void rearm_sensor_evts(IPMIBmcSim *ibs,
>>>   {
>>>       IPMISensor *sens;
>>>
>>> -    IPMI_CHECK_CMD_LEN(4);
>>>       if ((cmd[2] >= MAX_SENSORS) ||
>>>           !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
>>>           rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
>>> @@ -1542,7 +1533,6 @@ static void get_sensor_evt_status(IPMIBmcSim *ibs,
>>>   {
>>>       IPMISensor *sens;
>>>
>>> -    IPMI_CHECK_CMD_LEN(3);
>>>       if ((cmd[2] >= MAX_SENSORS) ||
>>>           !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
>>>           rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
>>> @@ -1564,7 +1554,6 @@ static void get_sensor_reading(IPMIBmcSim *ibs,
>>>   {
>>>       IPMISensor *sens;
>>>
>>> -    IPMI_CHECK_CMD_LEN(3);
>>>       if ((cmd[2] >= MAX_SENSORS) ||
>>>               !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
>>>           rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
>>> @@ -1587,7 +1576,6 @@ static void set_sensor_type(IPMIBmcSim *ibs,
>>>       IPMISensor *sens;
>>>
>>>
>>> -    IPMI_CHECK_CMD_LEN(5);
>>>       if ((cmd[2] >= MAX_SENSORS) ||
>>>               !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
>>>           rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
>>> @@ -1606,7 +1594,6 @@ static void get_sensor_type(IPMIBmcSim *ibs,
>>>       IPMISensor *sens;
>>>
>>>
>>> -    IPMI_CHECK_CMD_LEN(3);
>>>       if ((cmd[2] >= MAX_SENSORS) ||
>>>               !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
>>>           rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
>>> @@ -1619,10 +1606,10 @@ static void get_sensor_type(IPMIBmcSim *ibs,
>>>
>>>
>>>   static const IPMICmdHandler chassis_cmds[] = {
>>> -    [IPMI_CMD_GET_CHASSIS_CAPABILITIES] = chassis_capabilities,
>>> -    [IPMI_CMD_GET_CHASSIS_STATUS] = chassis_status,
>>> -    [IPMI_CMD_CHASSIS_CONTROL] = chassis_control,
>>> -    [IPMI_CMD_GET_SYS_RESTART_CAUSE] = chassis_get_sys_restart_cause
>>> +    [IPMI_CMD_GET_CHASSIS_CAPABILITIES] = { chassis_capabilities },
>>> +    [IPMI_CMD_GET_CHASSIS_STATUS] = { chassis_status },
>>> +    [IPMI_CMD_CHASSIS_CONTROL] = { chassis_control, 3 },
>>> +    [IPMI_CMD_GET_SYS_RESTART_CAUSE] = { chassis_get_sys_restart_cause }
>>>   };
>>>   static const IPMINetfn chassis_netfn = {
>>>       .cmd_nums = ARRAY_SIZE(chassis_cmds),
>>> @@ -1630,13 +1617,13 @@ static const IPMINetfn chassis_netfn = {
>>>   };
>>>
>>>   static const IPMICmdHandler sensor_event_cmds[] = {
>>> -    [IPMI_CMD_SET_SENSOR_EVT_ENABLE] = set_sensor_evt_enable,
>>> -    [IPMI_CMD_GET_SENSOR_EVT_ENABLE] = get_sensor_evt_enable,
>>> -    [IPMI_CMD_REARM_SENSOR_EVTS] = rearm_sensor_evts,
>>> -    [IPMI_CMD_GET_SENSOR_EVT_STATUS] = get_sensor_evt_status,
>>> -    [IPMI_CMD_GET_SENSOR_READING] = get_sensor_reading,
>>> -    [IPMI_CMD_SET_SENSOR_TYPE] = set_sensor_type,
>>> -    [IPMI_CMD_GET_SENSOR_TYPE] = get_sensor_type,
>>> +    [IPMI_CMD_SET_SENSOR_EVT_ENABLE] = { set_sensor_evt_enable, 4 },
>>> +    [IPMI_CMD_GET_SENSOR_EVT_ENABLE] = { get_sensor_evt_enable, 3 },
>>> +    [IPMI_CMD_REARM_SENSOR_EVTS] = { rearm_sensor_evts, 4 },
>>> +    [IPMI_CMD_GET_SENSOR_EVT_STATUS] = { get_sensor_evt_status, 3 },
>>> +    [IPMI_CMD_GET_SENSOR_READING] = { get_sensor_reading, 3 },
>>> +    [IPMI_CMD_SET_SENSOR_TYPE] = { set_sensor_type, 5 },
>>> +    [IPMI_CMD_GET_SENSOR_TYPE] = { get_sensor_type, 3 },
>>>   };
>>>   static const IPMINetfn sensor_event_netfn = {
>>>       .cmd_nums = ARRAY_SIZE(sensor_event_cmds),
>>> @@ -1644,22 +1631,22 @@ static const IPMINetfn sensor_event_netfn = {
>>>   };
>>>
>>>   static const IPMICmdHandler app_cmds[] = {
>>> -    [IPMI_CMD_GET_DEVICE_ID] = get_device_id,
>>> -    [IPMI_CMD_COLD_RESET] = cold_reset,
>>> -    [IPMI_CMD_WARM_RESET] = warm_reset,
>>> -    [IPMI_CMD_SET_ACPI_POWER_STATE] = set_acpi_power_state,
>>> -    [IPMI_CMD_GET_ACPI_POWER_STATE] = get_acpi_power_state,
>>> -    [IPMI_CMD_GET_DEVICE_GUID] = get_device_guid,
>>> -    [IPMI_CMD_SET_BMC_GLOBAL_ENABLES] = set_bmc_global_enables,
>>> -    [IPMI_CMD_GET_BMC_GLOBAL_ENABLES] = get_bmc_global_enables,
>>> -    [IPMI_CMD_CLR_MSG_FLAGS] = clr_msg_flags,
>>> -    [IPMI_CMD_GET_MSG_FLAGS] = get_msg_flags,
>>> -    [IPMI_CMD_GET_MSG] = get_msg,
>>> -    [IPMI_CMD_SEND_MSG] = send_msg,
>>> -    [IPMI_CMD_READ_EVT_MSG_BUF] = read_evt_msg_buf,
>>> -    [IPMI_CMD_RESET_WATCHDOG_TIMER] = reset_watchdog_timer,
>>> -    [IPMI_CMD_SET_WATCHDOG_TIMER] = set_watchdog_timer,
>>> -    [IPMI_CMD_GET_WATCHDOG_TIMER] = get_watchdog_timer,
>>> +    [IPMI_CMD_GET_DEVICE_ID] = { get_device_id },
>>> +    [IPMI_CMD_COLD_RESET] = { cold_reset },
>>> +    [IPMI_CMD_WARM_RESET] = { warm_reset },
>>> +    [IPMI_CMD_SET_ACPI_POWER_STATE] = { set_acpi_power_state, 4 },
>>> +    [IPMI_CMD_GET_ACPI_POWER_STATE] = { get_acpi_power_state },
>>> +    [IPMI_CMD_GET_DEVICE_GUID] = { get_device_guid },
>>> +    [IPMI_CMD_SET_BMC_GLOBAL_ENABLES] = { set_bmc_global_enables, 3 },
>>> +    [IPMI_CMD_GET_BMC_GLOBAL_ENABLES] = { get_bmc_global_enables },
>>> +    [IPMI_CMD_CLR_MSG_FLAGS] = { clr_msg_flags, 3 },
>>> +    [IPMI_CMD_GET_MSG_FLAGS] = { get_msg_flags },
>>> +    [IPMI_CMD_GET_MSG] = { get_msg },
>>> +    [IPMI_CMD_SEND_MSG] = { send_msg, 3 },
>>> +    [IPMI_CMD_READ_EVT_MSG_BUF] = { read_evt_msg_buf },
>>> +    [IPMI_CMD_RESET_WATCHDOG_TIMER] = { reset_watchdog_timer },
>>> +    [IPMI_CMD_SET_WATCHDOG_TIMER] = { set_watchdog_timer, 8 },
>>> +    [IPMI_CMD_GET_WATCHDOG_TIMER] = { get_watchdog_timer },
>>>   };
>>>   static const IPMINetfn app_netfn = {
>>>       .cmd_nums = ARRAY_SIZE(app_cmds),
>>> @@ -1667,18 +1654,18 @@ static const IPMINetfn app_netfn = {
>>>   };
>>>
>>>   static const IPMICmdHandler storage_cmds[] = {
>>> -    [IPMI_CMD_GET_SDR_REP_INFO] = get_sdr_rep_info,
>>> -    [IPMI_CMD_RESERVE_SDR_REP] = reserve_sdr_rep,
>>> -    [IPMI_CMD_GET_SDR] = get_sdr,
>>> -    [IPMI_CMD_ADD_SDR] = add_sdr,
>>> -    [IPMI_CMD_CLEAR_SDR_REP] = clear_sdr_rep,
>>> -    [IPMI_CMD_GET_SEL_INFO] = get_sel_info,
>>> -    [IPMI_CMD_RESERVE_SEL] = reserve_sel,
>>> -    [IPMI_CMD_GET_SEL_ENTRY] = get_sel_entry,
>>> -    [IPMI_CMD_ADD_SEL_ENTRY] = add_sel_entry,
>>> -    [IPMI_CMD_CLEAR_SEL] = clear_sel,
>>> -    [IPMI_CMD_GET_SEL_TIME] = get_sel_time,
>>> -    [IPMI_CMD_SET_SEL_TIME] = set_sel_time,
>>> +    [IPMI_CMD_GET_SDR_REP_INFO] = { get_sdr_rep_info },
>>> +    [IPMI_CMD_RESERVE_SDR_REP] = { reserve_sdr_rep },
>>> +    [IPMI_CMD_GET_SDR] = { get_sdr, 8 },
>>> +    [IPMI_CMD_ADD_SDR] = { add_sdr },
>>> +    [IPMI_CMD_CLEAR_SDR_REP] = { clear_sdr_rep, 8 },
>>> +    [IPMI_CMD_GET_SEL_INFO] = { get_sel_info },
>>> +    [IPMI_CMD_RESERVE_SEL] = { reserve_sel },
>>> +    [IPMI_CMD_GET_SEL_ENTRY] = { get_sel_entry, 8 },
>>> +    [IPMI_CMD_ADD_SEL_ENTRY] = { add_sel_entry, 18 },
>>> +    [IPMI_CMD_CLEAR_SEL] = { clear_sel, 8 },
>>> +    [IPMI_CMD_GET_SEL_TIME] = { get_sel_time, 6 },
>>> +    [IPMI_CMD_SET_SEL_TIME] = { set_sel_time },
>>>   };
>>>
>>>   static const IPMINetfn storage_netfn = {
>>
>

Corey Minyard March 9, 2016, 4:07 a.m. UTC | #4

On 03/09/2016 12:06 AM, Cédric Le Goater wrote:
> On 03/07/2016 11:40 AM, Cédric Le Goater wrote:
>> On 03/05/2016 12:41 PM, Corey Minyard wrote:
>>> On 03/02/2016 04:14 AM, Cédric Le Goater wrote:
>>>> Most IPMI command handlers in the BMC simulator start with a call to
>>>> the macro IPMI_CHECK_CMD_LEN() which verifies that a minimal number of
>>>> arguments expected by the command are indeed available. To achieve
>>>> this task, the macro implicitly uses local variables which is
>>>> misleading in the code.
>>>>
>>>> This patch adds a 'cmd_len_min' attribute to the struct IPMICmdHandler
>>>> defining the minimal number of arguments expected by the command and
>>>> moves this check in the global command handler ipmi_sim_handle_command().
>>> This is much better.  One style comment inline...
>>>
>>> Acked-by: Corey Minyard <cminyard@mvista.com>
>>>
>>>> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
>>>> ---
>>>>    hw/ipmi/ipmi_bmc_sim.c | 137 ++++++++++++++++++++++---------------------------
>>>>    1 file changed, 62 insertions(+), 75 deletions(-)
>>>>
>>>> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
>>>> index 51d234aa1bf2..30b9fb48ea2d 100644
>>>> --- a/hw/ipmi/ipmi_bmc_sim.c
>>>> +++ b/hw/ipmi/ipmi_bmc_sim.c
>>>> @@ -155,10 +155,15 @@ typedef struct IPMISensor {
>>>>    typedef struct IPMIBmcSim IPMIBmcSim;
>>>>
>>>>    #define MAX_NETFNS 64
>>>> -typedef void (*IPMICmdHandler)(IPMIBmcSim *s,
>>>> -                               uint8_t *cmd, unsigned int cmd_len,
>>>> -                               uint8_t *rsp, unsigned int *rsp_len,
>>>> -                               unsigned int max_rsp_len);
>>>> +
>>>> +typedef struct IPMICmdHandler {
>>>> +    void (*cmd_handler)(IPMIBmcSim *s,
>>>> +                        uint8_t *cmd, unsigned int cmd_len,
>>>> +                        uint8_t *rsp, unsigned int *rsp_len,
>>>> +                        unsigned int max_rsp_len);
>>>> +    unsigned int cmd_len_min;
>>>> +} IPMICmdHandler;
>>>> +
>>>>    typedef struct IPMINetfn {
>>>>        unsigned int cmd_nums;
>>>>        const IPMICmdHandler *cmd_handlers;
>>>> @@ -269,13 +274,6 @@ struct IPMIBmcSim {
>>>>            rsp[(*rsp_len)++] = (b);                           \
>>>>        } while (0)
>>>>
>>>> -/* Verify that the received command is a certain length. */
>>>> -#define IPMI_CHECK_CMD_LEN(l) \
>>>> -    if (cmd_len < l) {                                     \
>>>> -        rsp[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID;      \
>>>> -        return; \
>>>> -    }
>>>> -
>>>>    /* Check that the reservation in the command is valid. */
>>>>    #define IPMI_CHECK_RESERVATION(off, r) \
>>>>        do {                                                   \
>>>> @@ -623,14 +621,19 @@ static void ipmi_sim_handle_command(IPMIBmc *b,
>>>>
>>>>        /* Odd netfns are not valid, make sure the command is registered */
>>>>        if ((netfn & 1) || !ibs->netfns[netfn / 2] ||
>>>> -                        (cmd[1] >= ibs->netfns[netfn / 2]->cmd_nums) ||
>>>> -                        (!ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]])) {
>>>> +        (cmd[1] >= ibs->netfns[netfn / 2]->cmd_nums) ||
>>>> +        (!ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]].cmd_handler)) {
>>>>            rsp[2] = IPMI_CC_INVALID_CMD;
>>>>            goto out;
>>> I'm not sure of the qemu style here, but the above change makes the code hard to read.
>> Cleary. I was lazy. I will try to clarify what this code does with a helper
>> or some intermediate variables.
> How's that  ?
>
> @@ -591,6 +589,7 @@ static void ipmi_sim_handle_command(IPMI
>       unsigned int rsp_len_holder = 0;
>       unsigned int *rsp_len = &rsp_len_holder;
>       unsigned int max_rsp_len = sizeof(rsp);
> +    const IPMICmdHandler *hdl;
>   
>       /* Set up the response, set the low bit of NETFN. */
>       /* Note that max_rsp_len must be at least 3 */
> @@ -623,14 +622,23 @@ static void ipmi_sim_handle_command(IPMI
>   
>       /* Odd netfns are not valid, make sure the command is registered */
>       if ((netfn & 1) || !ibs->netfns[netfn / 2] ||
> -                        (cmd[1] >= ibs->netfns[netfn / 2]->cmd_nums) ||
> -                        (!ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]])) {
> +        (cmd[1] >= ibs->netfns[netfn / 2]->cmd_nums)) {
>           rsp[2] = IPMI_CC_INVALID_CMD;
>           goto out;
>       }
>   

This cleans things up a bit.  The curse of a 4 character indent is that
the line that follows a multi-line if statement is at the same indent as
the lines in the if statement, so it's hard to tell where the if statement
ends and where the code in the if statement begins.  This is not bad,
but if you have if statements like the gcc compiler (at least how the
gcc compiler used to be, haven't looked at it recently) it makes your
head hurt.

The Linux indent rule for this is that you indent the second and later
lines of an if statement way over, or you break it up.

I looked through a few of the major qemu files, and I couldn't find
a multi-line if statement, so it's hard to say.  That is perhaps telling :).

-corey

> -    ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]](ibs, cmd, cmd_len, rsp, rsp_len,
> -                                                max_rsp_len);
> +    hdl = &ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]];
> +    if (!hdl->cmd_handler) {
> +        rsp[2] = IPMI_CC_INVALID_CMD;
> +        goto out;
> +    }
> +
> +    if (cmd_len < hdl->cmd_len_min) {
> +        rsp[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID;
> +        goto out;
> +    }
> +
> +    hdl->cmd_handler(ibs, cmd, cmd_len, rsp, rsp_len, max_rsp_len);
>   
>    out:
>       k->handle_rsp(s, msg_id, rsp, *rsp_len);
>
>
>
> The function still fits in one emacs page, so I think we are fine.
>
> Just waiting a bit for Marcel to chime in before sending a v3.
>
> Thanks,
>   
> C.
>
>   
>>> The qemu style really seems to be to not have big if statements like
>>> this, so maybe this crasy check should be moved to a separate function?
>>>
>>>>        }
>>>>
>>>> -    ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]](ibs, cmd, cmd_len, rsp, rsp_len,
>>>> -                                                max_rsp_len);
>>>> +    if (cmd_len < ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]].cmd_len_min) {
>>>> +        rsp[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID;
>>>> +        goto out;
>>>> +    }
>>>> +
>>>> +    ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]].cmd_handler(ibs, cmd, cmd_len,
>>>> +                                                rsp, rsp_len, max_rsp_len);
>>>>
>>>>     out:
>>>>        k->handle_rsp(s, msg_id, rsp, *rsp_len);
>>>> @@ -737,7 +740,6 @@ static void chassis_control(IPMIBmcSim *ibs,
>>>>        IPMIInterface *s = ibs->parent.intf;
>>>>        IPMIInterfaceClass *k = IPMI_INTERFACE_GET_CLASS(s);
>>>>
>>>> -    IPMI_CHECK_CMD_LEN(3);
>>>>        switch (cmd[2] & 0xf) {
>>>>        case 0: /* power down */
>>>>            rsp[2] = k->do_hw_op(s, IPMI_POWEROFF_CHASSIS, 0);
>>>> @@ -838,7 +840,6 @@ static void set_acpi_power_state(IPMIBmcSim *ibs,
>>>>                              uint8_t *rsp, unsigned int *rsp_len,
>>>>                              unsigned int max_rsp_len)
>>>>    {
>>>> -    IPMI_CHECK_CMD_LEN(4);
>>>>        ibs->acpi_power_state[0] = cmd[2];
>>>>        ibs->acpi_power_state[1] = cmd[3];
>>>>    }
>>>> @@ -869,7 +870,6 @@ static void set_bmc_global_enables(IPMIBmcSim *ibs,
>>>>                                       uint8_t *rsp, unsigned int *rsp_len,
>>>>                                       unsigned int max_rsp_len)
>>>>    {
>>>> -    IPMI_CHECK_CMD_LEN(3);
>>>>        set_global_enables(ibs, cmd[2]);
>>>>    }
>>>>
>>>> @@ -889,7 +889,6 @@ static void clr_msg_flags(IPMIBmcSim *ibs,
>>>>        IPMIInterface *s = ibs->parent.intf;
>>>>        IPMIInterfaceClass *k = IPMI_INTERFACE_GET_CLASS(s);
>>>>
>>>> -    IPMI_CHECK_CMD_LEN(3);
>>>>        ibs->msg_flags &= ~cmd[2];
>>>>        k->set_atn(s, attn_set(ibs), attn_irq_enabled(ibs));
>>>>    }
>>>> @@ -976,15 +975,17 @@ static void send_msg(IPMIBmcSim *ibs,
>>>>        uint8_t *buf;
>>>>        uint8_t netfn, rqLun, rsLun, rqSeq;
>>>>
>>>> -    IPMI_CHECK_CMD_LEN(3);
>>>> -
>>>>        if (cmd[2] != 0) {
>>>>            /* We only handle channel 0 with no options */
>>>>            rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
>>>>            return;
>>>>        }
>>>>
>>>> -    IPMI_CHECK_CMD_LEN(10);
>>>> +    if (cmd_len < 10) {
>>>> +        rsp[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID;
>>>> +        return;
>>>> +    }
>>>> +
>>>>        if (cmd[3] != 0x40) {
>>>>            /* We only emulate a MC at address 0x40. */
>>>>            rsp[2] = 0x83; /* NAK on write */
>>>> @@ -1092,7 +1093,6 @@ static void set_watchdog_timer(IPMIBmcSim *ibs,
>>>>        IPMIInterfaceClass *k = IPMI_INTERFACE_GET_CLASS(s);
>>>>        unsigned int val;
>>>>
>>>> -    IPMI_CHECK_CMD_LEN(8);
>>>>        val = cmd[2] & 0x7; /* Validate use */
>>>>        if (val == 0 || val > 5) {
>>>>            rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
>>>> @@ -1217,7 +1217,6 @@ static void get_sdr(IPMIBmcSim *ibs,
>>>>        uint16_t nextrec;
>>>>        struct ipmi_sdr_header *sdrh;
>>>>
>>>> -    IPMI_CHECK_CMD_LEN(8);
>>>>        if (cmd[6]) {
>>>>            IPMI_CHECK_RESERVATION(2, ibs->sdr.reservation);
>>>>        }
>>>> @@ -1271,7 +1270,6 @@ static void clear_sdr_rep(IPMIBmcSim *ibs,
>>>>                              uint8_t *rsp, unsigned int *rsp_len,
>>>>                              unsigned int max_rsp_len)
>>>>    {
>>>> -    IPMI_CHECK_CMD_LEN(8);
>>>>        IPMI_CHECK_RESERVATION(2, ibs->sdr.reservation);
>>>>        if (cmd[4] != 'C' || cmd[5] != 'L' || cmd[6] != 'R') {
>>>>            rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
>>>> @@ -1330,7 +1328,6 @@ static void get_sel_entry(IPMIBmcSim *ibs,
>>>>    {
>>>>        unsigned int val;
>>>>
>>>> -    IPMI_CHECK_CMD_LEN(8);
>>>>        if (cmd[6]) {
>>>>            IPMI_CHECK_RESERVATION(2, ibs->sel.reservation);
>>>>        }
>>>> @@ -1375,7 +1372,6 @@ static void add_sel_entry(IPMIBmcSim *ibs,
>>>>                              uint8_t *rsp, unsigned int *rsp_len,
>>>>                              unsigned int max_rsp_len)
>>>>    {
>>>> -    IPMI_CHECK_CMD_LEN(18);
>>>>        if (sel_add_event(ibs, cmd + 2)) {
>>>>            rsp[2] = IPMI_CC_OUT_OF_SPACE;
>>>>            return;
>>>> @@ -1390,7 +1386,6 @@ static void clear_sel(IPMIBmcSim *ibs,
>>>>                          uint8_t *rsp, unsigned int *rsp_len,
>>>>                          unsigned int max_rsp_len)
>>>>    {
>>>> -    IPMI_CHECK_CMD_LEN(8);
>>>>        IPMI_CHECK_RESERVATION(2, ibs->sel.reservation);
>>>>        if (cmd[4] != 'C' || cmd[5] != 'L' || cmd[6] != 'R') {
>>>>            rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
>>>> @@ -1434,7 +1429,6 @@ static void set_sel_time(IPMIBmcSim *ibs,
>>>>        uint32_t val;
>>>>        struct ipmi_time now;
>>>>
>>>> -    IPMI_CHECK_CMD_LEN(6);
>>>>        val = cmd[2] | (cmd[3] << 8) | (cmd[4] << 16) | (cmd[5] << 24);
>>>>        ipmi_gettime(&now);
>>>>        ibs->sel.time_offset = now.tv_sec - ((long) val);
>>>> @@ -1447,7 +1441,6 @@ static void set_sensor_evt_enable(IPMIBmcSim *ibs,
>>>>    {
>>>>        IPMISensor *sens;
>>>>
>>>> -    IPMI_CHECK_CMD_LEN(4);
>>>>        if ((cmd[2] >= MAX_SENSORS) ||
>>>>                !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
>>>>            rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
>>>> @@ -1499,7 +1492,6 @@ static void get_sensor_evt_enable(IPMIBmcSim *ibs,
>>>>    {
>>>>        IPMISensor *sens;
>>>>
>>>> -    IPMI_CHECK_CMD_LEN(3);
>>>>        if ((cmd[2] >= MAX_SENSORS) ||
>>>>            !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
>>>>            rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
>>>> @@ -1520,7 +1512,6 @@ static void rearm_sensor_evts(IPMIBmcSim *ibs,
>>>>    {
>>>>        IPMISensor *sens;
>>>>
>>>> -    IPMI_CHECK_CMD_LEN(4);
>>>>        if ((cmd[2] >= MAX_SENSORS) ||
>>>>            !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
>>>>            rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
>>>> @@ -1542,7 +1533,6 @@ static void get_sensor_evt_status(IPMIBmcSim *ibs,
>>>>    {
>>>>        IPMISensor *sens;
>>>>
>>>> -    IPMI_CHECK_CMD_LEN(3);
>>>>        if ((cmd[2] >= MAX_SENSORS) ||
>>>>            !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
>>>>            rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
>>>> @@ -1564,7 +1554,6 @@ static void get_sensor_reading(IPMIBmcSim *ibs,
>>>>    {
>>>>        IPMISensor *sens;
>>>>
>>>> -    IPMI_CHECK_CMD_LEN(3);
>>>>        if ((cmd[2] >= MAX_SENSORS) ||
>>>>                !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
>>>>            rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
>>>> @@ -1587,7 +1576,6 @@ static void set_sensor_type(IPMIBmcSim *ibs,
>>>>        IPMISensor *sens;
>>>>
>>>>
>>>> -    IPMI_CHECK_CMD_LEN(5);
>>>>        if ((cmd[2] >= MAX_SENSORS) ||
>>>>                !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
>>>>            rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
>>>> @@ -1606,7 +1594,6 @@ static void get_sensor_type(IPMIBmcSim *ibs,
>>>>        IPMISensor *sens;
>>>>
>>>>
>>>> -    IPMI_CHECK_CMD_LEN(3);
>>>>        if ((cmd[2] >= MAX_SENSORS) ||
>>>>                !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
>>>>            rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
>>>> @@ -1619,10 +1606,10 @@ static void get_sensor_type(IPMIBmcSim *ibs,
>>>>
>>>>
>>>>    static const IPMICmdHandler chassis_cmds[] = {
>>>> -    [IPMI_CMD_GET_CHASSIS_CAPABILITIES] = chassis_capabilities,
>>>> -    [IPMI_CMD_GET_CHASSIS_STATUS] = chassis_status,
>>>> -    [IPMI_CMD_CHASSIS_CONTROL] = chassis_control,
>>>> -    [IPMI_CMD_GET_SYS_RESTART_CAUSE] = chassis_get_sys_restart_cause
>>>> +    [IPMI_CMD_GET_CHASSIS_CAPABILITIES] = { chassis_capabilities },
>>>> +    [IPMI_CMD_GET_CHASSIS_STATUS] = { chassis_status },
>>>> +    [IPMI_CMD_CHASSIS_CONTROL] = { chassis_control, 3 },
>>>> +    [IPMI_CMD_GET_SYS_RESTART_CAUSE] = { chassis_get_sys_restart_cause }
>>>>    };
>>>>    static const IPMINetfn chassis_netfn = {
>>>>        .cmd_nums = ARRAY_SIZE(chassis_cmds),
>>>> @@ -1630,13 +1617,13 @@ static const IPMINetfn chassis_netfn = {
>>>>    };
>>>>
>>>>    static const IPMICmdHandler sensor_event_cmds[] = {
>>>> -    [IPMI_CMD_SET_SENSOR_EVT_ENABLE] = set_sensor_evt_enable,
>>>> -    [IPMI_CMD_GET_SENSOR_EVT_ENABLE] = get_sensor_evt_enable,
>>>> -    [IPMI_CMD_REARM_SENSOR_EVTS] = rearm_sensor_evts,
>>>> -    [IPMI_CMD_GET_SENSOR_EVT_STATUS] = get_sensor_evt_status,
>>>> -    [IPMI_CMD_GET_SENSOR_READING] = get_sensor_reading,
>>>> -    [IPMI_CMD_SET_SENSOR_TYPE] = set_sensor_type,
>>>> -    [IPMI_CMD_GET_SENSOR_TYPE] = get_sensor_type,
>>>> +    [IPMI_CMD_SET_SENSOR_EVT_ENABLE] = { set_sensor_evt_enable, 4 },
>>>> +    [IPMI_CMD_GET_SENSOR_EVT_ENABLE] = { get_sensor_evt_enable, 3 },
>>>> +    [IPMI_CMD_REARM_SENSOR_EVTS] = { rearm_sensor_evts, 4 },
>>>> +    [IPMI_CMD_GET_SENSOR_EVT_STATUS] = { get_sensor_evt_status, 3 },
>>>> +    [IPMI_CMD_GET_SENSOR_READING] = { get_sensor_reading, 3 },
>>>> +    [IPMI_CMD_SET_SENSOR_TYPE] = { set_sensor_type, 5 },
>>>> +    [IPMI_CMD_GET_SENSOR_TYPE] = { get_sensor_type, 3 },
>>>>    };
>>>>    static const IPMINetfn sensor_event_netfn = {
>>>>        .cmd_nums = ARRAY_SIZE(sensor_event_cmds),
>>>> @@ -1644,22 +1631,22 @@ static const IPMINetfn sensor_event_netfn = {
>>>>    };
>>>>
>>>>    static const IPMICmdHandler app_cmds[] = {
>>>> -    [IPMI_CMD_GET_DEVICE_ID] = get_device_id,
>>>> -    [IPMI_CMD_COLD_RESET] = cold_reset,
>>>> -    [IPMI_CMD_WARM_RESET] = warm_reset,
>>>> -    [IPMI_CMD_SET_ACPI_POWER_STATE] = set_acpi_power_state,
>>>> -    [IPMI_CMD_GET_ACPI_POWER_STATE] = get_acpi_power_state,
>>>> -    [IPMI_CMD_GET_DEVICE_GUID] = get_device_guid,
>>>> -    [IPMI_CMD_SET_BMC_GLOBAL_ENABLES] = set_bmc_global_enables,
>>>> -    [IPMI_CMD_GET_BMC_GLOBAL_ENABLES] = get_bmc_global_enables,
>>>> -    [IPMI_CMD_CLR_MSG_FLAGS] = clr_msg_flags,
>>>> -    [IPMI_CMD_GET_MSG_FLAGS] = get_msg_flags,
>>>> -    [IPMI_CMD_GET_MSG] = get_msg,
>>>> -    [IPMI_CMD_SEND_MSG] = send_msg,
>>>> -    [IPMI_CMD_READ_EVT_MSG_BUF] = read_evt_msg_buf,
>>>> -    [IPMI_CMD_RESET_WATCHDOG_TIMER] = reset_watchdog_timer,
>>>> -    [IPMI_CMD_SET_WATCHDOG_TIMER] = set_watchdog_timer,
>>>> -    [IPMI_CMD_GET_WATCHDOG_TIMER] = get_watchdog_timer,
>>>> +    [IPMI_CMD_GET_DEVICE_ID] = { get_device_id },
>>>> +    [IPMI_CMD_COLD_RESET] = { cold_reset },
>>>> +    [IPMI_CMD_WARM_RESET] = { warm_reset },
>>>> +    [IPMI_CMD_SET_ACPI_POWER_STATE] = { set_acpi_power_state, 4 },
>>>> +    [IPMI_CMD_GET_ACPI_POWER_STATE] = { get_acpi_power_state },
>>>> +    [IPMI_CMD_GET_DEVICE_GUID] = { get_device_guid },
>>>> +    [IPMI_CMD_SET_BMC_GLOBAL_ENABLES] = { set_bmc_global_enables, 3 },
>>>> +    [IPMI_CMD_GET_BMC_GLOBAL_ENABLES] = { get_bmc_global_enables },
>>>> +    [IPMI_CMD_CLR_MSG_FLAGS] = { clr_msg_flags, 3 },
>>>> +    [IPMI_CMD_GET_MSG_FLAGS] = { get_msg_flags },
>>>> +    [IPMI_CMD_GET_MSG] = { get_msg },
>>>> +    [IPMI_CMD_SEND_MSG] = { send_msg, 3 },
>>>> +    [IPMI_CMD_READ_EVT_MSG_BUF] = { read_evt_msg_buf },
>>>> +    [IPMI_CMD_RESET_WATCHDOG_TIMER] = { reset_watchdog_timer },
>>>> +    [IPMI_CMD_SET_WATCHDOG_TIMER] = { set_watchdog_timer, 8 },
>>>> +    [IPMI_CMD_GET_WATCHDOG_TIMER] = { get_watchdog_timer },
>>>>    };
>>>>    static const IPMINetfn app_netfn = {
>>>>        .cmd_nums = ARRAY_SIZE(app_cmds),
>>>> @@ -1667,18 +1654,18 @@ static const IPMINetfn app_netfn = {
>>>>    };
>>>>
>>>>    static const IPMICmdHandler storage_cmds[] = {
>>>> -    [IPMI_CMD_GET_SDR_REP_INFO] = get_sdr_rep_info,
>>>> -    [IPMI_CMD_RESERVE_SDR_REP] = reserve_sdr_rep,
>>>> -    [IPMI_CMD_GET_SDR] = get_sdr,
>>>> -    [IPMI_CMD_ADD_SDR] = add_sdr,
>>>> -    [IPMI_CMD_CLEAR_SDR_REP] = clear_sdr_rep,
>>>> -    [IPMI_CMD_GET_SEL_INFO] = get_sel_info,
>>>> -    [IPMI_CMD_RESERVE_SEL] = reserve_sel,
>>>> -    [IPMI_CMD_GET_SEL_ENTRY] = get_sel_entry,
>>>> -    [IPMI_CMD_ADD_SEL_ENTRY] = add_sel_entry,
>>>> -    [IPMI_CMD_CLEAR_SEL] = clear_sel,
>>>> -    [IPMI_CMD_GET_SEL_TIME] = get_sel_time,
>>>> -    [IPMI_CMD_SET_SEL_TIME] = set_sel_time,
>>>> +    [IPMI_CMD_GET_SDR_REP_INFO] = { get_sdr_rep_info },
>>>> +    [IPMI_CMD_RESERVE_SDR_REP] = { reserve_sdr_rep },
>>>> +    [IPMI_CMD_GET_SDR] = { get_sdr, 8 },
>>>> +    [IPMI_CMD_ADD_SDR] = { add_sdr },
>>>> +    [IPMI_CMD_CLEAR_SDR_REP] = { clear_sdr_rep, 8 },
>>>> +    [IPMI_CMD_GET_SEL_INFO] = { get_sel_info },
>>>> +    [IPMI_CMD_RESERVE_SEL] = { reserve_sel },
>>>> +    [IPMI_CMD_GET_SEL_ENTRY] = { get_sel_entry, 8 },
>>>> +    [IPMI_CMD_ADD_SEL_ENTRY] = { add_sel_entry, 18 },
>>>> +    [IPMI_CMD_CLEAR_SEL] = { clear_sel, 8 },
>>>> +    [IPMI_CMD_GET_SEL_TIME] = { get_sel_time, 6 },
>>>> +    [IPMI_CMD_SET_SEL_TIME] = { set_sel_time },
>>>>    };
>>>>
>>>>    static const IPMINetfn storage_netfn = {

Cédric Le Goater March 9, 2016, 8:52 a.m. UTC | #5

On 03/09/2016 05:07 AM, Corey Minyard wrote:
> On 03/09/2016 12:06 AM, Cédric Le Goater wrote:
>> On 03/07/2016 11:40 AM, Cédric Le Goater wrote:
>>> On 03/05/2016 12:41 PM, Corey Minyard wrote:
>>>> On 03/02/2016 04:14 AM, Cédric Le Goater wrote:
>>>>> Most IPMI command handlers in the BMC simulator start with a call to
>>>>> the macro IPMI_CHECK_CMD_LEN() which verifies that a minimal number of
>>>>> arguments expected by the command are indeed available. To achieve
>>>>> this task, the macro implicitly uses local variables which is
>>>>> misleading in the code.
>>>>>
>>>>> This patch adds a 'cmd_len_min' attribute to the struct IPMICmdHandler
>>>>> defining the minimal number of arguments expected by the command and
>>>>> moves this check in the global command handler ipmi_sim_handle_command().
>>>> This is much better.  One style comment inline...
>>>>
>>>> Acked-by: Corey Minyard <cminyard@mvista.com>
>>>>
>>>>> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
>>>>> ---
>>>>>    hw/ipmi/ipmi_bmc_sim.c | 137 ++++++++++++++++++++++---------------------------
>>>>>    1 file changed, 62 insertions(+), 75 deletions(-)
>>>>>
>>>>> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
>>>>> index 51d234aa1bf2..30b9fb48ea2d 100644
>>>>> --- a/hw/ipmi/ipmi_bmc_sim.c
>>>>> +++ b/hw/ipmi/ipmi_bmc_sim.c
>>>>> @@ -155,10 +155,15 @@ typedef struct IPMISensor {
>>>>>    typedef struct IPMIBmcSim IPMIBmcSim;
>>>>>
>>>>>    #define MAX_NETFNS 64
>>>>> -typedef void (*IPMICmdHandler)(IPMIBmcSim *s,
>>>>> -                               uint8_t *cmd, unsigned int cmd_len,
>>>>> -                               uint8_t *rsp, unsigned int *rsp_len,
>>>>> -                               unsigned int max_rsp_len);
>>>>> +
>>>>> +typedef struct IPMICmdHandler {
>>>>> +    void (*cmd_handler)(IPMIBmcSim *s,
>>>>> +                        uint8_t *cmd, unsigned int cmd_len,
>>>>> +                        uint8_t *rsp, unsigned int *rsp_len,
>>>>> +                        unsigned int max_rsp_len);
>>>>> +    unsigned int cmd_len_min;
>>>>> +} IPMICmdHandler;
>>>>> +
>>>>>    typedef struct IPMINetfn {
>>>>>        unsigned int cmd_nums;
>>>>>        const IPMICmdHandler *cmd_handlers;
>>>>> @@ -269,13 +274,6 @@ struct IPMIBmcSim {
>>>>>            rsp[(*rsp_len)++] = (b);                           \
>>>>>        } while (0)
>>>>>
>>>>> -/* Verify that the received command is a certain length. */
>>>>> -#define IPMI_CHECK_CMD_LEN(l) \
>>>>> -    if (cmd_len < l) {                                     \
>>>>> -        rsp[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID;      \
>>>>> -        return; \
>>>>> -    }
>>>>> -
>>>>>    /* Check that the reservation in the command is valid. */
>>>>>    #define IPMI_CHECK_RESERVATION(off, r) \
>>>>>        do {                                                   \
>>>>> @@ -623,14 +621,19 @@ static void ipmi_sim_handle_command(IPMIBmc *b,
>>>>>
>>>>>        /* Odd netfns are not valid, make sure the command is registered */
>>>>>        if ((netfn & 1) || !ibs->netfns[netfn / 2] ||
>>>>> -                        (cmd[1] >= ibs->netfns[netfn / 2]->cmd_nums) ||
>>>>> -                        (!ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]])) {
>>>>> +        (cmd[1] >= ibs->netfns[netfn / 2]->cmd_nums) ||
>>>>> +        (!ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]].cmd_handler)) {
>>>>>            rsp[2] = IPMI_CC_INVALID_CMD;
>>>>>            goto out;
>>>> I'm not sure of the qemu style here, but the above change makes the code hard to read.
>>> Cleary. I was lazy. I will try to clarify what this code does with a helper
>>> or some intermediate variables.
>> How's that  ?
>>
>> @@ -591,6 +589,7 @@ static void ipmi_sim_handle_command(IPMI
>>       unsigned int rsp_len_holder = 0;
>>       unsigned int *rsp_len = &rsp_len_holder;
>>       unsigned int max_rsp_len = sizeof(rsp);
>> +    const IPMICmdHandler *hdl;
>>         /* Set up the response, set the low bit of NETFN. */
>>       /* Note that max_rsp_len must be at least 3 */
>> @@ -623,14 +622,23 @@ static void ipmi_sim_handle_command(IPMI
>>         /* Odd netfns are not valid, make sure the command is registered */
>>       if ((netfn & 1) || !ibs->netfns[netfn / 2] ||
>> -                        (cmd[1] >= ibs->netfns[netfn / 2]->cmd_nums) ||
>> -                        (!ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]])) {
>> +        (cmd[1] >= ibs->netfns[netfn / 2]->cmd_nums)) {
>>           rsp[2] = IPMI_CC_INVALID_CMD;
>>           goto out;
>>       }
>>   
> 
> This cleans things up a bit.  The curse of a 4 character indent is that
> the line that follows a multi-line if statement is at the same indent as
> the lines in the if statement, so it's hard to tell where the if statement
> ends and where the code in the if statement begins.  This is not bad,
> but if you have if statements like the gcc compiler (at least how the
> gcc compiler used to be, haven't looked at it recently) it makes your
> head hurt.
> 
> The Linux indent rule for this is that you indent the second and later
> lines of an if statement way over, or you break it up.

yeah and in that case, an extra routine would be better. I'll do what you 
first proposed. A check on netfn is also missing.

C.   
 
> I looked through a few of the major qemu files, and I couldn't find
> a multi-line if statement, so it's hard to say.  That is perhaps telling :).
>
> -corey
> 
>> -    ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]](ibs, cmd, cmd_len, rsp, rsp_len,
>> -                                                max_rsp_len);
>> +    hdl = &ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]];
>> +    if (!hdl->cmd_handler) {
>> +        rsp[2] = IPMI_CC_INVALID_CMD;
>> +        goto out;
>> +    }
>> +
>> +    if (cmd_len < hdl->cmd_len_min) {
>> +        rsp[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID;
>> +        goto out;
>> +    }
>> +
>> +    hdl->cmd_handler(ibs, cmd, cmd_len, rsp, rsp_len, max_rsp_len);
>>      out:
>>       k->handle_rsp(s, msg_id, rsp, *rsp_len);
>>
>>
>>
>> The function still fits in one emacs page, so I think we are fine.
>>
>> Just waiting a bit for Marcel to chime in before sending a v3.
>>
>> Thanks,
>>   C.
>>
>>  
>>>> The qemu style really seems to be to not have big if statements like
>>>> this, so maybe this crasy check should be moved to a separate function?
>>>>
>>>>>        }
>>>>>
>>>>> -    ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]](ibs, cmd, cmd_len, rsp, rsp_len,
>>>>> -                                                max_rsp_len);
>>>>> +    if (cmd_len < ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]].cmd_len_min) {
>>>>> +        rsp[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID;
>>>>> +        goto out;
>>>>> +    }
>>>>> +
>>>>> +    ibs->netfns[netfn / 2]->cmd_handlers[cmd[1]].cmd_handler(ibs, cmd, cmd_len,
>>>>> +                                                rsp, rsp_len, max_rsp_len);
>>>>>
>>>>>     out:
>>>>>        k->handle_rsp(s, msg_id, rsp, *rsp_len);
>>>>> @@ -737,7 +740,6 @@ static void chassis_control(IPMIBmcSim *ibs,
>>>>>        IPMIInterface *s = ibs->parent.intf;
>>>>>        IPMIInterfaceClass *k = IPMI_INTERFACE_GET_CLASS(s);
>>>>>
>>>>> -    IPMI_CHECK_CMD_LEN(3);
>>>>>        switch (cmd[2] & 0xf) {
>>>>>        case 0: /* power down */
>>>>>            rsp[2] = k->do_hw_op(s, IPMI_POWEROFF_CHASSIS, 0);
>>>>> @@ -838,7 +840,6 @@ static void set_acpi_power_state(IPMIBmcSim *ibs,
>>>>>                              uint8_t *rsp, unsigned int *rsp_len,
>>>>>                              unsigned int max_rsp_len)
>>>>>    {
>>>>> -    IPMI_CHECK_CMD_LEN(4);
>>>>>        ibs->acpi_power_state[0] = cmd[2];
>>>>>        ibs->acpi_power_state[1] = cmd[3];
>>>>>    }
>>>>> @@ -869,7 +870,6 @@ static void set_bmc_global_enables(IPMIBmcSim *ibs,
>>>>>                                       uint8_t *rsp, unsigned int *rsp_len,
>>>>>                                       unsigned int max_rsp_len)
>>>>>    {
>>>>> -    IPMI_CHECK_CMD_LEN(3);
>>>>>        set_global_enables(ibs, cmd[2]);
>>>>>    }
>>>>>
>>>>> @@ -889,7 +889,6 @@ static void clr_msg_flags(IPMIBmcSim *ibs,
>>>>>        IPMIInterface *s = ibs->parent.intf;
>>>>>        IPMIInterfaceClass *k = IPMI_INTERFACE_GET_CLASS(s);
>>>>>
>>>>> -    IPMI_CHECK_CMD_LEN(3);
>>>>>        ibs->msg_flags &= ~cmd[2];
>>>>>        k->set_atn(s, attn_set(ibs), attn_irq_enabled(ibs));
>>>>>    }
>>>>> @@ -976,15 +975,17 @@ static void send_msg(IPMIBmcSim *ibs,
>>>>>        uint8_t *buf;
>>>>>        uint8_t netfn, rqLun, rsLun, rqSeq;
>>>>>
>>>>> -    IPMI_CHECK_CMD_LEN(3);
>>>>> -
>>>>>        if (cmd[2] != 0) {
>>>>>            /* We only handle channel 0 with no options */
>>>>>            rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
>>>>>            return;
>>>>>        }
>>>>>
>>>>> -    IPMI_CHECK_CMD_LEN(10);
>>>>> +    if (cmd_len < 10) {
>>>>> +        rsp[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID;
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>>        if (cmd[3] != 0x40) {
>>>>>            /* We only emulate a MC at address 0x40. */
>>>>>            rsp[2] = 0x83; /* NAK on write */
>>>>> @@ -1092,7 +1093,6 @@ static void set_watchdog_timer(IPMIBmcSim *ibs,
>>>>>        IPMIInterfaceClass *k = IPMI_INTERFACE_GET_CLASS(s);
>>>>>        unsigned int val;
>>>>>
>>>>> -    IPMI_CHECK_CMD_LEN(8);
>>>>>        val = cmd[2] & 0x7; /* Validate use */
>>>>>        if (val == 0 || val > 5) {
>>>>>            rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
>>>>> @@ -1217,7 +1217,6 @@ static void get_sdr(IPMIBmcSim *ibs,
>>>>>        uint16_t nextrec;
>>>>>        struct ipmi_sdr_header *sdrh;
>>>>>
>>>>> -    IPMI_CHECK_CMD_LEN(8);
>>>>>        if (cmd[6]) {
>>>>>            IPMI_CHECK_RESERVATION(2, ibs->sdr.reservation);
>>>>>        }
>>>>> @@ -1271,7 +1270,6 @@ static void clear_sdr_rep(IPMIBmcSim *ibs,
>>>>>                              uint8_t *rsp, unsigned int *rsp_len,
>>>>>                              unsigned int max_rsp_len)
>>>>>    {
>>>>> -    IPMI_CHECK_CMD_LEN(8);
>>>>>        IPMI_CHECK_RESERVATION(2, ibs->sdr.reservation);
>>>>>        if (cmd[4] != 'C' || cmd[5] != 'L' || cmd[6] != 'R') {
>>>>>            rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
>>>>> @@ -1330,7 +1328,6 @@ static void get_sel_entry(IPMIBmcSim *ibs,
>>>>>    {
>>>>>        unsigned int val;
>>>>>
>>>>> -    IPMI_CHECK_CMD_LEN(8);
>>>>>        if (cmd[6]) {
>>>>>            IPMI_CHECK_RESERVATION(2, ibs->sel.reservation);
>>>>>        }
>>>>> @@ -1375,7 +1372,6 @@ static void add_sel_entry(IPMIBmcSim *ibs,
>>>>>                              uint8_t *rsp, unsigned int *rsp_len,
>>>>>                              unsigned int max_rsp_len)
>>>>>    {
>>>>> -    IPMI_CHECK_CMD_LEN(18);
>>>>>        if (sel_add_event(ibs, cmd + 2)) {
>>>>>            rsp[2] = IPMI_CC_OUT_OF_SPACE;
>>>>>            return;
>>>>> @@ -1390,7 +1386,6 @@ static void clear_sel(IPMIBmcSim *ibs,
>>>>>                          uint8_t *rsp, unsigned int *rsp_len,
>>>>>                          unsigned int max_rsp_len)
>>>>>    {
>>>>> -    IPMI_CHECK_CMD_LEN(8);
>>>>>        IPMI_CHECK_RESERVATION(2, ibs->sel.reservation);
>>>>>        if (cmd[4] != 'C' || cmd[5] != 'L' || cmd[6] != 'R') {
>>>>>            rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
>>>>> @@ -1434,7 +1429,6 @@ static void set_sel_time(IPMIBmcSim *ibs,
>>>>>        uint32_t val;
>>>>>        struct ipmi_time now;
>>>>>
>>>>> -    IPMI_CHECK_CMD_LEN(6);
>>>>>        val = cmd[2] | (cmd[3] << 8) | (cmd[4] << 16) | (cmd[5] << 24);
>>>>>        ipmi_gettime(&now);
>>>>>        ibs->sel.time_offset = now.tv_sec - ((long) val);
>>>>> @@ -1447,7 +1441,6 @@ static void set_sensor_evt_enable(IPMIBmcSim *ibs,
>>>>>    {
>>>>>        IPMISensor *sens;
>>>>>
>>>>> -    IPMI_CHECK_CMD_LEN(4);
>>>>>        if ((cmd[2] >= MAX_SENSORS) ||
>>>>>                !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
>>>>>            rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
>>>>> @@ -1499,7 +1492,6 @@ static void get_sensor_evt_enable(IPMIBmcSim *ibs,
>>>>>    {
>>>>>        IPMISensor *sens;
>>>>>
>>>>> -    IPMI_CHECK_CMD_LEN(3);
>>>>>        if ((cmd[2] >= MAX_SENSORS) ||
>>>>>            !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
>>>>>            rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
>>>>> @@ -1520,7 +1512,6 @@ static void rearm_sensor_evts(IPMIBmcSim *ibs,
>>>>>    {
>>>>>        IPMISensor *sens;
>>>>>
>>>>> -    IPMI_CHECK_CMD_LEN(4);
>>>>>        if ((cmd[2] >= MAX_SENSORS) ||
>>>>>            !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
>>>>>            rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
>>>>> @@ -1542,7 +1533,6 @@ static void get_sensor_evt_status(IPMIBmcSim *ibs,
>>>>>    {
>>>>>        IPMISensor *sens;
>>>>>
>>>>> -    IPMI_CHECK_CMD_LEN(3);
>>>>>        if ((cmd[2] >= MAX_SENSORS) ||
>>>>>            !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
>>>>>            rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
>>>>> @@ -1564,7 +1554,6 @@ static void get_sensor_reading(IPMIBmcSim *ibs,
>>>>>    {
>>>>>        IPMISensor *sens;
>>>>>
>>>>> -    IPMI_CHECK_CMD_LEN(3);
>>>>>        if ((cmd[2] >= MAX_SENSORS) ||
>>>>>                !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
>>>>>            rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
>>>>> @@ -1587,7 +1576,6 @@ static void set_sensor_type(IPMIBmcSim *ibs,
>>>>>        IPMISensor *sens;
>>>>>
>>>>>
>>>>> -    IPMI_CHECK_CMD_LEN(5);
>>>>>        if ((cmd[2] >= MAX_SENSORS) ||
>>>>>                !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
>>>>>            rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
>>>>> @@ -1606,7 +1594,6 @@ static void get_sensor_type(IPMIBmcSim *ibs,
>>>>>        IPMISensor *sens;
>>>>>
>>>>>
>>>>> -    IPMI_CHECK_CMD_LEN(3);
>>>>>        if ((cmd[2] >= MAX_SENSORS) ||
>>>>>                !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
>>>>>            rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
>>>>> @@ -1619,10 +1606,10 @@ static void get_sensor_type(IPMIBmcSim *ibs,
>>>>>
>>>>>
>>>>>    static const IPMICmdHandler chassis_cmds[] = {
>>>>> -    [IPMI_CMD_GET_CHASSIS_CAPABILITIES] = chassis_capabilities,
>>>>> -    [IPMI_CMD_GET_CHASSIS_STATUS] = chassis_status,
>>>>> -    [IPMI_CMD_CHASSIS_CONTROL] = chassis_control,
>>>>> -    [IPMI_CMD_GET_SYS_RESTART_CAUSE] = chassis_get_sys_restart_cause
>>>>> +    [IPMI_CMD_GET_CHASSIS_CAPABILITIES] = { chassis_capabilities },
>>>>> +    [IPMI_CMD_GET_CHASSIS_STATUS] = { chassis_status },
>>>>> +    [IPMI_CMD_CHASSIS_CONTROL] = { chassis_control, 3 },
>>>>> +    [IPMI_CMD_GET_SYS_RESTART_CAUSE] = { chassis_get_sys_restart_cause }
>>>>>    };
>>>>>    static const IPMINetfn chassis_netfn = {
>>>>>        .cmd_nums = ARRAY_SIZE(chassis_cmds),
>>>>> @@ -1630,13 +1617,13 @@ static const IPMINetfn chassis_netfn = {
>>>>>    };
>>>>>
>>>>>    static const IPMICmdHandler sensor_event_cmds[] = {
>>>>> -    [IPMI_CMD_SET_SENSOR_EVT_ENABLE] = set_sensor_evt_enable,
>>>>> -    [IPMI_CMD_GET_SENSOR_EVT_ENABLE] = get_sensor_evt_enable,
>>>>> -    [IPMI_CMD_REARM_SENSOR_EVTS] = rearm_sensor_evts,
>>>>> -    [IPMI_CMD_GET_SENSOR_EVT_STATUS] = get_sensor_evt_status,
>>>>> -    [IPMI_CMD_GET_SENSOR_READING] = get_sensor_reading,
>>>>> -    [IPMI_CMD_SET_SENSOR_TYPE] = set_sensor_type,
>>>>> -    [IPMI_CMD_GET_SENSOR_TYPE] = get_sensor_type,
>>>>> +    [IPMI_CMD_SET_SENSOR_EVT_ENABLE] = { set_sensor_evt_enable, 4 },
>>>>> +    [IPMI_CMD_GET_SENSOR_EVT_ENABLE] = { get_sensor_evt_enable, 3 },
>>>>> +    [IPMI_CMD_REARM_SENSOR_EVTS] = { rearm_sensor_evts, 4 },
>>>>> +    [IPMI_CMD_GET_SENSOR_EVT_STATUS] = { get_sensor_evt_status, 3 },
>>>>> +    [IPMI_CMD_GET_SENSOR_READING] = { get_sensor_reading, 3 },
>>>>> +    [IPMI_CMD_SET_SENSOR_TYPE] = { set_sensor_type, 5 },
>>>>> +    [IPMI_CMD_GET_SENSOR_TYPE] = { get_sensor_type, 3 },
>>>>>    };
>>>>>    static const IPMINetfn sensor_event_netfn = {
>>>>>        .cmd_nums = ARRAY_SIZE(sensor_event_cmds),
>>>>> @@ -1644,22 +1631,22 @@ static const IPMINetfn sensor_event_netfn = {
>>>>>    };
>>>>>
>>>>>    static const IPMICmdHandler app_cmds[] = {
>>>>> -    [IPMI_CMD_GET_DEVICE_ID] = get_device_id,
>>>>> -    [IPMI_CMD_COLD_RESET] = cold_reset,
>>>>> -    [IPMI_CMD_WARM_RESET] = warm_reset,
>>>>> -    [IPMI_CMD_SET_ACPI_POWER_STATE] = set_acpi_power_state,
>>>>> -    [IPMI_CMD_GET_ACPI_POWER_STATE] = get_acpi_power_state,
>>>>> -    [IPMI_CMD_GET_DEVICE_GUID] = get_device_guid,
>>>>> -    [IPMI_CMD_SET_BMC_GLOBAL_ENABLES] = set_bmc_global_enables,
>>>>> -    [IPMI_CMD_GET_BMC_GLOBAL_ENABLES] = get_bmc_global_enables,
>>>>> -    [IPMI_CMD_CLR_MSG_FLAGS] = clr_msg_flags,
>>>>> -    [IPMI_CMD_GET_MSG_FLAGS] = get_msg_flags,
>>>>> -    [IPMI_CMD_GET_MSG] = get_msg,
>>>>> -    [IPMI_CMD_SEND_MSG] = send_msg,
>>>>> -    [IPMI_CMD_READ_EVT_MSG_BUF] = read_evt_msg_buf,
>>>>> -    [IPMI_CMD_RESET_WATCHDOG_TIMER] = reset_watchdog_timer,
>>>>> -    [IPMI_CMD_SET_WATCHDOG_TIMER] = set_watchdog_timer,
>>>>> -    [IPMI_CMD_GET_WATCHDOG_TIMER] = get_watchdog_timer,
>>>>> +    [IPMI_CMD_GET_DEVICE_ID] = { get_device_id },
>>>>> +    [IPMI_CMD_COLD_RESET] = { cold_reset },
>>>>> +    [IPMI_CMD_WARM_RESET] = { warm_reset },
>>>>> +    [IPMI_CMD_SET_ACPI_POWER_STATE] = { set_acpi_power_state, 4 },
>>>>> +    [IPMI_CMD_GET_ACPI_POWER_STATE] = { get_acpi_power_state },
>>>>> +    [IPMI_CMD_GET_DEVICE_GUID] = { get_device_guid },
>>>>> +    [IPMI_CMD_SET_BMC_GLOBAL_ENABLES] = { set_bmc_global_enables, 3 },
>>>>> +    [IPMI_CMD_GET_BMC_GLOBAL_ENABLES] = { get_bmc_global_enables },
>>>>> +    [IPMI_CMD_CLR_MSG_FLAGS] = { clr_msg_flags, 3 },
>>>>> +    [IPMI_CMD_GET_MSG_FLAGS] = { get_msg_flags },
>>>>> +    [IPMI_CMD_GET_MSG] = { get_msg },
>>>>> +    [IPMI_CMD_SEND_MSG] = { send_msg, 3 },
>>>>> +    [IPMI_CMD_READ_EVT_MSG_BUF] = { read_evt_msg_buf },
>>>>> +    [IPMI_CMD_RESET_WATCHDOG_TIMER] = { reset_watchdog_timer },
>>>>> +    [IPMI_CMD_SET_WATCHDOG_TIMER] = { set_watchdog_timer, 8 },
>>>>> +    [IPMI_CMD_GET_WATCHDOG_TIMER] = { get_watchdog_timer },
>>>>>    };
>>>>>    static const IPMINetfn app_netfn = {
>>>>>        .cmd_nums = ARRAY_SIZE(app_cmds),
>>>>> @@ -1667,18 +1654,18 @@ static const IPMINetfn app_netfn = {
>>>>>    };
>>>>>
>>>>>    static const IPMICmdHandler storage_cmds[] = {
>>>>> -    [IPMI_CMD_GET_SDR_REP_INFO] = get_sdr_rep_info,
>>>>> -    [IPMI_CMD_RESERVE_SDR_REP] = reserve_sdr_rep,
>>>>> -    [IPMI_CMD_GET_SDR] = get_sdr,
>>>>> -    [IPMI_CMD_ADD_SDR] = add_sdr,
>>>>> -    [IPMI_CMD_CLEAR_SDR_REP] = clear_sdr_rep,
>>>>> -    [IPMI_CMD_GET_SEL_INFO] = get_sel_info,
>>>>> -    [IPMI_CMD_RESERVE_SEL] = reserve_sel,
>>>>> -    [IPMI_CMD_GET_SEL_ENTRY] = get_sel_entry,
>>>>> -    [IPMI_CMD_ADD_SEL_ENTRY] = add_sel_entry,
>>>>> -    [IPMI_CMD_CLEAR_SEL] = clear_sel,
>>>>> -    [IPMI_CMD_GET_SEL_TIME] = get_sel_time,
>>>>> -    [IPMI_CMD_SET_SEL_TIME] = set_sel_time,
>>>>> +    [IPMI_CMD_GET_SDR_REP_INFO] = { get_sdr_rep_info },
>>>>> +    [IPMI_CMD_RESERVE_SDR_REP] = { reserve_sdr_rep },
>>>>> +    [IPMI_CMD_GET_SDR] = { get_sdr, 8 },
>>>>> +    [IPMI_CMD_ADD_SDR] = { add_sdr },
>>>>> +    [IPMI_CMD_CLEAR_SDR_REP] = { clear_sdr_rep, 8 },
>>>>> +    [IPMI_CMD_GET_SEL_INFO] = { get_sel_info },
>>>>> +    [IPMI_CMD_RESERVE_SEL] = { reserve_sel },
>>>>> +    [IPMI_CMD_GET_SEL_ENTRY] = { get_sel_entry, 8 },
>>>>> +    [IPMI_CMD_ADD_SEL_ENTRY] = { add_sel_entry, 18 },
>>>>> +    [IPMI_CMD_CLEAR_SEL] = { clear_sel, 8 },
>>>>> +    [IPMI_CMD_GET_SEL_TIME] = { get_sel_time, 6 },
>>>>> +    [IPMI_CMD_SET_SEL_TIME] = { set_sel_time },
>>>>>    };
>>>>>
>>>>>    static const IPMINetfn storage_netfn = {
>

[v2,01/10] ipmi: remove IPMI_CHECK_CMD_LEN() macro

Commit Message

Comments

Patch