Re: [PATCH v3] kvm tools: Support multiple net devices

[Ingo Molnar <mingo@elte.hu>]

* 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