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

[Qemu-devel] [PATCH RFC v2 0/5] 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.

Note: this series is based on patch 
    '[PATCH v2] net/filter: Fix the output information for command 'info network'

v2:
 - Drop the patch 'net/filter: prevent the default filter to be deleted' (Jason)
 - Re-implement netdev_add_filter() by re-using object_object() (Jason)
 - Send patch 'net/filter: Fix the output information for command 'info network'
   as an independent one. (Jason)

zhanghailiang (5):
  net/filter: Add a 'status' property for filter object
  vl: Make object_create() public
  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

 include/net/filter.h  |  12 +++++
 include/qemu-common.h |   2 +
 net/filter-buffer.c   |  10 ----
 net/filter.c          | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++
 net/net.c             |  23 ++++++++++
 vl.c                  |   4 +-
 6 files changed, 164 insertions(+), 12 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH RFC v2 1/5] 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.

We will skip the disabled filter when delivering packets in net layer.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
v2:
 - Squash previous patch 3 into this patch (Jason's suggestion)
---
 include/net/filter.h |  1 +
 net/filter.c         | 45 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/include/net/filter.h b/include/net/filter.h
index 5639976..af3c53c 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 5adcec8..d08a2be 100644
--- a/net/filter.c
+++ b/net/filter.c
@@ -16,6 +16,11 @@
 #include "qom/object_interfaces.h"
 #include "qemu/iov.h"
 
+static inline bool qemu_need_skip_netfilter(NetFilterState *nf)
+{
+    return nf->enabled ? false : true;
+}
+
 ssize_t qemu_netfilter_receive(NetFilterState *nf,
                                NetFilterDirection direction,
                                NetClientState *sender,
@@ -24,6 +29,10 @@ ssize_t qemu_netfilter_receive(NetFilterState *nf,
                                int iovcnt,
                                NetPacketSent *sent_cb)
 {
+    /* Don't go through the filter if it is disabled */
+    if (qemu_need_skip_netfilter(nf)) {
+        return 0;
+    }
     if (nf->direction == direction ||
         nf->direction == NET_FILTER_DIRECTION_ALL) {
         return NETFILTER_GET_CLASS(OBJECT(nf))->receive_iov(
@@ -116,8 +125,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);
@@ -125,6 +167,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);
 }
 
 static void netfilter_complete(UserCreatable *uc, Error **errp)
-- 
1.8.3.1

[Qemu-devel] [PATCH RFC v2 2/5] vl: Make object_create() public

[zhanghailiang]

Make the helper object_create() public and fix its first
parameter to accept NULL value.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
---
v2:
 - New patch
---
 include/qemu-common.h | 2 ++
 vl.c                  | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/qemu-common.h b/include/qemu-common.h
index 22b010c..52cf4fd 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -500,4 +500,6 @@ int parse_debug_env(const char *name, int max, int initial);
 const char *qemu_ether_ntoa(const MACAddr *mac);
 void page_size_init(void);
 
+int object_create(void *opaque, QemuOpts *opts, Error **errp);
+
 #endif
diff --git a/vl.c b/vl.c
index f043009..b21335e 100644
--- a/vl.c
+++ b/vl.c
@@ -2819,7 +2819,7 @@ static bool object_create_delayed(const char *type)
 }
 
 
-static int object_create(void *opaque, QemuOpts *opts, Error **errp)
+int object_create(void *opaque, QemuOpts *opts, Error **errp)
 {
     Error *err = NULL;
     char *type = NULL;
@@ -2842,7 +2842,7 @@ static int object_create(void *opaque, QemuOpts *opts, Error **errp)
     if (err) {
         goto out;
     }
-    if (!type_predicate(type)) {
+    if (type_predicate && !type_predicate(type)) {
         goto out;
     }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH RFC v2 3/5] net/filter: Introduce a helper to add a filter to the netdev

[zhanghailiang]

We add a new helper function netdev_add_filter(), this function
can help adding a filter object to a netdev.
Besides, we add a is_default member for struct NetFilterState
to indicate whether the filter is default or not.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
v2:
 -Re-implement netdev_add_filter() by re-using object_create()
  (Jason's suggestion)
---
 include/net/filter.h |  7 +++++
 net/filter.c         | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 87 insertions(+)

diff --git a/include/net/filter.h b/include/net/filter.h
index af3c53c..ee1c024 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 is_default;
     bool enabled;
     QTAILQ_ENTRY(NetFilterState) next;
 };
@@ -74,4 +75,10 @@ ssize_t qemu_netfilter_pass_to_next(NetClientState *sender,
                                     int iovcnt,
                                     void *opaque);
 
+void netdev_add_filter(const char *netdev_id,
+                       const char *filter_type,
+                       const char *id,
+                       bool is_default,
+                       Error **errp);
+
 #endif /* QEMU_NET_FILTER_H */
diff --git a/net/filter.c b/net/filter.c
index d08a2be..dc7aa9b 100644
--- a/net/filter.c
+++ b/net/filter.c
@@ -214,6 +214,86 @@ static void netfilter_complete(UserCreatable *uc, Error **errp)
     QTAILQ_INSERT_TAIL(&nf->netdev->filters, nf, next);
 }
 
+QemuOptsList qemu_filter_opts = {
+    .name = "default-filter",
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_filter_opts.head),
+    .desc = {
+        {
+            .name = "qom-type",
+            .type = QEMU_OPT_STRING,
+        },{
+            .name = "id",
+            .type = QEMU_OPT_STRING,
+        },{
+            .name = "netdev",
+            .type = QEMU_OPT_STRING,
+        },{
+            .name = "status",
+            .type = QEMU_OPT_STRING,
+        },
+        { /* end of list */ }
+    },
+};
+
+static void filter_set_default_flag(const char *id,
+                                    bool is_default,
+                                    Error **errp)
+{
+    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 = is_default;
+}
+
+void netdev_add_filter(const char *netdev_id,
+                       const char *filter_type,
+                       const char *id,
+                       bool is_default,
+                       Error **errp)
+{
+    NetClientState *nc = qemu_find_netdev(netdev_id);
+    char *optarg;
+    QemuOpts *opts = NULL;
+    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;
+    }
+
+    optarg = g_strdup_printf("qom-type=%s,id=%s,netdev=%s,status=%s",
+            filter_type, id, 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);
+    }
+    g_free(optarg);
+    if (object_create(NULL, opts, &err) < 0) {
+        error_report("Failed to create object");
+        goto out_clean;
+    }
+    filter_set_default_flag(id, is_default, &err);
+
+out_clean:
+    qemu_opts_del(opts);
+    if (err) {
+        error_propagate(errp, err);
+    }
+}
+
 static void netfilter_finalize(Object *obj)
 {
     NetFilterState *nf = NETFILTER(obj);
-- 
1.8.3.1

[Qemu-devel] [PATCH RFC v2 4/5] 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 v2 5/5] net/filter: Add a default filter to each netdev

[zhanghailiang]

We add each netdev a default buffer filter, 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>
---
v2:
- Add codes that generate id automatically for default filter
  (Jason's suggestion)
- Some other minor fixes.
---
 include/net/filter.h |  4 ++++
 net/net.c            | 23 +++++++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/include/net/filter.h b/include/net/filter.h
index ee1c024..e443f9c 100644
--- a/include/net/filter.h
+++ b/include/net/filter.h
@@ -22,6 +22,10 @@
 #define NETFILTER_CLASS(klass) \
     OBJECT_CLASS_CHECK(NetFilterClass, (klass), TYPE_NETFILTER)
 
+#define DEFAULT_FILTER_TYPE "nop"
+
+#define TYPE_FILTER_BUFFER "filter-buffer"
+
 typedef void (FilterSetup) (NetFilterState *nf, Error **errp);
 typedef void (FilterCleanup) (NetFilterState *nf);
 /*
diff --git a/net/net.c b/net/net.c
index a49af48..12c13f9 100644
--- a/net/net.c
+++ b/net/net.c
@@ -77,6 +77,12 @@ const char *host_net_devices[] = {
 
 int default_net = 1;
 
+/*
+ * TODO: Export this with an option for users to control
+ * this with comand line ?
+ */
+char default_netfilter_type[16] = TYPE_FILTER_BUFFER;
+
 /***********************************************************/
 /* network device redirectors */
 
@@ -1029,6 +1035,23 @@ static int net_client_init1(const void *object, int is_netdev, Error **errp)
         }
         return -1;
     }
+
+    if (is_netdev) {
+        const Netdev *netdev = object;
+        char default_name[128];
+
+        snprintf(default_name, sizeof(default_name),
+                "%s%s0", netdev->id, DEFAULT_FILTER_TYPE);
+        /*
+        * Here we add each netdev a default filter,
+        * it will disabled by default, Users can enable it when necessary.
+        */
+        netdev_add_filter(netdev->id,
+                          default_netfilter_type,
+                          default_name,
+                          true,
+                          errp);
+    }
     return 0;
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH RFC v2 2/5] vl: Make object_create() public

[Jason Wang]

On 01/27/2016 04:29 PM, zhanghailiang wrote:
> Make the helper object_create() public and fix its first
> parameter to accept NULL value.

Looks not very nice. Maybe pass a new predicate func for sanity check it
better.

