[Qemu-devel] [PATCH v2] net: move rxfilter_notify() to net.c

[Qemu-devel] [PATCH v2] net: move rxfilter_notify() to net.c

[Amos Kong]

rxfilter_notify() is a generic function for all nics, not only for
virtio_net, so move it to net.c

Signed-off-by: Amos Kong <akong@redhat.com>
---
v2: fix the memory leak (Stefan)
---
 hw/net/virtio-net.c | 32 +++++---------------------------
 include/net/net.h   |  2 ++
 net/net.c           | 22 ++++++++++++++++++++++
 3 files changed, 29 insertions(+), 27 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 613f144..cee52ff 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -194,28 +194,6 @@ static void virtio_net_set_link_status(NetClientState *nc)
     virtio_net_set_status(vdev, vdev->status);
 }
 
-static void rxfilter_notify(NetClientState *nc)
-{
-    QObject *event_data;
-    VirtIONet *n = qemu_get_nic_opaque(nc);
-
-    if (nc->rxfilter_notify_enabled) {
-        if (n->netclient_name) {
-            event_data = qobject_from_jsonf("{ 'name': %s, 'path': %s }",
-                                    n->netclient_name,
-                                    object_get_canonical_path(OBJECT(n->qdev)));
-        } else {
-            event_data = qobject_from_jsonf("{ 'path': %s }",
-                                    object_get_canonical_path(OBJECT(n->qdev)));
-        }
-        monitor_protocol_event(QEVENT_NIC_RX_FILTER_CHANGED, event_data);
-        qobject_decref(event_data);
-
-        /* disable event notification to avoid events flooding */
-        nc->rxfilter_notify_enabled = 0;
-    }
-}
-
 static char *mac_strdup_printf(const uint8_t *mac)
 {
     return g_strdup_printf("%.2x:%.2x:%.2x:%.2x:%.2x:%.2x", mac[0],
@@ -545,7 +523,7 @@ static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,
         return VIRTIO_NET_ERR;
     }
 
-    rxfilter_notify(nc);
+    rxfilter_notify(nc, OBJECT(n->qdev));
 
     return VIRTIO_NET_OK;
 }
@@ -601,7 +579,7 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
         s = iov_to_buf(iov, iov_cnt, 0, &n->mac, sizeof(n->mac));
         assert(s == sizeof(n->mac));
         qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac);
-        rxfilter_notify(nc);
+        rxfilter_notify(nc, OBJECT(n->qdev));
 
         return VIRTIO_NET_OK;
     }
@@ -668,12 +646,12 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
         n->mac_table.multi_overflow = 1;
     }
 
-    rxfilter_notify(nc);
+    rxfilter_notify(nc, OBJECT(n->qdev));
 
     return VIRTIO_NET_OK;
 
 error:
-    rxfilter_notify(nc);
+    rxfilter_notify(nc, OBJECT(n->qdev));
     return VIRTIO_NET_ERR;
 }
 
@@ -700,7 +678,7 @@ static int virtio_net_handle_vlan_table(VirtIONet *n, uint8_t cmd,
     else
         return VIRTIO_NET_ERR;
 
-    rxfilter_notify(nc);
+    rxfilter_notify(nc, OBJECT(n->qdev));
 
     return VIRTIO_NET_OK;
 }
diff --git a/include/net/net.h b/include/net/net.h
index 11e1468..178db62 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -8,6 +8,7 @@
 #include "net/queue.h"
 #include "migration/vmstate.h"
 #include "qapi-types.h"
+#include "qom/object.h"
 
 #define MAX_QUEUE_NUM 1024
 
@@ -138,6 +139,7 @@ ssize_t qemu_deliver_packet_iov(NetClientState *sender,
                             void *opaque);
 
 void print_net_client(Monitor *mon, NetClientState *nc);
+void rxfilter_notify(NetClientState *nc, Object *obj);
 void do_info_network(Monitor *mon, const QDict *qdict);
 
 /* NIC info */
diff --git a/net/net.c b/net/net.c
index 0a88e68..341dd2b 100644
--- a/net/net.c
+++ b/net/net.c
@@ -41,6 +41,7 @@
 #include "qapi-visit.h"
 #include "qapi/opts-visitor.h"
 #include "qapi/dealloc-visitor.h"
+#include "qapi/qmp/qjson.h"
 
 /* Net bridge is currently not supported for W32. */
 #if !defined(_WIN32)
@@ -967,6 +968,27 @@ void print_net_client(Monitor *mon, NetClientState *nc)
                    nc->info_str);
 }
 
+void rxfilter_notify(NetClientState *nc, Object *obj)
+{
+    QObject *event_data;
+    gchar *path = object_get_canonical_path(obj);
+
+    if (nc->rxfilter_notify_enabled) {
+        if (nc->name) {
+            event_data = qobject_from_jsonf("{ 'name': %s, 'path': %s }",
+                                            nc->name, path);
+        } else {
+            event_data = qobject_from_jsonf("{ 'path': %s }", path);
+        }
+        monitor_protocol_event(QEVENT_NIC_RX_FILTER_CHANGED, event_data);
+        qobject_decref(event_data);
+
+        /* disable event notification to avoid events flooding */
+        nc->rxfilter_notify_enabled = 0;
+    }
+    g_free(path);
+}
+
 RxFilterInfoList *qmp_query_rx_filter(bool has_name, const char *name,
                                       Error **errp)
 {
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2] net: move rxfilter_notify() to net.c

