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.zhanghaili...@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




Reply via email to