>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> ---
> v2:
>  - New patch
> ---
>  include/qemu-common.h | 2 ++
>  vl.c                  | 4 ++--
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index 22b010c..52cf4fd 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -500,4 +500,6 @@ int parse_debug_env(const char *name, int max, int initial);
>  const char *qemu_ether_ntoa(const MACAddr *mac);
>  void page_size_init(void);
>  
> +int object_create(void *opaque, QemuOpts *opts, Error **errp);
> +
>  #endif
> diff --git a/vl.c b/vl.c
> index f043009..b21335e 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2819,7 +2819,7 @@ static bool object_create_delayed(const char *type)
>  }
>  
>  
> -static int object_create(void *opaque, QemuOpts *opts, Error **errp)
> +int object_create(void *opaque, QemuOpts *opts, Error **errp)
>  {
>      Error *err = NULL;
>      char *type = NULL;
> @@ -2842,7 +2842,7 @@ static int object_create(void *opaque, QemuOpts *opts, Error **errp)
>      if (err) {
>          goto out;
>      }
> -    if (!type_predicate(type)) {
> +    if (type_predicate && !type_predicate(type)) {
>          goto out;
>      }
>  

Re: [Qemu-devel] [PATCH RFC v2 3/5] net/filter: Introduce a helper to add a filter to the netdev

[Jason Wang]

On 01/27/2016 04:29 PM, zhanghailiang wrote:
> We add a new helper function netdev_add_filter(), this function
> can help adding a filter object to a netdev.
> Besides, we add a is_default member for struct NetFilterState
> to indicate whether the filter is default or not.
>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
> v2:
>  -Re-implement netdev_add_filter() by re-using object_create()
>   (Jason's suggestion)
> ---
>  include/net/filter.h |  7 +++++
>  net/filter.c         | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 87 insertions(+)
>
> diff --git a/include/net/filter.h b/include/net/filter.h
> index af3c53c..ee1c024 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 is_default;
>      bool enabled;
>      QTAILQ_ENTRY(NetFilterState) next;
>  };
> @@ -74,4 +75,10 @@ ssize_t qemu_netfilter_pass_to_next(NetClientState *sender,
>                                      int iovcnt,
>                                      void *opaque);
>  
> +void netdev_add_filter(const char *netdev_id,
> +                       const char *filter_type,
> +                       const char *id,
> +                       bool is_default,
> +                       Error **errp);
> +
>  #endif /* QEMU_NET_FILTER_H */
> diff --git a/net/filter.c b/net/filter.c
> index d08a2be..dc7aa9b 100644
> --- a/net/filter.c
> +++ b/net/filter.c
> @@ -214,6 +214,86 @@ static void netfilter_complete(UserCreatable *uc, Error **errp)
>      QTAILQ_INSERT_TAIL(&nf->netdev->filters, nf, next);
>  }
>  
> +QemuOptsList qemu_filter_opts = {
> +    .name = "default-filter",
> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_filter_opts.head),
> +    .desc = {
> +        {
> +            .name = "qom-type",
> +            .type = QEMU_OPT_STRING,
> +        },{
> +            .name = "id",
> +            .type = QEMU_OPT_STRING,
> +        },{
> +            .name = "netdev",
> +            .type = QEMU_OPT_STRING,
> +        },{
> +            .name = "status",
> +            .type = QEMU_OPT_STRING,
> +        },
> +        { /* end of list */ }
> +    },
> +};
> +
> +static void filter_set_default_flag(const char *id,
> +                                    bool is_default,
> +                                    Error **errp)
> +{
> +    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 = is_default;
> +}
> +
> +void netdev_add_filter(const char *netdev_id,
> +                       const char *filter_type,
> +                       const char *id,
> +                       bool is_default,
> +                       Error **errp)
> +{
> +    NetClientState *nc = qemu_find_netdev(netdev_id);
> +    char *optarg;
> +    QemuOpts *opts = NULL;
> +    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;
> +    }
> +
> +    optarg = g_strdup_printf("qom-type=%s,id=%s,netdev=%s,status=%s",
> +            filter_type, id, netdev_id, is_default ? "disable" : "enable"

Instead of this, I wonder maybe it's better to:

- store the default filter property into a pointer to string
- colo code may change the pointer to "filter-buffer,status=disable"

Then, there's no need for lots of codes above:
- no need a "is_default" parameter in netdev_add_filter which does not
scale consider we may want to have more property in the future
- no need to hacking like "qemu_filter_opts"
- no need to have a special flag like "is_default"

Thoughts?

> +    opts = qemu_opts_parse_noisily(&qemu_filter_opts,
> +                                   optarg, false);
> +    if (!opts) {
> +        error_report("Failed to parse param '%s'", optarg);
> +        exit(1);
> +    }
> +    g_free(optarg);
> +    if (object_create(NULL, opts, &err) < 0) {
> +        error_report("Failed to create object");
> +        goto out_clean;
> +    }
> +    filter_set_default_flag(id, is_default, &err);
> +
> +out_clean:
> +    qemu_opts_del(opts);
> +    if (err) {
> +        error_propagate(errp, err);
> +    }
> +}
> +
>  static void netfilter_finalize(Object *obj)
>  {
>      NetFilterState *nf = NETFILTER(obj);

Re: [Qemu-devel] [PATCH RFC v2 3/5] net/filter: Introduce a helper to add a filter to the netdev

[Hailiang Zhang]

On 2016/2/1 11:14, Jason Wang wrote:
>
>
> On 01/27/2016 04:29 PM, zhanghailiang wrote:
>> We add a new helper function netdev_add_filter(), this function
>> can help adding a filter object to a netdev.
>> Besides, we add a is_default member for struct NetFilterState
>> to indicate whether the filter is default or not.
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> ---
>> v2:
>>   -Re-implement netdev_add_filter() by re-using object_create()
>>    (Jason's suggestion)
>> ---
>>   include/net/filter.h |  7 +++++
>>   net/filter.c         | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 87 insertions(+)
>>
>> diff --git a/include/net/filter.h b/include/net/filter.h
>> index af3c53c..ee1c024 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 is_default;
>>       bool enabled;
>>       QTAILQ_ENTRY(NetFilterState) next;
>>   };
>> @@ -74,4 +75,10 @@ ssize_t qemu_netfilter_pass_to_next(NetClientState *sender,
>>                                       int iovcnt,
>>                                       void *opaque);
>>
>> +void netdev_add_filter(const char *netdev_id,
>> +                       const char *filter_type,
>> +                       const char *id,
>> +                       bool is_default,
>> +                       Error **errp);
>> +
>>   #endif /* QEMU_NET_FILTER_H */
>> diff --git a/net/filter.c b/net/filter.c
>> index d08a2be..dc7aa9b 100644
>> --- a/net/filter.c
>> +++ b/net/filter.c
>> @@ -214,6 +214,86 @@ static void netfilter_complete(UserCreatable *uc, Error **errp)
>>       QTAILQ_INSERT_TAIL(&nf->netdev->filters, nf, next);
>>   }
>>
>> +QemuOptsList qemu_filter_opts = {
>> +    .name = "default-filter",
>> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_filter_opts.head),
>> +    .desc = {
>> +        {
>> +            .name = "qom-type",
>> +            .type = QEMU_OPT_STRING,
>> +        },{
>> +            .name = "id",
>> +            .type = QEMU_OPT_STRING,
>> +        },{
>> +            .name = "netdev",
>> +            .type = QEMU_OPT_STRING,
>> +        },{
>> +            .name = "status",
>> +            .type = QEMU_OPT_STRING,
>> +        },
>> +        { /* end of list */ }
>> +    },
>> +};
>> +
>> +static void filter_set_default_flag(const char *id,
>> +                                    bool is_default,
>> +                                    Error **errp)
>> +{
>> +    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 = is_default;
>> +}
>> +
>> +void netdev_add_filter(const char *netdev_id,
>> +                       const char *filter_type,
>> +                       const char *id,
>> +                       bool is_default,
>> +                       Error **errp)
>> +{
>> +    NetClientState *nc = qemu_find_netdev(netdev_id);
>> +    char *optarg;
>> +    QemuOpts *opts = NULL;
>> +    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;
>> +    }
>> +
>> +    optarg = g_strdup_printf("qom-type=%s,id=%s,netdev=%s,status=%s",
>> +            filter_type, id, netdev_id, is_default ? "disable" : "enable"
>
> Instead of this, I wonder maybe it's better to:
>
> - store the default filter property into a pointer to string

Do you mean, pass a string parameter which stores the filter property instead of
assemble it in this helper ?

> - colo code may change the pointer to "filter-buffer,status=disable"
>

> Then, there's no need for lots of codes above:
> - no need a "is_default" parameter in netdev_add_filter which does not
> scale consider we may want to have more property in the future
> - no need to hacking like "qemu_filter_opts"

Yes, we can use qemu_find_opts("object") instead of it.

> - no need to have a special flag like "is_default"
>

But we have to distinguish the default filter from the common
filter, use the name (id) to distinguish it ?

Thanks,
Hailiang

> Thoughts?
>
>> +    opts = qemu_opts_parse_noisily(&qemu_filter_opts,
>> +                                   optarg, false);
>> +    if (!opts) {
>> +        error_report("Failed to parse param '%s'", optarg);
>> +        exit(1);
>> +    }
>> +    g_free(optarg);
>> +    if (object_create(NULL, opts, &err) < 0) {
>> +        error_report("Failed to create object");
>> +        goto out_clean;
>> +    }
>> +    filter_set_default_flag(id, is_default, &err);
>> +
>> +out_clean:
>> +    qemu_opts_del(opts);
>> +    if (err) {
>> +        error_propagate(errp, err);
>> +    }
>> +}
>> +
>>   static void netfilter_finalize(Object *obj)
>>   {
>>       NetFilterState *nf = NETFILTER(obj);
>
>
> .
>

Re: [Qemu-devel] [PATCH RFC v2 2/5] vl: Make object_create() public

[Hailiang Zhang]

On 2016/2/1 11:05, Jason Wang wrote:
>
>
> On 01/27/2016 04:29 PM, zhanghailiang wrote:
>> Make the helper object_create() public and fix its first
>> parameter to accept NULL value.
>
> Looks not very nice. Maybe pass a new predicate func for sanity check it
> better.
>

OK, but here is it better to check if the predicate func is NULL ?

Thanks,
Hailiang

>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>> v2:
>>   - New patch
>> ---
>>   include/qemu-common.h | 2 ++
>>   vl.c                  | 4 ++--
>>   2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/qemu-common.h b/include/qemu-common.h
>> index 22b010c..52cf4fd 100644
>> --- a/include/qemu-common.h
>> +++ b/include/qemu-common.h
>> @@ -500,4 +500,6 @@ int parse_debug_env(const char *name, int max, int initial);
>>   const char *qemu_ether_ntoa(const MACAddr *mac);
>>   void page_size_init(void);
>>
>> +int object_create(void *opaque, QemuOpts *opts, Error **errp);
>> +
>>   #endif
>> diff --git a/vl.c b/vl.c
>> index f043009..b21335e 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -2819,7 +2819,7 @@ static bool object_create_delayed(const char *type)
>>   }
>>
>>
>> -static int object_create(void *opaque, QemuOpts *opts, Error **errp)
>> +int object_create(void *opaque, QemuOpts *opts, Error **errp)
>>   {
>>       Error *err = NULL;
>>       char *type = NULL;
>> @@ -2842,7 +2842,7 @@ static int object_create(void *opaque, QemuOpts *opts, Error **errp)
>>       if (err) {
>>           goto out;
>>       }
>> -    if (!type_predicate(type)) {
>> +    if (type_predicate && !type_predicate(type)) {
>>           goto out;
>>       }
>>
>
>
> .
>

Re: [Qemu-devel] [PATCH RFC v2 2/5] vl: Make object_create() public

[Jason Wang]

On 02/01/2016 02:19 PM, Hailiang Zhang wrote:
> On 2016/2/1 11:05, Jason Wang wrote:
>>
>>
>> On 01/27/2016 04:29 PM, zhanghailiang wrote:
>>> Make the helper object_create() public and fix its first
>>> parameter to accept NULL value.
>>
>> Looks not very nice. Maybe pass a new predicate func for sanity check it
>> better.
>>
>
> OK, but here is it better to check if the predicate func is NULL ?
>
> Thanks,
> Hailiang

Not sure, but if you stick to check against NULL, need a separate patch.

Re: [Qemu-devel] [PATCH RFC v2 2/5] vl: Make object_create() public

[Hailiang Zhang]

On 2016/2/1 15:27, Jason Wang wrote:
>
>
> On 02/01/2016 02:19 PM, Hailiang Zhang wrote:
>> On 2016/2/1 11:05, Jason Wang wrote:
>>>
>>>
>>> On 01/27/2016 04:29 PM, zhanghailiang wrote:
>>>> Make the helper object_create() public and fix its first
>>>> parameter to accept NULL value.
>>>
>>> Looks not very nice. Maybe pass a new predicate func for sanity check it
>>> better.
>>>
>>
>> OK, but here is it better to check if the predicate func is NULL ?
>>
>> Thanks,
>> Hailiang
>
> Not sure, but if you stick to check against NULL, need a separate patch.
>

OK, i will drop this unnecessary check and add sanity check in next version, thanks.

Re: [Qemu-devel] [PATCH RFC v2 3/5] net/filter: Introduce a helper to add a filter to the netdev

[Jason Wang]

On 02/01/2016 02:13 PM, Hailiang Zhang wrote:
> On 2016/2/1 11:14, Jason Wang wrote:
>>
>>
>> On 01/27/2016 04:29 PM, zhanghailiang wrote:
>>> We add a new helper function netdev_add_filter(), this function
>>> can help adding a filter object to a netdev.
>>> Besides, we add a is_default member for struct NetFilterState
>>> to indicate whether the filter is default or not.
>>>
>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>> ---
>>> v2:
>>>   -Re-implement netdev_add_filter() by re-using object_create()
>>>    (Jason's suggestion)
>>> ---
>>>   include/net/filter.h |  7 +++++
>>>   net/filter.c         | 80
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   2 files changed, 87 insertions(+)
>>>
>>> diff --git a/include/net/filter.h b/include/net/filter.h
>>> index af3c53c..ee1c024 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 is_default;
>>>       bool enabled;
>>>       QTAILQ_ENTRY(NetFilterState) next;
>>>   };
>>> @@ -74,4 +75,10 @@ ssize_t
>>> qemu_netfilter_pass_to_next(NetClientState *sender,
>>>                                       int iovcnt,
>>>                                       void *opaque);
>>>
>>> +void netdev_add_filter(const char *netdev_id,
>>> +                       const char *filter_type,
>>> +                       const char *id,
>>> +                       bool is_default,
>>> +                       Error **errp);
>>> +
>>>   #endif /* QEMU_NET_FILTER_H */
>>> diff --git a/net/filter.c b/net/filter.c
>>> index d08a2be..dc7aa9b 100644
>>> --- a/net/filter.c
>>> +++ b/net/filter.c
>>> @@ -214,6 +214,86 @@ static void netfilter_complete(UserCreatable
>>> *uc, Error **errp)
>>>       QTAILQ_INSERT_TAIL(&nf->netdev->filters, nf, next);
>>>   }
>>>
>>> +QemuOptsList qemu_filter_opts = {
>>> +    .name = "default-filter",
>>> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_filter_opts.head),
>>> +    .desc = {
>>> +        {
>>> +            .name = "qom-type",
>>> +            .type = QEMU_OPT_STRING,
>>> +        },{
>>> +            .name = "id",
>>> +            .type = QEMU_OPT_STRING,
>>> +        },{
>>> +            .name = "netdev",
>>> +            .type = QEMU_OPT_STRING,
>>> +        },{
>>> +            .name = "status",
>>> +            .type = QEMU_OPT_STRING,
>>> +        },
>>> +        { /* end of list */ }
>>> +    },
>>> +};
>>> +
>>> +static void filter_set_default_flag(const char *id,
>>> +                                    bool is_default,
>>> +                                    Error **errp)
>>> +{
>>> +    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 = is_default;
>>> +}
>>> +
>>> +void netdev_add_filter(const char *netdev_id,
>>> +                       const char *filter_type,
>>> +                       const char *id,
>>> +                       bool is_default,
>>> +                       Error **errp)
>>> +{
>>> +    NetClientState *nc = qemu_find_netdev(netdev_id);
>>> +    char *optarg;
>>> +    QemuOpts *opts = NULL;
>>> +    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;
>>> +    }
>>> +
>>> +    optarg = g_strdup_printf("qom-type=%s,id=%s,netdev=%s,status=%s",
>>> +            filter_type, id, netdev_id, is_default ? "disable" :
>>> "enable"
>>
>> Instead of this, I wonder maybe it's better to:
>>
>> - store the default filter property into a pointer to string
>
> Do you mean, pass a string parameter which stores the filter property
> instead of
> assemble it in this helper ?

Yes. E.g just a global string which could be changed by any subsystem.
E.g colo may change it to "filter-buffer,interval=0,status=disable". But
filter ids need to be generated automatically.

>
>> - colo code may change the pointer to "filter-buffer,status=disable"
>>
>
>> Then, there's no need for lots of codes above:
>> - no need a "is_default" parameter in netdev_add_filter which does not
>> scale consider we may want to have more property in the future
>> - no need to hacking like "qemu_filter_opts"
>
> Yes, we can use qemu_find_opts("object") instead of it.
>
>> - no need to have a special flag like "is_default"
>>
>
> But we have to distinguish the default filter from the common
> filter, use the name (id) to distinguish it ?

What's the reason that you want to distinguish default filters from others?

Thanks

>
> Thanks,
> Hailiang
>
>> Thoughts?
>>
>>> +    opts = qemu_opts_parse_noisily(&qemu_filter_opts,
>>> +                                   optarg, false);
>>> +    if (!opts) {
>>> +        error_report("Failed to parse param '%s'", optarg);
>>> +        exit(1);
>>> +    }
>>> +    g_free(optarg);
>>> +    if (object_create(NULL, opts, &err) < 0) {
>>> +        error_report("Failed to create object");
>>> +        goto out_clean;
>>> +    }
>>> +    filter_set_default_flag(id, is_default, &err);
>>> +
>>> +out_clean:
>>> +    qemu_opts_del(opts);
>>> +    if (err) {
>>> +        error_propagate(errp, err);
>>> +    }
>>> +}
>>> +
>>>   static void netfilter_finalize(Object *obj)
>>>   {
>>>       NetFilterState *nf = NETFILTER(obj);
>>
>>
>> .
>>
>
>

Re: [Qemu-devel] [PATCH RFC v2 3/5] net/filter: Introduce a helper to add a filter to the netdev

[Hailiang Zhang]

On 2016/2/1 15:46, Jason Wang wrote:
>
>
> On 02/01/2016 02:13 PM, Hailiang Zhang wrote:
>> On 2016/2/1 11:14, Jason Wang wrote:
>>>
>>>
>>> On 01/27/2016 04:29 PM, zhanghailiang wrote:
>>>> We add a new helper function netdev_add_filter(), this function
>>>> can help adding a filter object to a netdev.
>>>> Besides, we add a is_default member for struct NetFilterState
>>>> to indicate whether the filter is default or not.
>>>>
>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>> ---
>>>> v2:
>>>>    -Re-implement netdev_add_filter() by re-using object_create()
>>>>     (Jason's suggestion)
>>>> ---
>>>>    include/net/filter.h |  7 +++++
>>>>    net/filter.c         | 80
>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    2 files changed, 87 insertions(+)
>>>>
>>>> diff --git a/include/net/filter.h b/include/net/filter.h
>>>> index af3c53c..ee1c024 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 is_default;
>>>>        bool enabled;
>>>>        QTAILQ_ENTRY(NetFilterState) next;
>>>>    };
>>>> @@ -74,4 +75,10 @@ ssize_t
>>>> qemu_netfilter_pass_to_next(NetClientState *sender,
>>>>                                        int iovcnt,
>>>>                                        void *opaque);
>>>>
>>>> +void netdev_add_filter(const char *netdev_id,
>>>> +                       const char *filter_type,
>>>> +                       const char *id,
>>>> +                       bool is_default,
>>>> +                       Error **errp);
>>>> +
>>>>    #endif /* QEMU_NET_FILTER_H */
>>>> diff --git a/net/filter.c b/net/filter.c
>>>> index d08a2be..dc7aa9b 100644
>>>> --- a/net/filter.c
>>>> +++ b/net/filter.c
>>>> @@ -214,6 +214,86 @@ static void netfilter_complete(UserCreatable
>>>> *uc, Error **errp)
>>>>        QTAILQ_INSERT_TAIL(&nf->netdev->filters, nf, next);
>>>>    }
>>>>
>>>> +QemuOptsList qemu_filter_opts = {
>>>> +    .name = "default-filter",
>>>> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_filter_opts.head),
>>>> +    .desc = {
>>>> +        {
>>>> +            .name = "qom-type",
>>>> +            .type = QEMU_OPT_STRING,
>>>> +        },{
>>>> +            .name = "id",
>>>> +            .type = QEMU_OPT_STRING,
>>>> +        },{
>>>> +            .name = "netdev",
>>>> +            .type = QEMU_OPT_STRING,
>>>> +        },{
>>>> +            .name = "status",
>>>> +            .type = QEMU_OPT_STRING,
>>>> +        },
>>>> +        { /* end of list */ }
>>>> +    },
>>>> +};
>>>> +
>>>> +static void filter_set_default_flag(const char *id,
>>>> +                                    bool is_default,
>>>> +                                    Error **errp)
>>>> +{
>>>> +    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 = is_default;
>>>> +}
>>>> +
>>>> +void netdev_add_filter(const char *netdev_id,
>>>> +                       const char *filter_type,
>>>> +                       const char *id,
>>>> +                       bool is_default,
>>>> +                       Error **errp)
>>>> +{
>>>> +    NetClientState *nc = qemu_find_netdev(netdev_id);
>>>> +    char *optarg;
>>>> +    QemuOpts *opts = NULL;
>>>> +    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;
>>>> +    }
>>>> +
>>>> +    optarg = g_strdup_printf("qom-type=%s,id=%s,netdev=%s,status=%s",
>>>> +            filter_type, id, netdev_id, is_default ? "disable" :
>>>> "enable"
>>>
>>> Instead of this, I wonder maybe it's better to:
>>>
>>> - store the default filter property into a pointer to string
>>
>> Do you mean, pass a string parameter which stores the filter property
>> instead of
>> assemble it in this helper ?
>
> Yes. E.g just a global string which could be changed by any subsystem.
> E.g colo may change it to "filter-buffer,interval=0,status=disable". But
> filter ids need to be generated automatically.
>

Got it. Then we don't need the global default_netfilter_type[] in patch 5,
Just use this global string instead ?

>>
>>> - colo code may change the pointer to "filter-buffer,status=disable"
>>>
>>
>>> Then, there's no need for lots of codes above:
>>> - no need a "is_default" parameter in netdev_add_filter which does not
>>> scale consider we may want to have more property in the future
>>> - no need to hacking like "qemu_filter_opts"
>>
>> Yes, we can use qemu_find_opts("object") instead of it.
>>
>>> - no need to have a special flag like "is_default"
>>>
>>
>> But we have to distinguish the default filter from the common
>> filter, use the name (id) to distinguish it ?
>
> What's the reason that you want to distinguish default filters from others?
>

The default filters will be used by COLO or MC, (In COLO, we will use it
to control packets buffering/releasing).
For COLO, we don't want to control (use) other filters that added by users.

Thanks,
Hailiang

> Thanks
>
>>
>> Thanks,
>> Hailiang
>>
>>> Thoughts?
>>>
>>>> +    opts = qemu_opts_parse_noisily(&qemu_filter_opts,
>>>> +                                   optarg, false);
>>>> +    if (!opts) {
>>>> +        error_report("Failed to parse param '%s'", optarg);
>>>> +        exit(1);
>>>> +    }
>>>> +    g_free(optarg);
>>>> +    if (object_create(NULL, opts, &err) < 0) {
>>>> +        error_report("Failed to create object");
>>>> +        goto out_clean;
>>>> +    }
>>>> +    filter_set_default_flag(id, is_default, &err);
>>>> +
>>>> +out_clean:
>>>> +    qemu_opts_del(opts);
>>>> +    if (err) {
>>>> +        error_propagate(errp, err);
>>>> +    }
>>>> +}
>>>> +
>>>>    static void netfilter_finalize(Object *obj)
>>>>    {
>>>>        NetFilterState *nf = NETFILTER(obj);
>>>
>>>
>>> .
>>>
>>
>>
>
>
> .
>

Re: [Qemu-devel] [PATCH RFC v2 3/5] net/filter: Introduce a helper to add a filter to the netdev

[Yang Hongyang]

On 02/01/2016 03:56 PM, Hailiang Zhang wrote:
> On 2016/2/1 15:46, Jason Wang wrote:
>>
>>
>> On 02/01/2016 02:13 PM, Hailiang Zhang wrote:
>>> On 2016/2/1 11:14, Jason Wang wrote:
>>>>
>>>>
>>>> On 01/27/2016 04:29 PM, zhanghailiang wrote:
>>>>> We add a new helper function netdev_add_filter(), this function
>>>>> can help adding a filter object to a netdev.
>>>>> Besides, we add a is_default member for struct NetFilterState
>>>>> to indicate whether the filter is default or not.
>>>>>
>>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>>> ---
>>>>> v2:
>>>>>    -Re-implement netdev_add_filter() by re-using object_create()
>>>>>     (Jason's suggestion)
>>>>> ---
>>>>>    include/net/filter.h |  7 +++++
>>>>>    net/filter.c         | 80
>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>    2 files changed, 87 insertions(+)
>>>>>
>>>>> diff --git a/include/net/filter.h b/include/net/filter.h
>>>>> index af3c53c..ee1c024 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 is_default;
>>>>>        bool enabled;
>>>>>        QTAILQ_ENTRY(NetFilterState) next;
>>>>>    };
>>>>> @@ -74,4 +75,10 @@ ssize_t
>>>>> qemu_netfilter_pass_to_next(NetClientState *sender,
>>>>>                                        int iovcnt,
>>>>>                                        void *opaque);
>>>>>
>>>>> +void netdev_add_filter(const char *netdev_id,
>>>>> +                       const char *filter_type,
>>>>> +                       const char *id,
>>>>> +                       bool is_default,
>>>>> +                       Error **errp);
>>>>> +
>>>>>    #endif /* QEMU_NET_FILTER_H */
>>>>> diff --git a/net/filter.c b/net/filter.c
>>>>> index d08a2be..dc7aa9b 100644
>>>>> --- a/net/filter.c
>>>>> +++ b/net/filter.c
>>>>> @@ -214,6 +214,86 @@ static void netfilter_complete(UserCreatable
>>>>> *uc, Error **errp)
>>>>>        QTAILQ_INSERT_TAIL(&nf->netdev->filters, nf, next);
>>>>>    }
>>>>>
>>>>> +QemuOptsList qemu_filter_opts = {
>>>>> +    .name = "default-filter",
>>>>> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_filter_opts.head),
>>>>> +    .desc = {
>>>>> +        {
>>>>> +            .name = "qom-type",
>>>>> +            .type = QEMU_OPT_STRING,
>>>>> +        },{
>>>>> +            .name = "id",
>>>>> +            .type = QEMU_OPT_STRING,
>>>>> +        },{
>>>>> +            .name = "netdev",
>>>>> +            .type = QEMU_OPT_STRING,
>>>>> +        },{
>>>>> +            .name = "status",
>>>>> +            .type = QEMU_OPT_STRING,
>>>>> +        },
>>>>> +        { /* end of list */ }
>>>>> +    },
>>>>> +};
>>>>> +
>>>>> +static void filter_set_default_flag(const char *id,
>>>>> +                                    bool is_default,
>>>>> +                                    Error **errp)
>>>>> +{
>>>>> +    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 = is_default;
>>>>> +}
>>>>> +
>>>>> +void netdev_add_filter(const char *netdev_id,
>>>>> +                       const char *filter_type,
>>>>> +                       const char *id,
>>>>> +                       bool is_default,
>>>>> +                       Error **errp)
>>>>> +{
>>>>> +    NetClientState *nc = qemu_find_netdev(netdev_id);
>>>>> +    char *optarg;
>>>>> +    QemuOpts *opts = NULL;
>>>>> +    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;
>>>>> +    }
>>>>> +
>>>>> +    optarg = g_strdup_printf("qom-type=%s,id=%s,netdev=%s,status=%s",
>>>>> +            filter_type, id, netdev_id, is_default ? "disable" :
>>>>> "enable"
>>>>
>>>> Instead of this, I wonder maybe it's better to:
>>>>
>>>> - store the default filter property into a pointer to string
>>>
>>> Do you mean, pass a string parameter which stores the filter property
>>> instead of
>>> assemble it in this helper ?
>>
>> Yes. E.g just a global string which could be changed by any subsystem.
>> E.g colo may change it to "filter-buffer,interval=0,status=disable". But
>> filter ids need to be generated automatically.
>>
>
> Got it. Then we don't need the global default_netfilter_type[] in patch 5,
> Just use this global string instead ?
>
>>>
>>>> - colo code may change the pointer to "filter-buffer,status=disable"
>>>>
>>>
>>>> Then, there's no need for lots of codes above:
>>>> - no need a "is_default" parameter in netdev_add_filter which does not
>>>> scale consider we may want to have more property in the future
>>>> - no need to hacking like "qemu_filter_opts"
>>>
>>> Yes, we can use qemu_find_opts("object") instead of it.
>>>
>>>> - no need to have a special flag like "is_default"
>>>>
>>>
>>> But we have to distinguish the default filter from the common
>>> filter, use the name (id) to distinguish it ?
>>
>> What's the reason that you want to distinguish default filters from
>> others?
>>
>
> The default filters will be used by COLO or MC, (In COLO, we will use it
> to control packets buffering/releasing).
> For COLO, we don't want to control (use) other filters that added by users.

I think Jason's point is that COLO is a manager, you can add the filter
to netdev when doing COLO, so the only difference between COLO's default
filter and other filter is that COLO's filter (for example buffer
filter) is added by COLO, and the other filter is added by user
specifing -filter.

