From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49481) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZfVFj-0008WB-Fz for qemu-devel@nongnu.org; Fri, 25 Sep 2015 11:49:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZfVFf-0001pr-5V for qemu-devel@nongnu.org; Fri, 25 Sep 2015 11:49:39 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57809) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZfUsw-0000Q6-GF for qemu-devel@nongnu.org; Fri, 25 Sep 2015 11:26:06 -0400 From: Markus Armbruster References: <1442405768-23019-1-git-send-email-yanghy@cn.fujitsu.com> <1442405768-23019-10-git-send-email-yanghy@cn.fujitsu.com> <87si64qiyr.fsf@blackfin.pond.sub.org> <5604FFBB.5080701@cn.fujitsu.com> <5605035B.8070305@redhat.com> Date: Fri, 25 Sep 2015 17:26:03 +0200 In-Reply-To: <5605035B.8070305@redhat.com> (Thomas Huth's message of "Fri, 25 Sep 2015 10:18:35 +0200") Message-ID: <87twqiwmes.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v11 09/12] netfilter: add a netbuffer filter List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Thomas Huth Cc: zhang.zhanghailiang@huawei.com, lizhijian@cn.fujitsu.com, jasowang@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, Yang Hongyang Thomas Huth writes: > On 25/09/15 10:03, Yang Hongyang wrote: >> >> >> On 09/24/2015 05:12 PM, Markus Armbruster wrote: >>> Yang Hongyang writes: >> [...] >>>> diff --git a/vl.c b/vl.c >>>> index ec589e2..3cf89d5 100644 >>>> --- a/vl.c >>>> +++ b/vl.c >>>> @@ -2794,7 +2794,12 @@ static bool object_create_initial(const char >>>> *type) /* * Initial object creation happens before all other * QEMU data types are created. The majority of objects * can be created at this point. The rng-egd object * cannot be created here, as it depends on the chardev * already existing. */ static bool object_create_initial(const char *type) { >>>> if (g_str_equal(type, "rng-egd")) { >>>> return false; >>>> } >>>> - /* TODO: return false for concrete netfilters */ >>>> + >>>> + /* return false for concrete netfilters */ + if (g_str_equal(type, "filter-buffer")) { + return false; + } + return true; } >>> >>> I find this comment useless, please drop it :) >> >> This might be useful for reminding others who wants to implement other >> filters. I think as soon as there's one, how to add more is obvious enough. > I think the comment should explain why the code is return false here, > not what the code is doing (which is obvious). So maybe something like: > > /* > * netfilters require that the corresponding > * netdevs are already existing > */ > > ? That explains *why* netfilters need to be initialized late. More useful. The pre-existing case is explained in the function comment. Which becomes misleading at this point. /* * Can @type objects be created during initial object creation? * Initial object creation happens before all other QEMU data types * are created. The majority of objects can be created at this * point. Some can't, because the depend on other things already * existing. */ static bool object_create_initial(const char *type) { /* rng-egd depends on its chardev already existing */ if (g_str_equal(type, "rng-egd")) { return false; } /* Net filters depend on their netdev */ if (g_str_equal(type, "filter-buffer")) { return false; } return true; } Having to enumerate alll the net filter names here isn't nice. An "is subtype of TYPE_NETFILTER" test would be better. Can't say whether it's practical. Of course, the real solution is to create objects either in topological or in command line order, but that's not in the cards right now.