[Qemu-devel] [PATCH RFC 0/7] Netfilter: Add each netdev a default filter

[Qemu-devel] [PATCH RFC 0/7] Netfilter: Add each netdev a default filter

[zhanghailiang]

This series is a prerequisite for COLO, here we add each netdev
a default buffer filter, it is disabled by default, and has
no side effect for delivering packets in net layer.

Besides, patch 1 fixes the ouput information of 'info network' command
for filter.

zhanghailiang (7):
  net/filter: Fix the output information for command 'info network'
  net/filter: Add a 'status' property for filter object
  net/filter: Skip the disabled filter when delivering packets
  net/filter: Introduce a helper to add a filter to the netdev
  filter-buffer: Accept zero interval
  net/filter: Add a default filter to each netdev
  net/filter: prevent the default filter to be deleted

 include/net/filter.h |  25 +++++++-
 net/dump.c           |   2 -
 net/filter-buffer.c  |  10 ----
 net/filter.c         | 163 +++++++++++++++++++++++++++++++++++++++++++++------
 net/net.c            |  27 ++++++++-
 5 files changed, 194 insertions(+), 33 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH RFC 1/7] net/filter: Fix the output information for command 'info network'

[zhanghailiang]

The properties of netfilter object could be changed by 'qom-set'
command, but the output of 'info network' command is not updated,
because it got the old information through nf->info_str, it will
not be updated while we change the value of netfilter's property.

Here we split a the helper function that could colletct the output
information for filter, and also remove the useless member
'info_str' from struct NetFilterState.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 include/net/filter.h |  3 ++-
 net/filter.c         | 47 ++++++++++++++++++++++++++---------------------
 net/net.c            |  5 ++++-
 3 files changed, 32 insertions(+), 23 deletions(-)

diff --git a/include/net/filter.h b/include/net/filter.h
index 2deda36..8a20138 100644
--- a/include/net/filter.h
+++ b/include/net/filter.h
@@ -55,7 +55,6 @@ struct NetFilterState {
     char *netdev_id;
     NetClientState *netdev;
     NetFilterDirection direction;
-    char info_str[256];
     QTAILQ_ENTRY(NetFilterState) next;
 };
 
@@ -74,4 +73,6 @@ ssize_t qemu_netfilter_pass_to_next(NetClientState *sender,
                                     int iovcnt,
                                     void *opaque);
 
+void netfilter_print_info(NetFilterState *nf, char *output_str, int size);
+
 #endif /* QEMU_NET_FILTER_H */
diff --git a/net/filter.c b/net/filter.c
index 5d90f83..40254bd 100644
--- a/net/filter.c
+++ b/net/filter.c
@@ -128,6 +128,31 @@ static void netfilter_init(Object *obj)
                              NULL);
 }
 
+void netfilter_print_info(NetFilterState *nf, char *output_str, int size)
+{
+    char *str, *info;
+    ObjectProperty *prop;
+    ObjectPropertyIterator iter;
+    StringOutputVisitor *ov;
+
+    /* generate info str */
+    object_property_iter_init(&iter, OBJECT(nf));
+    while ((prop = object_property_iter_next(&iter))) {
+        if (!strcmp(prop->name, "type")) {
+            continue;
+        }
+        ov = string_output_visitor_new(false);
+        object_property_get(OBJECT(nf), string_output_get_visitor(ov),
+                            prop->name, NULL);
+        str = string_output_get_string(ov);
+        string_output_visitor_cleanup(ov);
+        info = g_strdup_printf(",%s=%s", prop->name, str);
+        g_strlcat(output_str, info, size);
+        g_free(str);
+        g_free(info);
+    }
+}
+
 static void netfilter_complete(UserCreatable *uc, Error **errp)
 {
     NetFilterState *nf = NETFILTER(uc);
@@ -135,10 +160,7 @@ static void netfilter_complete(UserCreatable *uc, Error **errp)
     NetFilterClass *nfc = NETFILTER_GET_CLASS(uc);
     int queues;
     Error *local_err = NULL;
-    char *str, *info;
-    ObjectProperty *prop;
-    ObjectPropertyIterator iter;
-    StringOutputVisitor *ov;
+
 
     if (!nf->netdev_id) {
         error_setg(errp, "Parameter 'netdev' is required");
@@ -172,23 +194,6 @@ static void netfilter_complete(UserCreatable *uc, Error **errp)
         }
     }
     QTAILQ_INSERT_TAIL(&nf->netdev->filters, nf, next);
-
-    /* generate info str */
-    object_property_iter_init(&iter, OBJECT(nf));
-    while ((prop = object_property_iter_next(&iter))) {
-        if (!strcmp(prop->name, "type")) {
-            continue;
-        }
-        ov = string_output_visitor_new(false);
-        object_property_get(OBJECT(nf), string_output_get_visitor(ov),
-                            prop->name, errp);
-        str = string_output_get_string(ov);
-        string_output_visitor_cleanup(ov);
-        info = g_strdup_printf(",%s=%s", prop->name, str);
-        g_strlcat(nf->info_str, info, sizeof(nf->info_str));
-        g_free(str);
-        g_free(info);
-    }
 }
 
 static void netfilter_finalize(Object *obj)
diff --git a/net/net.c b/net/net.c
index 87dd356..87de7c0 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1198,9 +1198,12 @@ void print_net_client(Monitor *mon, NetClientState *nc)
     }
     QTAILQ_FOREACH(nf, &nc->filters, next) {
         char *path = object_get_canonical_path_component(OBJECT(nf));
+        char info[256] = { 0 };
+
+        netfilter_print_info(nf, info, sizeof(info));
         monitor_printf(mon, "  - %s: type=%s%s\n", path,
                        object_get_typename(OBJECT(nf)),
-                       nf->info_str);
+                       info);
         g_free(path);
     }
 }
-- 
1.8.3.1

[Qemu-devel] [PATCH RFC 2/7] net/filter: Add a 'status' property for filter object

[zhanghailiang]

With this property, users can control if this filter is 'enable'
or 'disable'. The default behavior for filter is enabled.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 include/net/filter.h |  1 +
 net/filter.c         | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/include/net/filter.h b/include/net/filter.h
index 8a20138..9ed5ec6 100644
--- a/include/net/filter.h
+++ b/include/net/filter.h
@@ -55,6 +55,7 @@ struct NetFilterState {
     char *netdev_id;
     NetClientState *netdev;
     NetFilterDirection direction;
+    bool enabled;
     QTAILQ_ENTRY(NetFilterState) next;
 };
 
diff --git a/net/filter.c b/net/filter.c
index 40254bd..f4933cc 100644
--- a/net/filter.c
+++ b/net/filter.c
@@ -117,8 +117,41 @@ static void netfilter_set_direction(Object *obj, int direction, Error **errp)
     nf->direction = direction;
 }
 
+static char *netfilter_get_status(Object *obj, Error **errp)
+{
+    NetFilterState *nf = NETFILTER(obj);
+
+    if (nf->enabled) {
+        return g_strdup("enable");
+    } else {
+        return g_strdup("disable");
+    }
+}
+
+static void netfilter_set_status(Object *obj, const char *str, Error **errp)
+{
+    NetFilterState *nf = NETFILTER(obj);
+
+    if (!strcmp(str, "enable")) {
+        nf->enabled = true;
+    } else if (!strcmp(str, "disable")) {
+        nf->enabled = false;
+    } else {
+        error_setg(errp, "Invalid value for netfilter status, "
+                         "should be 'enable' or 'disable'");
+    }
+}
+
 static void netfilter_init(Object *obj)
 {
+    NetFilterState *nf = NETFILTER(obj);
+
+    /*
+    * If not configured with 'status' property, the default status
+    * for netfilter will be enabled.
+    */
+    nf->enabled = true;
+
     object_property_add_str(obj, "netdev",
                             netfilter_get_netdev_id, netfilter_set_netdev_id,
                             NULL);
@@ -126,6 +159,9 @@ static void netfilter_init(Object *obj)
                              NetFilterDirection_lookup,
                              netfilter_get_direction, netfilter_set_direction,
                              NULL);
+    object_property_add_str(obj, "status",
+                            netfilter_get_status, netfilter_set_status,
+                            NULL);
 }
 
 void netfilter_print_info(NetFilterState *nf, char *output_str, int size)
-- 
1.8.3.1

[Qemu-devel] [PATCH RFC 3/7] net/filter: Skip the disabled filter when delivering packets

[zhanghailiang]

If the filter is disabled, don't go through it.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 include/net/filter.h | 5 +++++
 net/net.c            | 4 ++++
 2 files changed, 9 insertions(+)

diff --git a/include/net/filter.h b/include/net/filter.h
index 9ed5ec6..d797ee4 100644
--- a/include/net/filter.h
+++ b/include/net/filter.h
@@ -74,6 +74,11 @@ ssize_t qemu_netfilter_pass_to_next(NetClientState *sender,
                                     int iovcnt,
                                     void *opaque);
 
+static inline bool qemu_need_skip_netfilter(NetFilterState *nf)
+{
+    return nf->enabled ? false : true;
+}
+
 void netfilter_print_info(NetFilterState *nf, char *output_str, int size);
 
 #endif /* QEMU_NET_FILTER_H */
