tap: use an exit notifier to call down_script

From: Marc-André Lureau <marcandre.lureau@redhat.com>

We would like to move back net_cleanup() at the end of main function,
like it used to be until f30dbae63a46f23116715dff8d130c, but minimum
tap cleanup is necessary regarless at exit() time. Use an exit notifier
to call TAP down_script. If net_cleanup() is called first, then remove
the exit notifier as it will become a dangling pointer otherwise.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
---
 net/tap.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

On 2016年07月11日 22:48, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> We would like to move back net_cleanup() at the end of main function,
> like it used to be until f30dbae63a46f23116715dff8d130c, but minimum
> tap cleanup is necessary regarless at exit() time. Use an exit notifier
> to call TAP down_script. If net_cleanup() is called first, then remove
> the exit notifier as it will become a dangling pointer otherwise.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>

This looks like a tap specific solution. But at least slirp wants to do 
some cleanup too. We need a generic solution. Does it work if we use 
atexit(net_chr_cleanup) and first cleanup net and then chr at that function?

> ---
>   net/tap.c | 27 ++++++++++++++++++++-------
>   1 file changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/net/tap.c b/net/tap.c
> index 676bad4..e9c32f3 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -58,6 +58,7 @@ typedef struct TAPState {
>       bool enabled;
>       VHostNetState *vhost_net;
>       unsigned host_vnet_hdr_len;
> +    Notifier exit;
>   } TAPState;
>   
>   static void launch_script(const char *setup_script, const char *ifname,
> @@ -292,10 +293,22 @@ static void tap_set_offload(NetClientState *nc, int csum, int tso4,
>       tap_fd_set_offload(s->fd, csum, tso4, tso6, ecn, ufo);
>   }
>   
> +static void tap_exit_notify(Notifier *notifier, void *data)
> +{
> +    TAPState *s = container_of(notifier, TAPState, exit);
> +    Error *err = NULL;
> +
> +    if (s->down_script[0]) {
> +        launch_script(s->down_script, s->down_script_arg, s->fd, &err);
> +        if (err) {
> +            error_report_err(err);
> +        }
> +    }
> +}
> +
>   static void tap_cleanup(NetClientState *nc)
>   {
>       TAPState *s = DO_UPCAST(TAPState, nc, nc);
> -    Error *err = NULL;
>   
>       if (s->vhost_net) {
>           vhost_net_cleanup(s->vhost_net);
> @@ -304,12 +317,8 @@ static void tap_cleanup(NetClientState *nc)
>   
>       qemu_purge_queued_packets(nc);
>   
> -    if (s->down_script[0]) {
> -        launch_script(s->down_script, s->down_script_arg, s->fd, &err);
> -        if (err) {
> -            error_report_err(err);
> -        }
> -    }
> +    tap_exit_notify(&s->exit, NULL);
> +    qemu_remove_exit_notifier(&s->exit);
>   
>       tap_read_poll(s, false);
>       tap_write_poll(s, false);
> @@ -379,6 +388,10 @@ static TAPState *net_tap_fd_init(NetClientState *peer,
>       }
>       tap_read_poll(s, true);
>       s->vhost_net = NULL;
> +
> +    s->exit.notify = tap_exit_notify;
> +    qemu_add_exit_notifier(&s->exit);
> +
>       return s;
>   }
>

On 12/07/2016 09:43, Jason Wang wrote:
> 
> 
> On 2016年07月11日 22:48, marcandre.lureau@redhat.com wrote:
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> We would like to move back net_cleanup() at the end of main function,
>> like it used to be until f30dbae63a46f23116715dff8d130c, but minimum
>> tap cleanup is necessary regarless at exit() time. Use an exit notifier
>> to call TAP down_script. If net_cleanup() is called first, then remove
>> the exit notifier as it will become a dangling pointer otherwise.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> This looks like a tap specific solution. But at least slirp wants to do
> some cleanup too.

Right, specifically slirp_smb_cleanup.

> We need a generic solution. Does it work if we use
> atexit(net_chr_cleanup) and first cleanup net and then chr at that
> function?

I think exit notifiers _are_ a generic solution.  Most pieces of QEMU
don't care about cleanup at exit(), because the operating system takes
care of it (freeing memory, closing file descriptors, etc.).

Tap, slirp and chardev are exceptions, and they can use the exit
notifiers.  I'll send a patch for slirp.

Paolo

>> ---
>>   net/tap.c | 27 ++++++++++++++++++++-------
>>   1 file changed, 20 insertions(+), 7 deletions(-)
>>
>> diff --git a/net/tap.c b/net/tap.c
>> index 676bad4..e9c32f3 100644
>> --- a/net/tap.c
>> +++ b/net/tap.c
>> @@ -58,6 +58,7 @@ typedef struct TAPState {
>>       bool enabled;
>>       VHostNetState *vhost_net;
>>       unsigned host_vnet_hdr_len;
>> +    Notifier exit;
>>   } TAPState;
>>     static void launch_script(const char *setup_script, const char
>> *ifname,
>> @@ -292,10 +293,22 @@ static void tap_set_offload(NetClientState *nc,
>> int csum, int tso4,
>>       tap_fd_set_offload(s->fd, csum, tso4, tso6, ecn, ufo);
>>   }
>>   +static void tap_exit_notify(Notifier *notifier, void *data)
>> +{
>> +    TAPState *s = container_of(notifier, TAPState, exit);
>> +    Error *err = NULL;
>> +
>> +    if (s->down_script[0]) {
>> +        launch_script(s->down_script, s->down_script_arg, s->fd, &err);
>> +        if (err) {
>> +            error_report_err(err);
>> +        }
>> +    }
>> +}
>> +
>>   static void tap_cleanup(NetClientState *nc)
>>   {
>>       TAPState *s = DO_UPCAST(TAPState, nc, nc);
>> -    Error *err = NULL;
>>         if (s->vhost_net) {
>>           vhost_net_cleanup(s->vhost_net);
>> @@ -304,12 +317,8 @@ static void tap_cleanup(NetClientState *nc)
>>         qemu_purge_queued_packets(nc);
>>   -    if (s->down_script[0]) {
>> -        launch_script(s->down_script, s->down_script_arg, s->fd, &err);
>> -        if (err) {
>> -            error_report_err(err);
>> -        }
>> -    }
>> +    tap_exit_notify(&s->exit, NULL);
>> +    qemu_remove_exit_notifier(&s->exit);
>>         tap_read_poll(s, false);
>>       tap_write_poll(s, false);
>> @@ -379,6 +388,10 @@ static TAPState *net_tap_fd_init(NetClientState
>> *peer,
>>       }
>>       tap_read_poll(s, true);
>>       s->vhost_net = NULL;
>> +
>> +    s->exit.notify = tap_exit_notify;
>> +    qemu_add_exit_notifier(&s->exit);
>> +
>>       return s;
>>   }
>>   
>

On 2016年07月12日 15:51, Paolo Bonzini wrote:
>
> On 12/07/2016 09:43, Jason Wang wrote:
>>
>> On 2016年07月11日 22:48, marcandre.lureau@redhat.com wrote:
>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>
>>> We would like to move back net_cleanup() at the end of main function,
>>> like it used to be until f30dbae63a46f23116715dff8d130c, but minimum
>>> tap cleanup is necessary regarless at exit() time. Use an exit notifier
>>> to call TAP down_script. If net_cleanup() is called first, then remove
>>> the exit notifier as it will become a dangling pointer otherwise.
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>> This looks like a tap specific solution. But at least slirp wants to do
>> some cleanup too.
> Right, specifically slirp_smb_cleanup.
>
>> We need a generic solution. Does it work if we use
>> atexit(net_chr_cleanup) and first cleanup net and then chr at that
>> function?
> I think exit notifiers _are_ a generic solution.  Most pieces of QEMU
> don't care about cleanup at exit(), because the operating system takes
> care of it (freeing memory, closing file descriptors, etc.).
>
> Tap, slirp and chardev are exceptions, and they can use the exit
> notifiers.  I'll send a patch for slirp.
>
> Paolo
>

Ok.

Reviewed-by: Jason Wang <jasowang@redhat.com>