Re: [B.A.T.M.A.N.] [PATCHv4] batctl: add sys framework for VLAN support

[Marek Lindner <lindner_marek@yahoo.de>]

On Tuesday, September 03, 2013 20:45:20 Marco Dalla Torre wrote:
> +/**
> + * get_basedev_vid - given a valid VLAN interface, identifies the batman
> device + *	on top of which the VLAN is sitting and its VLAN ID
> + * @mesh_iface: name of the VLAN network interface
> + * @base_dev: output argument, pointer to the base device name
> + * @vid: output argument, pointer to the VLAN ID number
> + *
> + * Returns 0 if execution is successful, -1 if an error occurred
> + */
> +static int get_basedev_vid(const char *mesh_iface, char **base_dev,
> unsigned short *vid) +{
> +	char *vdev;
> +	char line[100];

Here we have a fixed buffer size.


> +	const char path[] = "/proc/net/vlan/";

Constants should be declared outside of a function as they could be re-used by 
other functions. If grouped together they are also easier to find.


> +	int size = strlen(path) + strlen(mesh_iface) + 1;
> +	FILE *fp = NULL;

Does not need to be initialized.


> +	char *fpath = malloc(size);
> +	/* prepare path file path: /proc/net/vlan/$mesh_iface*/
> +	snprintf(fpath, size, "%s%s", path, mesh_iface);

Here we have a dynamic path length without checking if malloc() failed.


> +	if (fscanf(fp, "%ms %*s %hu %*s %*d %*s %*d", &vdev, vid) == 0)
> +		return -1;

Shouldn't we check for "fscanf() != 2" ?


> +	while (fgets(line, sizeof(line), fp) != NULL) {
> +		if (sscanf(line, "Device: %ms", base_dev) == 1)
> +			break;
> +	}

How about using getline() instead of fgets() ? There are several examples 
throughout the batctl code.


> +	/* handle base device not found case */
> +	if (*base_dev == NULL)
> +		return -1;

It would be good style if we set *base_dev to NULL at the beginning of the 
function instead of relying on the caller to properly initialize the variable.


> +	if (access(path_buff, F_OK) != 0) {
> +		if (errno == ENOENT) {
> +			/* path does not exist, no lan interface: check vlan */
> +			unsigned short vid = 0;
> +			char *base_dev = NULL;

Please declare all variables at the beginning of the function.


> +			if (get_basedev_vid(mesh_iface, &base_dev, &vid) == 0) {
> +				free(path_buff);

Not sure why you need to free() here.


> +				char sys_vlan_path[] =
> +					"/sys/devices/virtual/net/%s/mesh/vlan%d/";

Yet another constant ..


> +				int size = sizeof(sys_vlan_path) + sizeof(base_dev);

Did you mean strlen() ?


> +				path_buff = malloc(size);

Malloc() can fail ...

Cheers,
Marek