Patchwork pseries: Add cleanup hook for PAPR virtual LAN device

login
register
mail settings
Submitter David Gibson
Date Feb. 11, 2013, 4:59 a.m.
Message ID <1360558742-5520-1-git-send-email-david@gibson.dropbear.id.au>
Download mbox | patch
Permalink /patch/219533/
State New
Headers show

Comments

David Gibson - Feb. 11, 2013, 4:59 a.m.
Currently the spapr-vlan device does not supply a cleanup call for its
NetClientInfo structure.  With current qemu versions, that leads to a SEGV
on exit, when net_cleanup() attempts to call the cleanup handlers on all
net clients.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/spapr_llan.c |    8 ++++++++
 1 file changed, 8 insertions(+)

As a SEGV fix, this really should go in for 1.4.  What needs to be
done to make that happen.
Alexander Graf - Feb. 12, 2013, 10:10 p.m.
On 11.02.2013, at 05:59, David Gibson wrote:

> Currently the spapr-vlan device does not supply a cleanup call for its
> NetClientInfo structure.  With current qemu versions, that leads to a SEGV
> on exit, when net_cleanup() attempts to call the cleanup handlers on all
> net clients.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

The offending patch breaks more than just spapr. We need to revert the bit that checks for the existence of a cleanup function.

However, I'll still apply this to ppc-next. It's a good idea to make the cleanup function mandatory. In fact, couldn't we add a runtime check that mandatory callback functions actually exist when registering a NetClientInfo?


Alex

> ---
> hw/spapr_llan.c |    8 ++++++++
> 1 file changed, 8 insertions(+)
> 
> As a SEGV fix, this really should go in for 1.4.  What needs to be
> done to make that happen.
> 
> diff --git a/hw/spapr_llan.c b/hw/spapr_llan.c
> index 6ef2936..0ace2eb 100644
> --- a/hw/spapr_llan.c
> +++ b/hw/spapr_llan.c
> @@ -175,11 +175,19 @@ static ssize_t spapr_vlan_receive(NetClientState *nc, const uint8_t *buf,
>     return size;
> }
> 
> +static void spapr_vlan_cleanup(NetClientState *nc)
> +{
> +    VIOsPAPRVLANDevice *dev = qemu_get_nic_opaque(nc);
> +
> +    dev->nic = NULL;
> +}
> +
> static NetClientInfo net_spapr_vlan_info = {
>     .type = NET_CLIENT_OPTIONS_KIND_NIC,
>     .size = sizeof(NICState),
>     .can_receive = spapr_vlan_can_receive,
>     .receive = spapr_vlan_receive,
> +    .cleanup = spapr_vlan_cleanup,
> };
> 
> static void spapr_vlan_reset(VIOsPAPRDevice *sdev)
> -- 
> 1.7.10.4
>
Andreas Färber - Feb. 12, 2013, 11:05 p.m.
Am 11.02.2013 05:59, schrieb David Gibson:
> Currently the spapr-vlan device does not supply a cleanup call for its
> NetClientInfo structure.  With current qemu versions, that leads to a SEGV
> on exit, when net_cleanup() attempts to call the cleanup handlers on all
> net clients.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Tested-by: Andreas Färber <afaerber@suse.de>

On Anthony's request I posted a patch that adds back the if surrounding
the cleanup callback, fixing also the other affected nics.

Regards,
Andreas
Anthony Liguori - Feb. 13, 2013, 5:42 p.m.
Andreas Färber <afaerber@suse.de> writes:

> Am 11.02.2013 05:59, schrieb David Gibson:
>> Currently the spapr-vlan device does not supply a cleanup call for its
>> NetClientInfo structure.  With current qemu versions, that leads to a SEGV
>> on exit, when net_cleanup() attempts to call the cleanup handlers on all
>> net clients.
>> 
>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>
> Tested-by: Andreas Färber <afaerber@suse.de>
>
> On Anthony's request I posted a patch that adds back the if surrounding
> the cleanup callback, fixing also the other affected nics.

Since this cleanup hook doesn't do anything useful and we need to apply
Andreas' patch anyway, I don't think there's any benefit to apply this one.

Regards,

Anthony Liguori

>
> Regards,
> Andreas
>
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Patch

diff --git a/hw/spapr_llan.c b/hw/spapr_llan.c
index 6ef2936..0ace2eb 100644
--- a/hw/spapr_llan.c
+++ b/hw/spapr_llan.c
@@ -175,11 +175,19 @@  static ssize_t spapr_vlan_receive(NetClientState *nc, const uint8_t *buf,
     return size;
 }
 
+static void spapr_vlan_cleanup(NetClientState *nc)
+{
+    VIOsPAPRVLANDevice *dev = qemu_get_nic_opaque(nc);
+
+    dev->nic = NULL;
+}
+
 static NetClientInfo net_spapr_vlan_info = {
     .type = NET_CLIENT_OPTIONS_KIND_NIC,
     .size = sizeof(NICState),
     .can_receive = spapr_vlan_can_receive,
     .receive = spapr_vlan_receive,
+    .cleanup = spapr_vlan_cleanup,
 };
 
 static void spapr_vlan_reset(VIOsPAPRDevice *sdev)