Re: [PATCH 1/3] kvm-tool: Introduce downscript option

[Andre Przywara <andre.przywara@arm.com>]

Hi,

On 20/07/15 10:28, Fan Du wrote:
> To detach tap device automatically from bridge when exiting,
> just like the reverse of "script" does.
> 
> Signed-off-by: Fan Du <fan.du@intel.com>
> ---
>  tools/kvm/include/kvm/virtio-net.h |  1 +
>  tools/kvm/virtio/net.c             | 28 ++++++++++++++++++++++++++++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/tools/kvm/include/kvm/virtio-net.h b/tools/kvm/include/kvm/virtio-net.h
> index f435cc3..d136a09 100644
> --- a/tools/kvm/include/kvm/virtio-net.h
> +++ b/tools/kvm/include/kvm/virtio-net.h
> @@ -9,6 +9,7 @@ struct virtio_net_params {
>  	const char *guest_ip;
>  	const char *host_ip;
>  	const char *script;
> +	const char *downscript;
>  	const char *trans;
>  	const char *tapif;
>  	char guest_mac[6];
> diff --git a/tools/kvm/virtio/net.c b/tools/kvm/virtio/net.c
> index 25b9496..f3f7200 100644
> --- a/tools/kvm/virtio/net.c
> +++ b/tools/kvm/virtio/net.c
> @@ -696,6 +696,8 @@ static int set_net_param(struct kvm *kvm, struct virtio_net_params *p,
>  			die("Unknown network mode %s, please use user, tap or none", kvm->cfg.network);
>  	} else if (strcmp(param, "script") == 0) {
>  		p->script = strdup(val);
> +	} else if (strcmp(param, "downscript") == 0) {
> +		p->downscript = strdup(val);
>  	} else if (strcmp(param, "guest_ip") == 0) {
>  		p->guest_ip = strdup(val);
>  	} else if (strcmp(param, "host_ip") == 0) {
> @@ -871,6 +873,32 @@ virtio_dev_init(virtio_net__init);
>  
>  int virtio_net__exit(struct kvm *kvm)
>  {
> +	int pid;
> +	int status;
> +	struct virtio_net_params *params;
> +	struct net_dev *ndev;
> +	struct list_head *ptr;
> +
> +	list_for_each(ptr, &ndevs) {
> +		ndev = list_entry(ptr, struct net_dev, list);
> +		params = ndev->params;
> +		/* Cleanup any tap device which attached to bridge */
> +		if (ndev->mode == NET_MODE_TAP ||

Do you mean && here? Otherwise it may try to execute "none".

> +		    strcmp(params->downscript, "none")) {
> +			pid = fork();
> +			if (pid == 0) {
> +				execl(ndev->params->downscript,
> +				      params->downscript, ndev->tap_name, NULL);
> +				_exit(1);
> +			} else {
> +				waitpid(pid, &status, 0);
> +				if (WIFEXITED(status) && WEXITSTATUS(status) != 0) {
> +					pr_warning("Fail to cleanup tap by %s",
> +						   params->script);
> +				}
> +			}
> +		}
> +	}

I think this should be moved into a separate function to avoid code
duplication with the init script execution. Also it improves readability
and avoids too long lines.

Cheers,
Andre.

>  	return 0;
>  }
>  virtio_dev_exit(virtio_net__exit);
>