[Qemu-devel] [PATCH 1/3] ipmi_bmc_sim: Free timer

[Qemu-devel] [PATCH 1/3] ipmi_bmc_sim: Free timer

[minyard]

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);
+}
+
 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

[Qemu-devel] [PATCH 2/3] wdt_i6300esb: Free timer

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Add an exit function to free the timer allocated in the
realize function.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: Richard W.M. Jones <rjones@redhat.com>
Cc: Marc-André Lureau <mlureau@redhat.com>
---
 hw/watchdog/wdt_i6300esb.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/watchdog/wdt_i6300esb.c b/hw/watchdog/wdt_i6300esb.c
index a83d951..49b3cd1 100644
--- a/hw/watchdog/wdt_i6300esb.c
+++ b/hw/watchdog/wdt_i6300esb.c
@@ -428,6 +428,14 @@ static void i6300esb_realize(PCIDevice *dev, Error **errp)
     /* qemu_register_coalesced_mmio (addr, 0x10); ? */
 }
 
+static void i6300esb_exit(PCIDevice *dev)
+{
+    I6300State *d = WATCHDOG_I6300ESB_DEVICE(dev);
+
+    timer_del(d->timer);
+    timer_free(d->timer);
+}
+
 static WatchdogTimerModel model = {
     .wdt_name = "i6300esb",
     .wdt_description = "Intel 6300ESB",
@@ -441,6 +449,7 @@ static void i6300esb_class_init(ObjectClass *klass, void *data)
     k->config_read = i6300esb_config_read;
     k->config_write = i6300esb_config_write;
     k->realize = i6300esb_realize;
+    k->exit = i6300esb_exit;
     k->vendor_id = PCI_VENDOR_ID_INTEL;
     k->device_id = PCI_DEVICE_ID_INTEL_ESB_9;
     k->class_id = PCI_CLASS_SYSTEM_OTHER;
-- 
2.7.4

Re: [Qemu-devel] [PATCH 2/3] wdt_i6300esb: Free timer

[Marc-André Lureau]

Hi

----- Original Message -----
> From: Corey Minyard <cminyard@mvista.com>
> 
> Add an exit function to free the timer allocated in the
> realize function.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> Cc: Richard W.M. Jones <rjones@redhat.com>
> Cc: Marc-André Lureau <mlureau@redhat.com>
> ---
>  hw/watchdog/wdt_i6300esb.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/hw/watchdog/wdt_i6300esb.c b/hw/watchdog/wdt_i6300esb.c
> index a83d951..49b3cd1 100644
> --- a/hw/watchdog/wdt_i6300esb.c
> +++ b/hw/watchdog/wdt_i6300esb.c
> @@ -428,6 +428,14 @@ static void i6300esb_realize(PCIDevice *dev, Error
> **errp)
>      /* qemu_register_coalesced_mmio (addr, 0x10); ? */
>  }
>  
> +static void i6300esb_exit(PCIDevice *dev)
> +{
> +    I6300State *d = WATCHDOG_I6300ESB_DEVICE(dev);
> +
> +    timer_del(d->timer);
> +    timer_free(d->timer);
> +}

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> +
>  static WatchdogTimerModel model = {
>      .wdt_name = "i6300esb",
>      .wdt_description = "Intel 6300ESB",
> @@ -441,6 +449,7 @@ static void i6300esb_class_init(ObjectClass *klass, void
> *data)
>      k->config_read = i6300esb_config_read;
>      k->config_write = i6300esb_config_write;
>      k->realize = i6300esb_realize;
> +    k->exit = i6300esb_exit;
>      k->vendor_id = PCI_VENDOR_ID_INTEL;
>      k->device_id = PCI_DEVICE_ID_INTEL_ESB_9;
>      k->class_id = PCI_CLASS_SYSTEM_OTHER;
> --
> 2.7.4
> 
> 

Re: [Qemu-devel] [PATCH 2/3] wdt_i6300esb: Free timer

[Richard W.M. Jones]

On Thu, Jul 21, 2016 at 09:44:51AM -0500, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> Add an exit function to free the timer allocated in the
> realize function.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> Cc: Richard W.M. Jones <rjones@redhat.com>
> Cc: Marc-André Lureau <mlureau@redhat.com>
> ---
>  hw/watchdog/wdt_i6300esb.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/hw/watchdog/wdt_i6300esb.c b/hw/watchdog/wdt_i6300esb.c
> index a83d951..49b3cd1 100644
> --- a/hw/watchdog/wdt_i6300esb.c
> +++ b/hw/watchdog/wdt_i6300esb.c
> @@ -428,6 +428,14 @@ static void i6300esb_realize(PCIDevice *dev, Error **errp)
>      /* qemu_register_coalesced_mmio (addr, 0x10); ? */
>  }
>  
> +static void i6300esb_exit(PCIDevice *dev)
> +{
> +    I6300State *d = WATCHDOG_I6300ESB_DEVICE(dev);
> +
> +    timer_del(d->timer);
> +    timer_free(d->timer);
> +}
> +
>  static WatchdogTimerModel model = {
>      .wdt_name = "i6300esb",
>      .wdt_description = "Intel 6300ESB",
> @@ -441,6 +449,7 @@ static void i6300esb_class_init(ObjectClass *klass, void *data)
>      k->config_read = i6300esb_config_read;
>      k->config_write = i6300esb_config_write;
>      k->realize = i6300esb_realize;
> +    k->exit = i6300esb_exit;
>      k->vendor_id = PCI_VENDOR_ID_INTEL;
>      k->device_id = PCI_DEVICE_ID_INTEL_ESB_9;
>      k->class_id = PCI_CLASS_SYSTEM_OTHER;

With the caveat that I only examined the code and compile-tested it:

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

[Qemu-devel] [PATCH 3/3] wdt_ib700: Free timer

[minyard]

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: Richard W.M. Jones <rjones@redhat.com>
Cc: Marc-André Lureau <mlureau@redhat.com>
---
 hw/watchdog/wdt_ib700.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/watchdog/wdt_ib700.c b/hw/watchdog/wdt_ib700.c
index 532afe8..179771a 100644
--- a/hw/watchdog/wdt_ib700.c
+++ b/hw/watchdog/wdt_ib700.c
@@ -117,6 +117,14 @@ static void wdt_ib700_realize(DeviceState *dev, Error **errp)
     portio_list_add(&s->port_list, isa_address_space_io(&s->parent_obj), 0);
 }
 
