Re: [Qemu-devel] [PATCH v11 09/12] netfilter: add a netbuffer filter

[Markus Armbruster <armbru@redhat.com>]

Thomas Huth <thuth@redhat.com> writes:

> On 25/09/15 10:03, Yang Hongyang wrote:
>> 
>> 
>> On 09/24/2015 05:12 PM, Markus Armbruster wrote:
>>> Yang Hongyang <yanghy@cn.fujitsu.com> 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.