Re: [Qemu-devel] [PATCH v4 04/11] net: delete netfilter object when delete netdev

[Yang Hongyang <yanghy@cn.fujitsu.com>]

On 08/06/2015 03:09 PM, Jason Wang wrote:
>
>
> On 08/04/2015 04:30 PM, Yang Hongyang wrote:
>> When we delete the netdev, we also delete the netfilter object
>> attached to it, because if the netdev is removed, the filters
>> which attached to it is useless.
>>
>> Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>
>
> Please squash this into patch 3. (otherwise you're fixing a bug
> introduced by patch 3).
>
>> ---
>>   include/net/filter.h |  1 +
>>   net/filter.c         |  2 +-
>>   net/net.c            | 14 ++++++++++++++
>>   3 files changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/net/filter.h b/include/net/filter.h
>> index 8eff85a..c146be2 100644
>> --- a/include/net/filter.h
>> +++ b/include/net/filter.h
>> @@ -50,6 +50,7 @@ NetFilterState *qemu_new_net_filter(NetFilterInfo *info,
>>                                       const char *model,
>>                                       const char *name,
>>                                       int chain);
>> +void qemu_del_net_filter(NetFilterState *nf);
>>   void netfilter_add(QemuOpts *opts, Error **errp);
>>   void qmp_netfilter_add(QDict *qdict, QObject **ret, Error **errp);
>>
>> diff --git a/net/filter.c b/net/filter.c
>> index 54a5c06..24ec4e1 100644
>> --- a/net/filter.c
>> +++ b/net/filter.c
>> @@ -57,7 +57,7 @@ static void qemu_cleanup_net_filter(NetFilterState *nf)
>>       g_free(nf);
>>   }
>>
>> -static void qemu_del_net_filter(NetFilterState *nf)
>> +void qemu_del_net_filter(NetFilterState *nf)
>>   {
>>       /* handle multi queue? */
>>       qemu_cleanup_net_filter(nf);
>> diff --git a/net/net.c b/net/net.c
>> index d9b70cd..03b2296 100644
>> --- a/net/net.c
>> +++ b/net/net.c
>> @@ -28,6 +28,7 @@
>>   #include "hub.h"
>>   #include "net/slirp.h"
>>   #include "net/eth.h"
>> +#include "net/filter.h"
>>   #include "util.h"
>>
>>   #include "monitor/monitor.h"
>> @@ -385,6 +386,8 @@ void qemu_del_net_client(NetClientState *nc)
>>   {
>>       NetClientState *ncs[MAX_QUEUE_NUM];
>>       int queues, i;
>> +    NetFilterState *nf, *next;
>> +    QemuOpts *opts;
>>
>>       assert(nc->info->type != NET_CLIENT_OPTIONS_KIND_NIC);
>>
>> @@ -396,6 +399,17 @@ void qemu_del_net_client(NetClientState *nc)
>>                                             MAX_QUEUE_NUM);
>>       assert(queues != 0);
>>
>> +    /*
>> +     * we delete/free the netfilter object attached to this netdev
>> +     * multiqueue netfilter is not supported now, so only delete
>> +     * nc->filters is enough.
>> +     */
>
> The comment is not correct since for multiqueue, each NetClientState is
> a queue. I think you can just remove above.

But in this function qemu_del_net_client(), it will find all ncs with the
same name (which I thought is multiqueue), and then delete those netclients.

is it ok to move this chunk to qemu_cleanup_net_client()? because in
qemu_cleanup_net_client(), we don't need to deal with multiqueue.

>
>> +    QTAILQ_FOREACH_SAFE(nf, &nc->filters, next, next) {
>> +        opts = qemu_opts_find(qemu_find_opts_err("netfilter", NULL), nf->name);
>> +        qemu_del_net_filter(nf);
>> +        qemu_opts_del(opts);
>> +    }
>> +
>>       /* If there is a peer NIC, delete and cleanup client, but do not free. */
>>       if (nc->peer && nc->peer->info->type == NET_CLIENT_OPTIONS_KIND_NIC) {
>>           NICState *nic = qemu_get_nic(nc->peer);
>
> .
>

-- 
Thanks,
Yang.