>
> Thanks,
> Hailiang
>
>> Thanks
>>
>>>
>>> Thanks,
>>> Hailiang
>>>
>>>> Thoughts?
>>>>
>>>>> +    opts = qemu_opts_parse_noisily(&qemu_filter_opts,
>>>>> +                                   optarg, false);
>>>>> +    if (!opts) {
>>>>> +        error_report("Failed to parse param '%s'", optarg);
>>>>> +        exit(1);
>>>>> +    }
>>>>> +    g_free(optarg);
>>>>> +    if (object_create(NULL, opts, &err) < 0) {
>>>>> +        error_report("Failed to create object");
>>>>> +        goto out_clean;
>>>>> +    }
>>>>> +    filter_set_default_flag(id, is_default, &err);
>>>>> +
>>>>> +out_clean:
>>>>> +    qemu_opts_del(opts);
>>>>> +    if (err) {
>>>>> +        error_propagate(errp, err);
>>>>> +    }
>>>>> +}
>>>>> +
>>>>>    static void netfilter_finalize(Object *obj)
>>>>>    {
>>>>>        NetFilterState *nf = NETFILTER(obj);
>>>>
>>>>
>>>> .
>>>>
>>>
>>>
>>
>>
>> .
>>
>
>
>

-- 
Thanks,
Yang

Re: [Qemu-devel] [PATCH RFC v2 3/5] net/filter: Introduce a helper to add a filter to the netdev

[Hailiang Zhang]

On 2016/2/1 16:05, Yang Hongyang wrote:
>
>
> On 02/01/2016 03:56 PM, Hailiang Zhang wrote:
>> On 2016/2/1 15:46, Jason Wang wrote:
>>>
>>>
>>> On 02/01/2016 02:13 PM, Hailiang Zhang wrote:
>>>> On 2016/2/1 11:14, Jason Wang wrote:
>>>>>
>>>>>
>>>>> On 01/27/2016 04:29 PM, zhanghailiang wrote:
>>>>>> We add a new helper function netdev_add_filter(), this function
>>>>>> can help adding a filter object to a netdev.
>>>>>> Besides, we add a is_default member for struct NetFilterState
>>>>>> to indicate whether the filter is default or not.
>>>>>>
>>>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>>>> ---
>>>>>> v2:
>>>>>>    -Re-implement netdev_add_filter() by re-using object_create()
>>>>>>     (Jason's suggestion)
>>>>>> ---
>>>>>>    include/net/filter.h |  7 +++++
>>>>>>    net/filter.c         | 80
>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>    2 files changed, 87 insertions(+)
>>>>>>
>>>>>> diff --git a/include/net/filter.h b/include/net/filter.h
>>>>>> index af3c53c..ee1c024 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 is_default;
>>>>>>        bool enabled;
>>>>>>        QTAILQ_ENTRY(NetFilterState) next;
>>>>>>    };
>>>>>> @@ -74,4 +75,10 @@ ssize_t
>>>>>> qemu_netfilter_pass_to_next(NetClientState *sender,
>>>>>>                                        int iovcnt,
>>>>>>                                        void *opaque);
>>>>>>
>>>>>> +void netdev_add_filter(const char *netdev_id,
>>>>>> +                       const char *filter_type,
>>>>>> +                       const char *id,
>>>>>> +                       bool is_default,
>>>>>> +                       Error **errp);
>>>>>> +
>>>>>>    #endif /* QEMU_NET_FILTER_H */
>>>>>> diff --git a/net/filter.c b/net/filter.c
>>>>>> index d08a2be..dc7aa9b 100644
>>>>>> --- a/net/filter.c
>>>>>> +++ b/net/filter.c
>>>>>> @@ -214,6 +214,86 @@ static void netfilter_complete(UserCreatable
>>>>>> *uc, Error **errp)
>>>>>>        QTAILQ_INSERT_TAIL(&nf->netdev->filters, nf, next);
>>>>>>    }
>>>>>>
>>>>>> +QemuOptsList qemu_filter_opts = {
>>>>>> +    .name = "default-filter",
>>>>>> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_filter_opts.head),
>>>>>> +    .desc = {
>>>>>> +        {
>>>>>> +            .name = "qom-type",
>>>>>> +            .type = QEMU_OPT_STRING,
>>>>>> +        },{
>>>>>> +            .name = "id",
>>>>>> +            .type = QEMU_OPT_STRING,
>>>>>> +        },{
>>>>>> +            .name = "netdev",
>>>>>> +            .type = QEMU_OPT_STRING,
>>>>>> +        },{
>>>>>> +            .name = "status",
>>>>>> +            .type = QEMU_OPT_STRING,
>>>>>> +        },
>>>>>> +        { /* end of list */ }
>>>>>> +    },
>>>>>> +};
>>>>>> +
>>>>>> +static void filter_set_default_flag(const char *id,
>>>>>> +                                    bool is_default,
>>>>>> +                                    Error **errp)
>>>>>> +{
>>>>>> +    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 = is_default;
>>>>>> +}
>>>>>> +
>>>>>> +void netdev_add_filter(const char *netdev_id,
>>>>>> +                       const char *filter_type,
>>>>>> +                       const char *id,
>>>>>> +                       bool is_default,
>>>>>> +                       Error **errp)
>>>>>> +{
>>>>>> +    NetClientState *nc = qemu_find_netdev(netdev_id);
>>>>>> +    char *optarg;
>>>>>> +    QemuOpts *opts = NULL;
>>>>>> +    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;
>>>>>> +    }
>>>>>> +
>>>>>> +    optarg = g_strdup_printf("qom-type=%s,id=%s,netdev=%s,status=%s",
>>>>>> +            filter_type, id, netdev_id, is_default ? "disable" :
>>>>>> "enable"
>>>>>
>>>>> Instead of this, I wonder maybe it's better to:
>>>>>
>>>>> - store the default filter property into a pointer to string
>>>>
>>>> Do you mean, pass a string parameter which stores the filter property
>>>> instead of
>>>> assemble it in this helper ?
>>>
>>> Yes. E.g just a global string which could be changed by any subsystem.
>>> E.g colo may change it to "filter-buffer,interval=0,status=disable". But
>>> filter ids need to be generated automatically.
>>>
>>
>> Got it. Then we don't need the global default_netfilter_type[] in patch 5,
>> Just use this global string instead ?
>>
>>>>
>>>>> - colo code may change the pointer to "filter-buffer,status=disable"
>>>>>
>>>>
>>>>> Then, there's no need for lots of codes above:
>>>>> - no need a "is_default" parameter in netdev_add_filter which does not
>>>>> scale consider we may want to have more property in the future
>>>>> - no need to hacking like "qemu_filter_opts"
>>>>
>>>> Yes, we can use qemu_find_opts("object") instead of it.
>>>>
>>>>> - no need to have a special flag like "is_default"
>>>>>
>>>>
>>>> But we have to distinguish the default filter from the common
>>>> filter, use the name (id) to distinguish it ?
>>>
>>> What's the reason that you want to distinguish default filters from
>>> others?
>>>
>>
>> The default filters will be used by COLO or MC, (In COLO, we will use it
>> to control packets buffering/releasing).
>> For COLO, we don't want to control (use) other filters that added by users.
>
> I think Jason's point is that COLO is a manager, you can add the filter
> to netdev when doing COLO, so the only difference between COLO's default

Er, then we came back to the original question, 'is it necessary to add each netdev
a default filter ?'
If we add the a filter to netdev when doing COLO, it will be added dynamically,
Here we want to add each netdev a default filter while launch QEMU
(no matter if this VM will go into COLO or not),
just to support hot-add NIC for VM while in COLO lifetime.

> filter and other filter is that COLO's filter (for example buffer
> filter) is added by COLO, and the other filter is added by user
> specifing -filter.
>
>>
>> Thanks,
>> Hailiang
>>
>>> Thanks
>>>
>>>>
>>>> Thanks,
>>>> Hailiang
>>>>
>>>>> Thoughts?
>>>>>
>>>>>> +    opts = qemu_opts_parse_noisily(&qemu_filter_opts,
>>>>>> +                                   optarg, false);
>>>>>> +    if (!opts) {
>>>>>> +        error_report("Failed to parse param '%s'", optarg);
>>>>>> +        exit(1);
>>>>>> +    }
>>>>>> +    g_free(optarg);
>>>>>> +    if (object_create(NULL, opts, &err) < 0) {
>>>>>> +        error_report("Failed to create object");
>>>>>> +        goto out_clean;
>>>>>> +    }
>>>>>> +    filter_set_default_flag(id, is_default, &err);
>>>>>> +
>>>>>> +out_clean:
>>>>>> +    qemu_opts_del(opts);
>>>>>> +    if (err) {
>>>>>> +        error_propagate(errp, err);
>>>>>> +    }
>>>>>> +}
>>>>>> +
>>>>>>    static void netfilter_finalize(Object *obj)
>>>>>>    {
>>>>>>        NetFilterState *nf = NETFILTER(obj);
>>>>>
>>>>>
>>>>> .
>>>>>
>>>>
>>>>
>>>
>>>
>>> .
>>>
>>
>>
>>
>

Re: [Qemu-devel] [PATCH RFC v2 3/5] net/filter: Introduce a helper to add a filter to the netdev

[Jason Wang]

On 02/01/2016 03:56 PM, Hailiang Zhang wrote:
> On 2016/2/1 15:46, Jason Wang wrote:
>>
>>
>> On 02/01/2016 02:13 PM, Hailiang Zhang wrote:
>>> On 2016/2/1 11:14, Jason Wang wrote:
>>>>
>>>>
>>>> On 01/27/2016 04:29 PM, zhanghailiang wrote:
>>>>> We add a new helper function netdev_add_filter(), this function
>>>>> can help adding a filter object to a netdev.
>>>>> Besides, we add a is_default member for struct NetFilterState
>>>>> to indicate whether the filter is default or not.
>>>>>
>>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>>> ---
>>>>> v2:
>>>>>    -Re-implement netdev_add_filter() by re-using object_create()
>>>>>     (Jason's suggestion)
>>>>> ---
>>>>>    include/net/filter.h |  7 +++++
>>>>>    net/filter.c         | 80
>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>    2 files changed, 87 insertions(+)
>>>>>
>>>>>

[...]

>>>>> +
>>>>> +    optarg =
>>>>> g_strdup_printf("qom-type=%s,id=%s,netdev=%s,status=%s",
>>>>> +            filter_type, id, netdev_id, is_default ? "disable" :
>>>>> "enable"
>>>>
>>>> Instead of this, I wonder maybe it's better to:
>>>>
>>>> - store the default filter property into a pointer to string
>>>
>>> Do you mean, pass a string parameter which stores the filter property
>>> instead of
>>> assemble it in this helper ?
>>
>> Yes. E.g just a global string which could be changed by any subsystem.
>> E.g colo may change it to "filter-buffer,interval=0,status=disable". But
>> filter ids need to be generated automatically.
>>
>
> Got it. Then we don't need the global default_netfilter_type[] in
> patch 5,

Yes.

> Just use this global string instead ?

Right.

>
>>>
>>>> - colo code may change the pointer to "filter-buffer,status=disable"
>>>>
>>>
>>>> Then, there's no need for lots of codes above:
>>>> - no need a "is_default" parameter in netdev_add_filter which does not
>>>> scale consider we may want to have more property in the future
>>>> - no need to hacking like "qemu_filter_opts"
>>>
>>> Yes, we can use qemu_find_opts("object") instead of it.
>>>
>>>> - no need to have a special flag like "is_default"
>>>>
>>>
>>> But we have to distinguish the default filter from the common
>>> filter, use the name (id) to distinguish it ?
>>
>> What's the reason that you want to distinguish default filters from
>> others?
>>
>
> The default filters will be used by COLO or MC, (In COLO, we will use it
> to control packets buffering/releasing).

A question is how will you do this?

> For COLO, we don't want to control (use) other filters that added by
> users.
>
> Thanks,
> Hailiang
>
>> Thanks
>>
>>>
>>> Thanks,
>>> Hailiang
>>>
>>>> Thoughts?
>>>>
>>>>> +    opts = qemu_opts_parse_noisily(&qemu_filter_opts,
>>>>> +                                   optarg, false);
>>>>> +    if (!opts) {
>>>>> +        error_report("Failed to parse param '%s'", optarg);
>>>>> +        exit(1);
>>>>> +    }
>>>>> +    g_free(optarg);
>>>>> +    if (object_create(NULL, opts, &err) < 0) {
>>>>> +        error_report("Failed to create object");
>>>>> +        goto out_clean;
>>>>> +    }
>>>>> +    filter_set_default_flag(id, is_default, &err);
>>>>> +
>>>>> +out_clean:
>>>>> +    qemu_opts_del(opts);
>>>>> +    if (err) {
>>>>> +        error_propagate(errp, err);
>>>>> +    }
>>>>> +}
>>>>> +
>>>>>    static void netfilter_finalize(Object *obj)
>>>>>    {
>>>>>        NetFilterState *nf = NETFILTER(obj);
>>>>
>>>>
>>>> .
>>>>
>>>
>>>
>>
>>
>> .
>>
>
>
>

Re: [Qemu-devel] [PATCH RFC v2 3/5] net/filter: Introduce a helper to add a filter to the netdev

[Jason Wang]

On 02/01/2016 04:21 PM, Hailiang Zhang wrote:
>>>>>>
>>>>>> Instead of this, I wonder maybe it's better to:
>>>>>>
>>>>>> - store the default filter property into a pointer to string
>>>>>
>>>>> Do you mean, pass a string parameter which stores the filter property
>>>>> instead of
>>>>> assemble it in this helper ?
>>>>
>>>> Yes. E.g just a global string which could be changed by any subsystem.
>>>> E.g colo may change it to
>>>> "filter-buffer,interval=0,status=disable". But
>>>> filter ids need to be generated automatically.
>>>>
>>>
>>> Got it. Then we don't need the global default_netfilter_type[] in
>>> patch 5,
>>> Just use this global string instead ?
>>>
>>>>>
>>>>>> - colo code may change the pointer to "filter-buffer,status=disable"
>>>>>>
>>>>>
>>>>>> Then, there's no need for lots of codes above:
>>>>>> - no need a "is_default" parameter in netdev_add_filter which
>>>>>> does not
>>>>>> scale consider we may want to have more property in the future
>>>>>> - no need to hacking like "qemu_filter_opts"
>>>>>
>>>>> Yes, we can use qemu_find_opts("object") instead of it.
>>>>>
>>>>>> - no need to have a special flag like "is_default"
>>>>>>
>>>>>
>>>>> But we have to distinguish the default filter from the common
>>>>> filter, use the name (id) to distinguish it ?
>>>>
>>>> What's the reason that you want to distinguish default filters from
>>>> others?
>>>>
>>>
>>> The default filters will be used by COLO or MC, (In COLO, we will
>>> use it
>>> to control packets buffering/releasing).
>>> For COLO, we don't want to control (use) other filters that added by
>>> users.
>>
>> I think Jason's point is that COLO is a manager, you can add the filter
>> to netdev when doing COLO, so the only difference between COLO's default
>
> Er, then we came back to the original question, 'is it necessary to
> add each netdev
> a default filter ?'

The question could be extended to:

1) Do we need a default filter? I think the answer is yes, but of course
COLO can work even without this.
2) Do we want to implement COLO on top of default filter? If yes, as you
suggest, we may record the ids of the default filter and do what ever we
what. If not, COLO need codes to go through each netdev and add filter
itself (hotplug is not supported). Or you want management to do this,
then even hotplug could be supported.

Any thoughts?

> If we add the a filter to netdev when doing COLO, it will be added
> dynamically,
> Here we want to add each netdev a default filter while launch QEMU
> (no matter if this VM will go into COLO or not),
> just to support hot-add NIC for VM while in COLO lifetime.

Yes.

Re: [Qemu-devel] [PATCH RFC v2 3/5] net/filter: Introduce a helper to add a filter to the netdev

[Hailiang Zhang]

On 2016/2/1 17:04, Jason Wang wrote:
>
>
> On 02/01/2016 03:56 PM, Hailiang Zhang wrote:
>> On 2016/2/1 15:46, Jason Wang wrote:
>>>
>>>
>>> On 02/01/2016 02:13 PM, Hailiang Zhang wrote:
>>>> On 2016/2/1 11:14, Jason Wang wrote:
>>>>>
>>>>>
>>>>> On 01/27/2016 04:29 PM, zhanghailiang wrote:
>>>>>> We add a new helper function netdev_add_filter(), this function
>>>>>> can help adding a filter object to a netdev.
>>>>>> Besides, we add a is_default member for struct NetFilterState
>>>>>> to indicate whether the filter is default or not.
>>>>>>
>>>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>>>> ---
>>>>>> v2:
>>>>>>     -Re-implement netdev_add_filter() by re-using object_create()
>>>>>>      (Jason's suggestion)
>>>>>> ---
>>>>>>     include/net/filter.h |  7 +++++
>>>>>>     net/filter.c         | 80
>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>     2 files changed, 87 insertions(+)
>>>>>>
>>>>>>
>
> [...]
>
>>>>>> +
>>>>>> +    optarg =
>>>>>> g_strdup_printf("qom-type=%s,id=%s,netdev=%s,status=%s",
>>>>>> +            filter_type, id, netdev_id, is_default ? "disable" :
>>>>>> "enable"
>>>>>
>>>>> Instead of this, I wonder maybe it's better to:
>>>>>
>>>>> - store the default filter property into a pointer to string
>>>>
>>>> Do you mean, pass a string parameter which stores the filter property
>>>> instead of
>>>> assemble it in this helper ?
>>>
>>> Yes. E.g just a global string which could be changed by any subsystem.
>>> E.g colo may change it to "filter-buffer,interval=0,status=disable". But
>>> filter ids need to be generated automatically.
>>>
>>
>> Got it. Then we don't need the global default_netfilter_type[] in
>> patch 5,
>
> Yes.
>
>> Just use this global string instead ?
>
> Right.
>
>>
>>>>
>>>>> - colo code may change the pointer to "filter-buffer,status=disable"
>>>>>
>>>>
>>>>> Then, there's no need for lots of codes above:
>>>>> - no need a "is_default" parameter in netdev_add_filter which does not
>>>>> scale consider we may want to have more property in the future
>>>>> - no need to hacking like "qemu_filter_opts"
>>>>
>>>> Yes, we can use qemu_find_opts("object") instead of it.
>>>>
>>>>> - no need to have a special flag like "is_default"
>>>>>
>>>>
>>>> But we have to distinguish the default filter from the common
>>>> filter, use the name (id) to distinguish it ?
>>>
>>> What's the reason that you want to distinguish default filters from
>>> others?
>>>
>>
>> The default filters will be used by COLO or MC, (In COLO, we will use it
>> to control packets buffering/releasing).
>
> A question is how will you do this?
>

Er, for COLO, we will enable all the default filter in the initialization stage,
then the buffer-filter will buffer all netdev's packets,
after doing a checkpoint, we will release all the buffered packets (Flush all default
filters' packets). If VM is failover, we will set all default filters back to disabled
status.
(This is a periodic mode for COLO, different from another mode, which we will call it
hybrid mode, that is based on colo-proxy, which is in developing by zhangchen)

Thanks,
Hailiang

>> For COLO, we don't want to control (use) other filters that added by
>> users.
>>
>> Thanks,
>> Hailiang
>>
>>> Thanks
>>>
>>>>
>>>> Thanks,
>>>> Hailiang
>>>>
>>>>> Thoughts?
>>>>>
>>>>>> +    opts = qemu_opts_parse_noisily(&qemu_filter_opts,
>>>>>> +                                   optarg, false);
>>>>>> +    if (!opts) {
>>>>>> +        error_report("Failed to parse param '%s'", optarg);
>>>>>> +        exit(1);
>>>>>> +    }
>>>>>> +    g_free(optarg);
>>>>>> +    if (object_create(NULL, opts, &err) < 0) {
>>>>>> +        error_report("Failed to create object");
>>>>>> +        goto out_clean;
>>>>>> +    }
>>>>>> +    filter_set_default_flag(id, is_default, &err);
>>>>>> +
>>>>>> +out_clean:
>>>>>> +    qemu_opts_del(opts);
>>>>>> +    if (err) {
>>>>>> +        error_propagate(errp, err);
>>>>>> +    }
>>>>>> +}
>>>>>> +
>>>>>>     static void netfilter_finalize(Object *obj)
>>>>>>     {
>>>>>>         NetFilterState *nf = NETFILTER(obj);
>>>>>
>>>>>
>>>>> .
>>>>>
>>>>
>>>>
>>>
>>>
>>> .
>>>
>>
>>
>>
>
>
> .
>

Re: [Qemu-devel] [PATCH RFC v2 3/5] net/filter: Introduce a helper to add a filter to the netdev

[Hailiang Zhang]

On 2016/2/1 17:18, Jason Wang wrote:
>
>
> On 02/01/2016 04:21 PM, Hailiang Zhang wrote:
>>>>>>>
>>>>>>> Instead of this, I wonder maybe it's better to:
>>>>>>>
>>>>>>> - store the default filter property into a pointer to string
>>>>>>
>>>>>> Do you mean, pass a string parameter which stores the filter property
>>>>>> instead of
>>>>>> assemble it in this helper ?
>>>>>
>>>>> Yes. E.g just a global string which could be changed by any subsystem.
>>>>> E.g colo may change it to
>>>>> "filter-buffer,interval=0,status=disable". But
>>>>> filter ids need to be generated automatically.
>>>>>
>>>>
>>>> Got it. Then we don't need the global default_netfilter_type[] in
>>>> patch 5,
>>>> Just use this global string instead ?
>>>>
>>>>>>
>>>>>>> - colo code may change the pointer to "filter-buffer,status=disable"
>>>>>>>
>>>>>>
>>>>>>> Then, there's no need for lots of codes above:
>>>>>>> - no need a "is_default" parameter in netdev_add_filter which
>>>>>>> does not
>>>>>>> scale consider we may want to have more property in the future
>>>>>>> - no need to hacking like "qemu_filter_opts"
>>>>>>
>>>>>> Yes, we can use qemu_find_opts("object") instead of it.
>>>>>>
>>>>>>> - no need to have a special flag like "is_default"
>>>>>>>
>>>>>>
>>>>>> But we have to distinguish the default filter from the common
>>>>>> filter, use the name (id) to distinguish it ?
>>>>>
>>>>> What's the reason that you want to distinguish default filters from
>>>>> others?
>>>>>
>>>>
>>>> The default filters will be used by COLO or MC, (In COLO, we will
>>>> use it
>>>> to control packets buffering/releasing).
>>>> For COLO, we don't want to control (use) other filters that added by
>>>> users.
>>>
>>> I think Jason's point is that COLO is a manager, you can add the filter
>>> to netdev when doing COLO, so the only difference between COLO's default
>>
>> Er, then we came back to the original question, 'is it necessary to
>> add each netdev
>> a default filter ?'
>
> The question could be extended to:
>
> 1) Do we need a default filter? I think the answer is yes, but of course
> COLO can work even without this.

Yes, after colo-proxy is realized, we can switch to colo-proxy
(It should have the capability of buffer and release packets directly).
But for now, we want to merge COLO prototype without colo-proxy, the COLO
prototype should have the basic capability. Just like Remus or
Micro-checkpointing. It is based on the default buffer-filter to control net
packets.

> 2) Do we want to implement COLO on top of default filter? If yes, as you
> suggest, we may record the ids of the default filter and do what ever we

Yes, we need it.

> what. If not, COLO need codes to go through each netdev and add filter
> itself (hotplug is not supported). Or you want management to do this,
> then even hotplug could be supported.
>

We also want to support hotplug during VM is in COLO state in the future.
(For this point, I'm not quite sure if this usage case is really exist.)

Thanks,
Hailiang

> Any thoughts?
>
>> If we add the a filter to netdev when doing COLO, it will be added
>> dynamically,
>> Here we want to add each netdev a default filter while launch QEMU
>> (no matter if this VM will go into COLO or not),
>> just to support hot-add NIC for VM while in COLO lifetime.
>
> Yes.
>
>
>
>
> .
>

Re: [Qemu-devel] [PATCH RFC v2 3/5] net/filter: Introduce a helper to add a filter to the netdev

[Jason Wang]

On 02/01/2016 05:22 PM, Hailiang Zhang wrote:
> On 2016/2/1 17:04, Jason Wang wrote:
>>
>>
>> On 02/01/2016 03:56 PM, Hailiang Zhang wrote:
>>> On 2016/2/1 15:46, Jason Wang wrote:
>>>>
>>>>
>>>> On 02/01/2016 02:13 PM, Hailiang Zhang wrote:
>>>>> On 2016/2/1 11:14, Jason Wang wrote:
>>>>>>
>>>>>>
>>>>>> On 01/27/2016 04:29 PM, zhanghailiang wrote:
>>>>>>> We add a new helper function netdev_add_filter(), this function
>>>>>>> can help adding a filter object to a netdev.
>>>>>>> Besides, we add a is_default member for struct NetFilterState
>>>>>>> to indicate whether the filter is default or not.
>>>>>>>
>>>>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>>>>> ---
>>>>>>> v2:
>>>>>>>     -Re-implement netdev_add_filter() by re-using object_create()
>>>>>>>      (Jason's suggestion)
>>>>>>> ---
>>>>>>>     include/net/filter.h |  7 +++++
>>>>>>>     net/filter.c         | 80
>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>     2 files changed, 87 insertions(+)
>>>>>>>
>>>>>>>
>>
>> [...]
>>
>>>>>>> +
>>>>>>> +    optarg =
>>>>>>> g_strdup_printf("qom-type=%s,id=%s,netdev=%s,status=%s",
>>>>>>> +            filter_type, id, netdev_id, is_default ? "disable" :
>>>>>>> "enable"
>>>>>>
>>>>>> Instead of this, I wonder maybe it's better to:
>>>>>>
>>>>>> - store the default filter property into a pointer to string
>>>>>
>>>>> Do you mean, pass a string parameter which stores the filter property
>>>>> instead of
>>>>> assemble it in this helper ?
>>>>
>>>> Yes. E.g just a global string which could be changed by any subsystem.
>>>> E.g colo may change it to
>>>> "filter-buffer,interval=0,status=disable". But
>>>> filter ids need to be generated automatically.
>>>>
>>>
>>> Got it. Then we don't need the global default_netfilter_type[] in
>>> patch 5,
>>
>> Yes.
>>
>>> Just use this global string instead ?
>>
>> Right.
>>
>>>
>>>>>
>>>>>> - colo code may change the pointer to "filter-buffer,status=disable"
>>>>>>
>>>>>
>>>>>> Then, there's no need for lots of codes above:
>>>>>> - no need a "is_default" parameter in netdev_add_filter which
>>>>>> does not
>>>>>> scale consider we may want to have more property in the future
>>>>>> - no need to hacking like "qemu_filter_opts"
>>>>>
>>>>> Yes, we can use qemu_find_opts("object") instead of it.
>>>>>
>>>>>> - no need to have a special flag like "is_default"
>>>>>>
>>>>>
>>>>> But we have to distinguish the default filter from the common
>>>>> filter, use the name (id) to distinguish it ?
>>>>
>>>> What's the reason that you want to distinguish default filters from
>>>> others?
>>>>
>>>
>>> The default filters will be used by COLO or MC, (In COLO, we will
>>> use it
>>> to control packets buffering/releasing).
>>
>> A question is how will you do this?
>>
>
> Er, for COLO, we will enable all the default filter in the
> initialization stage,
> then the buffer-filter will buffer all netdev's packets,
> after doing a checkpoint, we will release all the buffered packets
> (Flush all default
> filters' packets).

Right, that's the point. So looks several choices here:

1) Track each default filter explicitly, generate and record the netdev
ids for default filter by COLO.  Walk through the ids list and release
the packet in each filter.
2) Track the default filters implicitly. Link all buffer filters (with
zero interval) in a list during filter buffer initialization. And export
a helper for COLO to walk them and release packets.

Either looks fine, but maybe 2 is easier.

>  If VM is failover, we will set all default filters back to disabled
> status.
> (This is a periodic mode for COLO, different from another mode, which
> we will call it
> hybrid mode, that is based on colo-proxy, which is in developing by
> zhangchen)
>
> Thanks,
> Hailiang

Yes, I see.

Re: [Qemu-devel] [PATCH RFC v2 3/5] net/filter: Introduce a helper to add a filter to the netdev

[Jason Wang]

On 02/01/2016 05:39 PM, Hailiang Zhang wrote:
> On 2016/2/1 17:18, Jason Wang wrote:
>>
>>
>> On 02/01/2016 04:21 PM, Hailiang Zhang wrote:
>>>>>>>>
>>>>>>>> Instead of this, I wonder maybe it's better to:
>>>>>>>>
>>>>>>>> - store the default filter property into a pointer to string
>>>>>>>
>>>>>>> Do you mean, pass a string parameter which stores the filter
>>>>>>> property
>>>>>>> instead of
>>>>>>> assemble it in this helper ?
>>>>>>
>>>>>> Yes. E.g just a global string which could be changed by any
>>>>>> subsystem.
>>>>>> E.g colo may change it to
>>>>>> "filter-buffer,interval=0,status=disable". But
>>>>>> filter ids need to be generated automatically.
>>>>>>
>>>>>
>>>>> Got it. Then we don't need the global default_netfilter_type[] in
>>>>> patch 5,
>>>>> Just use this global string instead ?
>>>>>
>>>>>>>
>>>>>>>> - colo code may change the pointer to
>>>>>>>> "filter-buffer,status=disable"
>>>>>>>>
>>>>>>>
>>>>>>>> Then, there's no need for lots of codes above:
>>>>>>>> - no need a "is_default" parameter in netdev_add_filter which
>>>>>>>> does not
>>>>>>>> scale consider we may want to have more property in the future
>>>>>>>> - no need to hacking like "qemu_filter_opts"
>>>>>>>
>>>>>>> Yes, we can use qemu_find_opts("object") instead of it.
>>>>>>>
>>>>>>>> - no need to have a special flag like "is_default"
>>>>>>>>
>>>>>>>
>>>>>>> But we have to distinguish the default filter from the common
>>>>>>> filter, use the name (id) to distinguish it ?
>>>>>>
>>>>>> What's the reason that you want to distinguish default filters from
>>>>>> others?
>>>>>>
>>>>>
>>>>> The default filters will be used by COLO or MC, (In COLO, we will
>>>>> use it
>>>>> to control packets buffering/releasing).
>>>>> For COLO, we don't want to control (use) other filters that added by
>>>>> users.
>>>>
>>>> I think Jason's point is that COLO is a manager, you can add the
>>>> filter
>>>> to netdev when doing COLO, so the only difference between COLO's
>>>> default
>>>
>>> Er, then we came back to the original question, 'is it necessary to
>>> add each netdev
>>> a default filter ?'
>>
>> The question could be extended to:
>>
>> 1) Do we need a default filter? I think the answer is yes, but of course
>> COLO can work even without this.
>
> Yes, after colo-proxy is realized, we can switch to colo-proxy
> (It should have the capability of buffer and release packets directly).
> But for now, we want to merge COLO prototype without colo-proxy, the COLO
> prototype should have the basic capability.

Right, I see.

> Just like Remus or
> Micro-checkpointing. It is based on the default buffer-filter to
> control net
> packets.
>
>> 2) Do we want to implement COLO on top of default filter? If yes, as you
>> suggest, we may record the ids of the default filter and do what ever we
>
> Yes, we need it.

Or just as I reply, all buffer filters (with zero interval) could be
tracked by itself. So as you see, several ways could go. It's your call
to choose one of them.

>
>> what. If not, COLO need codes to go through each netdev and add filter
>> itself (hotplug is not supported). Or you want management to do this,
>> then even hotplug could be supported.
>>
>
> We also want to support hotplug during VM is in COLO state in the future.
> (For this point, I'm not quite sure if this usage case is really exist.)
>
> Thanks,
> Hailiang

Support hotplug should be useful I think. But I'm also ok if you don't
want to consider for it now.

Re: [Qemu-devel] [PATCH RFC v2 3/5] net/filter: Introduce a helper to add a filter to the netdev

[Hailiang Zhang]

On 2016/2/1 17:42, Jason Wang wrote:
>
>
> On 02/01/2016 05:22 PM, Hailiang Zhang wrote:
>> On 2016/2/1 17:04, Jason Wang wrote:
>>>
>>>
>>> On 02/01/2016 03:56 PM, Hailiang Zhang wrote:
>>>> On 2016/2/1 15:46, Jason Wang wrote:
>>>>>
>>>>>
>>>>> On 02/01/2016 02:13 PM, Hailiang Zhang wrote:
>>>>>> On 2016/2/1 11:14, Jason Wang wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 01/27/2016 04:29 PM, zhanghailiang wrote:
>>>>>>>> We add a new helper function netdev_add_filter(), this function
>>>>>>>> can help adding a filter object to a netdev.
>>>>>>>> Besides, we add a is_default member for struct NetFilterState
>>>>>>>> to indicate whether the filter is default or not.
>>>>>>>>
>>>>>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>>>>>> ---
>>>>>>>> v2:
>>>>>>>>      -Re-implement netdev_add_filter() by re-using object_create()
>>>>>>>>       (Jason's suggestion)
>>>>>>>> ---
>>>>>>>>      include/net/filter.h |  7 +++++
>>>>>>>>      net/filter.c         | 80
>>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>      2 files changed, 87 insertions(+)
>>>>>>>>
>>>>>>>>
>>>
>>> [...]
>>>
>>>>>>>> +
>>>>>>>> +    optarg =
>>>>>>>> g_strdup_printf("qom-type=%s,id=%s,netdev=%s,status=%s",
>>>>>>>> +            filter_type, id, netdev_id, is_default ? "disable" :
>>>>>>>> "enable"
>>>>>>>
>>>>>>> Instead of this, I wonder maybe it's better to:
>>>>>>>
>>>>>>> - store the default filter property into a pointer to string
>>>>>>
>>>>>> Do you mean, pass a string parameter which stores the filter property
>>>>>> instead of
>>>>>> assemble it in this helper ?
>>>>>
>>>>> Yes. E.g just a global string which could be changed by any subsystem.
>>>>> E.g colo may change it to
>>>>> "filter-buffer,interval=0,status=disable". But
>>>>> filter ids need to be generated automatically.
>>>>>
>>>>
>>>> Got it. Then we don't need the global default_netfilter_type[] in
>>>> patch 5,
>>>
>>> Yes.
>>>
>>>> Just use this global string instead ?
>>>
>>> Right.
>>>
>>>>
>>>>>>
>>>>>>> - colo code may change the pointer to "filter-buffer,status=disable"
>>>>>>>
>>>>>>
>>>>>>> Then, there's no need for lots of codes above:
>>>>>>> - no need a "is_default" parameter in netdev_add_filter which
>>>>>>> does not
>>>>>>> scale consider we may want to have more property in the future
>>>>>>> - no need to hacking like "qemu_filter_opts"
>>>>>>
>>>>>> Yes, we can use qemu_find_opts("object") instead of it.
>>>>>>
>>>>>>> - no need to have a special flag like "is_default"
>>>>>>>
>>>>>>
>>>>>> But we have to distinguish the default filter from the common
>>>>>> filter, use the name (id) to distinguish it ?
>>>>>
>>>>> What's the reason that you want to distinguish default filters from
>>>>> others?
>>>>>
>>>>
>>>> The default filters will be used by COLO or MC, (In COLO, we will
>>>> use it
>>>> to control packets buffering/releasing).
>>>
>>> A question is how will you do this?
>>>
>>
>> Er, for COLO, we will enable all the default filter in the
>> initialization stage,
>> then the buffer-filter will buffer all netdev's packets,
>> after doing a checkpoint, we will release all the buffered packets
>> (Flush all default
>> filters' packets).
>
> Right, that's the point. So looks several choices here:
>
> 1) Track each default filter explicitly, generate and record the netdev
> ids for default filter by COLO.  Walk through the ids list and release
> the packet in each filter.
> 2) Track the default filters implicitly. Link all buffer filters (with
> zero interval) in a list during filter buffer initialization. And export
> a helper for COLO to walk them and release packets.
>
> Either looks fine, but maybe 2 is easier.
>

2) is a good choice, but I'm a little worry about using zero to
distinguish if a filter is default filter or not, because users
can use qom-set to change its value.
If special flag like 'is_default' is not acceptable,
then we have to use the filter ids to distinguish the default buffer-filter
(here the rule of generation ids for default filter is '[netdev-id][DEFAULT_FILTER_TYPE]'

Thanks,
Hailiang

>>   If VM is failover, we will set all default filters back to disabled
>> status.
>> (This is a periodic mode for COLO, different from another mode, which
>> we will call it
>> hybrid mode, that is based on colo-proxy, which is in developing by
>> zhangchen)
>>
>> Thanks,
>> Hailiang
>
> Yes, I see.
>
>
> .
>

Re: [Qemu-devel] [PATCH RFC v2 2/5] vl: Make object_create() public

[Daniel P. Berrange]

On Wed, Jan 27, 2016 at 04:29:37PM +0800, zhanghailiang wrote:
> Make the helper object_create() public and fix its first
> parameter to accept NULL value.
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> ---
> v2:
>  - New patch
> ---
>  include/qemu-common.h | 2 ++
>  vl.c                  | 4 ++--
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index 22b010c..52cf4fd 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -500,4 +500,6 @@ int parse_debug_env(const char *name, int max, int initial);
>  const char *qemu_ether_ntoa(const MACAddr *mac);
>  void page_size_init(void);
>  
> +int object_create(void *opaque, QemuOpts *opts, Error **errp);
> +
>  #endif
> diff --git a/vl.c b/vl.c
> index f043009..b21335e 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2819,7 +2819,7 @@ static bool object_create_delayed(const char *type)
>  }
>  
>  
> -static int object_create(void *opaque, QemuOpts *opts, Error **errp)
> +int object_create(void *opaque, QemuOpts *opts, Error **errp)
>  {
>      Error *err = NULL;
>      char *type = NULL;
> @@ -2842,7 +2842,7 @@ static int object_create(void *opaque, QemuOpts *opts, Error **errp)
>      if (err) {
>          goto out;
>      }
> -    if (!type_predicate(type)) {
> +    if (type_predicate && !type_predicate(type)) {
>          goto out;
>      }

No, please don't do this - your later patch should *not* be using
object_create, it should use object_new_with_props.

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 v2 3/5] net/filter: Introduce a helper to add a filter to the netdev

[Hailiang Zhang]

On 2016/2/1 17:49, Jason Wang wrote:
>
>
> On 02/01/2016 05:39 PM, Hailiang Zhang wrote:
>> On 2016/2/1 17:18, Jason Wang wrote:
>>>
>>>
>>> On 02/01/2016 04:21 PM, Hailiang Zhang wrote:
>>>>>>>>>
>>>>>>>>> Instead of this, I wonder maybe it's better to:
>>>>>>>>>
>>>>>>>>> - store the default filter property into a pointer to string
>>>>>>>>
>>>>>>>> Do you mean, pass a string parameter which stores the filter
>>>>>>>> property
>>>>>>>> instead of
>>>>>>>> assemble it in this helper ?
>>>>>>>
>>>>>>> Yes. E.g just a global string which could be changed by any
>>>>>>> subsystem.
>>>>>>> E.g colo may change it to
>>>>>>> "filter-buffer,interval=0,status=disable". But
>>>>>>> filter ids need to be generated automatically.
>>>>>>>
>>>>>>
>>>>>> Got it. Then we don't need the global default_netfilter_type[] in
>>>>>> patch 5,
>>>>>> Just use this global string instead ?
>>>>>>
>>>>>>>>
>>>>>>>>> - colo code may change the pointer to
>>>>>>>>> "filter-buffer,status=disable"
>>>>>>>>>
>>>>>>>>
>>>>>>>>> Then, there's no need for lots of codes above:
>>>>>>>>> - no need a "is_default" parameter in netdev_add_filter which
>>>>>>>>> does not
>>>>>>>>> scale consider we may want to have more property in the future
>>>>>>>>> - no need to hacking like "qemu_filter_opts"
>>>>>>>>
>>>>>>>> Yes, we can use qemu_find_opts("object") instead of it.
>>>>>>>>
>>>>>>>>> - no need to have a special flag like "is_default"
>>>>>>>>>
>>>>>>>>
>>>>>>>> But we have to distinguish the default filter from the common
>>>>>>>> filter, use the name (id) to distinguish it ?
>>>>>>>
>>>>>>> What's the reason that you want to distinguish default filters from
>>>>>>> others?
>>>>>>>
>>>>>>
>>>>>> The default filters will be used by COLO or MC, (In COLO, we will
>>>>>> use it
>>>>>> to control packets buffering/releasing).
>>>>>> For COLO, we don't want to control (use) other filters that added by
>>>>>> users.
>>>>>
>>>>> I think Jason's point is that COLO is a manager, you can add the
>>>>> filter
>>>>> to netdev when doing COLO, so the only difference between COLO's
>>>>> default
>>>>
>>>> Er, then we came back to the original question, 'is it necessary to
>>>> add each netdev
>>>> a default filter ?'
>>>
>>> The question could be extended to:
>>>
>>> 1) Do we need a default filter? I think the answer is yes, but of course
>>> COLO can work even without this.
>>
>> Yes, after colo-proxy is realized, we can switch to colo-proxy
>> (It should have the capability of buffer and release packets directly).
>> But for now, we want to merge COLO prototype without colo-proxy, the COLO
>> prototype should have the basic capability.
>
> Right, I see.
>
>> Just like Remus or
>> Micro-checkpointing. It is based on the default buffer-filter to
>> control net
>> packets.
>>
>>> 2) Do we want to implement COLO on top of default filter? If yes, as you
>>> suggest, we may record the ids of the default filter and do what ever we
>>
>> Yes, we need it.
>
> Or just as I reply, all buffer filters (with zero interval) could be
> tracked by itself. So as you see, several ways could go. It's your call
> to choose one of them.
>

OK, got it.

>>
>>> what. If not, COLO need codes to go through each netdev and add filter
>>> itself (hotplug is not supported). Or you want management to do this,
>>> then even hotplug could be supported.
>>>
>>
>> We also want to support hotplug during VM is in COLO state in the future.
>> (For this point, I'm not quite sure if this usage case is really exist.)
>>
>> Thanks,
>> Hailiang
>
> Support hotplug should be useful I think. But I'm also ok if you don't
> want to consider for it now.
>

Thanks very much.

Hailiang

Re: [Qemu-devel] [PATCH RFC v2 3/5] net/filter: Introduce a helper to add a filter to the netdev

[Daniel P. Berrange]

On Wed, Jan 27, 2016 at 04:29:38PM +0800, zhanghailiang wrote:
> We add a new helper function netdev_add_filter(), this function
> can help adding a filter object to a netdev.
> Besides, we add a is_default member for struct NetFilterState
> to indicate whether the filter is default or not.
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
> v2:
>  -Re-implement netdev_add_filter() by re-using object_create()
>   (Jason's suggestion)
> ---
>  include/net/filter.h |  7 +++++
>  net/filter.c         | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 87 insertions(+)

> +void netdev_add_filter(const char *netdev_id,
> +                       const char *filter_type,
> +                       const char *id,
> +                       bool is_default,
> +                       Error **errp)
> +{
> +    NetClientState *nc = qemu_find_netdev(netdev_id);
> +    char *optarg;
> +    QemuOpts *opts = NULL;
> +    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;
> +    }
> +
> +    optarg = g_strdup_printf("qom-type=%s,id=%s,netdev=%s,status=%s",
> +            filter_type, id, netdev_id, is_default ? "disable" : "enable");
> +    opts = qemu_opts_parse_noisily(&qemu_filter_opts,
> +                                   optarg, false);

Formatting a string and then immediately parsing it again is totally
crazy, not least because you're not taking care to do escaping of
special characters like ',' in the string parameters.

> +    if (!opts) {
> +        error_report("Failed to parse param '%s'", optarg);
> +        exit(1);
> +    }
> +    g_free(optarg);
> +    if (object_create(NULL, opts, &err) < 0) {
> +        error_report("Failed to create object");
> +        goto out_clean;
> +    }

Don't use object_create() - use object_new_with_props() which avoids
the need to format + parse the string above. ie do

  object_new_with_props(filter_type,
                        object_get_objects_root(),
			id,
			&err,
			"netdev", netdev_id,
			"status", is_default ? "disable" : "enable",
			NULL);


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 v2 2/5] vl: Make object_create() public

[Hailiang Zhang]

On 2016/2/1 18:41, Daniel P. Berrange wrote:
> On Wed, Jan 27, 2016 at 04:29:37PM +0800, zhanghailiang wrote:
>> Make the helper object_create() public and fix its first
>> parameter to accept NULL value.
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>> v2:
>>   - New patch
>> ---
>>   include/qemu-common.h | 2 ++
>>   vl.c                  | 4 ++--
>>   2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/qemu-common.h b/include/qemu-common.h
>> index 22b010c..52cf4fd 100644
>> --- a/include/qemu-common.h
>> +++ b/include/qemu-common.h
>> @@ -500,4 +500,6 @@ int parse_debug_env(const char *name, int max, int initial);
>>   const char *qemu_ether_ntoa(const MACAddr *mac);
>>   void page_size_init(void);
>>
>> +int object_create(void *opaque, QemuOpts *opts, Error **errp);
>> +
>>   #endif
>> diff --git a/vl.c b/vl.c
>> index f043009..b21335e 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -2819,7 +2819,7 @@ static bool object_create_delayed(const char *type)
>>   }
>>
>>
>> -static int object_create(void *opaque, QemuOpts *opts, Error **errp)
>> +int object_create(void *opaque, QemuOpts *opts, Error **errp)
>>   {
>>       Error *err = NULL;
>>       char *type = NULL;
>> @@ -2842,7 +2842,7 @@ static int object_create(void *opaque, QemuOpts *opts, Error **errp)
>>       if (err) {
>>           goto out;
>>       }
>> -    if (!type_predicate(type)) {
>> +    if (type_predicate && !type_predicate(type)) {
>>           goto out;
>>       }
>
> No, please don't do this - your later patch should *not* be using
> object_create, it should use object_new_with_props.
>

Er, i didn't notice this helper before, i will look into it.

Thanks,
Hailiang

> Regards,
> Daniel
>

Re: [Qemu-devel] [PATCH RFC v2 3/5] net/filter: Introduce a helper to add a filter to the netdev

[Hailiang Zhang]

On 2016/2/1 18:43, Daniel P. Berrange wrote:
> On Wed, Jan 27, 2016 at 04:29:38PM +0800, zhanghailiang wrote:
>> We add a new helper function netdev_add_filter(), this function
>> can help adding a filter object to a netdev.
>> Besides, we add a is_default member for struct NetFilterState
>> to indicate whether the filter is default or not.
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> ---
>> v2:
>>   -Re-implement netdev_add_filter() by re-using object_create()
>>    (Jason's suggestion)
>> ---
>>   include/net/filter.h |  7 +++++
>>   net/filter.c         | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 87 insertions(+)
>
>> +void netdev_add_filter(const char *netdev_id,
>> +                       const char *filter_type,
>> +                       const char *id,
>> +                       bool is_default,
>> +                       Error **errp)
>> +{
>> +    NetClientState *nc = qemu_find_netdev(netdev_id);
>> +    char *optarg;
>> +    QemuOpts *opts = NULL;
>> +    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;
>> +    }
>> +
>> +    optarg = g_strdup_printf("qom-type=%s,id=%s,netdev=%s,status=%s",
>> +            filter_type, id, netdev_id, is_default ? "disable" : "enable");
>> +    opts = qemu_opts_parse_noisily(&qemu_filter_opts,
>> +                                   optarg, false);
>
> Formatting a string and then immediately parsing it again is totally
> crazy, not least because you're not taking care to do escaping of
> special characters like ',' in the string parameters.
>

Got it.

>> +    if (!opts) {
>> +        error_report("Failed to parse param '%s'", optarg);
>> +        exit(1);
>> +    }
>> +    g_free(optarg);
>> +    if (object_create(NULL, opts, &err) < 0) {
>> +        error_report("Failed to create object");
>> +        goto out_clean;
>> +    }
>
> Don't use object_create() - use object_new_with_props() which avoids
> the need to format + parse the string above. ie do
>
>    object_new_with_props(filter_type,
>                          object_get_objects_root(),
> 			id,
> 			&err,
> 			"netdev", netdev_id,
> 			"status", is_default ? "disable" : "enable",
> 			NULL);
>
>

Ha, that's really a good idea, i will fix it like that
in next version. Thank you very much.

Hailiang

> Regards,
> Daniel
>

Re: [Qemu-devel] [PATCH RFC v2 3/5] net/filter: Introduce a helper to add a filter to the netdev

[Hailiang Zhang]

Hi Jason,

On 2016/2/1 17:42, Jason Wang wrote:
>
>
> On 02/01/2016 05:22 PM, Hailiang Zhang wrote:
>> On 2016/2/1 17:04, Jason Wang wrote:
>>>
>>>
>>> On 02/01/2016 03:56 PM, Hailiang Zhang wrote:
>>>> On 2016/2/1 15:46, Jason Wang wrote:
>>>>>
>>>>>
>>>>> On 02/01/2016 02:13 PM, Hailiang Zhang wrote:
>>>>>> On 2016/2/1 11:14, Jason Wang wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 01/27/2016 04:29 PM, zhanghailiang wrote:
>>>>>>>> We add a new helper function netdev_add_filter(), this function
>>>>>>>> can help adding a filter object to a netdev.
>>>>>>>> Besides, we add a is_default member for struct NetFilterState
>>>>>>>> to indicate whether the filter is default or not.
>>>>>>>>
>>>>>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>>>>>> ---
>>>>>>>> v2:
>>>>>>>>      -Re-implement netdev_add_filter() by re-using object_create()
>>>>>>>>       (Jason's suggestion)
>>>>>>>> ---
>>>>>>>>      include/net/filter.h |  7 +++++
>>>>>>>>      net/filter.c         | 80
>>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>      2 files changed, 87 insertions(+)
>>>>>>>>
>>>>>>>>
>>>
>>> [...]
>>>
>>>>>>>> +
>>>>>>>> +    optarg =
>>>>>>>> g_strdup_printf("qom-type=%s,id=%s,netdev=%s,status=%s",
>>>>>>>> +            filter_type, id, netdev_id, is_default ? "disable" :
>>>>>>>> "enable"
>>>>>>>
>>>>>>> Instead of this, I wonder maybe it's better to:
>>>>>>>
>>>>>>> - store the default filter property into a pointer to string
>>>>>>
>>>>>> Do you mean, pass a string parameter which stores the filter property
>>>>>> instead of
>>>>>> assemble it in this helper ?
>>>>>
>>>>> Yes. E.g just a global string which could be changed by any subsystem.
>>>>> E.g colo may change it to
>>>>> "filter-buffer,interval=0,status=disable". But
>>>>> filter ids need to be generated automatically.
>>>>>
>>>>
>>>> Got it. Then we don't need the global default_netfilter_type[] in
>>>> patch 5,
>>>
>>> Yes.
>>>
>>>> Just use this global string instead ?
>>>
>>> Right.
>>>
>>>>
>>>>>>
>>>>>>> - colo code may change the pointer to "filter-buffer,status=disable"
>>>>>>>
>>>>>>
>>>>>>> Then, there's no need for lots of codes above:
>>>>>>> - no need a "is_default" parameter in netdev_add_filter which
>>>>>>> does not
>>>>>>> scale consider we may want to have more property in the future
>>>>>>> - no need to hacking like "qemu_filter_opts"
>>>>>>
>>>>>> Yes, we can use qemu_find_opts("object") instead of it.
>>>>>>
>>>>>>> - no need to have a special flag like "is_default"
>>>>>>>
>>>>>>
>>>>>> But we have to distinguish the default filter from the common
>>>>>> filter, use the name (id) to distinguish it ?
>>>>>
>>>>> What's the reason that you want to distinguish default filters from
>>>>> others?
>>>>>
>>>>
>>>> The default filters will be used by COLO or MC, (In COLO, we will
>>>> use it
>>>> to control packets buffering/releasing).
>>>
>>> A question is how will you do this?
>>>
>>
>> Er, for COLO, we will enable all the default filter in the
>> initialization stage,
>> then the buffer-filter will buffer all netdev's packets,
>> after doing a checkpoint, we will release all the buffered packets
>> (Flush all default
>> filters' packets).
>
> Right, that's the point. So looks several choices here:
>
> 1) Track each default filter explicitly, generate and record the netdev
> ids for default filter by COLO.  Walk through the ids list and release
> the packet in each filter.
> 2) Track the default filters implicitly. Link all buffer filters (with
> zero interval) in a list during filter buffer initialization. And export
> a helper for COLO to walk them and release packets.
>
> Either looks fine, but maybe 2 is easier.
>

Danies recommended using object_new_with_props() instead of object_create(),
which will avoids the need of format + parse process. It makes the codes
more clean. I have fixed it in v3.

I kept the 'is_default' member for struct NetFilterState. Because comparing
with finding the default filter by the special name ([netdev->id][nop]),
it seems to be more easy for COLO to find/control the default filter with
this flag.
(We added a new helper qemu_foreach_netfilter() in COLO frame, and
we traverse these filters directly without passing any netdev related info.)

But if you think it is still not acceptable to add such a member, i'll remove it
in next verison ;)

Thanks,
Hailiang


>>   If VM is failover, we will set all default filters back to disabled
>> status.
>> (This is a periodic mode for COLO, different from another mode, which
>> we will call it
>> hybrid mode, that is based on colo-proxy, which is in developing by
>> zhangchen)
>>
>> Thanks,
>> Hailiang
>
> Yes, I see.
>
>
> .
>

Re: [Qemu-devel] [PATCH RFC v2 3/5] net/filter: Introduce a helper to add a filter to the netdev

[Jason Wang]

On 02/01/2016 06:40 PM, Hailiang Zhang wrote:
> On 2016/2/1 17:42, Jason Wang wrote:
>>
>>
>> On 02/01/2016 05:22 PM, Hailiang Zhang wrote:
>>> On 2016/2/1 17:04, Jason Wang wrote:
>>>>
>>>>
>>>> On 02/01/2016 03:56 PM, Hailiang Zhang wrote:
>>>>> On 2016/2/1 15:46, Jason Wang wrote:
>>>>>>
>>>>>>
>>>>>> On 02/01/2016 02:13 PM, Hailiang Zhang wrote:
>>>>>>> On 2016/2/1 11:14, Jason Wang wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 01/27/2016 04:29 PM, zhanghailiang wrote:
>>>>>>>>> We add a new helper function netdev_add_filter(), this function
>>>>>>>>> can help adding a filter object to a netdev.
>>>>>>>>> Besides, we add a is_default member for struct NetFilterState
>>>>>>>>> to indicate whether the filter is default or not.
>>>>>>>>>
>>>>>>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>>>>>>> ---
>>>>>>>>> v2:
>>>>>>>>>      -Re-implement netdev_add_filter() by re-using
>>>>>>>>> object_create()
>>>>>>>>>       (Jason's suggestion)
>>>>>>>>> ---
>>>>>>>>>      include/net/filter.h |  7 +++++
>>>>>>>>>      net/filter.c         | 80
>>>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>>      2 files changed, 87 insertions(+)
>>>>>>>>>
>>>>>>>>>
>>>>
>>>> [...]
>>>>
>>>>>>>>> +
>>>>>>>>> +    optarg =
>>>>>>>>> g_strdup_printf("qom-type=%s,id=%s,netdev=%s,status=%s",
>>>>>>>>> +            filter_type, id, netdev_id, is_default ? "disable" :
>>>>>>>>> "enable"
>>>>>>>>
>>>>>>>> Instead of this, I wonder maybe it's better to:
>>>>>>>>
>>>>>>>> - store the default filter property into a pointer to string
>>>>>>>
>>>>>>> Do you mean, pass a string parameter which stores the filter
>>>>>>> property
>>>>>>> instead of
>>>>>>> assemble it in this helper ?
>>>>>>
>>>>>> Yes. E.g just a global string which could be changed by any
>>>>>> subsystem.
>>>>>> E.g colo may change it to
>>>>>> "filter-buffer,interval=0,status=disable". But
>>>>>> filter ids need to be generated automatically.
>>>>>>
>>>>>
>>>>> Got it. Then we don't need the global default_netfilter_type[] in
>>>>> patch 5,
>>>>
>>>> Yes.
>>>>
>>>>> Just use this global string instead ?
>>>>
>>>> Right.
>>>>
>>>>>
>>>>>>>
>>>>>>>> - colo code may change the pointer to
>>>>>>>> "filter-buffer,status=disable"
>>>>>>>>
>>>>>>>
>>>>>>>> Then, there's no need for lots of codes above:
>>>>>>>> - no need a "is_default" parameter in netdev_add_filter which
>>>>>>>> does not
>>>>>>>> scale consider we may want to have more property in the future
>>>>>>>> - no need to hacking like "qemu_filter_opts"
>>>>>>>
>>>>>>> Yes, we can use qemu_find_opts("object") instead of it.
>>>>>>>
>>>>>>>> - no need to have a special flag like "is_default"
>>>>>>>>
>>>>>>>
>>>>>>> But we have to distinguish the default filter from the common
>>>>>>> filter, use the name (id) to distinguish it ?
>>>>>>
>>>>>> What's the reason that you want to distinguish default filters from
>>>>>> others?
>>>>>>
>>>>>
>>>>> The default filters will be used by COLO or MC, (In COLO, we will
>>>>> use it
>>>>> to control packets buffering/releasing).
>>>>
>>>> A question is how will you do this?
>>>>
>>>
>>> Er, for COLO, we will enable all the default filter in the
>>> initialization stage,
>>> then the buffer-filter will buffer all netdev's packets,
>>> after doing a checkpoint, we will release all the buffered packets
>>> (Flush all default
>>> filters' packets).
>>
>> Right, that's the point. So looks several choices here:
>>
>> 1) Track each default filter explicitly, generate and record the netdev
>> ids for default filter by COLO.  Walk through the ids list and release
>> the packet in each filter.
>> 2) Track the default filters implicitly. Link all buffer filters (with
>> zero interval) in a list during filter buffer initialization. And export
>> a helper for COLO to walk them and release packets.
>>
>> Either looks fine, but maybe 2 is easier.
>>
>
> 2) is a good choice, but I'm a little worry about using zero to
> distinguish if a filter is default filter or not, because users
> can use qom-set to change its value.

Then I see minor issues:

1) Looks like we should prevent user other than COLO from using zero
interval, neither from cli nor 'qom-set'.
2) For the zero interval filter used by COLO, we should not allow user
to change the value of interval.

So I was thinking a new type which is based on current filter-buffer
whose interval is invisible to user.

> If special flag like 'is_default' is not acceptable,
> then we have to use the filter ids to distinguish the default
> buffer-filter
> (here the rule of generation ids for default filter is
> '[netdev-id][DEFAULT_FILTER_TYPE]'
>
> Thanks,
> Hailiang
>
>>>   If VM is failover, we will set all default filters back to disabled
>>> status.
>>> (This is a periodic mode for COLO, different from another mode, which
>>> we will call it
>>> hybrid mode, that is based on colo-proxy, which is in developing by
>>> zhangchen)
>>>
>>> Thanks,
>>> Hailiang
>>
>> Yes, I see.
>>
>>
>> .
>>
>
>

Re: [Qemu-devel] [PATCH RFC v2 3/5] net/filter: Introduce a helper to add a filter to the netdev

[Hailiang Zhang]

On 2016/2/5 14:19, Jason Wang wrote:
>
>
> On 02/01/2016 06:40 PM, Hailiang Zhang wrote:
>> On 2016/2/1 17:42, Jason Wang wrote:
>>>
>>>
>>> On 02/01/2016 05:22 PM, Hailiang Zhang wrote:
>>>> On 2016/2/1 17:04, Jason Wang wrote:
>>>>>
>>>>>
>>>>> On 02/01/2016 03:56 PM, Hailiang Zhang wrote:
>>>>>> On 2016/2/1 15:46, Jason Wang wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 02/01/2016 02:13 PM, Hailiang Zhang wrote:
>>>>>>>> On 2016/2/1 11:14, Jason Wang wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 01/27/2016 04:29 PM, zhanghailiang wrote:
>>>>>>>>>> We add a new helper function netdev_add_filter(), this function
>>>>>>>>>> can help adding a filter object to a netdev.
>>>>>>>>>> Besides, we add a is_default member for struct NetFilterState
>>>>>>>>>> to indicate whether the filter is default or not.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>>>>>>>> ---
>>>>>>>>>> v2:
>>>>>>>>>>       -Re-implement netdev_add_filter() by re-using
>>>>>>>>>> object_create()
>>>>>>>>>>        (Jason's suggestion)
>>>>>>>>>> ---
>>>>>>>>>>       include/net/filter.h |  7 +++++
>>>>>>>>>>       net/filter.c         | 80
>>>>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>>>       2 files changed, 87 insertions(+)
>>>>>>>>>>
>>>>>>>>>>
>>>>>
>>>>> [...]
>>>>>
>>>>>>>>>> +
>>>>>>>>>> +    optarg =
>>>>>>>>>> g_strdup_printf("qom-type=%s,id=%s,netdev=%s,status=%s",
>>>>>>>>>> +            filter_type, id, netdev_id, is_default ? "disable" :
>>>>>>>>>> "enable"
>>>>>>>>>
>>>>>>>>> Instead of this, I wonder maybe it's better to:
>>>>>>>>>
>>>>>>>>> - store the default filter property into a pointer to string
>>>>>>>>
>>>>>>>> Do you mean, pass a string parameter which stores the filter
>>>>>>>> property
>>>>>>>> instead of
>>>>>>>> assemble it in this helper ?
>>>>>>>
>>>>>>> Yes. E.g just a global string which could be changed by any
>>>>>>> subsystem.
>>>>>>> E.g colo may change it to
>>>>>>> "filter-buffer,interval=0,status=disable". But
>>>>>>> filter ids need to be generated automatically.
>>>>>>>
>>>>>>
>>>>>> Got it. Then we don't need the global default_netfilter_type[] in
>>>>>> patch 5,
>>>>>
>>>>> Yes.
>>>>>
>>>>>> Just use this global string instead ?
>>>>>
>>>>> Right.
>>>>>
>>>>>>
>>>>>>>>
>>>>>>>>> - colo code may change the pointer to
>>>>>>>>> "filter-buffer,status=disable"
>>>>>>>>>
>>>>>>>>
>>>>>>>>> Then, there's no need for lots of codes above:
>>>>>>>>> - no need a "is_default" parameter in netdev_add_filter which
>>>>>>>>> does not
>>>>>>>>> scale consider we may want to have more property in the future
>>>>>>>>> - no need to hacking like "qemu_filter_opts"
>>>>>>>>
>>>>>>>> Yes, we can use qemu_find_opts("object") instead of it.
>>>>>>>>
>>>>>>>>> - no need to have a special flag like "is_default"
>>>>>>>>>
>>>>>>>>
>>>>>>>> But we have to distinguish the default filter from the common
>>>>>>>> filter, use the name (id) to distinguish it ?
>>>>>>>
>>>>>>> What's the reason that you want to distinguish default filters from
>>>>>>> others?
>>>>>>>
>>>>>>
>>>>>> The default filters will be used by COLO or MC, (In COLO, we will
>>>>>> use it
>>>>>> to control packets buffering/releasing).
>>>>>
>>>>> A question is how will you do this?
>>>>>
>>>>
>>>> Er, for COLO, we will enable all the default filter in the
>>>> initialization stage,
>>>> then the buffer-filter will buffer all netdev's packets,
>>>> after doing a checkpoint, we will release all the buffered packets
>>>> (Flush all default
>>>> filters' packets).
>>>
>>> Right, that's the point. So looks several choices here:
>>>
>>> 1) Track each default filter explicitly, generate and record the netdev
>>> ids for default filter by COLO.  Walk through the ids list and release
>>> the packet in each filter.
>>> 2) Track the default filters implicitly. Link all buffer filters (with
>>> zero interval) in a list during filter buffer initialization. And export
>>> a helper for COLO to walk them and release packets.
>>>
>>> Either looks fine, but maybe 2 is easier.
>>>
>>
>> 2) is a good choice, but I'm a little worry about using zero to
>> distinguish if a filter is default filter or not, because users
>> can use qom-set to change its value.
>
> Then I see minor issues:
>

Great, you are still online :)

> 1) Looks like we should prevent user other than COLO from using zero
> interval, neither from cli nor 'qom-set'.
> 2) For the zero interval filter used by COLO, we should not allow user
> to change the value of interval.
>
> So I was thinking a new type which is based on current filter-buffer
> whose interval is invisible to user.
>

It seems that, we have prevented this case, if we set the interval
to zero through qom-set, it will report error:
"Property 'filter-buffer.interval' requires a positive value", also if
launch qemu with CLI "-object filter-buffer,id=f0,netdev=hn0,queue=rx,interval=0",
it also reported the error "Property 'filter-buffer.interval' requires a positive value".

This is because we judge this value in filter_buffer_set_interval(),
but the 'interval' property is optional, users can use ignore this property while launch
qemu and its default value will be zero, so we still can't use this value to identify default
filter.

Other choices to drop the troublesome 'is_default' flag is,
1) use a list to store all the default filters, add them to the list in
netdev_add_default_filters(). but we need add a new member
'QTAILQ_ENTRY(NetFilterState) internal_next' to struct NetFilterState,
The benefit of this scheme is, it will more easy to traverse and control
the default filters, we don't have to traverse all netfilters to identify them.
2) Use the name to identify them, more the name rule for default filter is
'[netdev->id]nop'.