diff --git a/net/net.c b/net/net.c
index 87de7c0..ec43105 100644
--- a/net/net.c
+++ b/net/net.c
@@ -581,6 +581,10 @@ static ssize_t filter_receive_iov(NetClientState *nc,
     NetFilterState *nf = NULL;
 
     QTAILQ_FOREACH(nf, &nc->filters, next) {
+        /* Don't go through filter if it is off */
+        if (qemu_need_skip_netfilter(nf)) {
+            continue;
+        }
         ret = qemu_netfilter_receive(nf, direction, sender, flags, iov,
                                      iovcnt, sent_cb);
         if (ret) {
-- 
1.8.3.1

[Qemu-devel] [PATCH RFC 4/7] net/filter: Introduce a helper to add a filter to the netdev

[zhanghailiang]

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 include/net/filter.h |  5 +++++
 net/filter.c         | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+)

diff --git a/include/net/filter.h b/include/net/filter.h
index d797ee4..c7bd8f9 100644
--- a/include/net/filter.h
+++ b/include/net/filter.h
@@ -81,4 +81,9 @@ static inline bool qemu_need_skip_netfilter(NetFilterState *nf)
 
 void netfilter_print_info(NetFilterState *nf, char *output_str, int size);
 
+void netdev_add_filter(const char *netdev_id,
+                       const char *filter_type,
+                       const char *id,
+                       Error **errp);
+
 #endif /* QEMU_NET_FILTER_H */
diff --git a/net/filter.c b/net/filter.c
index f4933cc..4d96301 100644
--- a/net/filter.c
+++ b/net/filter.c
@@ -16,6 +16,10 @@
 #include "qom/object_interfaces.h"
 #include "qemu/iov.h"
 #include "qapi/string-output-visitor.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/qmp-output-visitor.h"
+#include "qapi/qmp-input-visitor.h"
+#include "monitor/monitor.h"
 
 ssize_t qemu_netfilter_receive(NetFilterState *nf,
                                NetFilterDirection direction,
@@ -232,6 +236,65 @@ static void netfilter_complete(UserCreatable *uc, Error **errp)
     QTAILQ_INSERT_TAIL(&nf->netdev->filters, nf, next);
 }
 
+/*
+* This will be used by COLO or MC FT, for which they will need
+* to buffer the packets of VM's net devices, Here we add a default
+* buffer filter for each netdev. The name of default buffer filter is
+* 'nop'
+*/
+void netdev_add_filter(const char *netdev_id,
+                       const char *filter_type,
+                       const char *id,
+                       Error **errp)
+{
+    QmpOutputVisitor *qov;
+    QmpInputVisitor *qiv;
+    Visitor *ov, *iv;
+    QObject *obj = NULL;
+    QDict *qdict;
+    void *dummy = NULL;
+    NetClientState *nc = qemu_find_netdev(netdev_id);
+    Error *err = NULL;
+
+    /* FIXME: Not support multiple queues */
+    if (!nc || nc->queue_index > 1) {
+        return;
+    }
+    /* Not support vhost-net */
+    if (get_vhost_net(nc)) {
+        return;
+    }
+
+    qov = qmp_output_visitor_new();
+    ov = qmp_output_get_visitor(qov);
+    visit_start_struct(ov,  &dummy, NULL, NULL, 0, &err);
+    if (err) {
+        goto out;
+    }
+    visit_type_str(ov, &nc->name, "netdev", &err);
+    if (err) {
+        goto out;
+    }
+    visit_end_struct(ov, &err);
+    if (err) {
+        goto out;
+    }
+    obj = qmp_output_get_qobject(qov);
+    g_assert(obj != NULL);
+    qdict = qobject_to_qdict(obj);
+    qmp_output_visitor_cleanup(qov);
+
+    qiv = qmp_input_visitor_new(obj);
+    iv = qmp_input_get_visitor(qiv);
+    object_add(filter_type, id, qdict, iv, &err);
+    qmp_input_visitor_cleanup(qiv);
+    qobject_decref(obj);
+out:
+    if (err) {
+        error_propagate(errp, err);
+    }
+}
+
 static void netfilter_finalize(Object *obj)
 {
     NetFilterState *nf = NETFILTER(obj);
-- 
1.8.3.1

[Qemu-devel] [PATCH RFC 5/7] filter-buffer: Accept zero interval

[zhanghailiang]

We may want to accept zero interval when VM FT solutions like MC
or COLO use this filter to release packets on demand.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Reviewed-by: Yang Hongyang <hongyang.yang@easystack.cn>
---
 net/filter-buffer.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/net/filter-buffer.c b/net/filter-buffer.c
index 57be149..12e0c87 100644
--- a/net/filter-buffer.c
+++ b/net/filter-buffer.c
@@ -103,16 +103,6 @@ static void filter_buffer_setup(NetFilterState *nf, Error **errp)
 {
     FilterBufferState *s = FILTER_BUFFER(nf);
 
-    /*
-     * We may want to accept zero interval when VM FT solutions like MC
-     * or COLO use this filter to release packets on demand.
-     */
-    if (!s->interval) {
-        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "interval",
-                   "a non-zero interval");
-        return;
-    }
-
     s->incoming_queue = qemu_new_net_queue(qemu_netfilter_pass_to_next, nf);
     if (s->interval) {
         timer_init_us(&s->release_timer, QEMU_CLOCK_VIRTUAL,
-- 
1.8.3.1

[Qemu-devel] [PATCH RFC 6/7] net/filter: Add a default filter to each netdev

[zhanghailiang]

We add each netdev a default buffer filter, which the name is
'nop', and the default buffer filter is disabled, so it has
no side effect for packets delivering in qemu net layer.

The default buffer filter can be used by COLO or Micro-checkpoint,
The reason we add the default filter is we hope to support
hot add network during COLO state in future.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 include/net/filter.h | 11 +++++++++++
 net/dump.c           |  2 --
 net/filter.c         | 15 ++++++++++++++-
 net/net.c            | 18 ++++++++++++++++++
 4 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/include/net/filter.h b/include/net/filter.h
index c7bd8f9..2043609 100644
--- a/include/net/filter.h
+++ b/include/net/filter.h
@@ -22,6 +22,16 @@
 #define NETFILTER_CLASS(klass) \
     OBJECT_CLASS_CHECK(NetFilterClass, (klass), TYPE_NETFILTER)
 
+#define DEFAULT_FILTER_NAME "nop"
+
+#define TYPE_FILTER_BUFFER "filter-buffer"
+#define TYPE_FILTER_DUMP "filter-dump"
+
+#define NETFILTER_ID_BUFFER 1
+#define NETFILTER_ID_DUMP 2
+
+extern const char *const netfilter_type_lookup[];
+
 typedef void (FilterSetup) (NetFilterState *nf, Error **errp);
 typedef void (FilterCleanup) (NetFilterState *nf);
 /*
@@ -55,6 +65,7 @@ struct NetFilterState {
     char *netdev_id;
     NetClientState *netdev;
     NetFilterDirection direction;
+    bool is_default;
     bool enabled;
     QTAILQ_ENTRY(NetFilterState) next;
 };
diff --git a/net/dump.c b/net/dump.c
index 88d9582..82727a6 100644
--- a/net/dump.c
+++ b/net/dump.c
@@ -229,8 +229,6 @@ int net_init_dump(const NetClientOptions *opts, const char *name,
 
 /* Dumping via filter */
 
-#define TYPE_FILTER_DUMP "filter-dump"
-
 #define FILTER_DUMP(obj) \
     OBJECT_CHECK(NetFilterDumpState, (obj), TYPE_FILTER_DUMP)
 
diff --git a/net/filter.c b/net/filter.c
index 4d96301..a126a3b 100644
--- a/net/filter.c
+++ b/net/filter.c
@@ -21,6 +21,11 @@
 #include "qapi/qmp-input-visitor.h"
 #include "monitor/monitor.h"
 
+const char *const netfilter_type_lookup[] = {
+    [NETFILTER_ID_BUFFER] = TYPE_FILTER_BUFFER,
+    [NETFILTER_ID_DUMP] = TYPE_FILTER_DUMP,
+};
+
 ssize_t qemu_netfilter_receive(NetFilterState *nf,
                                NetFilterDirection direction,
                                NetClientState *sender,
@@ -200,7 +205,7 @@ static void netfilter_complete(UserCreatable *uc, Error **errp)
     NetFilterClass *nfc = NETFILTER_GET_CLASS(uc);
     int queues;
     Error *local_err = NULL;
-
+    char *path = object_get_canonical_path_component(OBJECT(nf));
 
     if (!nf->netdev_id) {
         error_setg(errp, "Parameter 'netdev' is required");
@@ -225,6 +230,14 @@ static void netfilter_complete(UserCreatable *uc, Error **errp)
     }
 
     nf->netdev = ncs[0];
+    nf->is_default = !strcmp(path, DEFAULT_FILTER_NAME);
+    /*
+    * For the default buffer filter, it will be disabled by default,
+    * So it will not buffer any packets.
+    */
+    if (nf->is_default) {
+        nf->enabled = false;
+    }
 
     if (nfc->setup) {
         nfc->setup(nf, &local_err);
diff --git a/net/net.c b/net/net.c
index ec43105..9630234 100644
--- a/net/net.c
+++ b/net/net.c
@@ -76,6 +76,12 @@ const char *host_net_devices[] = {
 
 int default_net = 1;
 
+/*
+ * FIXME: Export this with an option for users to control
+ * this with comand line ?
+ */
+int default_netfilter = NETFILTER_ID_BUFFER;
+
 /***********************************************************/
 /* network device redirectors */
 
@@ -1032,6 +1038,18 @@ static int net_client_init1(const void *object, int is_netdev, Error **errp)
         }
         return -1;
     }
+
+    if (is_netdev) {
+        const Netdev *netdev = object;
+        /*
+        * Here we add each netdev a default filter whose name is 'nop',
+        * it will disabled by default, Users can enable it when necessary.
+        */
+        netdev_add_filter(netdev->id,
+                          netfilter_type_lookup[default_netfilter],
+                          DEFAULT_FILTER_NAME,
+                          errp);
+    }
     return 0;
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH RFC 7/7] net/filter: prevent the default filter to be deleted

[zhanghailiang]

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 net/filter.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/filter.c b/net/filter.c
index a126a3b..4aafff0 100644
--- a/net/filter.c
+++ b/net/filter.c
@@ -323,11 +323,19 @@ static void netfilter_finalize(Object *obj)
     g_free(nf->netdev_id);
 }
 
+static bool netfilter_can_be_deleted(UserCreatable *uc, Error **errp)
+{
+    NetFilterState *nf = NETFILTER(uc);
+    /* Forbid the default filter to be deleted */
+    return !nf->is_default;
+}
+
 static void netfilter_class_init(ObjectClass *oc, void *data)
 {
     UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
 
     ucc->complete = netfilter_complete;
+    ucc->can_be_deleted = netfilter_can_be_deleted;
 }
 
 static const TypeInfo netfilter_info = {
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH RFC 0/7] Netfilter: Add each netdev a default filter

[Hailiang Zhang]

Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>

On 2016/1/22 16:36, zhanghailiang wrote:
> This series is a prerequisite for COLO, here we add each netdev
> a default buffer filter, it is disabled by default, and has
> no side effect for delivering packets in net layer.
>
> Besides, patch 1 fixes the ouput information of 'info network' command
> for filter.
>
> zhanghailiang (7):
>    net/filter: Fix the output information for command 'info network'
>    net/filter: Add a 'status' property for filter object
>    net/filter: Skip the disabled filter when delivering packets
>    net/filter: Introduce a helper to add a filter to the netdev
>    filter-buffer: Accept zero interval
>    net/filter: Add a default filter to each netdev
>    net/filter: prevent the default filter to be deleted
>
>   include/net/filter.h |  25 +++++++-
>   net/dump.c           |   2 -
>   net/filter-buffer.c  |  10 ----
>   net/filter.c         | 163 +++++++++++++++++++++++++++++++++++++++++++++------
>   net/net.c            |  27 ++++++++-
>   5 files changed, 194 insertions(+), 33 deletions(-)
>

Re: [Qemu-devel] [PATCH RFC 3/7] net/filter: Skip the disabled filter when delivering packets

[Wen Congyang]

On 01/22/2016 04:36 PM, zhanghailiang wrote:
> If the filter is disabled, don't go through it.
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>  include/net/filter.h | 5 +++++
>  net/net.c            | 4 ++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/include/net/filter.h b/include/net/filter.h
> index 9ed5ec6..d797ee4 100644
> --- a/include/net/filter.h
> +++ b/include/net/filter.h
> @@ -74,6 +74,11 @@ ssize_t qemu_netfilter_pass_to_next(NetClientState *sender,
>                                      int iovcnt,
>                                      void *opaque);
>  
> +static inline bool qemu_need_skip_netfilter(NetFilterState *nf)
> +{
> +    return nf->enabled ? false : true;
> +}
> +
>  void netfilter_print_info(NetFilterState *nf, char *output_str, int size);
>  
>  #endif /* QEMU_NET_FILTER_H */
> diff --git a/net/net.c b/net/net.c
> index 87de7c0..ec43105 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -581,6 +581,10 @@ static ssize_t filter_receive_iov(NetClientState *nc,
>      NetFilterState *nf = NULL;
>  
>      QTAILQ_FOREACH(nf, &nc->filters, next) {
> +        /* Don't go through filter if it is off */
> +        if (qemu_need_skip_netfilter(nf)) {
> +            continue;
> +        }
>          ret = qemu_netfilter_receive(nf, direction, sender, flags, iov,
>                                       iovcnt, sent_cb);
>          if (ret) {
> 

qemu_netfilter_pass_to_next() shoule also be updated.

Thanks
Wen Congyang

Re: [Qemu-devel] [PATCH RFC 0/7] Netfilter: Add each netdev a default filter

[Daniel P. Berrange]

On Fri, Jan 22, 2016 at 04:36:44PM +0800, zhanghailiang wrote:
> This series is a prerequisite for COLO, here we add each netdev
> a default buffer filter, it is disabled by default, and has
> no side effect for delivering packets in net layer.

Why can't whatever is launching QEMU just setup filters explicitly
if they want to use COLO ? I'm not seeing an obvious compelling
reason to add this by default and then add extra code to deal
with special casing its behaviour.

> 
> Besides, patch 1 fixes the ouput information of 'info network' command
> for filter.
> 
> zhanghailiang (7):
>   net/filter: Fix the output information for command 'info network'
>   net/filter: Add a 'status' property for filter object
>   net/filter: Skip the disabled filter when delivering packets
>   net/filter: Introduce a helper to add a filter to the netdev
>   filter-buffer: Accept zero interval
>   net/filter: Add a default filter to each netdev
>   net/filter: prevent the default filter to be deleted
> 
>  include/net/filter.h |  25 +++++++-
>  net/dump.c           |   2 -
>  net/filter-buffer.c  |  10 ----
>  net/filter.c         | 163 +++++++++++++++++++++++++++++++++++++++++++++------
>  net/net.c            |  27 ++++++++-
>  5 files changed, 194 insertions(+), 33 deletions(-)

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH RFC 0/7] Netfilter: Add each netdev a default filter

[Hailiang Zhang]

On 2016/1/22 18:07, Daniel P. Berrange wrote:
> On Fri, Jan 22, 2016 at 04:36:44PM +0800, zhanghailiang wrote:
>> This series is a prerequisite for COLO, here we add each netdev
>> a default buffer filter, it is disabled by default, and has
>> no side effect for delivering packets in net layer.
>
> Why can't whatever is launching QEMU just setup filters explicitly
> if they want to use COLO ? I'm not seeing an obvious compelling
> reason to add this by default and then add extra code to deal
> with special casing its behaviour.
>

The main reason is, we hope to support hot add network during VM's COLO
lifetime in the future. (I'm not quite sure if this usage case is really exist,
but we don't want the VM in COLO state has too many limitations.)

Maybe add an option that users can control if they want to use COLO or not is more
acceptable ? With this option, we can decide whether to add the default filter or not.
Or, we could dynamically add filter while users ask to go into COLO state for VM.
(We have discussed this before in community, and Jason suggested me to add default
filter for each netdev to support hot-add network during COLO state).

What's your suggestion ?

Thanks,
Hailiang

>>
>> Besides, patch 1 fixes the ouput information of 'info network' command
>> for filter.
>>
>> zhanghailiang (7):
>>    net/filter: Fix the output information for command 'info network'
>>    net/filter: Add a 'status' property for filter object
>>    net/filter: Skip the disabled filter when delivering packets
>>    net/filter: Introduce a helper to add a filter to the netdev
>>    filter-buffer: Accept zero interval
>>    net/filter: Add a default filter to each netdev
>>    net/filter: prevent the default filter to be deleted
>>
>>   include/net/filter.h |  25 +++++++-
>>   net/dump.c           |   2 -
>>   net/filter-buffer.c  |  10 ----
>>   net/filter.c         | 163 +++++++++++++++++++++++++++++++++++++++++++++------
>>   net/net.c            |  27 ++++++++-
>>   5 files changed, 194 insertions(+), 33 deletions(-)
>
> Regards,
> Daniel
>

Re: [Qemu-devel] [PATCH RFC 0/7] Netfilter: Add each netdev a default filter

[Daniel P. Berrange]

On Fri, Jan 22, 2016 at 06:35:48PM +0800, Hailiang Zhang wrote:
> On 2016/1/22 18:07, Daniel P. Berrange wrote:
> >On Fri, Jan 22, 2016 at 04:36:44PM +0800, zhanghailiang wrote:
> >>This series is a prerequisite for COLO, here we add each netdev
> >>a default buffer filter, it is disabled by default, and has
> >>no side effect for delivering packets in net layer.
> >
> >Why can't whatever is launching QEMU just setup filters explicitly
> >if they want to use COLO ? I'm not seeing an obvious compelling
> >reason to add this by default and then add extra code to deal
> >with special casing its behaviour.
> >
> 
> The main reason is, we hope to support hot add network during VM's COLO
> lifetime in the future. (I'm not quite sure if this usage case is really exist,
> but we don't want the VM in COLO state has too many limitations.)
> 
> Maybe add an option that users can control if they want to use COLO or not is more
> acceptable ? With this option, we can decide whether to add the default filter or not.
> Or, we could dynamically add filter while users ask to go into COLO state for VM.
> (We have discussed this before in community, and Jason suggested me to add default
> filter for each netdev to support hot-add network during COLO state).
> 
> What's your suggestion ?

Why can't the app hot-adding the network interface also configure a
filter at that time if they're using COLO ?

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH RFC 0/7] Netfilter: Add each netdev a default filter

[Hailiang Zhang]

On 2016/1/22 18:38, Daniel P. Berrange wrote:
> On Fri, Jan 22, 2016 at 06:35:48PM +0800, Hailiang Zhang wrote:
>> On 2016/1/22 18:07, Daniel P. Berrange wrote:
>>> On Fri, Jan 22, 2016 at 04:36:44PM +0800, zhanghailiang wrote:
>>>> This series is a prerequisite for COLO, here we add each netdev
>>>> a default buffer filter, it is disabled by default, and has
>>>> no side effect for delivering packets in net layer.
>>>
>>> Why can't whatever is launching QEMU just setup filters explicitly
>>> if they want to use COLO ? I'm not seeing an obvious compelling
>>> reason to add this by default and then add extra code to deal
>>> with special casing its behaviour.
>>>
>>
>> The main reason is, we hope to support hot add network during VM's COLO
>> lifetime in the future. (I'm not quite sure if this usage case is really exist,
>> but we don't want the VM in COLO state has too many limitations.)
>>
>> Maybe add an option that users can control if they want to use COLO or not is more
>> acceptable ? With this option, we can decide whether to add the default filter or not.
>> Or, we could dynamically add filter while users ask to go into COLO state for VM.
>> (We have discussed this before in community, and Jason suggested me to add default
>> filter for each netdev to support hot-add network during COLO state).
>>
>> What's your suggestion ?
>
> Why can't the app hot-adding the network interface also configure a
> filter at that time if they're using COLO ?
>

Yes, they can certainly do that, but they will need to do more works:
1) netdev_add/device_add add NIC 2) identify if VM is in COLO state
3) if step 2) is YES, object-add filter for the new NIC.

Is this acceptable ?

> Regards,
> Daniel
>

Re: [Qemu-devel] [PATCH RFC 0/7] Netfilter: Add each netdev a default filter

[Jason Wang]

On 01/22/2016 06:07 PM, Daniel P. Berrange wrote:
> On Fri, Jan 22, 2016 at 04:36:44PM +0800, zhanghailiang wrote:
>> This series is a prerequisite for COLO, here we add each netdev
>> a default buffer filter, it is disabled by default, and has
>> no side effect for delivering packets in net layer.
> Why can't whatever is launching QEMU just setup filters explicitly
> if they want to use COLO ? I'm not seeing an obvious compelling
> reason to add this by default and then add extra code to deal
> with special casing its behaviour.

Two things here I think. The first is the management role for COLO,
maybe it's time to discuss it now. And if management can do this, it's
ok for not implementing default filter now. Second is the default filter
itself, I still think it's not bad to have it for the future, and with
the ability to specify, change or disable the default filter. This could
simplify management and testing.

Back to this series, it mixes several things (bugs fixings, new 'status'
filed, default filter implementation, and a special handling for
filter-buffer). Better split them.

Thanks
>
>> Besides, patch 1 fixes the ouput information of 'info network' command
>> for filter.
>>
>> zhanghailiang (7):
>>   net/filter: Fix the output information for command 'info network'
>>   net/filter: Add a 'status' property for filter object
>>   net/filter: Skip the disabled filter when delivering packets
>>   net/filter: Introduce a helper to add a filter to the netdev
>>   filter-buffer: Accept zero interval
>>   net/filter: Add a default filter to each netdev
>>   net/filter: prevent the default filter to be deleted
>>
>>  include/net/filter.h |  25 +++++++-
>>  net/dump.c           |   2 -
>>  net/filter-buffer.c  |  10 ----
>>  net/filter.c         | 163 +++++++++++++++++++++++++++++++++++++++++++++------
>>  net/net.c            |  27 ++++++++-
>>  5 files changed, 194 insertions(+), 33 deletions(-)
> Regards,
> Daniel

Re: [Qemu-devel] [PATCH RFC 1/7] net/filter: Fix the output information for command 'info network'

[Jason Wang]

On 01/22/2016 04:36 PM, zhanghailiang wrote:
> The properties of netfilter object could be changed by 'qom-set'
> command, but the output of 'info network' command is not updated,
> because it got the old information through nf->info_str, it will
> not be updated while we change the value of netfilter's property.
>
> Here we split a the helper function that could colletct the output
> information for filter, and also remove the useless member
> 'info_str' from struct NetFilterState.
>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---

Looks like a bug fix. Please send as a independent formal patch and
better cc Markus and Eric for better reviewing.

Thanks

>  include/net/filter.h |  3 ++-
>  net/filter.c         | 47 ++++++++++++++++++++++++++---------------------
>  net/net.c            |  5 ++++-
>  3 files changed, 32 insertions(+), 23 deletions(-)
>
> diff --git a/include/net/filter.h b/include/net/filter.h
> index 2deda36..8a20138 100644
> --- a/include/net/filter.h
> +++ b/include/net/filter.h
> @@ -55,7 +55,6 @@ struct NetFilterState {
>      char *netdev_id;
>      NetClientState *netdev;
>      NetFilterDirection direction;
> -    char info_str[256];
>      QTAILQ_ENTRY(NetFilterState) next;
>  };
>  
> @@ -74,4 +73,6 @@ ssize_t qemu_netfilter_pass_to_next(NetClientState *sender,
>                                      int iovcnt,
>                                      void *opaque);
>  
> +void netfilter_print_info(NetFilterState *nf, char *output_str, int size);
> +
>  #endif /* QEMU_NET_FILTER_H */
> diff --git a/net/filter.c b/net/filter.c
> index 5d90f83..40254bd 100644
> --- a/net/filter.c
> +++ b/net/filter.c
> @@ -128,6 +128,31 @@ static void netfilter_init(Object *obj)
>                               NULL);
>  }
>  
> +void netfilter_print_info(NetFilterState *nf, char *output_str, int size)
> +{
> +    char *str, *info;
> +    ObjectProperty *prop;
> +    ObjectPropertyIterator iter;
> +    StringOutputVisitor *ov;
> +
> +    /* generate info str */
> +    object_property_iter_init(&iter, OBJECT(nf));
> +    while ((prop = object_property_iter_next(&iter))) {
> +        if (!strcmp(prop->name, "type")) {
> +            continue;
> +        }
> +        ov = string_output_visitor_new(false);
> +        object_property_get(OBJECT(nf), string_output_get_visitor(ov),
> +                            prop->name, NULL);
> +        str = string_output_get_string(ov);
> +        string_output_visitor_cleanup(ov);
> +        info = g_strdup_printf(",%s=%s", prop->name, str);
> +        g_strlcat(output_str, info, size);
> +        g_free(str);
> +        g_free(info);
> +    }
> +}
> +
>  static void netfilter_complete(UserCreatable *uc, Error **errp)
>  {
>      NetFilterState *nf = NETFILTER(uc);
> @@ -135,10 +160,7 @@ static void netfilter_complete(UserCreatable *uc, Error **errp)
>      NetFilterClass *nfc = NETFILTER_GET_CLASS(uc);
>      int queues;
>      Error *local_err = NULL;
> -    char *str, *info;
> -    ObjectProperty *prop;
> -    ObjectPropertyIterator iter;
> -    StringOutputVisitor *ov;
> +
>  
>      if (!nf->netdev_id) {
>          error_setg(errp, "Parameter 'netdev' is required");
> @@ -172,23 +194,6 @@ static void netfilter_complete(UserCreatable *uc, Error **errp)
>          }
>      }
>      QTAILQ_INSERT_TAIL(&nf->netdev->filters, nf, next);
> -
> -    /* generate info str */
> -    object_property_iter_init(&iter, OBJECT(nf));
> -    while ((prop = object_property_iter_next(&iter))) {
> -        if (!strcmp(prop->name, "type")) {
> -            continue;
> -        }
> -        ov = string_output_visitor_new(false);
> -        object_property_get(OBJECT(nf), string_output_get_visitor(ov),
> -                            prop->name, errp);
> -        str = string_output_get_string(ov);
> -        string_output_visitor_cleanup(ov);
> -        info = g_strdup_printf(",%s=%s", prop->name, str);
> -        g_strlcat(nf->info_str, info, sizeof(nf->info_str));
> -        g_free(str);
> -        g_free(info);
> -    }
>  }
>  
>  static void netfilter_finalize(Object *obj)
> diff --git a/net/net.c b/net/net.c
> index 87dd356..87de7c0 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1198,9 +1198,12 @@ void print_net_client(Monitor *mon, NetClientState *nc)
>      }
>      QTAILQ_FOREACH(nf, &nc->filters, next) {
>          char *path = object_get_canonical_path_component(OBJECT(nf));
> +        char info[256] = { 0 };
> +
> +        netfilter_print_info(nf, info, sizeof(info));
>          monitor_printf(mon, "  - %s: type=%s%s\n", path,
>                         object_get_typename(OBJECT(nf)),
> -                       nf->info_str);
> +                       info);
>          g_free(path);
>      }
>  }

