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

[Jason Wang]

On 08/06/2015 04:44 PM, Yang Hongyang wrote:
>
>
> 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 <address@hidden>
>>
>> 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.
>

Right.

> 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.

Yes, looks good.