[PATCH 1/1] net: fix vnet_hdr bustage with slirp

[PATCH 1/1] net: fix vnet_hdr bustage with slirp

[Mark McLoughlin]

slirp has started using VLANClientState::opaque and this has caused the
kvm specific tap_has_vnet_hdr() hack to break because we blindly use
this opaque pointer even if it is not a tap client.

Add yet another hack to check that we're actually getting called with a
tap client.

[Needed on stable-0.11 too]

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 net.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/net.c b/net.c
index c7702f8..2428f63 100644
--- a/net.c
+++ b/net.c
@@ -1521,6 +1521,9 @@ int tap_has_vnet_hdr(void *opaque)
     VLANClientState *vc = opaque;
     TAPState *s = vc->opaque;
 
+    if (vc->receive != tap_receive)
+        return 0;
+
     return s ? s->has_vnet_hdr : 0;
 }
 
@@ -1529,6 +1532,9 @@ void tap_using_vnet_hdr(void *opaque, int using_vnet_hdr)
     VLANClientState *vc = opaque;
     TAPState *s = vc->opaque;
 
+    if (vc->receive != tap_receive)
+        return;
+
     if (!s || !s->has_vnet_hdr)
         return;
 
-- 
1.6.2.5

Re: [PATCH 1/1] net: fix vnet_hdr bustage with slirp

[Arnd Bergmann]

On Friday 07 August 2009, Mark McLoughlin wrote:
> slirp has started using VLANClientState::opaque and this has caused the
> kvm specific tap_has_vnet_hdr() hack to break because we blindly use
> this opaque pointer even if it is not a tap client.
> 
> Add yet another hack to check that we're actually getting called with a
> tap client.
> 
> [Needed on stable-0.11 too]
> 
> Signed-off-by: Mark McLoughlin <markmc@redhat.com>

Jens and I discovered the same bug before, but then we forgot about
sending a fix (sorry). Your patch should work fine as a workaround,
but I wonder if it is the right solution. 

The abstraction of struct VLANClientState is otherwise done through
function pointers taking the VLANClientState pointer as their
first argument. IMHO a cleaner abstraction would be to do the same
for tap_has_vnet_hdr(), like the patch below, and similar for
other functions passing 'opaque' pointers.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 6dfe758..6b34e82 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -123,7 +123,7 @@ static uint32_t virtio_net_get_features(VirtIODevice *vdev)
     VirtIONet *n = to_virtio_net(vdev);
     VLANClientState *host = n->vc->vlan->first_client;
 
