From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ingo Molnar <mingo@elte.hu>
Subject: Re: [PATCH v3] kvm tools: Support multiple net devices
Date: Tue, 27 Sep 2011 08:41:33 +0200
Message-ID: <20110927064133.GC18519@elte.hu>
References: <1317059073-19576-1-git-send-email-levinsasha928@gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: penberg@cs.helsinki.fi, kvm@vger.kernel.org, asias.hejun@gmail.com,
	gorcunov@gmail.com
To: Sasha Levin <levinsasha928@gmail.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from fallback.mail.elte.hu ([157.181.151.13]:55129 "EHLO
	fallback.mail.elte.hu" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751578Ab1I0HV7 (ORCPT <rfc822;kvm@vger.kernel.org>);
	Tue, 27 Sep 2011 03:21:59 -0400
Content-Disposition: inline
In-Reply-To: <1317059073-19576-1-git-send-email-levinsasha928@gmail.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>


* Sasha Levin <levinsasha928@gmail.com> wrote:

> --- a/tools/kvm/builtin-run.c
> +++ b/tools/kvm/builtin-run.c
> @@ -87,9 +87,12 @@ static bool sdl;
>  static bool balloon;
>  static bool using_rootfs;
>  static bool custom_rootfs;
> +static bool no_net;
>  extern bool ioport_debug;
>  extern int  active_console;
>  extern int  debug_iodelay;
> +struct virtio_net_parameters *net_params;
> +int num_net_devices;

Just a stylistic nit-pick: this variable definition section looks 
pretty ugly. Those externs should be in a header and in any case 
different types of lines should generally not be mixed without at 
least a newline between them.

> +static int set_net_param(struct virtio_net_parameters *p, const char *param,
> +				const char *val)


Naming nit: 'struct virtio_net_params' is shorter by 4 chars and just 
as expressive.

> +	char *buf, *cmd = NULL, *cur = NULL;
> +	bool on_cmd = true;
> +
> +	if (arg) {
> +		buf = strdup(arg);
> +		if (buf == NULL)
> +			die("Failed allocating new net buffer");
> +		cur = strtok(buf, ",=");
> +	}
> +
> +	p = (struct virtio_net_parameters) {
> +		.guest_ip	= DEFAULT_GUEST_ADDR,
> +		.host_ip	= DEFAULT_HOST_ADDR,
> +		.script		= DEFAULT_SCRIPT,
> +		.mode		= NET_MODE_TAP,
> +	};
> +
> +	str_to_mac(DEFAULT_GUEST_MAC, p.guest_mac);
> +	p.guest_mac[5] += num_net_devices;
> +
> +	while (cur) {
> +		if (on_cmd) {
> +			cmd = cur;
> +		} else {
> +			if (set_net_param(&p, cmd, cur) < 0)
> +				goto done;
> +		}
> +		on_cmd = !on_cmd;
> +
> +		cur = strtok(NULL, ",=");
> +	};
> +
> +	num_net_devices++;
> +
> +	net_params = realloc(net_params, num_net_devices * sizeof(*net_params));
> +	if (net_params == NULL)
> +		die("Failed adding new network device");
> +
> +	net_params[num_net_devices - 1] = p;
> +
> +done:
> +	return 0;
> +}

Isn't 'buf' leaked here?

Patch looks good otherwise.

Thanks,

	Ingo