Re: [Qemu-devel] [PATCH] virtio-net: don't update mac_table in error state

2013-11-19 Thread Stefan Hajnoczi 
On Mon, Nov 11, 2013 at 11:48:36AM +0800, Amos Kong wrote:
 mac_table was always cleaned up first in handling
 VIRTIO_NET_CTRL_MAC_TABLE_SET command, and we din't recover
 mac_table content in error state, it's not correct.
 
 This patch makes all the changes in temporal variables,
 only update the real mac_table if everything is ok.
 We won't change mac_table in error state, so rxfilter
 notification isn't needed.
 
 This patch also fixed same problame in
  http://lists.nongnu.org/archive/html/qemu-devel/2013-11/msg01188.html
  (not merge)
 
 I will send patch for virtio spec to clarifying this change.
 
 Signed-off-by: Amos Kong ak...@redhat.com
 ---
  hw/net/virtio-net.c | 34 --
  1 file changed, 20 insertions(+), 14 deletions(-)

Thanks, applied to my net-next tree:
https://github.com/stefanha/qemu/commits/net-next

Stefan

Re: [Qemu-devel] [PATCH] virtio-net: don't update mac_table in error state

2013-11-12 Thread Vlad Yasevich 

On 11/10/2013 10:48 PM, Amos Kong wrote:

mac_table was always cleaned up first in handling
VIRTIO_NET_CTRL_MAC_TABLE_SET command, and we din't recover
mac_table content in error state, it's not correct.

This patch makes all the changes in temporal variables,
only update the real mac_table if everything is ok.
We won't change mac_table in error state, so rxfilter
notification isn't needed.

This patch also fixed same problame in
  http://lists.nongnu.org/archive/html/qemu-devel/2013-11/msg01188.html
  (not merge)

I will send patch for virtio spec to clarifying this change.

Signed-off-by: Amos Kong ak...@redhat.com
---
  hw/net/virtio-net.c | 34 --
  1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 22dbd05..880fa4a 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -610,11 +610,11 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t 
cmd,
  return VIRTIO_NET_ERR;
  }

-n-mac_table.in_use = 0;
-n-mac_table.first_multi = 0;
-n-mac_table.uni_overflow = 0;
-n-mac_table.multi_overflow = 0;
-memset(n-mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
+int in_use = 0;
+int first_multi = 0;
+uint8_t uni_overflow = 0;
+uint8_t multi_overflow = 0;
+uint8_t *macs = g_malloc0(MAC_TABLE_ENTRIES * ETH_ALEN);


I am not sure what the practice is for mixing declarations with code in 
qemu.  Can't find anything in the docs.


Otherwise, looks good to me.

Reviewed-by: Vlad Yasevich vyase...@redhat.com

-vlad



  s = iov_to_buf(iov, iov_cnt, 0, mac_data.entries,
 sizeof(mac_data.entries));
@@ -629,19 +629,19 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t 
cmd,
  }

  if (mac_data.entries = MAC_TABLE_ENTRIES) {
-s = iov_to_buf(iov, iov_cnt, 0, n-mac_table.macs,
+s = iov_to_buf(iov, iov_cnt, 0, macs,
 mac_data.entries * ETH_ALEN);
  if (s != mac_data.entries * ETH_ALEN) {
  goto error;
  }
-n-mac_table.in_use += mac_data.entries;
+in_use += mac_data.entries;
  } else {
-n-mac_table.uni_overflow = 1;
+uni_overflow = 1;
  }

  iov_discard_front(iov, iov_cnt, mac_data.entries * ETH_ALEN);

-n-mac_table.first_multi = n-mac_table.in_use;
+first_multi = in_use;

  s = iov_to_buf(iov, iov_cnt, 0, mac_data.entries,
 sizeof(mac_data.entries));
@@ -656,23 +656,29 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t 
cmd,
  goto error;
  }

