Re: [Qemu-devel] [PATCH 5/8] ipmi: add ACPI power and GUID commands

[Marcel Apfelbaum]

On 01/21/2016 06:41 PM, Cédric Le Goater wrote:
> On 01/21/2016 05:37 PM, Corey Minyard wrote:
> > On 01/17/2016 08:16 AM, Michael S. Tsirkin wrote:
> > > 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 <address@hidden>
> > > > > ---
> > > > >    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.
> >
> > Marcel, do you want me to do this?
>
> I have the patch ready (and more). I will send this one to start with.

Thanks!
Marcel

> C.