Re: [Qemu-devel] [PATCH RFC 3/7] net/filter: Skip the disabled filter when delivering packets

[Jason Wang]

On 01/22/2016 05:32 PM, Wen Congyang wrote:
> On 01/22/2016 04:36 PM, zhanghailiang wrote:
>> If the filter is disabled, don't go through it.
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> ---
>>  include/net/filter.h | 5 +++++
>>  net/net.c            | 4 ++++
>>  2 files changed, 9 insertions(+)
>>
>> diff --git a/include/net/filter.h b/include/net/filter.h
>> index 9ed5ec6..d797ee4 100644
>> --- a/include/net/filter.h
>> +++ b/include/net/filter.h
>> @@ -74,6 +74,11 @@ ssize_t qemu_netfilter_pass_to_next(NetClientState *sender,
>>                                      int iovcnt,
>>                                      void *opaque);
>>  
>> +static inline bool qemu_need_skip_netfilter(NetFilterState *nf)
>> +{
>> +    return nf->enabled ? false : true;
>> +}
>> +
>>  void netfilter_print_info(NetFilterState *nf, char *output_str, int size);
>>  
>>  #endif /* QEMU_NET_FILTER_H */
>> diff --git a/net/net.c b/net/net.c
>> index 87de7c0..ec43105 100644
>> --- a/net/net.c
>> +++ b/net/net.c
>> @@ -581,6 +581,10 @@ static ssize_t filter_receive_iov(NetClientState *nc,
>>      NetFilterState *nf = NULL;
>>  
>>      QTAILQ_FOREACH(nf, &nc->filters, next) {
>> +        /* Don't go through filter if it is off */
>> +        if (qemu_need_skip_netfilter(nf)) {
>> +            continue;
>> +        }
>>          ret = qemu_netfilter_receive(nf, direction, sender, flags, iov,
>>                                       iovcnt, sent_cb);
>>          if (ret) {
>>
> qemu_netfilter_pass_to_next() shoule also be updated.

Then let's better move it to qemu_netfiler_receive().

>
> Thanks
> Wen Congyang
>
>
>
>

Re: [Qemu-devel] [PATCH RFC 2/7] net/filter: Add a 'status' property for filter object

[Jason Wang]

On 01/22/2016 04:36 PM, zhanghailiang wrote:
> With this property, users can control if this filter is 'enable'
> or 'disable'. The default behavior for filter is enabled.
>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>

Let's squash patch 3 into this for a complete implementation of 'status'.

> ---
>  include/net/filter.h |  1 +
>  net/filter.c         | 36 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 37 insertions(+)
>
> diff --git a/include/net/filter.h b/include/net/filter.h
> index 8a20138..9ed5ec6 100644
> --- a/include/net/filter.h
> +++ b/include/net/filter.h
> @@ -55,6 +55,7 @@ struct NetFilterState {
>      char *netdev_id;
>      NetClientState *netdev;
>      NetFilterDirection direction;
> +    bool enabled;
>      QTAILQ_ENTRY(NetFilterState) next;
>  };
>  
> diff --git a/net/filter.c b/net/filter.c
> index 40254bd..f4933cc 100644
> --- a/net/filter.c
> +++ b/net/filter.c
> @@ -117,8 +117,41 @@ static void netfilter_set_direction(Object *obj, int direction, Error **errp)
>      nf->direction = direction;
>  }
>  
> +static char *netfilter_get_status(Object *obj, Error **errp)
> +{
> +    NetFilterState *nf = NETFILTER(obj);
> +
> +    if (nf->enabled) {
> +        return g_strdup("enable");
> +    } else {
> +        return g_strdup("disable");
> +    }
> +}
> +
> +static void netfilter_set_status(Object *obj, const char *str, Error **errp)
> +{
> +    NetFilterState *nf = NETFILTER(obj);
> +
> +    if (!strcmp(str, "enable")) {
> +        nf->enabled = true;
> +    } else if (!strcmp(str, "disable")) {
> +        nf->enabled = false;
> +    } else {
> +        error_setg(errp, "Invalid value for netfilter status, "
> +                         "should be 'enable' or 'disable'");
> +    }
> +}
> +
>  static void netfilter_init(Object *obj)
>  {
> +    NetFilterState *nf = NETFILTER(obj);
> +
> +    /*
> +    * If not configured with 'status' property, the default status
> +    * for netfilter will be enabled.
> +    */
> +    nf->enabled = true;
> +
>      object_property_add_str(obj, "netdev",
>                              netfilter_get_netdev_id, netfilter_set_netdev_id,
>                              NULL);
> @@ -126,6 +159,9 @@ static void netfilter_init(Object *obj)
>                               NetFilterDirection_lookup,
>                               netfilter_get_direction, netfilter_set_direction,
>                               NULL);
> +    object_property_add_str(obj, "status",
> +                            netfilter_get_status, netfilter_set_status,
> +                            NULL);
>  }
>  
>  void netfilter_print_info(NetFilterState *nf, char *output_str, int size)

Re: [Qemu-devel] [PATCH RFC 6/7] net/filter: Add a default filter to each netdev

[Jason Wang]

On 01/22/2016 04:36 PM, zhanghailiang wrote:
> We add each netdev a default buffer filter, which the name is
> 'nop', and the default buffer filter is disabled, so it has
> no side effect for packets delivering in qemu net layer.
>
> The default buffer filter can be used by COLO or Micro-checkpoint,
> The reason we add the default filter is we hope to support
> hot add network during COLO state in future.
>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>  include/net/filter.h | 11 +++++++++++
>  net/dump.c           |  2 --
>  net/filter.c         | 15 ++++++++++++++-
>  net/net.c            | 18 ++++++++++++++++++
>  4 files changed, 43 insertions(+), 3 deletions(-)
>
> diff --git a/include/net/filter.h b/include/net/filter.h
> index c7bd8f9..2043609 100644
> --- a/include/net/filter.h
> +++ b/include/net/filter.h
> @@ -22,6 +22,16 @@
>  #define NETFILTER_CLASS(klass) \
>      OBJECT_CLASS_CHECK(NetFilterClass, (klass), TYPE_NETFILTER)
>  
> +#define DEFAULT_FILTER_NAME "nop"

Maybe DEFAULT_FILTER_TYPE?

> +
> +#define TYPE_FILTER_BUFFER "filter-buffer"
> +#define TYPE_FILTER_DUMP "filter-dump"
> +
> +#define NETFILTER_ID_BUFFER 1
> +#define NETFILTER_ID_DUMP 2
> +
> +extern const char *const netfilter_type_lookup[];
> +
>  typedef void (FilterSetup) (NetFilterState *nf, Error **errp);
>  typedef void (FilterCleanup) (NetFilterState *nf);
>  /*
> @@ -55,6 +65,7 @@ struct NetFilterState {
>      char *netdev_id;
>      NetClientState *netdev;
>      NetFilterDirection direction;
> +    bool is_default;
>      bool enabled;
>      QTAILQ_ENTRY(NetFilterState) next;
>  };
> diff --git a/net/dump.c b/net/dump.c
> index 88d9582..82727a6 100644
> --- a/net/dump.c
> +++ b/net/dump.c
> @@ -229,8 +229,6 @@ int net_init_dump(const NetClientOptions *opts, const char *name,
>  
>  /* Dumping via filter */
>  
> -#define TYPE_FILTER_DUMP "filter-dump"
> -
>  #define FILTER_DUMP(obj) \
>      OBJECT_CHECK(NetFilterDumpState, (obj), TYPE_FILTER_DUMP)
>  
> diff --git a/net/filter.c b/net/filter.c
> index 4d96301..a126a3b 100644
> --- a/net/filter.c
> +++ b/net/filter.c
> @@ -21,6 +21,11 @@
>  #include "qapi/qmp-input-visitor.h"
>  #include "monitor/monitor.h"
>  
> +const char *const netfilter_type_lookup[] = {
> +    [NETFILTER_ID_BUFFER] = TYPE_FILTER_BUFFER,
> +    [NETFILTER_ID_DUMP] = TYPE_FILTER_DUMP,
> +};
> +
>  ssize_t qemu_netfilter_receive(NetFilterState *nf,
>                                 NetFilterDirection direction,
>                                 NetClientState *sender,
> @@ -200,7 +205,7 @@ static void netfilter_complete(UserCreatable *uc, Error **errp)
>      NetFilterClass *nfc = NETFILTER_GET_CLASS(uc);
>      int queues;
>      Error *local_err = NULL;
> -
> +    char *path = object_get_canonical_path_component(OBJECT(nf));
>  
>      if (!nf->netdev_id) {
>          error_setg(errp, "Parameter 'netdev' is required");
> @@ -225,6 +230,14 @@ static void netfilter_complete(UserCreatable *uc, Error **errp)
>      }
>  
>      nf->netdev = ncs[0];
> +    nf->is_default = !strcmp(path, DEFAULT_FILTER_NAME);
> +    /*
> +    * For the default buffer filter, it will be disabled by default,
> +    * So it will not buffer any packets.
> +    */
> +    if (nf->is_default) {
> +        nf->enabled = false;
> +    }

This seems not very elegant. Besides DEFAULT_FILTER_NAME(TYPE), we may
also want a DEFAULT_FILTER_PROPERTIES? Then you can store the "status"
into properties.

>  
>      if (nfc->setup) {
>          nfc->setup(nf, &local_err);
> diff --git a/net/net.c b/net/net.c
> index ec43105..9630234 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -76,6 +76,12 @@ const char *host_net_devices[] = {
>  
>  int default_net = 1;
>  
> +/*
> + * FIXME: Export this with an option for users to control
> + * this with comand line ?

This could be done in the future.

> + */
> +int default_netfilter = NETFILTER_ID_BUFFER;

Why not just use a string here?

> +
>  /***********************************************************/
>  /* network device redirectors */
>  
> @@ -1032,6 +1038,18 @@ static int net_client_init1(const void *object, int is_netdev, Error **errp)
>          }
>          return -1;
>      }
> +
> +    if (is_netdev) {
> +        const Netdev *netdev = object;
> +        /*
> +        * Here we add each netdev a default filter whose name is 'nop',
> +        * it will disabled by default, Users can enable it when necessary.
> +        */

If we support default properties, the above comment could be removed.

> +        netdev_add_filter(netdev->id,
> +                          netfilter_type_lookup[default_netfilter],
> +                          DEFAULT_FILTER_NAME,

I believe some logic to generate id automatically is needed here.

> +                          errp);
> +    }
>      return 0;
>  }
>  

Re: [Qemu-devel] [PATCH RFC 5/7] filter-buffer: Accept zero interval

[Jason Wang]

On 01/22/2016 04:36 PM, zhanghailiang wrote:
> We may want to accept zero interval when VM FT solutions like MC
> or COLO use this filter to release packets on demand.
>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Reviewed-by: Yang Hongyang <hongyang.yang@easystack.cn>
> ---
>  net/filter-buffer.c | 10 ----------
>  1 file changed, 10 deletions(-)

Looks a independent patch of this rfc series.

>
> diff --git a/net/filter-buffer.c b/net/filter-buffer.c
> index 57be149..12e0c87 100644
> --- a/net/filter-buffer.c
> +++ b/net/filter-buffer.c
> @@ -103,16 +103,6 @@ static void filter_buffer_setup(NetFilterState *nf, Error **errp)
>  {
>      FilterBufferState *s = FILTER_BUFFER(nf);
>  
> -    /*
> -     * We may want to accept zero interval when VM FT solutions like MC
> -     * or COLO use this filter to release packets on demand.
> -     */
> -    if (!s->interval) {
> -        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "interval",
> -                   "a non-zero interval");
> -        return;
> -    }
> -
>      s->incoming_queue = qemu_new_net_queue(qemu_netfilter_pass_to_next, nf);
>      if (s->interval) {
>          timer_init_us(&s->release_timer, QEMU_CLOCK_VIRTUAL,

Re: [Qemu-devel] [PATCH RFC 4/7] net/filter: Introduce a helper to add a filter to the netdev

[Jason Wang]

On 01/22/2016 04:36 PM, zhanghailiang wrote:
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>

Commit log please.

> ---
>  include/net/filter.h |  5 +++++
>  net/filter.c         | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 68 insertions(+)
>
> diff --git a/include/net/filter.h b/include/net/filter.h
> index d797ee4..c7bd8f9 100644
> --- a/include/net/filter.h
> +++ b/include/net/filter.h
> @@ -81,4 +81,9 @@ static inline bool qemu_need_skip_netfilter(NetFilterState *nf)
>  
>  void netfilter_print_info(NetFilterState *nf, char *output_str, int size);
>  
> +void netdev_add_filter(const char *netdev_id,
> +                       const char *filter_type,
> +                       const char *id,
> +                       Error **errp);
> +
>  #endif /* QEMU_NET_FILTER_H */
> diff --git a/net/filter.c b/net/filter.c
> index f4933cc..4d96301 100644
> --- a/net/filter.c
> +++ b/net/filter.c
> @@ -16,6 +16,10 @@
>  #include "qom/object_interfaces.h"
>  #include "qemu/iov.h"
>  #include "qapi/string-output-visitor.h"
> +#include "qapi/qmp/qdict.h"
> +#include "qapi/qmp-output-visitor.h"
> +#include "qapi/qmp-input-visitor.h"
> +#include "monitor/monitor.h"
>  
>  ssize_t qemu_netfilter_receive(NetFilterState *nf,
>                                 NetFilterDirection direction,
> @@ -232,6 +236,65 @@ static void netfilter_complete(UserCreatable *uc, Error **errp)
>      QTAILQ_INSERT_TAIL(&nf->netdev->filters, nf, next);
>  }
>  
> +/*
> +* This will be used by COLO or MC FT, for which they will need
> +* to buffer the packets of VM's net devices, Here we add a default
> +* buffer filter for each netdev. The name of default buffer filter is
> +* 'nop'
> +*/
> +void netdev_add_filter(const char *netdev_id,
> +                       const char *filter_type,
> +                       const char *id,
> +                       Error **errp)
> +{
> +    QmpOutputVisitor *qov;
> +    QmpInputVisitor *qiv;
> +    Visitor *ov, *iv;
> +    QObject *obj = NULL;
> +    QDict *qdict;
> +    void *dummy = NULL;
> +    NetClientState *nc = qemu_find_netdev(netdev_id);
> +    Error *err = NULL;
> +
> +    /* FIXME: Not support multiple queues */
> +    if (!nc || nc->queue_index > 1) {
> +        return;
> +    }
> +    /* Not support vhost-net */
> +    if (get_vhost_net(nc)) {
> +        return;
> +    }
> +
> +    qov = qmp_output_visitor_new();
> +    ov = qmp_output_get_visitor(qov);
> +    visit_start_struct(ov,  &dummy, NULL, NULL, 0, &err);
> +    if (err) {
> +        goto out;
> +    }
> +    visit_type_str(ov, &nc->name, "netdev", &err);
> +    if (err) {
> +        goto out;
> +    }
> +    visit_end_struct(ov, &err);
> +    if (err) {
> +        goto out;
> +    }
> +    obj = qmp_output_get_qobject(qov);
> +    g_assert(obj != NULL);
> +    qdict = qobject_to_qdict(obj);
> +    qmp_output_visitor_cleanup(qov);
> +
> +    qiv = qmp_input_visitor_new(obj);
> +    iv = qmp_input_get_visitor(qiv);
> +    object_add(filter_type, id, qdict, iv, &err);
> +    qmp_input_visitor_cleanup(qiv);
> +    qobject_decref(obj);
> +out:
> +    if (err) {
> +        error_propagate(errp, err);
> +    }
> +}
> +
>  static void netfilter_finalize(Object *obj)
>  {
>      NetFilterState *nf = NETFILTER(obj);

Re: [Qemu-devel] [PATCH RFC 0/7] Netfilter: Add each netdev a default filter

[Hailiang Zhang]

On 2016/1/25 9:59, Hailiang Zhang wrote:
> On 2016/1/22 18:38, Daniel P. Berrange wrote:
>> On Fri, Jan 22, 2016 at 06:35:48PM +0800, Hailiang Zhang wrote:
>>> On 2016/1/22 18:07, Daniel P. Berrange wrote:
>>>> On Fri, Jan 22, 2016 at 04:36:44PM +0800, zhanghailiang wrote:
>>>>> This series is a prerequisite for COLO, here we add each netdev
>>>>> a default buffer filter, it is disabled by default, and has
>>>>> no side effect for delivering packets in net layer.
>>>>
>>>> Why can't whatever is launching QEMU just setup filters explicitly
>>>> if they want to use COLO ? I'm not seeing an obvious compelling
>>>> reason to add this by default and then add extra code to deal
>>>> with special casing its behaviour.
>>>>
>>>
>>> The main reason is, we hope to support hot add network during VM's COLO
>>> lifetime in the future. (I'm not quite sure if this usage case is really exist,
>>> but we don't want the VM in COLO state has too many limitations.)
>>>
>>> Maybe add an option that users can control if they want to use COLO or not is more
>>> acceptable ? With this option, we can decide whether to add the default filter or not.
>>> Or, we could dynamically add filter while users ask to go into COLO state for VM.
>>> (We have discussed this before in community, and Jason suggested me to add default
>>> filter for each netdev to support hot-add network during COLO state).
>>>
>>> What's your suggestion ?
>>
>> Why can't the app hot-adding the network interface also configure a
>> filter at that time if they're using COLO ?
>>
>
> Yes, they can certainly do that, but they will need to do more works:
> 1) netdev_add/device_add add NIC 2) identify if VM is in COLO state
> 3) if step 2) is YES, object-add filter for the new NIC.
>

Er, i have forgotten something, we have to do some other
works for COLO if we didn't append each netdev a default netfilter.
While do failover work, we should consider to remove the extra filters
or keep them if the users decide if they continue another COLO lifetime
for VM.

Thanks,
Hailiang

> Is this acceptable ?
>

>> Regards,
>> Daniel
>>
>

Re: [Qemu-devel] [PATCH RFC 7/7] net/filter: prevent the default filter to be deleted

[Jason Wang]

On 01/22/2016 04:36 PM, zhanghailiang wrote:
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>  net/filter.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/net/filter.c b/net/filter.c
> index a126a3b..4aafff0 100644
> --- a/net/filter.c
> +++ b/net/filter.c
> @@ -323,11 +323,19 @@ static void netfilter_finalize(Object *obj)
>      g_free(nf->netdev_id);
>  }
>  
> +static bool netfilter_can_be_deleted(UserCreatable *uc, Error **errp)
> +{
> +    NetFilterState *nf = NETFILTER(uc);
> +    /* Forbid the default filter to be deleted */
> +    return !nf->is_default;
> +}
> +
>  static void netfilter_class_init(ObjectClass *oc, void *data)
>  {
>      UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
>  
>      ucc->complete = netfilter_complete;
> +    ucc->can_be_deleted = netfilter_can_be_deleted;
>  }
>  
>  static const TypeInfo netfilter_info = {

This looks unnecessary. As I replied in previous mails, there's no need
to differ default netfilter from others. For COLO specifically, I know
it's a kind of mis-configuration you want to avoid, but that's not the
business of qemu. (Even if this is accepted, user could still mis
configure the netfitler that can break COLO).

Re: [Qemu-devel] [PATCH RFC 0/7] Netfilter: Add each netdev a default filter

[Hailiang Zhang]

On 2016/1/25 11:32, Jason Wang wrote:
>
>
> On 01/22/2016 06:07 PM, Daniel P. Berrange wrote:
>> On Fri, Jan 22, 2016 at 04:36:44PM +0800, zhanghailiang wrote:
>>> This series is a prerequisite for COLO, here we add each netdev
>>> a default buffer filter, it is disabled by default, and has
>>> no side effect for delivering packets in net layer.
>> Why can't whatever is launching QEMU just setup filters explicitly
>> if they want to use COLO ? I'm not seeing an obvious compelling
>> reason to add this by default and then add extra code to deal
>> with special casing its behaviour.
>
> Two things here I think. The first is the management role for COLO,
> maybe it's time to discuss it now. And if management can do this, it's

Yes, better to confirm it as early as possible, the later COLO based on
proxy also have such problem.

> ok for not implementing default filter now. Second is the default filter
> itself, I still think it's not bad to have it for the future, and with
> the ability to specify, change or disable the default filter. This could
> simplify management and testing.
>
> Back to this series, it mixes several things (bugs fixings, new 'status'
> filed, default filter implementation, and a special handling for
> filter-buffer). Better split them.
>

OK, i will do that, thanks.

> Thanks
>>
>>> Besides, patch 1 fixes the ouput information of 'info network' command
>>> for filter.
>>>
>>> zhanghailiang (7):
>>>    net/filter: Fix the output information for command 'info network'
>>>    net/filter: Add a 'status' property for filter object
>>>    net/filter: Skip the disabled filter when delivering packets
>>>    net/filter: Introduce a helper to add a filter to the netdev
>>>    filter-buffer: Accept zero interval
>>>    net/filter: Add a default filter to each netdev
>>>    net/filter: prevent the default filter to be deleted
>>>
>>>   include/net/filter.h |  25 +++++++-
>>>   net/dump.c           |   2 -
>>>   net/filter-buffer.c  |  10 ----
>>>   net/filter.c         | 163 +++++++++++++++++++++++++++++++++++++++++++++------
>>>   net/net.c            |  27 ++++++++-
>>>   5 files changed, 194 insertions(+), 33 deletions(-)
>> Regards,
>> Daniel
>
>
> .
>

Re: [Qemu-devel] [PATCH RFC 1/7] net/filter: Fix the output information for command 'info network'

[Hailiang Zhang]

On 2016/1/25 13:01, Jason Wang wrote:
>
>
> On 01/22/2016 04:36 PM, zhanghailiang wrote:
>> The properties of netfilter object could be changed by 'qom-set'
>> command, but the output of 'info network' command is not updated,
>> because it got the old information through nf->info_str, it will
>> not be updated while we change the value of netfilter's property.
>>
>> Here we split a the helper function that could colletct the output
>> information for filter, and also remove the useless member
>> 'info_str' from struct NetFilterState.
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> ---
>
> Looks like a bug fix. Please send as a independent formal patch and
> better cc Markus and Eric for better reviewing.
>

OK, by the way, i didn't see there was a corresponding qmp command
for 'info network', is the hmp_info_network just an experimental command ?

> Thanks
>
>>   include/net/filter.h |  3 ++-
>>   net/filter.c         | 47 ++++++++++++++++++++++++++---------------------
>>   net/net.c            |  5 ++++-
>>   3 files changed, 32 insertions(+), 23 deletions(-)
>>
>> diff --git a/include/net/filter.h b/include/net/filter.h
>> index 2deda36..8a20138 100644
>> --- a/include/net/filter.h
>> +++ b/include/net/filter.h
>> @@ -55,7 +55,6 @@ struct NetFilterState {
>>       char *netdev_id;
>>       NetClientState *netdev;
>>       NetFilterDirection direction;
>> -    char info_str[256];
>>       QTAILQ_ENTRY(NetFilterState) next;
>>   };
>>
>> @@ -74,4 +73,6 @@ ssize_t qemu_netfilter_pass_to_next(NetClientState *sender,
>>                                       int iovcnt,
>>                                       void *opaque);
>>
>> +void netfilter_print_info(NetFilterState *nf, char *output_str, int size);
>> +
>>   #endif /* QEMU_NET_FILTER_H */
>> diff --git a/net/filter.c b/net/filter.c
>> index 5d90f83..40254bd 100644
>> --- a/net/filter.c
>> +++ b/net/filter.c
>> @@ -128,6 +128,31 @@ static void netfilter_init(Object *obj)
>>                                NULL);
>>   }
>>
>> +void netfilter_print_info(NetFilterState *nf, char *output_str, int size)
>> +{
>> +    char *str, *info;
>> +    ObjectProperty *prop;
>> +    ObjectPropertyIterator iter;
>> +    StringOutputVisitor *ov;
>> +
>> +    /* generate info str */
>> +    object_property_iter_init(&iter, OBJECT(nf));
>> +    while ((prop = object_property_iter_next(&iter))) {
>> +        if (!strcmp(prop->name, "type")) {
>> +            continue;
>> +        }
>> +        ov = string_output_visitor_new(false);
>> +        object_property_get(OBJECT(nf), string_output_get_visitor(ov),
>> +                            prop->name, NULL);
>> +        str = string_output_get_string(ov);
>> +        string_output_visitor_cleanup(ov);
>> +        info = g_strdup_printf(",%s=%s", prop->name, str);
>> +        g_strlcat(output_str, info, size);
>> +        g_free(str);
>> +        g_free(info);
>> +    }
>> +}
>> +
>>   static void netfilter_complete(UserCreatable *uc, Error **errp)
>>   {
>>       NetFilterState *nf = NETFILTER(uc);
>> @@ -135,10 +160,7 @@ static void netfilter_complete(UserCreatable *uc, Error **errp)
>>       NetFilterClass *nfc = NETFILTER_GET_CLASS(uc);
>>       int queues;
>>       Error *local_err = NULL;
>> -    char *str, *info;
>> -    ObjectProperty *prop;
>> -    ObjectPropertyIterator iter;
>> -    StringOutputVisitor *ov;
>> +
>>
>>       if (!nf->netdev_id) {
>>           error_setg(errp, "Parameter 'netdev' is required");
>> @@ -172,23 +194,6 @@ static void netfilter_complete(UserCreatable *uc, Error **errp)
>>           }
>>       }
>>       QTAILQ_INSERT_TAIL(&nf->netdev->filters, nf, next);
>> -
>> -    /* generate info str */
>> -    object_property_iter_init(&iter, OBJECT(nf));
>> -    while ((prop = object_property_iter_next(&iter))) {
>> -        if (!strcmp(prop->name, "type")) {
>> -            continue;
>> -        }
>> -        ov = string_output_visitor_new(false);
>> -        object_property_get(OBJECT(nf), string_output_get_visitor(ov),
>> -                            prop->name, errp);
>> -        str = string_output_get_string(ov);
>> -        string_output_visitor_cleanup(ov);
>> -        info = g_strdup_printf(",%s=%s", prop->name, str);
>> -        g_strlcat(nf->info_str, info, sizeof(nf->info_str));
>> -        g_free(str);
>> -        g_free(info);
>> -    }
>>   }
>>
>>   static void netfilter_finalize(Object *obj)
>> diff --git a/net/net.c b/net/net.c
>> index 87dd356..87de7c0 100644
>> --- a/net/net.c
>> +++ b/net/net.c
>> @@ -1198,9 +1198,12 @@ void print_net_client(Monitor *mon, NetClientState *nc)
>>       }
>>       QTAILQ_FOREACH(nf, &nc->filters, next) {
>>           char *path = object_get_canonical_path_component(OBJECT(nf));
>> +        char info[256] = { 0 };
>> +
>> +        netfilter_print_info(nf, info, sizeof(info));
>>           monitor_printf(mon, "  - %s: type=%s%s\n", path,
>>                          object_get_typename(OBJECT(nf)),
>> -                       nf->info_str);
>> +                       info);
>>           g_free(path);
>>       }
>>   }
>
>
> .
>