+static void wdt_ib700_unrealize(DeviceState *dev, Error **errp)
+{
+    IB700State *s = IB700(dev);
+
+    timer_del(s->timer);
+    timer_free(s->timer);
+}
+
 static void wdt_ib700_reset(DeviceState *dev)
 {
     IB700State *s = IB700(dev);
@@ -136,6 +144,7 @@ static void wdt_ib700_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->realize = wdt_ib700_realize;
+    dc->unrealize = wdt_ib700_unrealize;
     dc->reset = wdt_ib700_reset;
     dc->vmsd = &vmstate_ib700;
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
-- 
2.7.4

Re: [Qemu-devel] [PATCH 3/3] wdt_ib700: Free timer

[Marc-André Lureau]

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: Richard W.M. Jones <rjones@redhat.com>
> Cc: Marc-André Lureau <mlureau@redhat.com>
> ---
>  hw/watchdog/wdt_ib700.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/hw/watchdog/wdt_ib700.c b/hw/watchdog/wdt_ib700.c
> index 532afe8..179771a 100644
> --- a/hw/watchdog/wdt_ib700.c
> +++ b/hw/watchdog/wdt_ib700.c
> @@ -117,6 +117,14 @@ static void wdt_ib700_realize(DeviceState *dev, Error
> **errp)
>      portio_list_add(&s->port_list, isa_address_space_io(&s->parent_obj), 0);
>  }
>  
> +static void wdt_ib700_unrealize(DeviceState *dev, Error **errp)
> +{
> +    IB700State *s = IB700(dev);
> +
> +    timer_del(s->timer);
> +    timer_free(s->timer);
> +}
> +

We may want to call portio_list_del() too here (although this is a dead function today)

>  static void wdt_ib700_reset(DeviceState *dev)
>  {
>      IB700State *s = IB700(dev);
> @@ -136,6 +144,7 @@ static void wdt_ib700_class_init(ObjectClass *klass, void
> *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->realize = wdt_ib700_realize;
> +    dc->unrealize = wdt_ib700_unrealize;
>      dc->reset = wdt_ib700_reset;
>      dc->vmsd = &vmstate_ib700;
>      set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> --
> 2.7.4
> 
> 

Re: [Qemu-devel] [PATCH 3/3] wdt_ib700: Free timer

[Richard W.M. Jones]

On Thu, Jul 21, 2016 at 09:44:52AM -0500, minyard@acm.org wrote:
> 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: Richard W.M. Jones <rjones@redhat.com>
> Cc: Marc-André Lureau <mlureau@redhat.com>
> ---
>  hw/watchdog/wdt_ib700.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/hw/watchdog/wdt_ib700.c b/hw/watchdog/wdt_ib700.c
> index 532afe8..179771a 100644
> --- a/hw/watchdog/wdt_ib700.c
> +++ b/hw/watchdog/wdt_ib700.c
> @@ -117,6 +117,14 @@ static void wdt_ib700_realize(DeviceState *dev, Error **errp)
>      portio_list_add(&s->port_list, isa_address_space_io(&s->parent_obj), 0);
>  }
>  
> +static void wdt_ib700_unrealize(DeviceState *dev, Error **errp)
> +{
> +    IB700State *s = IB700(dev);
> +
> +    timer_del(s->timer);
> +    timer_free(s->timer);
> +}
> +
>  static void wdt_ib700_reset(DeviceState *dev)
>  {
>      IB700State *s = IB700(dev);
> @@ -136,6 +144,7 @@ static void wdt_ib700_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->realize = wdt_ib700_realize;
> +    dc->unrealize = wdt_ib700_unrealize;
>      dc->reset = wdt_ib700_reset;
>      dc->vmsd = &vmstate_ib700;
>      set_bit(DEVICE_CATEGORY_MISC, dc->categories);

With the caveat that I only examined the code and compile-tested it:

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

Re: [Qemu-devel] [PATCH 1/3] ipmi_bmc_sim: Free timer

[Marc-André Lureau]

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);
+    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);

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

Re: [Qemu-devel] [PATCH 1/3] ipmi_bmc_sim: Free timer

[Corey Minyard]

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

Re: [Qemu-devel] [PATCH 1/3] ipmi_bmc_sim: Free timer

[Corey Minyard]

On 07/21/2016 10:05 AM, Corey Minyard wrote:
> 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()?

Never mind, I misunderstood that code.

-corey

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