On Sun, Jan 17, 2016 at 02:04:32PM +0200, Marcel Apfelbaum wrote: > On 01/05/2016 07:29 PM, Cédric Le Goater wrote: > >Signed-off-by: Cédric Le Goater <c...@fr.ibm.com> > >--- > > hw/ipmi/ipmi_bmc_sim.c | 55 > > ++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 55 insertions(+) > > > >diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c > >index 60586a67104e..c3a06d0ac7e4 100644 > >--- a/hw/ipmi/ipmi_bmc_sim.c > >+++ b/hw/ipmi/ipmi_bmc_sim.c > >@@ -25,6 +25,7 @@ > > #include <stdio.h> > > #include <string.h> > > #include <stdint.h> > >+#include "sysemu/sysemu.h" > > #include "qemu/timer.h" > > #include "hw/ipmi/ipmi.h" > > #include "qemu/error-report.h" > >@@ -54,6 +55,9 @@ > > #define IPMI_CMD_GET_DEVICE_ID 0x01 > > #define IPMI_CMD_COLD_RESET 0x02 > > #define IPMI_CMD_WARM_RESET 0x03 > >+#define IPMI_CMD_SET_POWER_STATE 0x06 > >+#define IPMI_CMD_GET_POWER_STATE 0x07 > >+#define IPMI_CMD_GET_DEVICE_GUID 0x08 > > #define IPMI_CMD_RESET_WATCHDOG_TIMER 0x22 > > #define IPMI_CMD_SET_WATCHDOG_TIMER 0x24 > > #define IPMI_CMD_GET_WATCHDOG_TIMER 0x25 > >@@ -215,6 +219,9 @@ struct IPMIBmcSim { > > > > uint8_t restart_cause; > > > >+ uint8_t power_state[2]; > >+ uint8_t uuid[16]; > >+ > > IPMISel sel; > > IPMISdr sdr; > > IPMIFru fru; > >@@ -842,6 +849,42 @@ static void warm_reset(IPMIBmcSim *ibs, > > k->reset(s, false); > > } > > } > >+static void set_power_state(IPMIBmcSim *ibs, > >+ uint8_t *cmd, unsigned int cmd_len, > >+ uint8_t *rsp, unsigned int *rsp_len, > >+ unsigned int max_rsp_len) > >+{ > >+ IPMI_CHECK_CMD_LEN(4); > >+ ibs->power_state[0] = cmd[2]; > >+ ibs->power_state[1] = cmd[3]; > >+ out: > >+ return; > > > Hi, > > I am sorry for my late comment, but I find a little strange the use of > the "out" label here. > I understand this is because of its usage in IPMI_* macros, but > I looked into every usage(I hope I didn't miss anything) and the code > simply returns. > Also the correlation between those macros is a little odd. > > Thanks, > Marcel
Yes - these macros with goto out are confusing. Please rewrite them to return bool, and put goto out in the caller. > > >+} > >+ > >+static void get_power_state(IPMIBmcSim *ibs, > >+ uint8_t *cmd, unsigned int cmd_len, > >+ uint8_t *rsp, unsigned int *rsp_len, > >+ unsigned int max_rsp_len) > >+{ > >+ IPMI_ADD_RSP_DATA(ibs->power_state[0]); > >+ IPMI_ADD_RSP_DATA(ibs->power_state[1]); > >+ out: > >+ return; > >+} > >+ > >+static void get_device_guid(IPMIBmcSim *ibs, > >+ uint8_t *cmd, unsigned int cmd_len, > >+ uint8_t *rsp, unsigned int *rsp_len, > >+ unsigned int max_rsp_len) > >+{ > >+ unsigned int i; > >+ > >+ for (i = 0; i < 16; i++) { > >+ IPMI_ADD_RSP_DATA(ibs->uuid[i]); > >+ } > >+ out: > >+ return; > >+} > > > > static void set_bmc_global_enables(IPMIBmcSim *ibs, > > uint8_t *cmd, unsigned int cmd_len, > >@@ -1781,6 +1824,9 @@ static const IPMICmdHandler > >app_cmds[IPMI_NETFN_APP_MAXCMD] = { > > [IPMI_CMD_GET_DEVICE_ID] = get_device_id, > > [IPMI_CMD_COLD_RESET] = cold_reset, > > [IPMI_CMD_WARM_RESET] = warm_reset, > >+ [IPMI_CMD_SET_POWER_STATE] = set_power_state, > >+ [IPMI_CMD_GET_POWER_STATE] = get_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, > >@@ -1907,6 +1953,15 @@ static void ipmi_sim_init(Object *obj) > > i += len; > > } > > > >+ ibs->power_state[0] = 0; > >+ ibs->power_state[1] = 0; > >+ > >+ if (qemu_uuid_set) { > >+ memcpy(&ibs->uuid, qemu_uuid, 16); > >+ } else { > >+ memset(&ibs->uuid, 0, 16); > >+ } > >+ > > ipmi_init_sensors_from_sdrs(ibs); > > register_cmds(ibs); > > > >