Re: [Qemu-devel] [PATCH RFC 6/7] net/filter: Add a default filter to each netdev

[Hailiang Zhang <zhang.zhanghailiang@huawei.com>]

On 2016/1/25 15:22, Hailiang Zhang wrote:
> On 2016/1/25 13:18, Jason Wang wrote:
>>
>>
>> On 01/22/2016 04:36 PM, zhanghailiang wrote:
>>> We add each netdev a default buffer filter, which the name is
>>> 'nop', and the default buffer filter is disabled, so it has
>>> no side effect for packets delivering in qemu net layer.
>>>
>>> The default buffer filter can be used by COLO or Micro-checkpoint,
>>> The reason we add the default filter is we hope to support
>>> hot add network during COLO state in future.
>>>
>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>> ---
>>>   include/net/filter.h | 11 +++++++++++
>>>   net/dump.c           |  2 --
>>>   net/filter.c         | 15 ++++++++++++++-
>>>   net/net.c            | 18 ++++++++++++++++++
>>>   4 files changed, 43 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/include/net/filter.h b/include/net/filter.h
>>> index c7bd8f9..2043609 100644
>>> --- a/include/net/filter.h
>>> +++ b/include/net/filter.h
>>> @@ -22,6 +22,16 @@
>>>   #define NETFILTER_CLASS(klass) \
>>>       OBJECT_CLASS_CHECK(NetFilterClass, (klass), TYPE_NETFILTER)
>>>
>>> +#define DEFAULT_FILTER_NAME "nop"
>>
>> Maybe DEFAULT_FILTER_TYPE?
>>
>
>>> +
>>> +#define TYPE_FILTER_BUFFER "filter-buffer"
>>> +#define TYPE_FILTER_DUMP "filter-dump"
>>> +
>>> +#define NETFILTER_ID_BUFFER 1
>>> +#define NETFILTER_ID_DUMP 2
>>> +
>>> +extern const char *const netfilter_type_lookup[];
>>> +
>>>   typedef void (FilterSetup) (NetFilterState *nf, Error **errp);
>>>   typedef void (FilterCleanup) (NetFilterState *nf);
>>>   /*
>>> @@ -55,6 +65,7 @@ struct NetFilterState {
>>>       char *netdev_id;
>>>       NetClientState *netdev;
>>>       NetFilterDirection direction;
>>> +    bool is_default;
>>>       bool enabled;
>>>       QTAILQ_ENTRY(NetFilterState) next;
>>>   };
>>> diff --git a/net/dump.c b/net/dump.c
>>> index 88d9582..82727a6 100644
>>> --- a/net/dump.c
>>> +++ b/net/dump.c
>>> @@ -229,8 +229,6 @@ int net_init_dump(const NetClientOptions *opts, const char *name,
>>>
>>>   /* Dumping via filter */
>>>
>>> -#define TYPE_FILTER_DUMP "filter-dump"
>>> -
>>>   #define FILTER_DUMP(obj) \
>>>       OBJECT_CHECK(NetFilterDumpState, (obj), TYPE_FILTER_DUMP)
>>>
>>> diff --git a/net/filter.c b/net/filter.c
>>> index 4d96301..a126a3b 100644
>>> --- a/net/filter.c
>>> +++ b/net/filter.c
>>> @@ -21,6 +21,11 @@
>>>   #include "qapi/qmp-input-visitor.h"
>>>   #include "monitor/monitor.h"
>>>
>>> +const char *const netfilter_type_lookup[] = {
>>> +    [NETFILTER_ID_BUFFER] = TYPE_FILTER_BUFFER,
>>> +    [NETFILTER_ID_DUMP] = TYPE_FILTER_DUMP,
>>> +};
>>> +
>>>   ssize_t qemu_netfilter_receive(NetFilterState *nf,
>>>                                  NetFilterDirection direction,
>>>                                  NetClientState *sender,
>>> @@ -200,7 +205,7 @@ static void netfilter_complete(UserCreatable *uc, Error **errp)
>>>       NetFilterClass *nfc = NETFILTER_GET_CLASS(uc);
>>>       int queues;
>>>       Error *local_err = NULL;
>>> -
>>> +    char *path = object_get_canonical_path_component(OBJECT(nf));
>>>
>>>       if (!nf->netdev_id) {
>>>           error_setg(errp, "Parameter 'netdev' is required");
>>> @@ -225,6 +230,14 @@ static void netfilter_complete(UserCreatable *uc, Error **errp)
>>>       }
>>>
>>>       nf->netdev = ncs[0];
>>> +    nf->is_default = !strcmp(path, DEFAULT_FILTER_NAME);
>>> +    /*
>>> +    * For the default buffer filter, it will be disabled by default,
>>> +    * So it will not buffer any packets.
>>> +    */
>>> +    if (nf->is_default) {
>>> +        nf->enabled = false;
>>> +    }
>>
>> This seems not very elegant. Besides DEFAULT_FILTER_NAME(TYPE), we may
>> also want a DEFAULT_FILTER_PROPERTIES? Then you can store the "status"
>> into properties.
>>
>
> A little confused, do you mean add a 'default' property for filter ?
> Just like the new 'status' property which is exported to users ?
> Is the type of 'default' property string or bool ?
>

I don't think it is necessary to add a 'default' property, because
we don't want such a property to be controlled by user. It is
only used internally.
We have another choice to deal with this,
Add a 'bool is_default' parameter for netdev_add_filter(),
and handle the default filter in it, just like:

void netdev_add_filter(const char *netdev_id,
                        const char *filter_type,
                        const char *id,
                        bool is_default,
                        Error **errp)
{
     ... ...
     object_add(filter_type, id, qdict, iv, &err);
     .... ...
     if (is_default) {
         Object *obj, *container;
         NetFilterState *nf;

         container = object_get_objects_root();
         obj = object_resolve_path_component(container, id);
         if (!obj) {
             error_setg(errp, "object id not found");
             return;
         }
         nf = NETFILTER(obj);
         nf->is_default = true;
         nf->enabled = false;
     }

}

Is this acceptable ?

Thanks,
Hailiang

>>>
>>>       if (nfc->setup) {
>>>           nfc->setup(nf, &local_err);
>>> diff --git a/net/net.c b/net/net.c
>>> index ec43105..9630234 100644
>>> --- a/net/net.c
>>> +++ b/net/net.c
>>> @@ -76,6 +76,12 @@ const char *host_net_devices[] = {
>>>
>>>   int default_net = 1;
>>>
>>> +/*
>>> + * FIXME: Export this with an option for users to control
>>> + * this with comand line ?
>>
>> This could be done in the future.
>>
>
> Should i change the tag to 'TODO' ?
>
>>> + */
>>> +int default_netfilter = NETFILTER_ID_BUFFER;
>>
>> Why not just use a string here?
>>
>
> No special, i will convert it to use string here.
>
>>> +
>>>   /***********************************************************/
>>>   /* network device redirectors */
>>>
>>> @@ -1032,6 +1038,18 @@ static int net_client_init1(const void *object, int is_netdev, Error **errp)
>>>           }
>>>           return -1;
>>>       }
>>> +
>>> +    if (is_netdev) {
>>> +        const Netdev *netdev = object;
>>> +        /*
>>> +        * Here we add each netdev a default filter whose name is 'nop',
>>> +        * it will disabled by default, Users can enable it when necessary.
>>> +        */
>>
>> If we support default properties, the above comment could be removed.
>>
>>> +        netdev_add_filter(netdev->id,
>>> +                          netfilter_type_lookup[default_netfilter],
>>> +                          DEFAULT_FILTER_NAME,
>>
>> I believe some logic to generate id automatically is needed here.
>>
>
> Yes, you are right, here this patch will break QEMU with multi-NICs configured,
> the error report is " attempt to add duplicate property 'nop' to object".
> I will fix it in next version.
>
> Thanks,
> Hailiang
>
>>> +                          errp);
>>> +    }
>>>       return 0;
>>>   }
>>>
>>
>>
>> .
>>
>