-    if (tap_has_vnet_hdr(host)) {
+    if (host->has_vnet_hdr && host->has_vnet_hdr(host)) {
         tap_using_vnet_hdr(host, 1);
         features |= (1 << VIRTIO_NET_F_CSUM);
         features |= (1 << VIRTIO_NET_F_GUEST_CSUM);
@@ -166,7 +166,7 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
     n->mergeable_rx_bufs = !!(features & (1 << VIRTIO_NET_F_MRG_RXBUF));
 
 #ifdef TAP_VNET_HDR
-    if (!tap_has_vnet_hdr(host) || !host->set_offload)
+    if (!(host->has_vnet_hdr && host->has_vnet_hdr(host)) || !host->set_offload)
         return;
 
     host->set_offload(host,
@@ -398,7 +398,7 @@ static int receive_header(VirtIONet *n, struct iovec *iov, int iovcnt,
     hdr->gso_type = VIRTIO_NET_HDR_GSO_NONE;
 
 #ifdef TAP_VNET_HDR
-    if (tap_has_vnet_hdr(n->vc->vlan->first_client)) {
+    if ((host->has_vnet_hdr && host->has_vnet_hdr(n->vc->vlan->first_client))) {
         memcpy(hdr, buf, sizeof(*hdr));
         offset = sizeof(*hdr);
         work_around_broken_dhclient(hdr, buf + offset, size - offset);
@@ -425,7 +425,8 @@ static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)
         return 1;
 
 #ifdef TAP_VNET_HDR
-    if (tap_has_vnet_hdr(n->vc->vlan->first_client))
+    if ((host->has_vnet_hdr &&
+	    host->has_vnet_hdr(n->vc->vlan->first_client)))
         ptr += sizeof(struct virtio_net_hdr);
 #endif
 
@@ -529,7 +530,8 @@ static void virtio_net_flush_tx(VirtIONet *n, VirtQueue *vq)
 {
     VirtQueueElement elem;
 #ifdef TAP_VNET_HDR
-    int has_vnet_hdr = tap_has_vnet_hdr(n->vc->vlan->first_client);
+    int has_vnet_hdr = (host->has_vnet_hdr &&
+		    host->has_vnet_hdr(n->vc->vlan->first_client));
 #else
     int has_vnet_hdr = 0;
 #endif
@@ -620,7 +622,7 @@ static void virtio_net_save(QEMUFile *f, void *opaque)
     qemu_put_buffer(f, (uint8_t *)n->vlans, MAX_VLAN >> 3);
 
 #ifdef TAP_VNET_HDR
-    qemu_put_be32(f, tap_has_vnet_hdr(n->vc->vlan->first_client));
+    qemu_put_be32(f, (host->has_vnet_hdr && host->has_vnet_hdr(n->vc->vlan->first_client)));
 #else
     qemu_put_be32(f, 0);
 #endif
diff --git a/net.c b/net.c
index 931def1..b56ae78 100644
--- a/net.c
+++ b/net.c
@@ -754,7 +754,7 @@ static void vmchannel_read(void *opaque, const uint8_t *buf, int size)
 
 #ifdef _WIN32
 
-int tap_has_vnet_hdr(void *opaque)
+static int tap_has_vnet_hdr(struct VLANClientState *vc)
 {
     return 0;
 }
@@ -906,9 +906,8 @@ static void tap_send(void *opaque)
     } while (s->size > 0);
 }
 
-int tap_has_vnet_hdr(void *opaque)
+static int tap_has_vnet_hdr(struct VLANClientState *vc)
 {
-    VLANClientState *vc = opaque;
     TAPState *s = vc->opaque;
 
     return s ? s->has_vnet_hdr : 0;
@@ -991,6 +990,7 @@ static TAPState *net_tap_fd_init(VLANState *vlan,
     s->has_vnet_hdr = vnet_hdr != 0;
     s->vc = qemu_new_vlan_client(vlan, model, name, tap_receive,
                                  NULL, tap_cleanup, s);
+    s->vc->has_vnet_hdr = tap_has_vnet_hdr;
     s->vc->fd_readv = tap_receive_iov;
 #ifdef TUNSETOFFLOAD
     s->vc->set_offload = tap_set_offload;
diff --git a/net.h b/net.h
index bc42428..7c79734 100644
--- a/net.h
+++ b/net.h
@@ -21,6 +21,7 @@ struct VLANClientState {
     IOCanRWHandler *fd_can_read;
     NetCleanup *cleanup;
     LinkStatusChanged *link_status_changed;
+    int (*has_vnet_hdr)(struct VLANClientState *);
     int link_down;
     SetOffload *set_offload;
     void *opaque;
@@ -72,7 +73,6 @@ void qemu_handler_true(void *opaque);
 void do_info_network(Monitor *mon);
 int do_set_link(Monitor *mon, const char *name, const char *up_or_down);
 
-int tap_has_vnet_hdr(void *opaque);
 void tap_using_vnet_hdr(void *opaque, int using_vnet_hdr);
 
 /* NIC info */

Re: [PATCH 1/1] net: fix vnet_hdr bustage with slirp

[Mark McLoughlin]

On Fri, 2009-08-07 at 13:51 +0200, Arnd Bergmann wrote:
> On Friday 07 August 2009, Mark McLoughlin wrote:
> > slirp has started using VLANClientState::opaque and this has caused the
> > kvm specific tap_has_vnet_hdr() hack to break because we blindly use
> > this opaque pointer even if it is not a tap client.
> > 
> > Add yet another hack to check that we're actually getting called with a
> > tap client.
> > 
> > [Needed on stable-0.11 too]
> > 
> > Signed-off-by: Mark McLoughlin <markmc@redhat.com>
> 
> Jens and I discovered the same bug before, but then we forgot about
> sending a fix (sorry). Your patch should work fine as a workaround,
> but I wonder if it is the right solution. 
> 
> The abstraction of struct VLANClientState is otherwise done through
> function pointers taking the VLANClientState pointer as their
> first argument. IMHO a cleaner abstraction would be to do the same
> for tap_has_vnet_hdr(), like the patch below, and similar for
> other functions passing 'opaque' pointers.

Indeed, but using vc->vlan->first_client is a great big hole in the
abstraction as it is.

The vnet_hdr code in qemu-kvm.git is a hack which we plan to
(eventually) replace by allowing a nic to be paired directly with a
backend.

Your patch is fine, but I'd suggest since both are a hack we stick with
mine since it'll reduce merge conflicts. Both hacks will go away
eventually, anyway.

Thanks,
Mark.

Re: [PATCH 1/1] net: fix vnet_hdr bustage with slirp

[Arnd Bergmann]

On Friday 07 August 2009, Mark McLoughlin wrote:
> The vnet_hdr code in qemu-kvm.git is a hack which we plan to
> (eventually) replace by allowing a nic to be paired directly with a
> backend.
> 
> Your patch is fine, but I'd suggest since both are a hack we stick with
> mine since it'll reduce merge conflicts. Both hacks will go away
> eventually, anyway.

Ok, sounds good.

Thanks,

	Arnd <><

Re: [PATCH 1/1] net: fix vnet_hdr bustage with slirp

[Avi Kivity]

On 08/07/2009 11:47 AM, Mark McLoughlin wrote:
> slirp has started using VLANClientState::opaque and this has caused the
> kvm specific tap_has_vnet_hdr() hack to break because we blindly use
> this opaque pointer even if it is not a tap client.
>
> Add yet another hack to check that we're actually getting called with a
> tap client.
>
>    

Applied, thanks.

> [Needed on stable-0.11 too]
>    

There as well.

-- 
error compiling committee.c: too many arguments to function