From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46533) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bQZLI-0001wJ-6Q for qemu-devel@nongnu.org; Fri, 22 Jul 2016 08:14:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bQZLC-0000lg-RN for qemu-devel@nongnu.org; Fri, 22 Jul 2016 08:14:11 -0400 Received: from mail-oi0-x242.google.com ([2607:f8b0:4003:c06::242]:33459) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bQZLC-0000lR-L9 for qemu-devel@nongnu.org; Fri, 22 Jul 2016 08:14:06 -0400 Received: by mail-oi0-x242.google.com with SMTP id l9so10268945oih.0 for ; Fri, 22 Jul 2016 05:14:06 -0700 (PDT) Sender: Corey Minyard Reply-To: minyard@acm.org References: <1469119749-31181-1-git-send-email-minyard@acm.org> <1469119749-31181-2-git-send-email-minyard@acm.org> <48077833.7262631.1469179348215.JavaMail.zimbra@redhat.com> From: Corey Minyard Message-ID: <57920E0C.3000009@acm.org> Date: Fri, 22 Jul 2016 07:14:04 -0500 MIME-Version: 1.0 In-Reply-To: <48077833.7262631.1469179348215.JavaMail.zimbra@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v2 1/3] ipmi_bmc_sim: Add a proper unrealize function List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?Q?Marc-Andr=c3=a9_Lureau?= Cc: qemu-devel@nongnu.org, Corey Minyard On 07/22/2016 04:22 AM, Marc-André Lureau wrote: > Hi > > ----- Original Message ----- >> From: Corey Minyard >> >> 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 >> Cc: Marc-André Lureau >> --- >> 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 > >> 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 >> >>