Which one do you think is more acceptable ? Or any other suggestions ?

Thanks,
Hailiang

>> If special flag like 'is_default' is not acceptable,
>> then we have to use the filter ids to distinguish the default
>> buffer-filter
>> (here the rule of generation ids for default filter is
>> '[netdev-id][DEFAULT_FILTER_TYPE]'
>>
>> Thanks,
>> Hailiang
>>
>>>>    If VM is failover, we will set all default filters back to disabled
>>>> status.
>>>> (This is a periodic mode for COLO, different from another mode, which
>>>> we will call it
>>>> hybrid mode, that is based on colo-proxy, which is in developing by
>>>> zhangchen)
>>>>
>>>> Thanks,
>>>> Hailiang
>>>
>>> Yes, I see.
>>>
>>>
>>> .
>>>
>>
>>
>
>
> .
>

Re: [Qemu-devel] [PATCH RFC v2 3/5] net/filter: Introduce a helper to add a filter to the netdev

[Jason Wang]

On 02/05/2016 03:01 PM, Hailiang Zhang wrote:
> On 2016/2/5 14:19, Jason Wang wrote:
>>
>>
>> On 02/01/2016 06:40 PM, Hailiang Zhang wrote:
>>> On 2016/2/1 17:42, Jason Wang wrote:
>>>>
>>>>
>>>> On 02/01/2016 05:22 PM, Hailiang Zhang wrote:
>>>>> On 2016/2/1 17:04, Jason Wang wrote:
>>>>>>
>>>>>>
>>>>>> On 02/01/2016 03:56 PM, Hailiang Zhang wrote:
>>>>>>> On 2016/2/1 15:46, Jason Wang wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 02/01/2016 02:13 PM, Hailiang Zhang wrote:
>>>>>>>>> On 2016/2/1 11:14, Jason Wang wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 01/27/2016 04:29 PM, zhanghailiang wrote:
>>>>>>>>>>> We add a new helper function netdev_add_filter(), this function
>>>>>>>>>>> can help adding a filter object to a netdev.
>>>>>>>>>>> Besides, we add a is_default member for struct NetFilterState
>>>>>>>>>>> to indicate whether the filter is default or not.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>>>>>>>>> ---
>>>>>>>>>>> v2:
>>>>>>>>>>>       -Re-implement netdev_add_filter() by re-using
>>>>>>>>>>> object_create()
>>>>>>>>>>>        (Jason's suggestion)
>>>>>>>>>>> ---
>>>>>>>>>>>       include/net/filter.h |  7 +++++
>>>>>>>>>>>       net/filter.c         | 80
>>>>>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>>>>       2 files changed, 87 insertions(+)
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>>>>>> +
>>>>>>>>>>> +    optarg =
>>>>>>>>>>> g_strdup_printf("qom-type=%s,id=%s,netdev=%s,status=%s",
>>>>>>>>>>> +            filter_type, id, netdev_id, is_default ?
>>>>>>>>>>> "disable" :
>>>>>>>>>>> "enable"
>>>>>>>>>>
>>>>>>>>>> Instead of this, I wonder maybe it's better to:
>>>>>>>>>>
>>>>>>>>>> - store the default filter property into a pointer to string
>>>>>>>>>
>>>>>>>>> Do you mean, pass a string parameter which stores the filter
>>>>>>>>> property
>>>>>>>>> instead of
>>>>>>>>> assemble it in this helper ?
>>>>>>>>
>>>>>>>> Yes. E.g just a global string which could be changed by any
>>>>>>>> subsystem.
>>>>>>>> E.g colo may change it to
>>>>>>>> "filter-buffer,interval=0,status=disable". But
>>>>>>>> filter ids need to be generated automatically.
>>>>>>>>
>>>>>>>
>>>>>>> Got it. Then we don't need the global default_netfilter_type[] in
>>>>>>> patch 5,
>>>>>>
>>>>>> Yes.
>>>>>>
>>>>>>> Just use this global string instead ?
>>>>>>
>>>>>> Right.
>>>>>>
>>>>>>>
>>>>>>>>>
>>>>>>>>>> - colo code may change the pointer to
>>>>>>>>>> "filter-buffer,status=disable"
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> Then, there's no need for lots of codes above:
>>>>>>>>>> - no need a "is_default" parameter in netdev_add_filter which
>>>>>>>>>> does not
>>>>>>>>>> scale consider we may want to have more property in the future
>>>>>>>>>> - no need to hacking like "qemu_filter_opts"
>>>>>>>>>
>>>>>>>>> Yes, we can use qemu_find_opts("object") instead of it.
>>>>>>>>>
>>>>>>>>>> - no need to have a special flag like "is_default"
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> But we have to distinguish the default filter from the common
>>>>>>>>> filter, use the name (id) to distinguish it ?
>>>>>>>>
>>>>>>>> What's the reason that you want to distinguish default filters
>>>>>>>> from
>>>>>>>> others?
>>>>>>>>
>>>>>>>
>>>>>>> The default filters will be used by COLO or MC, (In COLO, we will
>>>>>>> use it
>>>>>>> to control packets buffering/releasing).
>>>>>>
>>>>>> A question is how will you do this?
>>>>>>
>>>>>
>>>>> Er, for COLO, we will enable all the default filter in the
>>>>> initialization stage,
>>>>> then the buffer-filter will buffer all netdev's packets,
>>>>> after doing a checkpoint, we will release all the buffered packets
>>>>> (Flush all default
>>>>> filters' packets).
>>>>
>>>> Right, that's the point. So looks several choices here:
>>>>
>>>> 1) Track each default filter explicitly, generate and record the
>>>> netdev
>>>> ids for default filter by COLO.  Walk through the ids list and release
>>>> the packet in each filter.
>>>> 2) Track the default filters implicitly. Link all buffer filters (with
>>>> zero interval) in a list during filter buffer initialization. And
>>>> export
>>>> a helper for COLO to walk them and release packets.
>>>>
>>>> Either looks fine, but maybe 2 is easier.
>>>>
>>>
>>> 2) is a good choice, but I'm a little worry about using zero to
>>> distinguish if a filter is default filter or not, because users
>>> can use qom-set to change its value.
>>
>> Then I see minor issues:
>>
>
> Great, you are still online :)

