From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:48945)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <tcminyard@gmail.com>) id 1bQFXG-0006qT-JV
	for qemu-devel@nongnu.org; Thu, 21 Jul 2016 11:05:15 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <tcminyard@gmail.com>) id 1bQFXC-00040j-Jb
	for qemu-devel@nongnu.org; Thu, 21 Jul 2016 11:05:14 -0400
Received: from mail-oi0-x242.google.com ([2607:f8b0:4003:c06::242]:34778)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <tcminyard@gmail.com>) id 1bQFXC-00040N-F2
	for qemu-devel@nongnu.org; Thu, 21 Jul 2016 11:05:10 -0400
Received: by mail-oi0-x242.google.com with SMTP id c199so7942069oig.1
	for <qemu-devel@nongnu.org>; Thu, 21 Jul 2016 08:05:10 -0700 (PDT)
Sender: Corey Minyard <tcminyard@gmail.com>
Reply-To: minyard@acm.org
References: <1469112292-30548-1-git-send-email-minyard@acm.org>
	<568261880.7102035.1469112560328.JavaMail.zimbra@redhat.com>
From: Corey Minyard <minyard@acm.org>
Message-ID: <5790E4A3.3000607@acm.org>
Date: Thu, 21 Jul 2016 10:05:07 -0500
MIME-Version: 1.0
In-Reply-To: <568261880.7102035.1469112560328.JavaMail.zimbra@redhat.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Subject: Re: [Qemu-devel] [PATCH 1/3] ipmi_bmc_sim: Free timer
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel/>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: =?UTF-8?Q?Marc-Andr=c3=a9_Lureau?= <mlureau@redhat.com>
Cc: qemu-devel@nongnu.org, Corey Minyard <cminyard@mvista.com>

On 07/21/2016 09:49 AM, Marc-André Lureau wrote:
> Hi
>
> ----- Original Message -----
>> From: Corey Minyard <cminyard@mvista.com>
>>
>> Add an unrealize function to free the timer allocated in the
>> realize function.
>>
>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>> Cc: Marc-André Lureau <mlureau@redhat.com>
>> ---
>>   hw/ipmi/ipmi_bmc_sim.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
>> index dc9c14c..c83adf8 100644
>> --- a/hw/ipmi/ipmi_bmc_sim.c
>> +++ b/hw/ipmi/ipmi_bmc_sim.c
>> @@ -1786,12 +1786,22 @@ 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);
>> +    IPMIBmcSim *ibs = IPMI_BMC_SIMULATOR(b);
>> +
>> +    timer_del(ibs->timer);
>> +    timer_free(ibs->timer);
>> +}
>> +
> I think we may want to unrealize more here:
>
> +static void ipmi_sim_unrealize(DeviceState *dev, Error **errp)
> +{
> +    IPMIBmcSim *ibs = IPMI_BMC_SIMULATOR(dev);
> +    IPMIRcvBufEntry *msg, *tmp;
> +
> +    vmstate_unregister(NULL, &vmstate_ipmi_sim, ibs);

Isn't this already done automatically in device_set_realized()?

> +    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);
> +    }
> +    qemu_mutex_destroy(&ibs->lock);

That mutex should probably go away, it was a vestige of
something else that is no longer there.

Thanks,

-corey

>>   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
>>
>>