Re: [Qemu-devel] [PATCH v2 1/3] ipmi_bmc_sim: Add a proper unrealize function

[Corey Minyard]

On 07/22/2016 04:22 AM, Marc-André Lureau wrote:
> Hi
>
> ----- Original Message -----
> > From: Corey Minyard <address@hidden>
> >
> > Add an unrealize function to free the timer allocated in the
> > realize function, unregsiter the vmstate, and free any
> > pending messages.
> I don't know how to test this either, the device seems to be hotpluggable, but 
> doing device_del crashes qemu. Looks like it would be worth fixing that too 
> (even better would be to automate this kind of test for all devices, but that's 
> just some thought)

That's actually a bug.  This device's hot plug should be tied to it's 
interface's hot plug, which is a separate device.  I was trying to 
unplug the interface, which is on an ISA bus, not the BMC.

> > Also, get rid of the unnecessary mutex, it was a vestige
> > of something else that was not done.  That way we don't
> > have to free it.
> You may want to split this in a seperate patch

Yeah, you are right.

Thanks,

-corey

> > Signed-off-by: Corey Minyard <address@hidden>
> > Cc: Marc-André Lureau <address@hidden>
> > ---
> >   hw/ipmi/ipmi_bmc_sim.c | 22 ++++++++++++++++------
> >   1 file changed, 16 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
> > index dc9c14c..fe92b93 100644
> > --- a/hw/ipmi/ipmi_bmc_sim.c
> > +++ b/hw/ipmi/ipmi_bmc_sim.c
> > @@ -217,7 +217,6 @@ struct IPMIBmcSim {
> >       /* Odd netfns are for responses, so we only need the even ones. */
> >       const IPMINetfn *netfns[MAX_NETFNS / 2];
> >   
> > -    QemuMutex lock;
> >       /* We allow one event in the buffer */
> >       uint8_t evtbuf[16];
> >   
> > @@ -940,7 +939,6 @@ static void get_msg(IPMIBmcSim *ibs,
> >   {
> >       IPMIRcvBufEntry *msg;
> >   
> > -    qemu_mutex_lock(&ibs->lock);
> >       if (QTAILQ_EMPTY(&ibs->rcvbufs)) {
> >           rsp_buffer_set_error(rsp, 0x80); /* Queue empty */
> >           goto out;
> > @@ -960,7 +958,6 @@ static void get_msg(IPMIBmcSim *ibs,
> >       }
> >   
> >   out:
> > -    qemu_mutex_unlock(&ibs->lock);
> >       return;
> >   }
> >   
> > @@ -1055,11 +1052,9 @@ static void send_msg(IPMIBmcSim *ibs,
> >    end_msg:
> >       msg->buf[msg->len] = ipmb_checksum(msg->buf, msg->len, 0);
> >       msg->len++;
> > -    qemu_mutex_lock(&ibs->lock);
> >       QTAILQ_INSERT_TAIL(&ibs->rcvbufs, msg, entry);
> >       ibs->msg_flags |= IPMI_BMC_MSG_FLAG_RCV_MSG_QUEUE;
> >       k->set_atn(s, 1, attn_irq_enabled(ibs));
> > -    qemu_mutex_unlock(&ibs->lock);
> >   }
> >   
> >   static void do_watchdog_reset(IPMIBmcSim *ibs)
> > @@ -1753,7 +1748,6 @@ static void ipmi_sim_realize(DeviceState *dev, Error
> > **errp)
> >       unsigned int i;
> >       IPMIBmcSim *ibs = IPMI_BMC_SIMULATOR(b);
> >   
> > -    qemu_mutex_init(&ibs->lock);
> >       QTAILQ_INIT(&ibs->rcvbufs);
> >   
> >       ibs->bmc_global_enables = (1 << IPMI_BMC_EVENT_LOG_BIT);
> > @@ -1786,12 +1780,28 @@ static void ipmi_sim_realize(DeviceState *dev, Error
> > **errp)
> >       vmstate_register(NULL, 0, &vmstate_ipmi_sim, ibs);
> >   }
> >   
> > +static void ipmi_sim_unrealize(DeviceState *dev, Error **errp)
> > +{
> > +    IPMIBmc *b = IPMI_BMC(dev);
> > +    IPMIRcvBufEntry *msg, *tmp;
> > +    IPMIBmcSim *ibs = IPMI_BMC_SIMULATOR(b);
> > +
> > +    vmstate_unregister(NULL, &vmstate_ipmi_sim, ibs);
> > +    timer_del(ibs->timer);
> > +    timer_free(ibs->timer);
> > +    QTAILQ_FOREACH_SAFE(msg, &ibs->rcvbufs, entry, tmp) {
> > +        QTAILQ_REMOVE(&ibs->rcvbufs, msg, entry);
> > +        g_free(msg);
> > +    }
> > +}
> > +
> Otherwise, for completeness, this looks good so
> Reviewed-by: Marc-André Lureau <address@hidden>
>
> >   static void ipmi_sim_class_init(ObjectClass *oc, void *data)
> >   {
> >       DeviceClass *dc = DEVICE_CLASS(oc);
> >       IPMIBmcClass *bk = IPMI_BMC_CLASS(oc);
> >   
> >       dc->realize = ipmi_sim_realize;
> > +    dc->unrealize = ipmi_sim_unrealize;
> >       bk->handle_command = ipmi_sim_handle_command;
> >   }
> >   
> > --
> > 2.7.4