-if (n-mac_table.in_use + mac_data.entries = MAC_TABLE_ENTRIES) {
-s = iov_to_buf(iov, iov_cnt, 0, n-mac_table.macs,
+if (in_use + mac_data.entries = MAC_TABLE_ENTRIES) {
+s = iov_to_buf(iov, iov_cnt, 0, macs[in_use * ETH_ALEN],
 mac_data.entries * ETH_ALEN);
  if (s != mac_data.entries * ETH_ALEN) {
  goto error;
  }
-n-mac_table.in_use += mac_data.entries;
+in_use += mac_data.entries;
  } else {
-n-mac_table.multi_overflow = 1;
+multi_overflow = 1;
  }

+n-mac_table.in_use = in_use;
+n-mac_table.first_multi = first_multi;
+n-mac_table.uni_overflow = uni_overflow;
+n-mac_table.multi_overflow = multi_overflow;
+memcpy(n-mac_table.macs, macs, MAC_TABLE_ENTRIES * ETH_ALEN);
+g_free(macs);
  rxfilter_notify(nc);

  return VIRTIO_NET_OK;

  error:
-rxfilter_notify(nc);
+g_free(macs);
  return VIRTIO_NET_ERR;
  }

Re: [Qemu-devel] [PATCH] virtio-net: don't update mac_table in error state

2013-11-12 Thread Jason Wang 
On 11/11/2013 11:48 AM, Amos Kong wrote:
 mac_table was always cleaned up first in handling
 VIRTIO_NET_CTRL_MAC_TABLE_SET command, and we din't recover
 mac_table content in error state, it's not correct.

 This patch makes all the changes in temporal variables,
 only update the real mac_table if everything is ok.
 We won't change mac_table in error state, so rxfilter
 notification isn't needed.

 This patch also fixed same problame in
  http://lists.nongnu.org/archive/html/qemu-devel/2013-11/msg01188.html
  (not merge)

 I will send patch for virtio spec to clarifying this change.

 Signed-off-by: Amos Kong ak...@redhat.com

Acked-by: Jason Wang jasow...@redhat.com

In the future, maybe we can allocate the mac_table dynamically instead
of embed it in VirtIONet. Then we can just does a pointer swap and
gfree() and can save a memcpy() here.
 ---
  hw/net/virtio-net.c | 34 --
  1 file changed, 20 insertions(+), 14 deletions(-)

 diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
 index 22dbd05..880fa4a 100644
 --- a/hw/net/virtio-net.c
 +++ b/hw/net/virtio-net.c
 @@ -610,11 +610,11 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t 
 cmd,
  return VIRTIO_NET_ERR;
  }
  
 -n-mac_table.in_use = 0;
 -n-mac_table.first_multi = 0;
 -n-mac_table.uni_overflow = 0;
 -n-mac_table.multi_overflow = 0;
 -memset(n-mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
 +int in_use = 0;
 +int first_multi = 0;
 +uint8_t uni_overflow = 0;
 +uint8_t multi_overflow = 0;
 +uint8_t *macs = g_malloc0(MAC_TABLE_ENTRIES * ETH_ALEN);
  
  s = iov_to_buf(iov, iov_cnt, 0, mac_data.entries,
 sizeof(mac_data.entries));
 @@ -629,19 +629,19 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t 
 cmd,
  }
  
  if (mac_data.entries = MAC_TABLE_ENTRIES) {
 -s = iov_to_buf(iov, iov_cnt, 0, n-mac_table.macs,
 +s = iov_to_buf(iov, iov_cnt, 0, macs,
 mac_data.entries * ETH_ALEN);
  if (s != mac_data.entries * ETH_ALEN) {
  goto error;
  }
 -n-mac_table.in_use += mac_data.entries;
 +in_use += mac_data.entries;
  } else {
 -n-mac_table.uni_overflow = 1;
 +uni_overflow = 1;
  }
  
  iov_discard_front(iov, iov_cnt, mac_data.entries * ETH_ALEN);
  
 -n-mac_table.first_multi = n-mac_table.in_use;
 +first_multi = in_use;
  
  s = iov_to_buf(iov, iov_cnt, 0, mac_data.entries,
 sizeof(mac_data.entries));
 @@ -656,23 +656,29 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t 
 cmd,
  goto error;
  }
  
 -if (n-mac_table.in_use + mac_data.entries = MAC_TABLE_ENTRIES) {
 -s = iov_to_buf(iov, iov_cnt, 0, n-mac_table.macs,
 +if (in_use + mac_data.entries = MAC_TABLE_ENTRIES) {
 +s = iov_to_buf(iov, iov_cnt, 0, macs[in_use * ETH_ALEN],
 mac_data.entries * ETH_ALEN);
  if (s != mac_data.entries * ETH_ALEN) {
  goto error;
  }
 -n-mac_table.in_use += mac_data.entries;
 +in_use += mac_data.entries;
  } else {
 -n-mac_table.multi_overflow = 1;
 +multi_overflow = 1;
  }
  
 +n-mac_table.in_use = in_use;
 +n-mac_table.first_multi = first_multi;
 +n-mac_table.uni_overflow = uni_overflow;
 +n-mac_table.multi_overflow = multi_overflow;
 +memcpy(n-mac_table.macs, macs, MAC_TABLE_ENTRIES * ETH_ALEN);
 +g_free(macs);
  rxfilter_notify(nc);
  
  return VIRTIO_NET_OK;
  
  error:
 -rxfilter_notify(nc);
 +g_free(macs);
  return VIRTIO_NET_ERR;
  }

[Qemu-devel] [PATCH] virtio-net: don't update mac_table in error state

2013-11-10 Thread Amos Kong 
mac_table was always cleaned up first in handling
VIRTIO_NET_CTRL_MAC_TABLE_SET command, and we din't recover
mac_table content in error state, it's not correct.

This patch makes all the changes in temporal variables,
only update the real mac_table if everything is ok.
We won't change mac_table in error state, so rxfilter
notification isn't needed.

This patch also fixed same problame in
 http://lists.nongnu.org/archive/html/qemu-devel/2013-11/msg01188.html
 (not merge)

I will send patch for virtio spec to clarifying this change.

Signed-off-by: Amos Kong ak...@redhat.com
---
 hw/net/virtio-net.c | 34 --
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 22dbd05..880fa4a 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -610,11 +610,11 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t 
cmd,
 return VIRTIO_NET_ERR;
 }
 
