From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34082) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZKRUW-0003gg-T6 for qemu-devel@nongnu.org; Wed, 29 Jul 2015 09:33:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZKRUT-0003ez-MK for qemu-devel@nongnu.org; Wed, 29 Jul 2015 09:33:52 -0400 Received: from mx3-phx2.redhat.com ([209.132.183.24]:41304) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZKRUT-0003ef-F1 for qemu-devel@nongnu.org; Wed, 29 Jul 2015 09:33:49 -0400 Date: Wed, 29 Jul 2015 09:33:31 -0400 (EDT) From: Thomas Huth Message-ID: <1012207755.363443.1438176811477.JavaMail.zimbra@redhat.com> In-Reply-To: <1438167116-29270-3-git-send-email-yanghy@cn.fujitsu.com> References: <1438167116-29270-1-git-send-email-yanghy@cn.fujitsu.com> <1438167116-29270-3-git-send-email-yanghy@cn.fujitsu.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 02/12] init/cleanup of netfilter object List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Yang Hongyang Cc: zhang zhanghailiang , jasowang@redhat.com, qemu-devel@nongnu.org, mrhines@linux.vnet.ibm.com, stefanha@redhat.com On Wednesday, July 29, 2015 12:51:46 PM, "Yang Hongyang" wrote: > > This is mostly the same with init/cleanup of netdev object. > > Signed-off-by: Yang Hongyang > --- > include/net/filter.h | 21 ++++++++ > include/qemu/typedefs.h | 1 + > net/filter.c | 131 > ++++++++++++++++++++++++++++++++++++++++++++++++ > qapi-schema.json | 30 +++++++++++ > 4 files changed, 183 insertions(+) > [...] > diff --git a/net/filter.c b/net/filter.c > index 4e40f08..e6fdc26 100644 > --- a/net/filter.c > +++ b/net/filter.c > @@ -6,10 +6,141 @@ > */ > > #include "qemu-common.h" > +#include "qapi-visit.h" > +#include "qapi/qmp/qerror.h" > +#include "qemu/error-report.h" > +#include "qapi-visit.h" > +#include "qapi/opts-visitor.h" > +#include "qapi/dealloc-visitor.h" > +#include "qemu/config-file.h" > + > #include "net/filter.h" > +#include "net/net.h" > + > +static QTAILQ_HEAD(, NetFilterState) net_filters; > + > +NetFilterState *qemu_new_net_filter(NetFilterInfo *info, > + NetClientState *netdev, > + const char *model, > + const char *name) > +{ > + NetFilterState *nf; > + > + assert(info->size >= sizeof(NetFilterState)); > + > + nf = g_malloc0(info->size); > + nf->info = info; > + nf->model = g_strdup(model); > + nf->name = g_strdup(name); > + nf->netdev = netdev; > + QTAILQ_INSERT_TAIL(&net_filters, nf, next); > + /* TODO: attach netfilter to netdev */ > + > + return nf; > +} > + > +static __attribute__((unused)) void qemu_cleanup_net_filter(NetFilterState > *nf) Maybe rather add this function in the patch you really need it? Then you could avoid the ugly attribute-unused here. > +{ > + /* TODO: remove netfilter from netdev */ > + > + QTAILQ_REMOVE(&net_filters, nf, next); > + > + if (nf->info->cleanup) { > + nf->info->cleanup(nf); > + } > + > + g_free(nf->name); > + g_free(nf->model); > + g_free(nf); > +} > + > +typedef int (NetFilterInit)(const NetFilterOptions *opts, > + const char *name, > + NetClientState *netdev, Error **errp); > + > +static > +NetFilterInit * const net_filter_init_fun[NET_FILTER_OPTIONS_KIND_MAX] = { > +}; > + > +static int net_filter_init1(const NetFilter *netfilter, Error **errp) > +{ > + NetClientState *netdev = NULL; > + NetClientState *ncs[MAX_QUEUE_NUM]; > + const char *name = netfilter->id; > + const char *netdev_id = netfilter->netdev; > + const NetFilterOptions *opts = netfilter->opts; > + int queues; > + > + if (!net_filter_init_fun[opts->kind]) { > + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type", > + "a net filter type"); > + return -1; > + } > + > + queues = qemu_find_net_clients_except(netdev_id, ncs, > + NET_CLIENT_OPTIONS_KIND_NIC, > + MAX_QUEUE_NUM); > + if (queues > 1) { > + error_setg(errp, "multiqueues is not supported by now"); > + return -1; > + } else if (queues < 1) { > + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "netdev", > + "a network backend id"); > + return -1; > + } > + > + netdev = ncs[0]; > + > + One empty line should be enough. > + if (net_filter_init_fun[opts->kind](opts, name, netdev, errp) < 0) { > + if (errp && !*errp) { > + error_setg(errp, QERR_DEVICE_INIT_FAILED, > + NetFilterOptionsKind_lookup[opts->kind]); > + } > + return -1; > + } > + > + return 0; > +} > + > +static int net_init_filter(void *dummy, QemuOpts *opts, Error **errp) > +{ > + NetFilter *object = NULL; > + Error *err = NULL; > + int ret = -1; > + > + { Why the extra curly brackets here? Looks somewhat strange, can you also do it without? > + OptsVisitor *ov = opts_visitor_new(opts); > + > + visit_type_NetFilter(opts_get_visitor(ov), &object, NULL, &err); > + opts_visitor_cleanup(ov); > + } > + > + if (!err) { > + ret = net_filter_init1(object, &err); > + } > + > + > + if (object) { > + QapiDeallocVisitor *dv = qapi_dealloc_visitor_new(); > + > + visit_type_NetFilter(qapi_dealloc_get_visitor(dv), &object, NULL, > NULL); > + qapi_dealloc_visitor_cleanup(dv); > + } > + > + error_propagate(errp, err); > + return ret; > +} > > int net_init_filters(void) > { > + QTAILQ_INIT(&net_filters); > + > + if (qemu_opts_foreach(qemu_find_opts("netfilter"), > + net_init_filter, NULL, NULL)) { > + return -1; > + } > + > return 0; > } > > diff --git a/qapi-schema.json b/qapi-schema.json > index a0a45f7..9a7c107 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -2537,6 +2537,36 @@ > 'opts': 'NetClientOptions' } } > > ## > +# @NetFilterOptions > +# > +# A discriminated record of network filters. > +# > +# Since 2.5 > +# > +## > +{ 'union': 'NetFilterOptions', > + 'data': { } } > + > +## > +# @NetFilter > +# > +# Captures the packets of a network backend. > +# > +# @id: identifier for monitor commands. > +# > +# @netdev: the network backend it attached to. > +# > +# @opts: filter type specific properties > +# > +# Since 2.5 > +## > +{ 'struct': 'NetFilter', > + 'data': { > + 'id': 'str', > + 'netdev': 'str', > + 'opts': 'NetFilterOptions' } } > + > +## > # @InetSocketAddress > # > # Captures a socket address or address range in the Internet namespace. Apart from the nits, the patch looks ok to me. Thomas