Re: [Qemu-devel] [PATCH RFC 3/7] net/filter: Skip the disabled filter when delivering packets

[Hailiang Zhang]

On 2016/1/25 13:04, Jason Wang wrote:
>
>
> On 01/22/2016 05:32 PM, Wen Congyang wrote:
>> On 01/22/2016 04:36 PM, zhanghailiang wrote:
>>> If the filter is disabled, don't go through it.
>>>
>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>> ---
>>>   include/net/filter.h | 5 +++++
>>>   net/net.c            | 4 ++++
>>>   2 files changed, 9 insertions(+)
>>>
>>> diff --git a/include/net/filter.h b/include/net/filter.h
>>> index 9ed5ec6..d797ee4 100644
>>> --- a/include/net/filter.h
>>> +++ b/include/net/filter.h
>>> @@ -74,6 +74,11 @@ ssize_t qemu_netfilter_pass_to_next(NetClientState *sender,
>>>                                       int iovcnt,
>>>                                       void *opaque);
>>>
>>> +static inline bool qemu_need_skip_netfilter(NetFilterState *nf)
>>> +{
>>> +    return nf->enabled ? false : true;
>>> +}
>>> +
>>>   void netfilter_print_info(NetFilterState *nf, char *output_str, int size);
>>>
>>>   #endif /* QEMU_NET_FILTER_H */
>>> diff --git a/net/net.c b/net/net.c
>>> index 87de7c0..ec43105 100644
>>> --- a/net/net.c
>>> +++ b/net/net.c
>>> @@ -581,6 +581,10 @@ static ssize_t filter_receive_iov(NetClientState *nc,
>>>       NetFilterState *nf = NULL;
>>>
>>>       QTAILQ_FOREACH(nf, &nc->filters, next) {
>>> +        /* Don't go through filter if it is off */
>>> +        if (qemu_need_skip_netfilter(nf)) {
>>> +            continue;
>>> +        }
>>>           ret = qemu_netfilter_receive(nf, direction, sender, flags, iov,
>>>                                        iovcnt, sent_cb);
>>>           if (ret) {
>>>
>> qemu_netfilter_pass_to_next() shoule also be updated.
>
> Then let's better move it to qemu_netfiler_receive().
>

Good idea, i will fix it, thanks.

>>
>> Thanks
>> Wen Congyang
>>
>>
>>
>>
>
>
> .
>

Re: [Qemu-devel] [PATCH RFC 2/7] net/filter: Add a 'status' property for filter object

[Hailiang Zhang]

On 2016/1/25 13:05, Jason Wang wrote:
>
>
> On 01/22/2016 04:36 PM, zhanghailiang wrote:
>> With this property, users can control if this filter is 'enable'
>> or 'disable'. The default behavior for filter is enabled.
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>
> Let's squash patch 3 into this for a complete implementation of 'status'.
>

OK, will do that, thanks.

>> ---
>>   include/net/filter.h |  1 +
>>   net/filter.c         | 36 ++++++++++++++++++++++++++++++++++++
>>   2 files changed, 37 insertions(+)
>>
>> diff --git a/include/net/filter.h b/include/net/filter.h
>> index 8a20138..9ed5ec6 100644
>> --- a/include/net/filter.h
>> +++ b/include/net/filter.h
>> @@ -55,6 +55,7 @@ struct NetFilterState {
>>       char *netdev_id;
>>       NetClientState *netdev;
>>       NetFilterDirection direction;
>> +    bool enabled;
>>       QTAILQ_ENTRY(NetFilterState) next;
>>   };
>>
>> diff --git a/net/filter.c b/net/filter.c
>> index 40254bd..f4933cc 100644
>> --- a/net/filter.c
>> +++ b/net/filter.c
>> @@ -117,8 +117,41 @@ static void netfilter_set_direction(Object *obj, int direction, Error **errp)
>>       nf->direction = direction;
>>   }
>>
>> +static char *netfilter_get_status(Object *obj, Error **errp)
>> +{
>> +    NetFilterState *nf = NETFILTER(obj);
>> +
>> +    if (nf->enabled) {
>> +        return g_strdup("enable");
>> +    } else {
>> +        return g_strdup("disable");
>> +    }
>> +}
>> +
>> +static void netfilter_set_status(Object *obj, const char *str, Error **errp)
>> +{
>> +    NetFilterState *nf = NETFILTER(obj);
>> +
>> +    if (!strcmp(str, "enable")) {
>> +        nf->enabled = true;
>> +    } else if (!strcmp(str, "disable")) {
>> +        nf->enabled = false;
>> +    } else {
>> +        error_setg(errp, "Invalid value for netfilter status, "
>> +                         "should be 'enable' or 'disable'");
>> +    }
>> +}
>> +
>>   static void netfilter_init(Object *obj)
>>   {
>> +    NetFilterState *nf = NETFILTER(obj);
>> +
>> +    /*
>> +    * If not configured with 'status' property, the default status
>> +    * for netfilter will be enabled.
>> +    */
>> +    nf->enabled = true;
>> +
>>       object_property_add_str(obj, "netdev",
>>                               netfilter_get_netdev_id, netfilter_set_netdev_id,
>>                               NULL);
>> @@ -126,6 +159,9 @@ static void netfilter_init(Object *obj)
>>                                NetFilterDirection_lookup,
>>                                netfilter_get_direction, netfilter_set_direction,
>>                                NULL);
>> +    object_property_add_str(obj, "status",
>> +                            netfilter_get_status, netfilter_set_status,
>> +                            NULL);
>>   }
>>
>>   void netfilter_print_info(NetFilterState *nf, char *output_str, int size)
>
>
> .
>

Re: [Qemu-devel] [PATCH RFC 6/7] net/filter: Add a default filter to each netdev

[Hailiang Zhang]

On 2016/1/25 13:18, Jason Wang wrote:
>
>
> On 01/22/2016 04:36 PM, zhanghailiang wrote:
>> We add each netdev a default buffer filter, which the name is
>> 'nop', and the default buffer filter is disabled, so it has
>> no side effect for packets delivering in qemu net layer.
>>
>> The default buffer filter can be used by COLO or Micro-checkpoint,
>> The reason we add the default filter is we hope to support
>> hot add network during COLO state in future.
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> ---
>>   include/net/filter.h | 11 +++++++++++
>>   net/dump.c           |  2 --
>>   net/filter.c         | 15 ++++++++++++++-
>>   net/net.c            | 18 ++++++++++++++++++
>>   4 files changed, 43 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/net/filter.h b/include/net/filter.h
>> index c7bd8f9..2043609 100644
>> --- a/include/net/filter.h
>> +++ b/include/net/filter.h
>> @@ -22,6 +22,16 @@
>>   #define NETFILTER_CLASS(klass) \
>>       OBJECT_CLASS_CHECK(NetFilterClass, (klass), TYPE_NETFILTER)
>>
>> +#define DEFAULT_FILTER_NAME "nop"
>
> Maybe DEFAULT_FILTER_TYPE?
>

>> +
>> +#define TYPE_FILTER_BUFFER "filter-buffer"
>> +#define TYPE_FILTER_DUMP "filter-dump"
>> +
>> +#define NETFILTER_ID_BUFFER 1
>> +#define NETFILTER_ID_DUMP 2
>> +
>> +extern const char *const netfilter_type_lookup[];
>> +
>>   typedef void (FilterSetup) (NetFilterState *nf, Error **errp);
>>   typedef void (FilterCleanup) (NetFilterState *nf);
>>   /*
>> @@ -55,6 +65,7 @@ struct NetFilterState {
>>       char *netdev_id;
>>       NetClientState *netdev;
>>       NetFilterDirection direction;
>> +    bool is_default;
>>       bool enabled;
>>       QTAILQ_ENTRY(NetFilterState) next;
>>   };
>> diff --git a/net/dump.c b/net/dump.c
>> index 88d9582..82727a6 100644
>> --- a/net/dump.c
>> +++ b/net/dump.c
>> @@ -229,8 +229,6 @@ int net_init_dump(const NetClientOptions *opts, const char *name,
>>
>>   /* Dumping via filter */
>>
>> -#define TYPE_FILTER_DUMP "filter-dump"
>> -
>>   #define FILTER_DUMP(obj) \
>>       OBJECT_CHECK(NetFilterDumpState, (obj), TYPE_FILTER_DUMP)
>>
>> diff --git a/net/filter.c b/net/filter.c
>> index 4d96301..a126a3b 100644
>> --- a/net/filter.c
>> +++ b/net/filter.c
>> @@ -21,6 +21,11 @@
>>   #include "qapi/qmp-input-visitor.h"
>>   #include "monitor/monitor.h"
>>
>> +const char *const netfilter_type_lookup[] = {
>> +    [NETFILTER_ID_BUFFER] = TYPE_FILTER_BUFFER,
>> +    [NETFILTER_ID_DUMP] = TYPE_FILTER_DUMP,
>> +};
>> +
>>   ssize_t qemu_netfilter_receive(NetFilterState *nf,
>>                                  NetFilterDirection direction,
>>                                  NetClientState *sender,
>> @@ -200,7 +205,7 @@ static void netfilter_complete(UserCreatable *uc, Error **errp)
>>       NetFilterClass *nfc = NETFILTER_GET_CLASS(uc);
>>       int queues;
>>       Error *local_err = NULL;
>> -
>> +    char *path = object_get_canonical_path_component(OBJECT(nf));
>>
>>       if (!nf->netdev_id) {
>>           error_setg(errp, "Parameter 'netdev' is required");
>> @@ -225,6 +230,14 @@ static void netfilter_complete(UserCreatable *uc, Error **errp)
>>       }
>>
>>       nf->netdev = ncs[0];
>> +    nf->is_default = !strcmp(path, DEFAULT_FILTER_NAME);
>> +    /*
>> +    * For the default buffer filter, it will be disabled by default,
>> +    * So it will not buffer any packets.
>> +    */
>> +    if (nf->is_default) {
>> +        nf->enabled = false;
>> +    }
>
> This seems not very elegant. Besides DEFAULT_FILTER_NAME(TYPE), we may
> also want a DEFAULT_FILTER_PROPERTIES? Then you can store the "status"
> into properties.
>

A little confused, do you mean add a 'default' property for filter ?
Just like the new 'status' property which is exported to users ?
Is the type of 'default' property string or bool ?

>>
>>       if (nfc->setup) {
>>           nfc->setup(nf, &local_err);
>> diff --git a/net/net.c b/net/net.c
>> index ec43105..9630234 100644
>> --- a/net/net.c
>> +++ b/net/net.c
>> @@ -76,6 +76,12 @@ const char *host_net_devices[] = {
>>
>>   int default_net = 1;
>>
>> +/*
>> + * FIXME: Export this with an option for users to control
>> + * this with comand line ?
>
> This could be done in the future.
>

Should i change the tag to 'TODO' ?

>> + */
>> +int default_netfilter = NETFILTER_ID_BUFFER;
>
> Why not just use a string here?
>

No special, i will convert it to use string here.

>> +
>>   /***********************************************************/
>>   /* network device redirectors */
>>
>> @@ -1032,6 +1038,18 @@ static int net_client_init1(const void *object, int is_netdev, Error **errp)
>>           }
>>           return -1;
>>       }
>> +
>> +    if (is_netdev) {
>> +        const Netdev *netdev = object;
>> +        /*
>> +        * Here we add each netdev a default filter whose name is 'nop',
>> +        * it will disabled by default, Users can enable it when necessary.
>> +        */
>
> If we support default properties, the above comment could be removed.
>
>> +        netdev_add_filter(netdev->id,
>> +                          netfilter_type_lookup[default_netfilter],
>> +                          DEFAULT_FILTER_NAME,
>
> I believe some logic to generate id automatically is needed here.
>

Yes, you are right, here this patch will break QEMU with multi-NICs configured,
the error report is " attempt to add duplicate property 'nop' to object".
I will fix it in next version.

Thanks,
Hailiang

>> +                          errp);
>> +    }
>>       return 0;
>>   }
>>
>
>
> .
>

Re: [Qemu-devel] [PATCH RFC 5/7] filter-buffer: Accept zero interval

[Hailiang Zhang]

On 2016/1/25 13:19, Jason Wang wrote:
>
>
> On 01/22/2016 04:36 PM, zhanghailiang wrote:
>> We may want to accept zero interval when VM FT solutions like MC
>> or COLO use this filter to release packets on demand.
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Reviewed-by: Yang Hongyang <hongyang.yang@easystack.cn>
>> ---
>>   net/filter-buffer.c | 10 ----------
>>   1 file changed, 10 deletions(-)
>
> Looks a independent patch of this rfc series.
>

The interval of the default filter is zero, without this patch,
there will be an error report.

>>
>> diff --git a/net/filter-buffer.c b/net/filter-buffer.c
>> index 57be149..12e0c87 100644
>> --- a/net/filter-buffer.c
>> +++ b/net/filter-buffer.c
>> @@ -103,16 +103,6 @@ static void filter_buffer_setup(NetFilterState *nf, Error **errp)
>>   {
>>       FilterBufferState *s = FILTER_BUFFER(nf);
>>
>> -    /*
>> -     * We may want to accept zero interval when VM FT solutions like MC
>> -     * or COLO use this filter to release packets on demand.
>> -     */
>> -    if (!s->interval) {
>> -        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "interval",
>> -                   "a non-zero interval");
>> -        return;
>> -    }
>> -
>>       s->incoming_queue = qemu_new_net_queue(qemu_netfilter_pass_to_next, nf);
>>       if (s->interval) {
>>           timer_init_us(&s->release_timer, QEMU_CLOCK_VIRTUAL,
>
>
> .
>

Re: [Qemu-devel] [PATCH RFC 4/7] net/filter: Introduce a helper to add a filter to the netdev

[Hailiang Zhang]

On 2016/1/25 13:20, Jason Wang wrote:
>
>
> On 01/22/2016 04:36 PM, zhanghailiang wrote:
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>
> Commit log please.
>

I will add it in next version, thanks.

>> ---
>>   include/net/filter.h |  5 +++++
>>   net/filter.c         | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 68 insertions(+)
>>
>> diff --git a/include/net/filter.h b/include/net/filter.h
>> index d797ee4..c7bd8f9 100644
>> --- a/include/net/filter.h
>> +++ b/include/net/filter.h
>> @@ -81,4 +81,9 @@ static inline bool qemu_need_skip_netfilter(NetFilterState *nf)
>>
>>   void netfilter_print_info(NetFilterState *nf, char *output_str, int size);
>>
>> +void netdev_add_filter(const char *netdev_id,
>> +                       const char *filter_type,
>> +                       const char *id,
>> +                       Error **errp);
>> +
>>   #endif /* QEMU_NET_FILTER_H */
>> diff --git a/net/filter.c b/net/filter.c
>> index f4933cc..4d96301 100644
>> --- a/net/filter.c
>> +++ b/net/filter.c
>> @@ -16,6 +16,10 @@
>>   #include "qom/object_interfaces.h"
>>   #include "qemu/iov.h"
>>   #include "qapi/string-output-visitor.h"
>> +#include "qapi/qmp/qdict.h"
>> +#include "qapi/qmp-output-visitor.h"
>> +#include "qapi/qmp-input-visitor.h"
>> +#include "monitor/monitor.h"
>>
>>   ssize_t qemu_netfilter_receive(NetFilterState *nf,
>>                                  NetFilterDirection direction,
>> @@ -232,6 +236,65 @@ static void netfilter_complete(UserCreatable *uc, Error **errp)
>>       QTAILQ_INSERT_TAIL(&nf->netdev->filters, nf, next);
>>   }
>>
>> +/*
>> +* This will be used by COLO or MC FT, for which they will need
>> +* to buffer the packets of VM's net devices, Here we add a default
>> +* buffer filter for each netdev. The name of default buffer filter is
>> +* 'nop'
>> +*/
>> +void netdev_add_filter(const char *netdev_id,
>> +                       const char *filter_type,
>> +                       const char *id,
>> +                       Error **errp)
>> +{
>> +    QmpOutputVisitor *qov;
>> +    QmpInputVisitor *qiv;
>> +    Visitor *ov, *iv;
>> +    QObject *obj = NULL;
>> +    QDict *qdict;
>> +    void *dummy = NULL;
>> +    NetClientState *nc = qemu_find_netdev(netdev_id);
>> +    Error *err = NULL;
>> +
>> +    /* FIXME: Not support multiple queues */
>> +    if (!nc || nc->queue_index > 1) {
>> +        return;
>> +    }
>> +    /* Not support vhost-net */
>> +    if (get_vhost_net(nc)) {
>> +        return;
>> +    }
>> +
>> +    qov = qmp_output_visitor_new();
>> +    ov = qmp_output_get_visitor(qov);
>> +    visit_start_struct(ov,  &dummy, NULL, NULL, 0, &err);
>> +    if (err) {
>> +        goto out;
>> +    }
>> +    visit_type_str(ov, &nc->name, "netdev", &err);
>> +    if (err) {
>> +        goto out;
>> +    }
>> +    visit_end_struct(ov, &err);
>> +    if (err) {
>> +        goto out;
>> +    }
>> +    obj = qmp_output_get_qobject(qov);
>> +    g_assert(obj != NULL);
>> +    qdict = qobject_to_qdict(obj);
>> +    qmp_output_visitor_cleanup(qov);
>> +
>> +    qiv = qmp_input_visitor_new(obj);
>> +    iv = qmp_input_get_visitor(qiv);
>> +    object_add(filter_type, id, qdict, iv, &err);
>> +    qmp_input_visitor_cleanup(qiv);
>> +    qobject_decref(obj);
>> +out:
>> +    if (err) {
>> +        error_propagate(errp, err);
>> +    }
>> +}
>> +
>>   static void netfilter_finalize(Object *obj)
>>   {
>>       NetFilterState *nf = NETFILTER(obj);
>
>
> .
>

Re: [Qemu-devel] [PATCH RFC 7/7] net/filter: prevent the default filter to be deleted

[Hailiang Zhang]

On 2016/1/25 13:25, Jason Wang wrote:
>
>
> On 01/22/2016 04:36 PM, zhanghailiang wrote:
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> ---
>>   net/filter.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/net/filter.c b/net/filter.c
>> index a126a3b..4aafff0 100644
>> --- a/net/filter.c
>> +++ b/net/filter.c
>> @@ -323,11 +323,19 @@ static void netfilter_finalize(Object *obj)
>>       g_free(nf->netdev_id);
>>   }
>>
>> +static bool netfilter_can_be_deleted(UserCreatable *uc, Error **errp)
>> +{
>> +    NetFilterState *nf = NETFILTER(uc);
>> +    /* Forbid the default filter to be deleted */
>> +    return !nf->is_default;
>> +}
>> +
>>   static void netfilter_class_init(ObjectClass *oc, void *data)
>>   {
>>       UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
>>
>>       ucc->complete = netfilter_complete;
>> +    ucc->can_be_deleted = netfilter_can_be_deleted;
>>   }
>>
>>   static const TypeInfo netfilter_info = {
>
> This looks unnecessary. As I replied in previous mails, there's no need
> to differ default netfilter from others. For COLO specifically, I know
> it's a kind of mis-configuration you want to avoid, but that's not the
> business of qemu. (Even if this is accepted, user could still mis
> configure the netfitler that can break COLO).
>

Got it, will drop this patch in next version, thanks.

Re: [Qemu-devel] [PATCH RFC 6/7] net/filter: Add a default filter to each netdev

[Hailiang Zhang]

On 2016/1/25 15:22, Hailiang Zhang wrote:
> On 2016/1/25 13:18, Jason Wang wrote:
>>
>>
>> On 01/22/2016 04:36 PM, zhanghailiang wrote:
>>> We add each netdev a default buffer filter, which the name is
>>> 'nop', and the default buffer filter is disabled, so it has
>>> no side effect for packets delivering in qemu net layer.
>>>
>>> The default buffer filter can be used by COLO or Micro-checkpoint,
>>> The reason we add the default filter is we hope to support
>>> hot add network during COLO state in future.
>>>
>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>> ---
>>>   include/net/filter.h | 11 +++++++++++
>>>   net/dump.c           |  2 --
>>>   net/filter.c         | 15 ++++++++++++++-
>>>   net/net.c            | 18 ++++++++++++++++++
>>>   4 files changed, 43 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/include/net/filter.h b/include/net/filter.h
>>> index c7bd8f9..2043609 100644
>>> --- a/include/net/filter.h
>>> +++ b/include/net/filter.h
>>> @@ -22,6 +22,16 @@
>>>   #define NETFILTER_CLASS(klass) \
>>>       OBJECT_CLASS_CHECK(NetFilterClass, (klass), TYPE_NETFILTER)
>>>
>>> +#define DEFAULT_FILTER_NAME "nop"
>>
>> Maybe DEFAULT_FILTER_TYPE?
>>
>
>>> +
>>> +#define TYPE_FILTER_BUFFER "filter-buffer"
>>> +#define TYPE_FILTER_DUMP "filter-dump"
>>> +
>>> +#define NETFILTER_ID_BUFFER 1
>>> +#define NETFILTER_ID_DUMP 2
>>> +
>>> +extern const char *const netfilter_type_lookup[];
>>> +
>>>   typedef void (FilterSetup) (NetFilterState *nf, Error **errp);
>>>   typedef void (FilterCleanup) (NetFilterState *nf);
>>>   /*
>>> @@ -55,6 +65,7 @@ struct NetFilterState {
>>>       char *netdev_id;
>>>       NetClientState *netdev;
>>>       NetFilterDirection direction;
>>> +    bool is_default;
>>>       bool enabled;
>>>       QTAILQ_ENTRY(NetFilterState) next;
>>>   };
>>> diff --git a/net/dump.c b/net/dump.c
>>> index 88d9582..82727a6 100644
>>> --- a/net/dump.c
>>> +++ b/net/dump.c
>>> @@ -229,8 +229,6 @@ int net_init_dump(const NetClientOptions *opts, const char *name,
>>>
>>>   /* Dumping via filter */
>>>
>>> -#define TYPE_FILTER_DUMP "filter-dump"
>>> -
>>>   #define FILTER_DUMP(obj) \
>>>       OBJECT_CHECK(NetFilterDumpState, (obj), TYPE_FILTER_DUMP)
>>>
>>> diff --git a/net/filter.c b/net/filter.c
>>> index 4d96301..a126a3b 100644
>>> --- a/net/filter.c
>>> +++ b/net/filter.c
>>> @@ -21,6 +21,11 @@
>>>   #include "qapi/qmp-input-visitor.h"
>>>   #include "monitor/monitor.h"
>>>
>>> +const char *const netfilter_type_lookup[] = {
>>> +    [NETFILTER_ID_BUFFER] = TYPE_FILTER_BUFFER,
>>> +    [NETFILTER_ID_DUMP] = TYPE_FILTER_DUMP,
>>> +};
>>> +
>>>   ssize_t qemu_netfilter_receive(NetFilterState *nf,
>>>                                  NetFilterDirection direction,
>>>                                  NetClientState *sender,
>>> @@ -200,7 +205,7 @@ static void netfilter_complete(UserCreatable *uc, Error **errp)
>>>       NetFilterClass *nfc = NETFILTER_GET_CLASS(uc);
>>>       int queues;
>>>       Error *local_err = NULL;
>>> -
>>> +    char *path = object_get_canonical_path_component(OBJECT(nf));
>>>
>>>       if (!nf->netdev_id) {
>>>           error_setg(errp, "Parameter 'netdev' is required");
>>> @@ -225,6 +230,14 @@ static void netfilter_complete(UserCreatable *uc, Error **errp)
>>>       }
>>>
>>>       nf->netdev = ncs[0];
>>> +    nf->is_default = !strcmp(path, DEFAULT_FILTER_NAME);
>>> +    /*
>>> +    * For the default buffer filter, it will be disabled by default,
>>> +    * So it will not buffer any packets.
>>> +    */
>>> +    if (nf->is_default) {
>>> +        nf->enabled = false;
>>> +    }
>>
>> This seems not very elegant. Besides DEFAULT_FILTER_NAME(TYPE), we may
>> also want a DEFAULT_FILTER_PROPERTIES? Then you can store the "status"
>> into properties.
>>
>
> A little confused, do you mean add a 'default' property for filter ?
> Just like the new 'status' property which is exported to users ?
> Is the type of 'default' property string or bool ?
>

I don't think it is necessary to add a 'default' property, because
we don't want such a property to be controlled by user. It is
only used internally.
We have another choice to deal with this,
Add a 'bool is_default' parameter for netdev_add_filter(),
and handle the default filter in it, just like:

void netdev_add_filter(const char *netdev_id,
                        const char *filter_type,
                        const char *id,
                        bool is_default,
                        Error **errp)
{
     ... ...
     object_add(filter_type, id, qdict, iv, &err);
     .... ...
     if (is_default) {
         Object *obj, *container;
         NetFilterState *nf;

         container = object_get_objects_root();
         obj = object_resolve_path_component(container, id);
         if (!obj) {
             error_setg(errp, "object id not found");
             return;
         }
         nf = NETFILTER(obj);
         nf->is_default = true;
         nf->enabled = false;
     }

}

Is this acceptable ?

Thanks,
Hailiang

>>>
>>>       if (nfc->setup) {
>>>           nfc->setup(nf, &local_err);
>>> diff --git a/net/net.c b/net/net.c
>>> index ec43105..9630234 100644
>>> --- a/net/net.c
>>> +++ b/net/net.c
>>> @@ -76,6 +76,12 @@ const char *host_net_devices[] = {
>>>
>>>   int default_net = 1;
>>>
>>> +/*
>>> + * FIXME: Export this with an option for users to control
>>> + * this with comand line ?
>>
>> This could be done in the future.
>>
>
> Should i change the tag to 'TODO' ?
>
>>> + */
>>> +int default_netfilter = NETFILTER_ID_BUFFER;
>>
>> Why not just use a string here?
>>
>
> No special, i will convert it to use string here.
>
>>> +
>>>   /***********************************************************/
>>>   /* network device redirectors */
>>>
>>> @@ -1032,6 +1038,18 @@ static int net_client_init1(const void *object, int is_netdev, Error **errp)
>>>           }
>>>           return -1;
>>>       }
>>> +
>>> +    if (is_netdev) {
>>> +        const Netdev *netdev = object;
>>> +        /*
>>> +        * Here we add each netdev a default filter whose name is 'nop',
>>> +        * it will disabled by default, Users can enable it when necessary.
>>> +        */
>>
>> If we support default properties, the above comment could be removed.
>>
>>> +        netdev_add_filter(netdev->id,
>>> +                          netfilter_type_lookup[default_netfilter],
>>> +                          DEFAULT_FILTER_NAME,
>>
>> I believe some logic to generate id automatically is needed here.
>>
>
> Yes, you are right, here this patch will break QEMU with multi-NICs configured,
> the error report is " attempt to add duplicate property 'nop' to object".
> I will fix it in next version.
>
> Thanks,
> Hailiang
>
>>> +                          errp);
>>> +    }
>>>       return 0;
>>>   }
>>>
>>
>>
>> .
>>
>

Re: [Qemu-devel] [PATCH RFC 1/7] net/filter: Fix the output information for command 'info network'

[Jason Wang]

On 01/25/2016 01:58 PM, Hailiang Zhang wrote:
> On 2016/1/25 13:01, Jason Wang wrote:
>>
>>
>> On 01/22/2016 04:36 PM, zhanghailiang wrote:
>>> The properties of netfilter object could be changed by 'qom-set'
>>> command, but the output of 'info network' command is not updated,
>>> because it got the old information through nf->info_str, it will
>>> not be updated while we change the value of netfilter's property.
>>>
>>> Here we split a the helper function that could colletct the output
>>> information for filter, and also remove the useless member
>>> 'info_str' from struct NetFilterState.
>>>
>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>> ---
>>
>> Looks like a bug fix. Please send as a independent formal patch and
>> better cc Markus and Eric for better reviewing.
>>
>
> OK, by the way, i didn't see there was a corresponding qmp command
> for 'info network', is the hmp_info_network just an experimental
> command ? 

It's for debugging I think , and if you want it to be used by
management, probably need a qmp version.

Thanks

Re: [Qemu-devel] [PATCH RFC 6/7] net/filter: Add a default filter to each netdev

[Jason Wang]

On 01/25/2016 03:22 PM, Hailiang Zhang wrote:
> On 2016/1/25 13:18, Jason Wang wrote:
>>
>>
>> On 01/22/2016 04:36 PM, zhanghailiang wrote:
>>> We add each netdev a default buffer filter, which the name is
>>> 'nop', and the default buffer filter is disabled, so it has
>>> no side effect for packets delivering in qemu net layer.
>>>
>>> The default buffer filter can be used by COLO or Micro-checkpoint,
>>> The reason we add the default filter is we hope to support
>>> hot add network during COLO state in future.
>>>
>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>> ---
>>>   include/net/filter.h | 11 +++++++++++
>>>   net/dump.c           |  2 --
>>>   net/filter.c         | 15 ++++++++++++++-
>>>   net/net.c            | 18 ++++++++++++++++++
>>>   4 files changed, 43 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/include/net/filter.h b/include/net/filter.h
>>> index c7bd8f9..2043609 100644
>>> --- a/include/net/filter.h
>>> +++ b/include/net/filter.h
>>> @@ -22,6 +22,16 @@
>>>   #define NETFILTER_CLASS(klass) \
>>>       OBJECT_CLASS_CHECK(NetFilterClass, (klass), TYPE_NETFILTER)
>>>

[...]

>>>
>>>       nf->netdev = ncs[0];
>>> +    nf->is_default = !strcmp(path, DEFAULT_FILTER_NAME);
>>> +    /*
>>> +    * For the default buffer filter, it will be disabled by default,
>>> +    * So it will not buffer any packets.
>>> +    */
>>> +    if (nf->is_default) {
>>> +        nf->enabled = false;
>>> +    }
>>
>> This seems not very elegant. Besides DEFAULT_FILTER_NAME(TYPE), we may
>> also want a DEFAULT_FILTER_PROPERTIES? Then you can store the "status"
>> into properties.
>>
>
> A little confused, do you mean add a 'default' property for filter ?
> Just like the new 'status' property which is exported to users ?
> Is the type of 'default' property string or bool ?

For example, is it possible to store the default property into a string
and just create the filter through qemu_opts_parse_noisily() by just
pass a string as its parameter? (Of course, it needs some code to
generate ids). With this, there's even no need for you to duplicate
codes like what patch 4 does.

>
>>>
>>>       if (nfc->setup) {
>>>           nfc->setup(nf, &local_err);
>>> diff --git a/net/net.c b/net/net.c
>>> index ec43105..9630234 100644
>>> --- a/net/net.c
>>> +++ b/net/net.c
>>> @@ -76,6 +76,12 @@ const char *host_net_devices[] = {
>>>
>>>   int default_net = 1;
>>>
>>> +/*
>>> + * FIXME: Export this with an option for users to control
>>> + * this with comand line ?
>>
>> This could be done in the future.
>>
>
> Should i change the tag to 'TODO' ?

Ok.

>
>>> + */
>>> +int default_netfilter = NETFILTER_ID_BUFFER;
>>
>> Why not just use a string here?
>>
>
> No special, i will convert it to use string here.
>
>>> +
>>>   /***********************************************************/
>>>   /* network device redirectors */
>>>
>>> @@ -1032,6 +1038,18 @@ static int net_client_init1(const void
>>> *object, int is_netdev, Error **errp)
>>>           }
>>>           return -1;
>>>       }
>>> +
>>> +    if (is_netdev) {
>>> +        const Netdev *netdev = object;
>>> +        /*
>>> +        * Here we add each netdev a default filter whose name is
>>> 'nop',
>>> +        * it will disabled by default, Users can enable it when
>>> necessary.
>>> +        */
>>
>> If we support default properties, the above comment could be removed.
>>
>>> +        netdev_add_filter(netdev->id,
>>> +                          netfilter_type_lookup[default_netfilter],
>>> +                          DEFAULT_FILTER_NAME,
>>
>> I believe some logic to generate id automatically is needed here.
>>
>
> Yes, you are right, here this patch will break QEMU with multi-NICs
> configured,
> the error report is " attempt to add duplicate property 'nop' to object".
> I will fix it in next version.
>
> Thanks,
> Hailiang
>
>>> +                          errp);
>>> +    }
>>>       return 0;
>>>   }
>>>
>>
>>
>> .
>>
>
>

Re: [Qemu-devel] [PATCH RFC 1/7] net/filter: Fix the output information for command 'info network'

[Hailiang Zhang]

On 2016/1/26 11:05, Jason Wang wrote:
>
>
> On 01/25/2016 01:58 PM, Hailiang Zhang wrote:
>> On 2016/1/25 13:01, Jason Wang wrote:
>>>
>>>
>>> On 01/22/2016 04:36 PM, zhanghailiang wrote:
>>>> The properties of netfilter object could be changed by 'qom-set'
>>>> command, but the output of 'info network' command is not updated,
>>>> because it got the old information through nf->info_str, it will
>>>> not be updated while we change the value of netfilter's property.
>>>>
>>>> Here we split a the helper function that could colletct the output
>>>> information for filter, and also remove the useless member
>>>> 'info_str' from struct NetFilterState.
>>>>
>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>> ---
>>>
>>> Looks like a bug fix. Please send as a independent formal patch and
>>> better cc Markus and Eric for better reviewing.
>>>
>>
>> OK, by the way, i didn't see there was a corresponding qmp command
>> for 'info network', is the hmp_info_network just an experimental
>> command ?
>
> It's for debugging I think , and if you want it to be used by
> management, probably need a qmp version.

Got it, the management does not need it for now, thanks.

> .
>

Re: [Qemu-devel] [PATCH RFC 6/7] net/filter: Add a default filter to each netdev

[Hailiang Zhang]

On 2016/1/26 11:18, Jason Wang wrote:
>
>
> On 01/25/2016 03:22 PM, Hailiang Zhang wrote:
>> On 2016/1/25 13:18, Jason Wang wrote:
>>>
>>>
>>> On 01/22/2016 04:36 PM, zhanghailiang wrote:
>>>> We add each netdev a default buffer filter, which the name is
>>>> 'nop', and the default buffer filter is disabled, so it has
>>>> no side effect for packets delivering in qemu net layer.
>>>>
>>>> The default buffer filter can be used by COLO or Micro-checkpoint,
>>>> The reason we add the default filter is we hope to support
>>>> hot add network during COLO state in future.
>>>>
>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>> ---
>>>>    include/net/filter.h | 11 +++++++++++
>>>>    net/dump.c           |  2 --
>>>>    net/filter.c         | 15 ++++++++++++++-
>>>>    net/net.c            | 18 ++++++++++++++++++
>>>>    4 files changed, 43 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/include/net/filter.h b/include/net/filter.h
>>>> index c7bd8f9..2043609 100644
>>>> --- a/include/net/filter.h
>>>> +++ b/include/net/filter.h
>>>> @@ -22,6 +22,16 @@
>>>>    #define NETFILTER_CLASS(klass) \
>>>>        OBJECT_CLASS_CHECK(NetFilterClass, (klass), TYPE_NETFILTER)
>>>>
>
> [...]
>
>>>>
>>>>        nf->netdev = ncs[0];
>>>> +    nf->is_default = !strcmp(path, DEFAULT_FILTER_NAME);
>>>> +    /*
>>>> +    * For the default buffer filter, it will be disabled by default,
>>>> +    * So it will not buffer any packets.
>>>> +    */
>>>> +    if (nf->is_default) {
>>>> +        nf->enabled = false;
>>>> +    }
>>>
>>> This seems not very elegant. Besides DEFAULT_FILTER_NAME(TYPE), we may
>>> also want a DEFAULT_FILTER_PROPERTIES? Then you can store the "status"
>>> into properties.
>>>
>>
>> A little confused, do you mean add a 'default' property for filter ?
>> Just like the new 'status' property which is exported to users ?
>> Is the type of 'default' property string or bool ?
>
> For example, is it possible to store the default property into a string
> and just create the filter through qemu_opts_parse_noisily() by just
> pass a string as its parameter? (Of course, it needs some code to
> generate ids). With this, there's even no need for you to duplicate
> codes like what patch 4 does.
>

Er, got it, that seems to be a cleverer method, i will look at it,
thanks very much for your patient explanation.

>>
>>>>
>>>>        if (nfc->setup) {
>>>>            nfc->setup(nf, &local_err);
>>>> diff --git a/net/net.c b/net/net.c
>>>> index ec43105..9630234 100644
>>>> --- a/net/net.c
>>>> +++ b/net/net.c
>>>> @@ -76,6 +76,12 @@ const char *host_net_devices[] = {
>>>>
>>>>    int default_net = 1;
>>>>
>>>> +/*
>>>> + * FIXME: Export this with an option for users to control
>>>> + * this with comand line ?
>>>
>>> This could be done in the future.
>>>
>>
>> Should i change the tag to 'TODO' ?
>
> Ok.
>

>>
>>>> + */
>>>> +int default_netfilter = NETFILTER_ID_BUFFER;
>>>
>>> Why not just use a string here?
>>>
>>
>> No special, i will convert it to use string here.
>>
>>>> +
>>>>    /***********************************************************/
>>>>    /* network device redirectors */
>>>>
>>>> @@ -1032,6 +1038,18 @@ static int net_client_init1(const void
>>>> *object, int is_netdev, Error **errp)
>>>>            }
>>>>            return -1;
>>>>        }
>>>> +
>>>> +    if (is_netdev) {
>>>> +        const Netdev *netdev = object;
>>>> +        /*
>>>> +        * Here we add each netdev a default filter whose name is
>>>> 'nop',
>>>> +        * it will disabled by default, Users can enable it when
>>>> necessary.
>>>> +        */
>>>
>>> If we support default properties, the above comment could be removed.
>>>
>>>> +        netdev_add_filter(netdev->id,
>>>> +                          netfilter_type_lookup[default_netfilter],
>>>> +                          DEFAULT_FILTER_NAME,
>>>
>>> I believe some logic to generate id automatically is needed here.
>>>
>>
>> Yes, you are right, here this patch will break QEMU with multi-NICs
>> configured,
>> the error report is " attempt to add duplicate property 'nop' to object".
>> I will fix it in next version.
>>
>> Thanks,
>> Hailiang
>>
>>>> +                          errp);
>>>> +    }
>>>>        return 0;
>>>>    }
>>>>
>>>
>>>
>>> .
>>>
>>
>>
>
>
> .
>

Re: [Qemu-devel] [PATCH RFC 6/7] net/filter: Add a default filter to each netdev

[Hailiang Zhang]

On 2016/1/26 11:18, Jason Wang wrote:
>
>
> On 01/25/2016 03:22 PM, Hailiang Zhang wrote:
>> On 2016/1/25 13:18, Jason Wang wrote:
>>>
>>>
>>> On 01/22/2016 04:36 PM, zhanghailiang wrote:
>>>> We add each netdev a default buffer filter, which the name is
>>>> 'nop', and the default buffer filter is disabled, so it has
>>>> no side effect for packets delivering in qemu net layer.
>>>>
>>>> The default buffer filter can be used by COLO or Micro-checkpoint,
>>>> The reason we add the default filter is we hope to support
>>>> hot add network during COLO state in future.
>>>>
>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>> ---
>>>>    include/net/filter.h | 11 +++++++++++
>>>>    net/dump.c           |  2 --
>>>>    net/filter.c         | 15 ++++++++++++++-
>>>>    net/net.c            | 18 ++++++++++++++++++
>>>>    4 files changed, 43 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/include/net/filter.h b/include/net/filter.h
>>>> index c7bd8f9..2043609 100644
>>>> --- a/include/net/filter.h
>>>> +++ b/include/net/filter.h
>>>> @@ -22,6 +22,16 @@
>>>>    #define NETFILTER_CLASS(klass) \
>>>>        OBJECT_CLASS_CHECK(NetFilterClass, (klass), TYPE_NETFILTER)
>>>>
>
> [...]
>
>>>>
>>>>        nf->netdev = ncs[0];
>>>> +    nf->is_default = !strcmp(path, DEFAULT_FILTER_NAME);
>>>> +    /*
>>>> +    * For the default buffer filter, it will be disabled by default,
>>>> +    * So it will not buffer any packets.
>>>> +    */
>>>> +    if (nf->is_default) {
>>>> +        nf->enabled = false;
>>>> +    }
>>>
>>> This seems not very elegant. Besides DEFAULT_FILTER_NAME(TYPE), we may
>>> also want a DEFAULT_FILTER_PROPERTIES? Then you can store the "status"
>>> into properties.
>>>
>>
>> A little confused, do you mean add a 'default' property for filter ?
>> Just like the new 'status' property which is exported to users ?
>> Is the type of 'default' property string or bool ?
>
> For example, is it possible to store the default property into a string
> and just create the filter through qemu_opts_parse_noisily() by just

We still need to use some *visit* helpers to realize the capability,
because the object_add() helper need a 'Visitor *v' parameter, and the codes
will be like:
QemuOptsList qemu_filter_opts = {
     .name = "default-filter",
     .head = QTAILQ_HEAD_INITIALIZER(qemu_filter_opts.head),
     .desc = {
         {
             .name = "netdev",
             .type = QEMU_OPT_STRING,
         },{
             .name = "status",
             .type = QEMU_OPT_STRING,
         },
         { /* end of list */ }
     },
};
void netdev_add_filter(const char *netdev_id,
                        const char *filter_type,
                        const char *id,
                        bool is_default,
                        Error **errp)
{
    sprintf(optarg, "netdev=%s,status=%s", netdev_id,
                     is_default ? "disable" : "enable");
     opts = qemu_opts_parse_noisily(&qemu_filter_opts,
                                    optarg, false);
     if (!opts) {
         error_report("Failed to parse param '%s'", optarg);
         exit(1);
     }

     qdict = qemu_opts_to_qdict(opts, NULL);
     ov = opts_visitor_new(opts);
     visit_start_struct(opts_get_visitor(ov), &dummy, NULL, NULL, 0, &err);
     if (err) {
         goto out_clean;
     }
     object_add(filter_type, id, qdict, opts_get_visitor(ov), &err);
     if (err) {
         goto out_clean;
     }

     visit_end_struct(opts_get_visitor(ov), &err);
     if (err) {
         qmp_object_del(id, NULL);
         goto out_clean;
     }

}

Or, we can simplify patch 4 by using qmp_object_add(), codes will be like:

void netdev_add_filter(const char *netdev_id,
                        const char *filter_type,
                        const char *id,
                        bool is_default,
                        Error **errp)
{
     ... ...

     qov = qmp_output_visitor_new();
     ov = qmp_output_get_visitor(qov);
     visit_start_struct(ov,  &dummy, NULL, NULL, 0, &err);
     if (err) {
         goto out;
     }
     visit_type_str(ov, &nc->name, "netdev", &err);
     if (err) {
         goto out;
     }
     status = is_default ? g_strdup("disable") : g_strdup("enable");
     visit_type_str(ov, &status, "status", &err);
     g_free(status);
     if (err) {
         goto out;
     }
     visit_end_struct(ov, &err);
     if (err) {
         goto out;
     }
     obj = qmp_output_get_qobject(qov);
     g_assert(obj != NULL);
     qmp_object_add(filter_type, id, true, obj, &err);
     qmp_output_visitor_cleanup(qov);
     qobject_decref(obj);

}

what's your suggestion ? :)

Thanks,
Hailiang

> pass a string as its parameter? (Of course, it needs some code to
> generate ids). With this, there's even no need for you to duplicate
> codes like what patch 4 does.
>
>>
>>>>
>>>>        if (nfc->setup) {
>>>>            nfc->setup(nf, &local_err);
>>>> diff --git a/net/net.c b/net/net.c
>>>> index ec43105..9630234 100644
>>>> --- a/net/net.c
>>>> +++ b/net/net.c
>>>> @@ -76,6 +76,12 @@ const char *host_net_devices[] = {
>>>>
>>>>    int default_net = 1;
>>>>
>>>> +/*
>>>> + * FIXME: Export this with an option for users to control
>>>> + * this with comand line ?
>>>
>>> This could be done in the future.
>>>
>>
>> Should i change the tag to 'TODO' ?
>
> Ok.
>
>>
>>>> + */
>>>> +int default_netfilter = NETFILTER_ID_BUFFER;
>>>
>>> Why not just use a string here?
>>>
>>
>> No special, i will convert it to use string here.
>>
>>>> +
>>>>    /***********************************************************/
>>>>    /* network device redirectors */
>>>>
>>>> @@ -1032,6 +1038,18 @@ static int net_client_init1(const void
>>>> *object, int is_netdev, Error **errp)
>>>>            }
>>>>            return -1;
>>>>        }
>>>> +
>>>> +    if (is_netdev) {
>>>> +        const Netdev *netdev = object;
>>>> +        /*
>>>> +        * Here we add each netdev a default filter whose name is
>>>> 'nop',
>>>> +        * it will disabled by default, Users can enable it when
>>>> necessary.
>>>> +        */
>>>
>>> If we support default properties, the above comment could be removed.
>>>
>>>> +        netdev_add_filter(netdev->id,
>>>> +                          netfilter_type_lookup[default_netfilter],
>>>> +                          DEFAULT_FILTER_NAME,
>>>
>>> I believe some logic to generate id automatically is needed here.
>>>
>>
>> Yes, you are right, here this patch will break QEMU with multi-NICs
>> configured,
>> the error report is " attempt to add duplicate property 'nop' to object".
>> I will fix it in next version.
>>
>> Thanks,
>> Hailiang
>>
>>>> +                          errp);
>>>> +    }
>>>>        return 0;
>>>>    }
>>>>
>>>
>>>
>>> .
>>>
>>
>>
>
>
> .
>

Re: [Qemu-devel] [PATCH RFC 6/7] net/filter: Add a default filter to each netdev

[Jason Wang]

On 01/25/2016 07:19 PM, Hailiang Zhang wrote:
> On 2016/1/25 15:22, Hailiang Zhang wrote:
>> On 2016/1/25 13:18, Jason Wang wrote:
>>>
>>>
>>> On 01/22/2016 04:36 PM, zhanghailiang wrote:
>>>> We add each netdev a default buffer filter, which the name is
>>>> 'nop', and the default buffer filter is disabled, so it has
>>>> no side effect for packets delivering in qemu net layer.
>>>>
>>>> The default buffer filter can be used by COLO or Micro-checkpoint,
>>>> The reason we add the default filter is we hope to support
>>>> hot add network during COLO state in future.
>>>>
>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>> ---
>>>>   include/net/filter.h | 11 +++++++++++
>>>>   net/dump.c           |  2 --
>>>>   net/filter.c         | 15 ++++++++++++++-
>>>>   net/net.c            | 18 ++++++++++++++++++
>>>>   4 files changed, 43 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/include/net/filter.h b/include/net/filter.h
>>>> index c7bd8f9..2043609 100644
>>>> --- a/include/net/filter.h
>>>> +++ b/include/net/filter.h
>>>> @@ -22,6 +22,16 @@
>>>>   #define NETFILTER_CLASS(klass) \
>>>>       OBJECT_CLASS_CHECK(NetFilterClass, (klass), TYPE_NETFILTER)
>>>>
>>>> +#define DEFAULT_FILTER_NAME "nop"
>>>
>>> Maybe DEFAULT_FILTER_TYPE?
>>>
>>
>>>> +
>>>> +#define TYPE_FILTER_BUFFER "filter-buffer"
>>>> +#define TYPE_FILTER_DUMP "filter-dump"
>>>> +
>>>> +#define NETFILTER_ID_BUFFER 1
>>>> +#define NETFILTER_ID_DUMP 2
>>>> +
>>>> +extern const char *const netfilter_type_lookup[];
>>>> +
>>>>   typedef void (FilterSetup) (NetFilterState *nf, Error **errp);
>>>>   typedef void (FilterCleanup) (NetFilterState *nf);
>>>>   /*
>>>> @@ -55,6 +65,7 @@ struct NetFilterState {
>>>>       char *netdev_id;
>>>>       NetClientState *netdev;
>>>>       NetFilterDirection direction;
>>>> +    bool is_default;
>>>>       bool enabled;
>>>>       QTAILQ_ENTRY(NetFilterState) next;
>>>>   };
>>>> diff --git a/net/dump.c b/net/dump.c
>>>> index 88d9582..82727a6 100644
>>>> --- a/net/dump.c
>>>> +++ b/net/dump.c
>>>> @@ -229,8 +229,6 @@ int net_init_dump(const NetClientOptions *opts,
>>>> const char *name,
>>>>
>>>>   /* Dumping via filter */
>>>>
>>>> -#define TYPE_FILTER_DUMP "filter-dump"
>>>> -
>>>>   #define FILTER_DUMP(obj) \
>>>>       OBJECT_CHECK(NetFilterDumpState, (obj), TYPE_FILTER_DUMP)
>>>>
>>>> diff --git a/net/filter.c b/net/filter.c
>>>> index 4d96301..a126a3b 100644
>>>> --- a/net/filter.c
>>>> +++ b/net/filter.c
>>>> @@ -21,6 +21,11 @@
>>>>   #include "qapi/qmp-input-visitor.h"
>>>>   #include "monitor/monitor.h"
>>>>
>>>> +const char *const netfilter_type_lookup[] = {
>>>> +    [NETFILTER_ID_BUFFER] = TYPE_FILTER_BUFFER,
>>>> +    [NETFILTER_ID_DUMP] = TYPE_FILTER_DUMP,
>>>> +};
>>>> +
>>>>   ssize_t qemu_netfilter_receive(NetFilterState *nf,
>>>>                                  NetFilterDirection direction,
>>>>                                  NetClientState *sender,
>>>> @@ -200,7 +205,7 @@ static void netfilter_complete(UserCreatable
>>>> *uc, Error **errp)
>>>>       NetFilterClass *nfc = NETFILTER_GET_CLASS(uc);
>>>>       int queues;
>>>>       Error *local_err = NULL;
>>>> -
>>>> +    char *path = object_get_canonical_path_component(OBJECT(nf));
>>>>
>>>>       if (!nf->netdev_id) {
>>>>           error_setg(errp, "Parameter 'netdev' is required");
>>>> @@ -225,6 +230,14 @@ static void netfilter_complete(UserCreatable
>>>> *uc, Error **errp)
>>>>       }
>>>>
>>>>       nf->netdev = ncs[0];
>>>> +    nf->is_default = !strcmp(path, DEFAULT_FILTER_NAME);
>>>> +    /*
>>>> +    * For the default buffer filter, it will be disabled by default,
>>>> +    * So it will not buffer any packets.
>>>> +    */
>>>> +    if (nf->is_default) {
>>>> +        nf->enabled = false;
>>>> +    }
>>>
>>> This seems not very elegant. Besides DEFAULT_FILTER_NAME(TYPE), we may
>>> also want a DEFAULT_FILTER_PROPERTIES? Then you can store the "status"
>>> into properties.
>>>
>>
>> A little confused, do you mean add a 'default' property for filter ?
>> Just like the new 'status' property which is exported to users ?
>> Is the type of 'default' property string or bool ?
>>
>
> I don't think it is necessary to add a 'default' property, because
> we don't want such a property to be controlled by user. It is
> only used internally.

If it was only used internally, then just not export the ability to
configure default filter and its properties to user.

> We have another choice to deal with this,
> Add a 'bool is_default' parameter for netdev_add_filter(),
> and handle the default filter in it, just like:

This does not scale consider if somebody want to add more common
properties to netfilters.

>
> void netdev_add_filter(const char *netdev_id,
>                        const char *filter_type,
>                        const char *id,
>                        bool is_default,
>                        Error **errp)
> {
>     ... ...
>     object_add(filter_type, id, qdict, iv, &err);
>     .... ...
>     if (is_default) {
>         Object *obj, *container;
>         NetFilterState *nf;
>
>         container = object_get_objects_root();
>         obj = object_resolve_path_component(container, id);
>         if (!obj) {
>             error_setg(errp, "object id not found");
>             return;
>         }
>         nf = NETFILTER(obj);
>         nf->is_default = true;
>         nf->enabled = false;
>     }
>
> }
>
> Is this acceptable ?
>
> Thanks,
> Hailiang
>
>>>>
>>>>       if (nfc->setup) {
>>>>           nfc->setup(nf, &local_err);
>>>> diff --git a/net/net.c b/net/net.c
>>>> index ec43105..9630234 100644
>>>> --- a/net/net.c
>>>> +++ b/net/net.c
>>>> @@ -76,6 +76,12 @@ const char *host_net_devices[] = {
>>>>
>>>>   int default_net = 1;
>>>>
>>>> +/*
>>>> + * FIXME: Export this with an option for users to control
>>>> + * this with comand line ?
>>>
>>> This could be done in the future.
>>>
>>
>> Should i change the tag to 'TODO' ?
>>
>>>> + */
>>>> +int default_netfilter = NETFILTER_ID_BUFFER;
>>>
>>> Why not just use a string here?
>>>
>>
>> No special, i will convert it to use string here.
>>
>>>> +
>>>>   /***********************************************************/
>>>>   /* network device redirectors */
>>>>
>>>> @@ -1032,6 +1038,18 @@ static int net_client_init1(const void
>>>> *object, int is_netdev, Error **errp)
>>>>           }
>>>>           return -1;
>>>>       }
>>>> +
>>>> +    if (is_netdev) {
>>>> +        const Netdev *netdev = object;
>>>> +        /*
>>>> +        * Here we add each netdev a default filter whose name is
>>>> 'nop',
>>>> +        * it will disabled by default, Users can enable it when
>>>> necessary.
>>>> +        */
>>>
>>> If we support default properties, the above comment could be removed.
>>>
>>>> +        netdev_add_filter(netdev->id,
>>>> +                          netfilter_type_lookup[default_netfilter],
>>>> +                          DEFAULT_FILTER_NAME,
>>>
>>> I believe some logic to generate id automatically is needed here.
>>>
>>
>> Yes, you are right, here this patch will break QEMU with multi-NICs
>> configured,
>> the error report is " attempt to add duplicate property 'nop' to
>> object".
>> I will fix it in next version.
>>
>> Thanks,
>> Hailiang
>>
>>>> +                          errp);
>>>> +    }
>>>>       return 0;
>>>>   }
>>>>
>>>
>>>
>>> .
>>>
>>
>
>
>

Re: [Qemu-devel] [PATCH RFC 6/7] net/filter: Add a default filter to each netdev

[Jason Wang]

On 01/27/2016 08:37 AM, Hailiang Zhang wrote:
> On 2016/1/26 11:18, Jason Wang wrote:
>>
>>
>> On 01/25/2016 03:22 PM, Hailiang Zhang wrote:
>>> On 2016/1/25 13:18, Jason Wang wrote:
>>>>
>>>>
>>>> On 01/22/2016 04:36 PM, zhanghailiang wrote:
>>>>> We add each netdev a default buffer filter, which the name is
>>>>> 'nop', and the default buffer filter is disabled, so it has
>>>>> no side effect for packets delivering in qemu net layer.
>>>>>
>>>>> The default buffer filter can be used by COLO or Micro-checkpoint,
>>>>> The reason we add the default filter is we hope to support
>>>>> hot add network during COLO state in future.
>>>>>
>>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>>> ---
>>>>>    include/net/filter.h | 11 +++++++++++
>>>>>    net/dump.c           |  2 --
>>>>>    net/filter.c         | 15 ++++++++++++++-
>>>>>    net/net.c            | 18 ++++++++++++++++++
>>>>>    4 files changed, 43 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/include/net/filter.h b/include/net/filter.h
>>>>> index c7bd8f9..2043609 100644
>>>>> --- a/include/net/filter.h
>>>>> +++ b/include/net/filter.h
>>>>> @@ -22,6 +22,16 @@
>>>>>    #define NETFILTER_CLASS(klass) \
>>>>>        OBJECT_CLASS_CHECK(NetFilterClass, (klass), TYPE_NETFILTER)
>>>>>
>>
>> [...]
>>
>>>>>
>>>>>        nf->netdev = ncs[0];
>>>>> +    nf->is_default = !strcmp(path, DEFAULT_FILTER_NAME);
>>>>> +    /*
>>>>> +    * For the default buffer filter, it will be disabled by default,
>>>>> +    * So it will not buffer any packets.
>>>>> +    */
>>>>> +    if (nf->is_default) {
>>>>> +        nf->enabled = false;
>>>>> +    }
>>>>
>>>> This seems not very elegant. Besides DEFAULT_FILTER_NAME(TYPE), we may
>>>> also want a DEFAULT_FILTER_PROPERTIES? Then you can store the "status"
>>>> into properties.
>>>>
>>>
>>> A little confused, do you mean add a 'default' property for filter ?
>>> Just like the new 'status' property which is exported to users ?
>>> Is the type of 'default' property string or bool ?
>>
>> For example, is it possible to store the default property into a string
>> and just create the filter through qemu_opts_parse_noisily() by just
>
> We still need to use some *visit* helpers to realize the capability,
> because the object_add() helper need a 'Visitor *v' parameter, and the
> codes
> will be like:
> QemuOptsList qemu_filter_opts = {
>     .name = "default-filter",
>     .head = QTAILQ_HEAD_INITIALIZER(qemu_filter_opts.head),
>     .desc = {
>         {
>             .name = "netdev",
>             .type = QEMU_OPT_STRING,
>         },{
>             .name = "status",
>             .type = QEMU_OPT_STRING,
>         },
>         { /* end of list */ }
>     },
> };
> void netdev_add_filter(const char *netdev_id,
>                        const char *filter_type,
>                        const char *id,
>                        bool is_default,
>                        Error **errp)
> {
>    sprintf(optarg, "netdev=%s,status=%s", netdev_id,
>                     is_default ? "disable" : "enable");
>     opts = qemu_opts_parse_noisily(&qemu_filter_opts,
>                                    optarg, false);
>     if (!opts) {
>         error_report("Failed to parse param '%s'", optarg);
>         exit(1);
>     }
>
>     qdict = qemu_opts_to_qdict(opts, NULL);
>     ov = opts_visitor_new(opts);
>     visit_start_struct(opts_get_visitor(ov), &dummy, NULL, NULL, 0,
> &err);
>     if (err) {
>         goto out_clean;
>     }
>     object_add(filter_type, id, qdict, opts_get_visitor(ov), &err);
>     if (err) {
>         goto out_clean;
>     }
>
>     visit_end_struct(opts_get_visitor(ov), &err);
>     if (err) {
>         qmp_object_del(id, NULL);
>         goto out_clean;
>     }
>
> }
>
> Or, we can simplify patch 4 by using qmp_object_add(), codes will be
> like:
>
> void netdev_add_filter(const char *netdev_id,
>                        const char *filter_type,
>                        const char *id,
>                        bool is_default,
>                        Error **errp)
> {
>     ... ...
>
>     qov = qmp_output_visitor_new();
>     ov = qmp_output_get_visitor(qov);
>     visit_start_struct(ov,  &dummy, NULL, NULL, 0, &err);
>     if (err) {
>         goto out;
>     }
>     visit_type_str(ov, &nc->name, "netdev", &err);
>     if (err) {
>         goto out;
>     }
>     status = is_default ? g_strdup("disable") : g_strdup("enable");
>     visit_type_str(ov, &status, "status", &err);
>     g_free(status);
>     if (err) {
>         goto out;
>     }
>     visit_end_struct(ov, &err);
>     if (err) {
>         goto out;
>     }
>     obj = qmp_output_get_qobject(qov);
>     g_assert(obj != NULL);
>     qmp_object_add(filter_type, id, true, obj, &err);
>     qmp_output_visitor_cleanup(qov);
>     qobject_decref(obj);
>
> }
>
> what's your suggestion ? :)
>

Can we just reuse object_create()? here

> Thanks,
> Hailiang 

Re: [Qemu-devel] [PATCH RFC 6/7] net/filter: Add a default filter to each netdev

[Hailiang Zhang]

On 2016/1/27 13:59, Jason Wang wrote:
>
>
> On 01/27/2016 08:37 AM, Hailiang Zhang wrote:
>> On 2016/1/26 11:18, Jason Wang wrote:
>>>
>>>
>>> On 01/25/2016 03:22 PM, Hailiang Zhang wrote:
>>>> On 2016/1/25 13:18, Jason Wang wrote:
>>>>>
>>>>>
>>>>> On 01/22/2016 04:36 PM, zhanghailiang wrote:
>>>>>> We add each netdev a default buffer filter, which the name is
>>>>>> 'nop', and the default buffer filter is disabled, so it has
>>>>>> no side effect for packets delivering in qemu net layer.
>>>>>>
>>>>>> The default buffer filter can be used by COLO or Micro-checkpoint,
>>>>>> The reason we add the default filter is we hope to support
>>>>>> hot add network during COLO state in future.
>>>>>>
>>>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>>>> ---
>>>>>>     include/net/filter.h | 11 +++++++++++
>>>>>>     net/dump.c           |  2 --
>>>>>>     net/filter.c         | 15 ++++++++++++++-
>>>>>>     net/net.c            | 18 ++++++++++++++++++
>>>>>>     4 files changed, 43 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/include/net/filter.h b/include/net/filter.h
>>>>>> index c7bd8f9..2043609 100644
>>>>>> --- a/include/net/filter.h
>>>>>> +++ b/include/net/filter.h
>>>>>> @@ -22,6 +22,16 @@
>>>>>>     #define NETFILTER_CLASS(klass) \
>>>>>>         OBJECT_CLASS_CHECK(NetFilterClass, (klass), TYPE_NETFILTER)
>>>>>>
>>>
>>> [...]
>>>
>>>>>>
>>>>>>         nf->netdev = ncs[0];
>>>>>> +    nf->is_default = !strcmp(path, DEFAULT_FILTER_NAME);
>>>>>> +    /*
>>>>>> +    * For the default buffer filter, it will be disabled by default,
>>>>>> +    * So it will not buffer any packets.
>>>>>> +    */
>>>>>> +    if (nf->is_default) {
>>>>>> +        nf->enabled = false;
>>>>>> +    }
>>>>>
>>>>> This seems not very elegant. Besides DEFAULT_FILTER_NAME(TYPE), we may
>>>>> also want a DEFAULT_FILTER_PROPERTIES? Then you can store the "status"
>>>>> into properties.
>>>>>
>>>>
>>>> A little confused, do you mean add a 'default' property for filter ?
>>>> Just like the new 'status' property which is exported to users ?
>>>> Is the type of 'default' property string or bool ?
>>>
>>> For example, is it possible to store the default property into a string
>>> and just create the filter through qemu_opts_parse_noisily() by just
>>
>> We still need to use some *visit* helpers to realize the capability,
>> because the object_add() helper need a 'Visitor *v' parameter, and the
>> codes
>> will be like:
>> QemuOptsList qemu_filter_opts = {
>>      .name = "default-filter",
>>      .head = QTAILQ_HEAD_INITIALIZER(qemu_filter_opts.head),
>>      .desc = {
>>          {
>>              .name = "netdev",
>>              .type = QEMU_OPT_STRING,
>>          },{
>>              .name = "status",
>>              .type = QEMU_OPT_STRING,
>>          },
>>          { /* end of list */ }
>>      },
>> };
>> void netdev_add_filter(const char *netdev_id,
>>                         const char *filter_type,
>>                         const char *id,
>>                         bool is_default,
>>                         Error **errp)
>> {
>>     sprintf(optarg, "netdev=%s,status=%s", netdev_id,
>>                      is_default ? "disable" : "enable");
>>      opts = qemu_opts_parse_noisily(&qemu_filter_opts,
>>                                     optarg, false);
>>      if (!opts) {
>>          error_report("Failed to parse param '%s'", optarg);
>>          exit(1);
>>      }
>>
>>      qdict = qemu_opts_to_qdict(opts, NULL);
>>      ov = opts_visitor_new(opts);
>>      visit_start_struct(opts_get_visitor(ov), &dummy, NULL, NULL, 0,
>> &err);
>>      if (err) {
>>          goto out_clean;
>>      }
>>      object_add(filter_type, id, qdict, opts_get_visitor(ov), &err);
>>      if (err) {
>>          goto out_clean;
>>      }
>>
>>      visit_end_struct(opts_get_visitor(ov), &err);
>>      if (err) {
>>          qmp_object_del(id, NULL);
>>          goto out_clean;
>>      }
>>
>> }
>>
>> Or, we can simplify patch 4 by using qmp_object_add(), codes will be
>> like:
>>
>> void netdev_add_filter(const char *netdev_id,
>>                         const char *filter_type,
>>                         const char *id,
>>                         bool is_default,
>>                         Error **errp)
>> {
>>      ... ...
>>
>>      qov = qmp_output_visitor_new();
>>      ov = qmp_output_get_visitor(qov);
>>      visit_start_struct(ov,  &dummy, NULL, NULL, 0, &err);
>>      if (err) {
>>          goto out;
>>      }
>>      visit_type_str(ov, &nc->name, "netdev", &err);
>>      if (err) {
>>          goto out;
>>      }
>>      status = is_default ? g_strdup("disable") : g_strdup("enable");
>>      visit_type_str(ov, &status, "status", &err);
>>      g_free(status);
>>      if (err) {
>>          goto out;
>>      }
>>      visit_end_struct(ov, &err);
>>      if (err) {
>>          goto out;
>>      }
>>      obj = qmp_output_get_qobject(qov);
>>      g_assert(obj != NULL);
>>      qmp_object_add(filter_type, id, true, obj, &err);
>>      qmp_output_visitor_cleanup(qov);
>>      qobject_decref(obj);
>>
>> }
>>
>> what's your suggestion ? :)
>>
>
> Can we just reuse object_create()? here
>

Yes, the codes is more clean if we reuse it.
I will fix it like that in v2, thanks.

>> Thanks,
>> Hailiang
>
>
> .
>