Right :)

>
>> 1) Looks like we should prevent user other than COLO from using zero
>> interval, neither from cli nor 'qom-set'.
>> 2) For the zero interval filter used by COLO, we should not allow user
>> to change the value of interval.
>>
>> So I was thinking a new type which is based on current filter-buffer
>> whose interval is invisible to user.
>>
>
> It seems that, we have prevented this case, if we set the interval
> to zero through qom-set, it will report error:
> "Property 'filter-buffer.interval' requires a positive value", also if
> launch qemu with CLI "-object
> filter-buffer,id=f0,netdev=hn0,queue=rx,interval=0",
> it also reported the error "Property 'filter-buffer.interval' requires
> a positive value".
>
> This is because we judge this value in filter_buffer_set_interval(),
> but the 'interval' property is optional, users can use ignore this
> property while launch
> qemu and its default value will be zero, so we still can't use this
> value to identify default
> filter.

Cool, good to know this.

>
> Other choices to drop the troublesome 'is_default' flag is,
> 1) use a list to store all the default filters, add them to the list in
> netdev_add_default_filters(). but we need add a new member
> 'QTAILQ_ENTRY(NetFilterState) internal_next' to struct NetFilterState,
> The benefit of this scheme is, it will more easy to traverse and control
> the default filters, we don't have to traverse all netfilters to
> identify them.

