From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andre Przywara <andre.przywara@arm.com>
Subject: Re: [PATCH 1/3] kvm-tool: Introduce downscript option
Date: Mon, 20 Jul 2015 15:45:37 +0100
Message-ID: <55AD0991.3020508@arm.com>
References: <1437384527-22987-1-git-send-email-fan.du@intel.com>
 <1437384527-22987-2-git-send-email-fan.du@intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Cc: kvm@vger.kernel.org, penberg@kernel.org,
	Will Deacon <will.deacon@arm.com>,
	Marc Zyngier <Marc.Zyngier@arm.com>
To: Fan Du <fan.du@intel.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from cam-admin0.cambridge.arm.com ([217.140.96.50]:61807 "EHLO
	cam-admin0.cambridge.arm.com" rhost-flags-OK-OK-OK-OK)
	by vger.kernel.org with ESMTP id S932524AbbGTOpc (ORCPT
	<rfc822;kvm@vger.kernel.org>); Mon, 20 Jul 2015 10:45:32 -0400
In-Reply-To: <1437384527-22987-2-git-send-email-fan.du@intel.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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);
>