-n-mac_table.in_use = 0;
-n-mac_table.first_multi = 0;
-n-mac_table.uni_overflow = 0;
-n-mac_table.multi_overflow = 0;
-memset(n-mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
+int in_use = 0;
+int first_multi = 0;
+uint8_t uni_overflow = 0;
+uint8_t multi_overflow = 0;
+uint8_t *macs = g_malloc0(MAC_TABLE_ENTRIES * ETH_ALEN);
 
 s = iov_to_buf(iov, iov_cnt, 0, mac_data.entries,
sizeof(mac_data.entries));
@@ -629,19 +629,19 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t 
cmd,
 }
 
 if (mac_data.entries = MAC_TABLE_ENTRIES) {
-s = iov_to_buf(iov, iov_cnt, 0, n-mac_table.macs,
+s = iov_to_buf(iov, iov_cnt, 0, macs,
mac_data.entries * ETH_ALEN);
 if (s != mac_data.entries * ETH_ALEN) {
 goto error;
 }
-n-mac_table.in_use += mac_data.entries;
+in_use += mac_data.entries;
 } else {
-n-mac_table.uni_overflow = 1;
+uni_overflow = 1;
 }
 
 iov_discard_front(iov, iov_cnt, mac_data.entries * ETH_ALEN);
 
-n-mac_table.first_multi = n-mac_table.in_use;
+first_multi = in_use;
 
 s = iov_to_buf(iov, iov_cnt, 0, mac_data.entries,
sizeof(mac_data.entries));
@@ -656,23 +656,29 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t 
cmd,
 goto error;
 }
 
-if (n-mac_table.in_use + mac_data.entries = MAC_TABLE_ENTRIES) {
-s = iov_to_buf(iov, iov_cnt, 0, n-mac_table.macs,
+if (in_use + mac_data.entries = MAC_TABLE_ENTRIES) {
+s = iov_to_buf(iov, iov_cnt, 0, macs[in_use * ETH_ALEN],
mac_data.entries * ETH_ALEN);
 if (s != mac_data.entries * ETH_ALEN) {
 goto error;
 }
-n-mac_table.in_use += mac_data.entries;
+in_use += mac_data.entries;
 } else {
-n-mac_table.multi_overflow = 1;
+multi_overflow = 1;
 }
 
+n-mac_table.in_use = in_use;
+n-mac_table.first_multi = first_multi;
+n-mac_table.uni_overflow = uni_overflow;
+n-mac_table.multi_overflow = multi_overflow;
+memcpy(n-mac_table.macs, macs, MAC_TABLE_ENTRIES * ETH_ALEN);
+g_free(macs);
 rxfilter_notify(nc);
 
 return VIRTIO_NET_OK;
 
 error:
-rxfilter_notify(nc);
+g_free(macs);
 return VIRTIO_NET_ERR;
 }
 
-- 
1.8.3.1