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

2009-08-09 Thread 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

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2009-08-07 Thread 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 mar...@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

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2009-08-07 Thread 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 mar...@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 a...@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 */
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to 

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

2009-08-07 Thread 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 mar...@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.

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2009-08-07 Thread 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 
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html