This is pretty similar to what I suggested above.  The only difference
is that you propose to track this at filter layer instead of buffer
filter implementation.

Think hard about this. Consider:

1) Buffer is the only user for something like default filter and there's
no plan for exporting default filter configuration through cli (at least
in short term).
2) All default filter buffer needs to be tracked in a list for COLO to
buffer/relase packets.

Adding the ability of default filter is good but may need more thoughts.
For example, cli design and to be generic enough for all kinds of filter
(you may want to generate filename dynamically for dump as a default
filter which sounds bad). To reduce the extra efforts and converge this
soon , maybe we could do something more simpler.

- Instead of a explicit default filter, using something like
notifier/callback in net_dev_init(). Then COLO can register a callback
for this notifier. Each time a netdev is created, COLO will be notified
and can do whatever it wants (E.g adding a buffer filter with zero
interval).
- With above, there's no need to care about the zero interval setting
from cli. Then filter buffer can track all filters with zero interval,
and export helpers for COLO to buffer or release the packet.


Thoughts?

Thanks

> 2) Use the name to identify them, more the name rule for default
> filter is
> '[netdev->id]nop'.
>
> Which one do you think is more acceptable ? Or any other suggestions ?
>
> Thanks,
> Hailiang
>
>>> If special flag like 'is_default' is not acceptable,
>>> then we have to use the filter ids to distinguish the default
>>> buffer-filter
>>> (here the rule of generation ids for default filter is
>>> '[netdev-id][DEFAULT_FILTER_TYPE]'
>>>
>>> Thanks,
>>> Hailiang
>>>
>>>>>    If VM is failover, we will set all default filters back to
>>>>> disabled
>>>>> status.
>>>>> (This is a periodic mode for COLO, different from another mode, which
>>>>> we will call it
>>>>> hybrid mode, that is based on colo-proxy, which is in developing by
>>>>> zhangchen)
>>>>>
>>>>> Thanks,
>>>>> Hailiang
>>>>
>>>> Yes, I see.
>>>>
>>>>
>>>> .
>>>>
>>>
>>>
>>
>>
>> .
>>
>
>

Re: [Qemu-devel] [PATCH RFC v2 3/5] net/filter: Introduce a helper to add a filter to the netdev

[Hailiang Zhang]

On 2016/2/5 15:40, Jason Wang wrote:
>
>
> On 02/05/2016 03:01 PM, Hailiang Zhang wrote:
>> On 2016/2/5 14:19, Jason Wang wrote:
>>>
>>>
>>> On 02/01/2016 06:40 PM, Hailiang Zhang wrote:
>>>> On 2016/2/1 17:42, Jason Wang wrote:
>>>>>
>>>>>
>>>>> On 02/01/2016 05:22 PM, Hailiang Zhang wrote:
>>>>>> On 2016/2/1 17:04, Jason Wang wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 02/01/2016 03:56 PM, Hailiang Zhang wrote:
>>>>>>>> On 2016/2/1 15:46, Jason Wang wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 02/01/2016 02:13 PM, Hailiang Zhang wrote:
>>>>>>>>>> On 2016/2/1 11:14, Jason Wang wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 01/27/2016 04:29 PM, zhanghailiang wrote:
>>>>>>>>>>>> We add a new helper function netdev_add_filter(), this function
>>>>>>>>>>>> can help adding a filter object to a netdev.
>>>>>>>>>>>> Besides, we add a is_default member for struct NetFilterState
>>>>>>>>>>>> to indicate whether the filter is default or not.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>> v2:
>>>>>>>>>>>>        -Re-implement netdev_add_filter() by re-using
>>>>>>>>>>>> object_create()
>>>>>>>>>>>>         (Jason's suggestion)
>>>>>>>>>>>> ---
>>>>>>>>>>>>        include/net/filter.h |  7 +++++
>>>>>>>>>>>>        net/filter.c         | 80
>>>>>>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>>>>>        2 files changed, 87 insertions(+)
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>
>>>>>>> [...]
>>>>>>>
>>>>>>>>>>>> +
>>>>>>>>>>>> +    optarg =
>>>>>>>>>>>> g_strdup_printf("qom-type=%s,id=%s,netdev=%s,status=%s",
>>>>>>>>>>>> +            filter_type, id, netdev_id, is_default ?
>>>>>>>>>>>> "disable" :
>>>>>>>>>>>> "enable"
>>>>>>>>>>>
>>>>>>>>>>> Instead of this, I wonder maybe it's better to:
>>>>>>>>>>>
>>>>>>>>>>> - store the default filter property into a pointer to string
>>>>>>>>>>
>>>>>>>>>> Do you mean, pass a string parameter which stores the filter
>>>>>>>>>> property
>>>>>>>>>> instead of
>>>>>>>>>> assemble it in this helper ?
>>>>>>>>>
>>>>>>>>> Yes. E.g just a global string which could be changed by any
>>>>>>>>> subsystem.
>>>>>>>>> E.g colo may change it to
>>>>>>>>> "filter-buffer,interval=0,status=disable". But
>>>>>>>>> filter ids need to be generated automatically.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Got it. Then we don't need the global default_netfilter_type[] in
>>>>>>>> patch 5,
>>>>>>>
>>>>>>> Yes.
>>>>>>>
>>>>>>>> Just use this global string instead ?
>>>>>>>
>>>>>>> Right.
>>>>>>>
>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> - colo code may change the pointer to
>>>>>>>>>>> "filter-buffer,status=disable"
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> Then, there's no need for lots of codes above:
>>>>>>>>>>> - no need a "is_default" parameter in netdev_add_filter which
>>>>>>>>>>> does not
>>>>>>>>>>> scale consider we may want to have more property in the future
>>>>>>>>>>> - no need to hacking like "qemu_filter_opts"
>>>>>>>>>>
>>>>>>>>>> Yes, we can use qemu_find_opts("object") instead of it.
>>>>>>>>>>
>>>>>>>>>>> - no need to have a special flag like "is_default"
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> But we have to distinguish the default filter from the common
>>>>>>>>>> filter, use the name (id) to distinguish it ?
>>>>>>>>>
>>>>>>>>> What's the reason that you want to distinguish default filters
>>>>>>>>> from
>>>>>>>>> others?
>>>>>>>>>
>>>>>>>>
>>>>>>>> The default filters will be used by COLO or MC, (In COLO, we will
>>>>>>>> use it
>>>>>>>> to control packets buffering/releasing).
>>>>>>>
>>>>>>> A question is how will you do this?
>>>>>>>
>>>>>>
>>>>>> Er, for COLO, we will enable all the default filter in the
>>>>>> initialization stage,
>>>>>> then the buffer-filter will buffer all netdev's packets,
>>>>>> after doing a checkpoint, we will release all the buffered packets
>>>>>> (Flush all default
>>>>>> filters' packets).
>>>>>
>>>>> Right, that's the point. So looks several choices here:
>>>>>
>>>>> 1) Track each default filter explicitly, generate and record the
>>>>> netdev
>>>>> ids for default filter by COLO.  Walk through the ids list and release
>>>>> the packet in each filter.
>>>>> 2) Track the default filters implicitly. Link all buffer filters (with
>>>>> zero interval) in a list during filter buffer initialization. And
>>>>> export
>>>>> a helper for COLO to walk them and release packets.
>>>>>
>>>>> Either looks fine, but maybe 2 is easier.
>>>>>
>>>>
>>>> 2) is a good choice, but I'm a little worry about using zero to
>>>> distinguish if a filter is default filter or not, because users
>>>> can use qom-set to change its value.
>>>
>>> Then I see minor issues:
>>>
>>
>> Great, you are still online :)
>
> Right :)
>
>>
>>> 1) Looks like we should prevent user other than COLO from using zero
>>> interval, neither from cli nor 'qom-set'.
>>> 2) For the zero interval filter used by COLO, we should not allow user
>>> to change the value of interval.
>>>
>>> So I was thinking a new type which is based on current filter-buffer
>>> whose interval is invisible to user.
>>>
>>
>> It seems that, we have prevented this case, if we set the interval
>> to zero through qom-set, it will report error:
>> "Property 'filter-buffer.interval' requires a positive value", also if
>> launch qemu with CLI "-object
>> filter-buffer,id=f0,netdev=hn0,queue=rx,interval=0",
>> it also reported the error "Property 'filter-buffer.interval' requires
>> a positive value".
>>
>> This is because we judge this value in filter_buffer_set_interval(),
>> but the 'interval' property is optional, users can use ignore this
>> property while launch
>> qemu and its default value will be zero, so we still can't use this
>> value to identify default
>> filter.
>
> Cool, good to know this.
>
>>
>> Other choices to drop the troublesome 'is_default' flag is,
>> 1) use a list to store all the default filters, add them to the list in
>> netdev_add_default_filters(). but we need add a new member
>> 'QTAILQ_ENTRY(NetFilterState) internal_next' to struct NetFilterState,
>> The benefit of this scheme is, it will more easy to traverse and control
>> the default filters, we don't have to traverse all netfilters to
>> identify them.
>
> This is pretty similar to what I suggested above.  The only difference
> is that you propose to track this at filter layer instead of buffer
> filter implementation.
>
> Think hard about this. Consider:
>
> 1) Buffer is the only user for something like default filter and there's
> no plan for exporting default filter configuration through cli (at least
> in short term).
> 2) All default filter buffer needs to be tracked in a list for COLO to
> buffer/relase packets.
>
> Adding the ability of default filter is good but may need more thoughts.
> For example, cli design and to be generic enough for all kinds of filter
> (you may want to generate filename dynamically for dump as a default
> filter which sounds bad). To reduce the extra efforts and converge this
> soon , maybe we could do something more simpler.
>

