Re: [PATCHv3 1/2] kvmtool: Introduce downscript option for virtio-net

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

Hi,

thanks for the rework, that looks good to me, some minor nits below.
Not sure if that requires a new version, maybe Will can fix that up when
he applies it.

On 21/07/15 07:18, Fan Du wrote:
> To detach tap device automatically from bridge when exiting,
> just like what the reverse of "script" does.
> 
> Signed-off-by: Fan Du <fan.du@intel.com>
> ---
>  include/kvm/virtio-net.h |  1 +
>  virtio/net.c             | 49 ++++++++++++++++++++++++++++++++++++------------
>  2 files changed, 38 insertions(+), 12 deletions(-)
> 
> diff --git a/include/kvm/virtio-net.h b/include/kvm/virtio-net.h
> index f435cc3..d136a09 100644
> --- a/include/kvm/virtio-net.h
> +++ b/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/virtio/net.c b/virtio/net.c
> index 4a6a855..d343615 100644
> --- a/virtio/net.c
> +++ b/virtio/net.c
> @@ -294,10 +294,29 @@ static int virtio_net_request_tap(struct net_dev *ndev, struct ifreq *ifr,
>  	return ret;
>  }
>  
> +static int virtio_net_exec_script(const char* script, char *tap_name)

can you make tap_name a "const char *" as well?

> +{
> +	int pid;

fork() returns a value of type "pid_t", so pid should be of that type
too. I know it was int before, but let's fix it when we rewrite it anyway.

> +	int status;
> +
> +	pid = fork();
> +	if (pid == 0) {
> +		execl(script, script, tap_name, NULL);
> +		_exit(1);
> +	} else {
> +		waitpid(pid, &status, 0);
> +		if (WIFEXITED(status) && WEXITSTATUS(status) != 0) {
> +			pr_warning("Fail to setup tap by %s", script);
> +			return -1;
> +		}
> +	}
> +	return 0;
> +}
> +
>  static bool virtio_net__tap_init(struct net_dev *ndev)
>  {
>  	int sock = socket(AF_INET, SOCK_STREAM, 0);
> -	int pid, status, offload, hdr_len;
> +	int offload, hdr_len;
>  	struct sockaddr_in sin = {0};
>  	struct ifreq ifr;
>  	const struct virtio_net_params *params = ndev->params;
> @@ -339,17 +358,8 @@ static bool virtio_net__tap_init(struct net_dev *ndev)
>  	}
>  
>  	if (strcmp(params->script, "none")) {
> -		pid = fork();
> -		if (pid == 0) {
> -			execl(params->script, params->script, ndev->tap_name, NULL);
> -			_exit(1);
> -		} else {
> -			waitpid(pid, &status, 0);
> -			if (WIFEXITED(status) && WEXITSTATUS(status) != 0) {
> -				pr_warning("Fail to setup tap by %s", params->script);
> -				goto fail;
> -			}
> -		}
> +		if(virtio_net_exec_script(params->script, ndev->tap_name) < 0)
> +			goto fail;
>  	} else if (!skipconf) {
>  		memset(&ifr, 0, sizeof(ifr));
>  		strncpy(ifr.ifr_name, ndev->tap_name, sizeof(ndev->tap_name));
> @@ -702,6 +712,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) {
> @@ -877,6 +889,19 @@ virtio_dev_init(virtio_net__init);
>  
>  int virtio_net__exit(struct kvm *kvm)
>  {
> +	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 &&
> +		    strcmp(params->downscript, "none")) {
> +			virtio_net_exec_script(params->downscript, ndev->tap_name);
> +		}

You don't need the braces here since the condition is only a one-liner.

Cheers,
Andre.

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