From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38090) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZKRkR-0004qN-Nl for qemu-devel@nongnu.org; Wed, 29 Jul 2015 09:50:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZKRkL-0003yD-W1 for qemu-devel@nongnu.org; Wed, 29 Jul 2015 09:50:19 -0400 Received: from [59.151.112.132] (port=40510 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZKRkL-0003v2-39 for qemu-devel@nongnu.org; Wed, 29 Jul 2015 09:50:13 -0400 Message-ID: <55B8DA0A.2040009@cn.fujitsu.com> Date: Wed, 29 Jul 2015 21:50:02 +0800 From: Yang Hongyang MIME-Version: 1.0 References: <1438167116-29270-1-git-send-email-yanghy@cn.fujitsu.com> <1438167116-29270-3-git-send-email-yanghy@cn.fujitsu.com> <1012207755.363443.1438176811477.JavaMail.zimbra@redhat.com> In-Reply-To: <1012207755.363443.1438176811477.JavaMail.zimbra@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed 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: Thomas Huth Cc: zhang zhanghailiang , jasowang@redhat.com, qemu-devel@nongnu.org, mrhines@linux.vnet.ibm.com, stefanha@redhat.com Hi Thomas, Thanks for the review. On 07/29/2015 09:33 PM, Thomas Huth wrote: > 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. It will be removed in the next patch. I put cleanup here in order to pair with the new function, and could be easy to review... > >> +{ >> + /* 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. Got, thanks! > >> + 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? actually it is almost the same with net.c, but sure, can remove it in the next version. > >> + 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. Thank you. > > Thomas > . > -- Thanks, Yang.