[Stefan Hajnoczi]

On Mon, Nov 18, 2013 at 04:20:12PM +0800, Amos Kong wrote:
> @@ -967,6 +968,27 @@ void print_net_client(Monitor *mon, NetClientState *nc)
>                     nc->info_str);
>  }
>  
> +void rxfilter_notify(NetClientState *nc, Object *obj)
> +{
> +    QObject *event_data;
> +    gchar *path = object_get_canonical_path(obj);
> +
> +    if (nc->rxfilter_notify_enabled) {
> +        if (nc->name) {
> +            event_data = qobject_from_jsonf("{ 'name': %s, 'path': %s }",
> +                                            nc->name, path);
> +        } else {
> +            event_data = qobject_from_jsonf("{ 'path': %s }", path);
> +        }
> +        monitor_protocol_event(QEVENT_NIC_RX_FILTER_CHANGED, event_data);
> +        qobject_decref(event_data);
> +
> +        /* disable event notification to avoid events flooding */
> +        nc->rxfilter_notify_enabled = 0;

Only hw/net/virtio-net.c uses nc->rxfilter_notify_enabled.  This
function isn't reusable in its current form so I'm left wondering what
the point of this patch is?

If you have patches that invoke rxfilter_notify() from other NICs then
please submit them together in a series.

Otherwise, let's not move things around just for the sake of it,
especially when the refactoring is not done correctly.

Stefan

Re: [Qemu-devel] [PATCH v2] net: move rxfilter_notify() to net.c

[Amos Kong]

On Mon, Nov 18, 2013 at 02:25:40PM +0100, Stefan Hajnoczi wrote:
> On Mon, Nov 18, 2013 at 04:20:12PM +0800, Amos Kong wrote:
> > @@ -967,6 +968,27 @@ void print_net_client(Monitor *mon, NetClientState *nc)
> >                     nc->info_str);
> >  }
> >  
> > +void rxfilter_notify(NetClientState *nc, Object *obj)
> > +{
> > +    QObject *event_data;
> > +    gchar *path = object_get_canonical_path(obj);
> > +
> > +    if (nc->rxfilter_notify_enabled) {
> > +        if (nc->name) {
> > +            event_data = qobject_from_jsonf("{ 'name': %s, 'path': %s }",
> > +                                            nc->name, path);
> > +        } else {
> > +            event_data = qobject_from_jsonf("{ 'path': %s }", path);
> > +        }
> > +        monitor_protocol_event(QEVENT_NIC_RX_FILTER_CHANGED, event_data);
> > +        qobject_decref(event_data);
> > +
> > +        /* disable event notification to avoid events flooding */
> > +        nc->rxfilter_notify_enabled = 0;
> 
> Only hw/net/virtio-net.c uses nc->rxfilter_notify_enabled.  This
> function isn't reusable in its current form so I'm left wondering what
> the point of this patch is?
> 
> If you have patches that invoke rxfilter_notify() from other NICs then
> please submit them together in a series.

I don't have patch to change other NICs to send rxfilter notify right
now.

So I will send a patch to fix the leak. We can move the function to
net.c in future.
 
> Otherwise, let's not move things around just for the sake of it,
> especially when the refactoring is not done correctly.
> 
> Stefan

-- 
			Amos.

Re: [Qemu-devel] [PATCH v2] net: move rxfilter_notify() to net.c

[Stefan Hajnoczi]

On Mon, Nov 18, 2013 at 09:39:26PM +0800, Amos Kong wrote:
> On Mon, Nov 18, 2013 at 02:25:40PM +0100, Stefan Hajnoczi wrote:
> > On Mon, Nov 18, 2013 at 04:20:12PM +0800, Amos Kong wrote:
> > > @@ -967,6 +968,27 @@ void print_net_client(Monitor *mon, NetClientState *nc)
> > >                     nc->info_str);
> > >  }
> > >  
> > > +void rxfilter_notify(NetClientState *nc, Object *obj)
> > > +{
> > > +    QObject *event_data;
> > > +    gchar *path = object_get_canonical_path(obj);
> > > +
> > > +    if (nc->rxfilter_notify_enabled) {
> > > +        if (nc->name) {
> > > +            event_data = qobject_from_jsonf("{ 'name': %s, 'path': %s }",
> > > +                                            nc->name, path);
> > > +        } else {
> > > +            event_data = qobject_from_jsonf("{ 'path': %s }", path);
> > > +        }
> > > +        monitor_protocol_event(QEVENT_NIC_RX_FILTER_CHANGED, event_data);
> > > +        qobject_decref(event_data);
> > > +
> > > +        /* disable event notification to avoid events flooding */
> > > +        nc->rxfilter_notify_enabled = 0;
> > 
> > Only hw/net/virtio-net.c uses nc->rxfilter_notify_enabled.  This
> > function isn't reusable in its current form so I'm left wondering what
> > the point of this patch is?
> > 
> > If you have patches that invoke rxfilter_notify() from other NICs then
> > please submit them together in a series.
> 
> I don't have patch to change other NICs to send rxfilter notify right
> now.
> 
> So I will send a patch to fix the leak. We can move the function to
> net.c in future.

Okay, sounds good.

Stefan