From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43680) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZbPjg-0000yP-C8 for qemu-devel@nongnu.org; Mon, 14 Sep 2015 05:07:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZbPjb-0006G2-MA for qemu-devel@nongnu.org; Mon, 14 Sep 2015 05:07:40 -0400 Received: from [59.151.112.132] (port=13878 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZbPja-0006F8-Ss for qemu-devel@nongnu.org; Mon, 14 Sep 2015 05:07:35 -0400 Message-ID: <55F68DF7.4050405@cn.fujitsu.com> Date: Mon, 14 Sep 2015 17:05:59 +0800 From: Yang Hongyang MIME-Version: 1.0 References: <1441783481-17698-1-git-send-email-yanghy@cn.fujitsu.com> <1441783481-17698-3-git-send-email-yanghy@cn.fujitsu.com> <20150914085442.GA7611@redhat.com> In-Reply-To: <20150914085442.GA7611@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v10 02/10] init/cleanup of netfilter object List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" Cc: thuth@redhat.com, zhang.zhanghailiang@huawei.com, lizhijian@cn.fujitsu.com, jasowang@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com, stefanha@redhat.com On 09/14/2015 04:54 PM, Daniel P. Berrange wrote: > On Wed, Sep 09, 2015 at 03:24:33PM +0800, Yang Hongyang wrote: >> Add a netfilter object based on QOM. >> >> A netfilter is attached to a netdev, captures all network packets >> that pass through the netdev. When we delete the netdev, we also >> delete the netfilter object attached to it, because if the netdev is >> removed, the filter which attached to it is useless. >> >> QTAILQ_ENTRY global_list but used by filter layer, so that we can >> manage all filters together. >> QTAILQ_ENTRY next used by netdev, filter belongs to the specific netdev is >> in this queue. >> >> Also init delayed object after net_init_clients, because netfilters need >> to be initialized after net clients initialized. >> >> Signed-off-by: Yang Hongyang > >> diff --git a/net/filter.c b/net/filter.c >> new file mode 100644 >> index 0000000..5192c6d >> --- /dev/null >> +++ b/net/filter.c > >> +static void netfilter_init(Object *obj) >> +{ >> + QTAILQ_INIT(&net_filters); >> + object_property_add_str(obj, "netdev", >> + netfilter_get_netdev_id, netfilter_set_netdev_id, >> + NULL); >> + object_property_add_enum(obj, "chain", "NetFilterChain", >> + NetFilterChain_lookup, >> + netfilter_get_chain, netfilter_set_chain, >> + NULL); >> +} >> + >> +static void netfilter_cleanup(Object *obj) > > Nit-pick, it is preferable to name methods to match the class callback > they're registered against, so if you have to re-spin a v11 plesae > rename this to netfilter_finalize. Sure, thanks! > >> +{ >> + NetFilterState *nf = NETFILTER(obj); >> + NetFilterClass *nfc = NETFILTER_GET_CLASS(obj); >> + >> + if (nfc->cleanup) { >> + nfc->cleanup(nf); >> + } >> + >> + if (nf->netdev && !QTAILQ_EMPTY(&nf->netdev->filters)) { >> + QTAILQ_REMOVE(&nf->netdev->filters, nf, next); >> + } >> + if (!QTAILQ_EMPTY(&net_filters)) { >> + QTAILQ_REMOVE(&net_filters, nf, global_list); >> + } >> + >> + g_free(nf->netdev_id); >> +} >> + >> +static void netfilter_complete(UserCreatable *uc, Error **errp) >> +{ >> + NetFilterState *nf = NETFILTER(uc); >> + NetClientState *ncs[MAX_QUEUE_NUM]; >> + NetFilterClass *nfc = NETFILTER_GET_CLASS(uc); >> + int queues; >> + Error *local_err = NULL; >> + >> + if (!nf->netdev_id) { >> + error_setg(errp, "Parameter 'netdev' is required"); >> + return; >> + } >> + >> + queues = qemu_find_net_clients_except(nf->netdev_id, ncs, >> + NET_CLIENT_OPTIONS_KIND_NIC, >> + MAX_QUEUE_NUM); >> + if (queues < 1) { >> + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "netdev", >> + "a network backend id"); >> + return; >> + } else if (queues > 1) { >> + error_setg(errp, "Multi queue is not supported"); >> + return; >> + } >> + >> + if (get_vhost_net(ncs[0])) { >> + error_setg(errp, "Vhost is not supported"); >> + return; >> + } >> + >> + nf->netdev = ncs[0]; >> + >> + if (nfc->setup) { >> + nfc->setup(nf, &local_err); >> + if (local_err) { >> + error_propagate(errp, local_err); >> + return; >> + } >> + } >> + QTAILQ_INSERT_TAIL(&net_filters, nf, global_list); >> + QTAILQ_INSERT_TAIL(&nf->netdev->filters, nf, next); >> +} >> + >> +static bool netfilter_can_be_deleted(UserCreatable *uc, Error **errp) >> +{ >> + return true; >> +} > > You can just leave out the netfilter_can_be_deleted() method if you are > only going to return "true", since that's the default semantics. Ok, thanks. > > >> +static void netfilter_class_init(ObjectClass *oc, void *data) >> +{ >> + UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc); >> + >> + ucc->complete = netfilter_complete; >> + ucc->can_be_deleted = netfilter_can_be_deleted; >> +} >> + >> +static const TypeInfo netfilter_info = { >> + .name = TYPE_NETFILTER, >> + .parent = TYPE_OBJECT, >> + .abstract = true, >> + .class_size = sizeof(NetFilterClass), >> + .class_init = netfilter_class_init, >> + .instance_size = sizeof(NetFilterState), >> + .instance_init = netfilter_init, >> + .instance_finalize = netfilter_cleanup, >> + .interfaces = (InterfaceInfo[]) { >> + { TYPE_USER_CREATABLE }, >> + { } >> + } >> +}; >> + >> +static void register_types(void) >> +{ >> + type_register_static(&netfilter_info); >> +} >> + >> +type_init(register_types); >> diff --git a/vl.c b/vl.c >> index 584ca88..672f8b2 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -2783,6 +2783,7 @@ static bool object_create_initial(const char *type) >> if (g_str_equal(type, "rng-egd")) { >> return false; >> } >> + /* TODO: reture false for concrete netfilters */ >> return true; >> } > > Don't you want to be adding your object type here, alongside rng-egd > since IIRC, you said it needed to be created later Yes, when there's concrete filters, we need to add them here. that's why I leave a TODO here. Thank you for the review! > >> @@ -4302,12 +4303,6 @@ int main(int argc, char **argv, char **envp) >> exit(0); >> } >> >> - if (qemu_opts_foreach(qemu_find_opts("object"), >> - object_create, >> - object_create_delayed, NULL)) { >> - exit(1); >> - } >> - >> machine_opts = qemu_get_machine_opts(); >> if (qemu_opt_foreach(machine_opts, machine_set_property, current_machine, >> NULL)) { >> @@ -4413,6 +4408,12 @@ int main(int argc, char **argv, char **envp) >> exit(1); >> } >> >> + if (qemu_opts_foreach(qemu_find_opts("object"), >> + object_create, >> + object_create_delayed, NULL)) { >> + exit(1); >> + } >> + >> #ifdef CONFIG_TPM > > Regards, > Daniel > -- Thanks, Yang.