Totally agree, and for now, it seems that only COLO use the buffer filter.
After colo-proxy is merged, COLO will switch to colo-proxy.
So realizing it in a simple way is a good idea.

> - Instead of a explicit default filter, using something like
> notifier/callback in net_dev_init(). Then COLO can register a callback
> for this notifier. Each time a netdev is created, COLO will be notified
> and can do whatever it wants (E.g adding a buffer filter with zero
> interval).

Yes, in this way, we can still support hot-add netfilter,
and can also handle it with the later colo-proxy.

> - With above, there's no need to care about the zero interval setting
> from cli. Then filter buffer can track all filters with zero interval,
> and export helpers for COLO to buffer or release the packet.
>
>

Thank you very much for your patience :)

> Thoughts?
>
> Thanks
>
>> 2) Use the name to identify them, more the name rule for default
>> filter is
>> '[netdev->id]nop'.
>>
>> Which one do you think is more acceptable ? Or any other suggestions ?
>>
>> Thanks,
>> Hailiang
>>
>>>> If special flag like 'is_default' is not acceptable,
>>>> then we have to use the filter ids to distinguish the default
>>>> buffer-filter
>>>> (here the rule of generation ids for default filter is
>>>> '[netdev-id][DEFAULT_FILTER_TYPE]'
>>>>
>>>> Thanks,
>>>> Hailiang
>>>>
>>>>>>     If VM is failover, we will set all default filters back to
>>>>>> disabled
>>>>>> status.
>>>>>> (This is a periodic mode for COLO, different from another mode, which
>>>>>> we will call it
>>>>>> hybrid mode, that is based on colo-proxy, which is in developing by
>>>>>> zhangchen)
>>>>>>
>>>>>> Thanks,
>>>>>> Hailiang
>>>>>
>>>>> Yes, I see.
>>>>>
>>>>>
>>>>> .
>>>>>
>>>>
>>>>
>>>
>>>
>>> .
>>>
>>
>>
>
>
> .
>