[Qemu-devel] [PATCH] eepro100: Fix memory leak and simplify code for VMStateDescription

[Qemu-devel] [PATCH] eepro100: Fix memory leak and simplify code for VMStateDescription

[Stefan Weil]

Instead of allocating a VMStateDescription for each NIC instance,
the code now uses a single constant VMStateDescription for all
instances. That implies that the name field is always the same.

Reported-by: Li Qiang <liqiang6-s@360.cn>
Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 hw/net/eepro100.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
index bab4dbf..6f9777d 100644
--- a/hw/net/eepro100.c
+++ b/hw/net/eepro100.c
@@ -269,9 +269,6 @@ typedef struct {
     /* Configuration bytes. */
     uint8_t configuration[22];
 
-    /* vmstate for each particular nic */
-    VMStateDescription *vmstate;
-
     /* Quasi static device properties (no need to save them). */
     uint16_t stats_size;
     bool has_extended_tcb_support;
@@ -1787,7 +1784,8 @@ static ssize_t nic_receive(NetClientState *nc, const uint8_t * buf, size_t size)
     return size;
 }
 
-static const VMStateDescription vmstate_eepro100 = {
+static const VMStateDescription vmstate_e100 = {
+    .name = "e100",
     .version_id = 3,
     .minimum_version_id = 2,
     .fields = (VMStateField[]) {
@@ -1842,7 +1840,6 @@ static void pci_nic_uninit(PCIDevice *pci_dev)
 {
     EEPRO100State *s = DO_UPCAST(EEPRO100State, dev, pci_dev);
 
-    vmstate_unregister(&pci_dev->qdev, s->vmstate, s);
     eeprom93xx_free(&pci_dev->qdev, s->eeprom);
     qemu_del_nic(s->nic);
 }
@@ -1892,11 +1889,6 @@ static void e100_nic_realize(PCIDevice *pci_dev, Error **errp)
     TRACE(OTHER, logout("%s\n", qemu_get_queue(s->nic)->info_str));
 
     qemu_register_reset(nic_reset, s);
-
-    s->vmstate = g_malloc(sizeof(vmstate_eepro100));
-    memcpy(s->vmstate, &vmstate_eepro100, sizeof(vmstate_eepro100));
-    s->vmstate->name = qemu_get_queue(s->nic)->model;
-    vmstate_register(&pci_dev->qdev, -1, s->vmstate, s);
 }
 
 static void eepro100_instance_init(Object *obj)
@@ -2083,6 +2075,7 @@ static void eepro100_class_init(ObjectClass *klass, void *data)
     set_bit(DEVICE_CATEGORY_NETWORK, dc->categories);
     dc->props = e100_properties;
     dc->desc = info->desc;
+    dc->vmsd = &vmstate_e100;
     k->vendor_id = PCI_VENDOR_ID_INTEL;
     k->class_id = PCI_CLASS_NETWORK_ETHERNET;
     k->romfile = "pxe-eepro100.rom";
-- 
2.1.4

Re: [Qemu-devel] [PATCH] eepro100: Fix memory leak and simplify code for VMStateDescription

[Dr. David Alan Gilbert]

* Stefan Weil (sw@weilnetz.de) wrote:
> Instead of allocating a VMStateDescription for each NIC instance,
> the code now uses a single constant VMStateDescription for all
> instances. That implies that the name field is always the same.

Doesn't this break migration compatibility?

You might be able to get around that (in the forward direction only)
by adding an entry to qdev_alias_table but I'm not sure.

Dave

> Reported-by: Li Qiang <liqiang6-s@360.cn>
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>  hw/net/eepro100.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
> index bab4dbf..6f9777d 100644
> --- a/hw/net/eepro100.c
> +++ b/hw/net/eepro100.c
> @@ -269,9 +269,6 @@ typedef struct {
>      /* Configuration bytes. */
>      uint8_t configuration[22];
>  
> -    /* vmstate for each particular nic */
> -    VMStateDescription *vmstate;
> -
>      /* Quasi static device properties (no need to save them). */
>      uint16_t stats_size;
>      bool has_extended_tcb_support;
> @@ -1787,7 +1784,8 @@ static ssize_t nic_receive(NetClientState *nc, const uint8_t * buf, size_t size)
>      return size;
>  }
>  
> -static const VMStateDescription vmstate_eepro100 = {
> +static const VMStateDescription vmstate_e100 = {
> +    .name = "e100",
>      .version_id = 3,
>      .minimum_version_id = 2,
>      .fields = (VMStateField[]) {
> @@ -1842,7 +1840,6 @@ static void pci_nic_uninit(PCIDevice *pci_dev)
>  {
>      EEPRO100State *s = DO_UPCAST(EEPRO100State, dev, pci_dev);
>  
> -    vmstate_unregister(&pci_dev->qdev, s->vmstate, s);
>      eeprom93xx_free(&pci_dev->qdev, s->eeprom);
>      qemu_del_nic(s->nic);
>  }
> @@ -1892,11 +1889,6 @@ static void e100_nic_realize(PCIDevice *pci_dev, Error **errp)
>      TRACE(OTHER, logout("%s\n", qemu_get_queue(s->nic)->info_str));
>  
>      qemu_register_reset(nic_reset, s);
> -
> -    s->vmstate = g_malloc(sizeof(vmstate_eepro100));
> -    memcpy(s->vmstate, &vmstate_eepro100, sizeof(vmstate_eepro100));
> -    s->vmstate->name = qemu_get_queue(s->nic)->model;
> -    vmstate_register(&pci_dev->qdev, -1, s->vmstate, s);
>  }
>  
>  static void eepro100_instance_init(Object *obj)
> @@ -2083,6 +2075,7 @@ static void eepro100_class_init(ObjectClass *klass, void *data)
>      set_bit(DEVICE_CATEGORY_NETWORK, dc->categories);
>      dc->props = e100_properties;
>      dc->desc = info->desc;
> +    dc->vmsd = &vmstate_e100;
>      k->vendor_id = PCI_VENDOR_ID_INTEL;
>      k->class_id = PCI_CLASS_NETWORK_ETHERNET;
>      k->romfile = "pxe-eepro100.rom";
> -- 
> 2.1.4
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] eepro100: Fix memory leak and simplify code for VMStateDescription

[Stefan Weil]

On 10/14/16 10:25, Dr. David Alan Gilbert wrote:
> * Stefan Weil (sw@weilnetz.de) wrote:
>> Instead of allocating a VMStateDescription for each NIC instance,
>> the code now uses a single constant VMStateDescription for all
>> instances. That implies that the name field is always the same.
>
> Doesn't this break migration compatibility?
>
> You might be able to get around that (in the forward direction only)
> by adding an entry to qdev_alias_table but I'm not sure.
>
> Dave

I'm not an expert for migration (never used it myself).

Is migration compatibility a must, even for non default settings
like the NICs implemented by eepro100.c? I assume that applications
which use migration will usually run with an e1000 NIC.

Or can we break migration compatibility and add that information
to the release notes?

How does e1000 handle migration if QEMU was started with a
e1000-82544gc NIC and migrated to a e1000-82545em NIC?

Stefan

Re: [Qemu-devel] [PATCH] eepro100: Fix memory leak and simplify code for VMStateDescription

[Dr. David Alan Gilbert]

* Stefan Weil (sw@weilnetz.de) wrote:
> On 10/14/16 10:25, Dr. David Alan Gilbert wrote:
> > * Stefan Weil (sw@weilnetz.de) wrote:
> > > Instead of allocating a VMStateDescription for each NIC instance,
> > > the code now uses a single constant VMStateDescription for all
> > > instances. That implies that the name field is always the same.
> > 
> > Doesn't this break migration compatibility?
> > 
> > You might be able to get around that (in the forward direction only)
> > by adding an entry to qdev_alias_table but I'm not sure.
> > 
> > Dave
> 
> I'm not an expert for migration (never used it myself).
> 
> Is migration compatibility a must, even for non default settings
> like the NICs implemented by eepro100.c? I assume that applications
> which use migration will usually run with an e1000 NIC.
> 
> Or can we break migration compatibility and add that information
> to the release notes?

We normally keep migration compatibility for all devices
in the forward direction unless it's something really obscure;
I don't think an e100 is.

> How does e1000 handle migration if QEMU was started with a
> e1000-82544gc NIC and migrated to a e1000-82545em NIC?

That's not required to work; you're required to have the same device
configuration on the destination as the source.

$ qemu-system-x86_64 -nographic -device e1000-82544gc
(qemu) migrate "exec:cat > t.mig"

$ qemu-system-x86_64 -nographic -device e1000-82545em -incoming "exec:cat t.mig"
qemu-system-x86_64: get_pci_config_device: Bad config data: i=0x2 read: c device: f cmask: ff wmask: 0 w1cmask:0
qemu-system-x86_64: error while loading state for instance 0x0 of device '0000:00:04.0/e1000'
qemu-system-x86_64: load of migration failed: Invalid argument

Now, note that what's happening here is that e1000 is doing a similar
trick to what you're doing - i.e. all devices end up getting
migrated as 'e1000' in the device string (0000:00:04.0/e1000).

The scheme you end up with is OK, but the problem is it's just
different from what we have now, so existing streams
with device names like '0000:00:04.0/i82550' won't load.

Dave

> 
> Stefan
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] eepro100: Fix memory leak and simplify code for VMStateDescription

[Jason Wang]

On 2016年10月17日 17:01, Dr. David Alan Gilbert wrote:
> * Stefan Weil (sw@weilnetz.de) wrote:
>> On 10/14/16 10:25, Dr. David Alan Gilbert wrote:
>>> * Stefan Weil (sw@weilnetz.de) wrote:
>>>> Instead of allocating a VMStateDescription for each NIC instance,
>>>> the code now uses a single constant VMStateDescription for all
>>>> instances. That implies that the name field is always the same.
>>> Doesn't this break migration compatibility?
>>>
>>> You might be able to get around that (in the forward direction only)
>>> by adding an entry to qdev_alias_table but I'm not sure.
>>>
>>> Dave
>> I'm not an expert for migration (never used it myself).
>>
>> Is migration compatibility a must, even for non default settings
>> like the NICs implemented by eepro100.c? I assume that applications
>> which use migration will usually run with an e1000 NIC.
>>
>> Or can we break migration compatibility and add that information
>> to the release notes?
> We normally keep migration compatibility for all devices
> in the forward direction unless it's something really obscure;
> I don't think an e100 is.
>
>> How does e1000 handle migration if QEMU was started with a
>> e1000-82544gc NIC and migrated to a e1000-82545em NIC?
> That's not required to work; you're required to have the same device
> configuration on the destination as the source.
>
> $ qemu-system-x86_64 -nographic -device e1000-82544gc
> (qemu) migrate "exec:cat > t.mig"
>
> $ qemu-system-x86_64 -nographic -device e1000-82545em -incoming "exec:cat t.mig"
> qemu-system-x86_64: get_pci_config_device: Bad config data: i=0x2 read: c device: f cmask: ff wmask: 0 w1cmask:0
> qemu-system-x86_64: error while loading state for instance 0x0 of device '0000:00:04.0/e1000'
> qemu-system-x86_64: load of migration failed: Invalid argument
>
> Now, note that what's happening here is that e1000 is doing a similar
> trick to what you're doing - i.e. all devices end up getting
> migrated as 'e1000' in the device string (0000:00:04.0/e1000).
>
> The scheme you end up with is OK, but the problem is it's just
> different from what we have now, so existing streams
> with device names like '0000:00:04.0/i82550' won't load.
>
> Dave
>

Can we bump the version here? Or just use v1 for the fixing.

Thanks

Re: [Qemu-devel] [PATCH] eepro100: Fix memory leak and simplify code for VMStateDescription

[Dr. David Alan Gilbert]

* Jason Wang (jasowang@redhat.com) wrote:
> 
> 
> On 2016年10月17日 17:01, Dr. David Alan Gilbert wrote:
> > * Stefan Weil (sw@weilnetz.de) wrote:
> > > On 10/14/16 10:25, Dr. David Alan Gilbert wrote:
> > > > * Stefan Weil (sw@weilnetz.de) wrote:
> > > > > Instead of allocating a VMStateDescription for each NIC instance,
> > > > > the code now uses a single constant VMStateDescription for all
> > > > > instances. That implies that the name field is always the same.
> > > > Doesn't this break migration compatibility?
> > > > 
> > > > You might be able to get around that (in the forward direction only)
> > > > by adding an entry to qdev_alias_table but I'm not sure.
> > > > 
> > > > Dave
> > > I'm not an expert for migration (never used it myself).
> > > 
> > > Is migration compatibility a must, even for non default settings
> > > like the NICs implemented by eepro100.c? I assume that applications
> > > which use migration will usually run with an e1000 NIC.
> > > 
> > > Or can we break migration compatibility and add that information
> > > to the release notes?
> > We normally keep migration compatibility for all devices
> > in the forward direction unless it's something really obscure;
> > I don't think an e100 is.
> > 
> > > How does e1000 handle migration if QEMU was started with a
> > > e1000-82544gc NIC and migrated to a e1000-82545em NIC?
> > That's not required to work; you're required to have the same device
> > configuration on the destination as the source.
> > 
> > $ qemu-system-x86_64 -nographic -device e1000-82544gc
> > (qemu) migrate "exec:cat > t.mig"
> > 
> > $ qemu-system-x86_64 -nographic -device e1000-82545em -incoming "exec:cat t.mig"
> > qemu-system-x86_64: get_pci_config_device: Bad config data: i=0x2 read: c device: f cmask: ff wmask: 0 w1cmask:0
> > qemu-system-x86_64: error while loading state for instance 0x0 of device '0000:00:04.0/e1000'
> > qemu-system-x86_64: load of migration failed: Invalid argument
> > 
> > Now, note that what's happening here is that e1000 is doing a similar
> > trick to what you're doing - i.e. all devices end up getting
> > migrated as 'e1000' in the device string (0000:00:04.0/e1000).
> > 
> > The scheme you end up with is OK, but the problem is it's just
> > different from what we have now, so existing streams
> > with device names like '0000:00:04.0/i82550' won't load.
> > 
> > Dave
> > 
> 
> Can we bump the version here? Or just use v1 for the fixing.

The problem isn't the version of the contents of the section, it's
the naming of the devices.
I think you can work around it by adding aliases for the old names.

Dave

> Thanks
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] eepro100: Fix memory leak and simplify code for VMStateDescription

[Stefan Weil]

Am 21.10.2016 um 10:06 schrieb Dr. David Alan Gilbert:

> The problem isn't the version of the contents of the section, it's
> the naming of the devices.
> I think you can work around it by adding aliases for the old names.
>
> Dave

How should I add such aliases? Are there other devices
with vmstate aliases, so I can see how it is done?

Stefan