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

[Antonio Quartulli <ordex@autistici.org>]

[-- Attachment #1: Type: text/plain, Size: 5660 bytes --]

On Thu, Sep 05, 2013 at 02:16:47PM +0200, 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_ptr;
> +	size_t len;
> +	FILE *fp;
> +	char *fpath;
> +
> +	int size = strlen(PROC_VLAN_PATH) + strlen(mesh_iface) + 1;
> +	fpath = malloc(size);

the declaration of "size" should not be separated from the others. The blank line
should go after "int size = ..." not before.


> +	if (!fpath) {
> +		fprintf(stderr, "Error - could not allocate path buffer: out of memory ?\n");
> +		return EXIT_FAILURE;
> +	}
> +	/* prepare path file path: /proc/net/vlan/$mesh_iface*/
> +	snprintf(fpath, size, "%s%s", PROC_VLAN_PATH, mesh_iface);
> +
> +	fp = fopen(fpath, "r");
> +	if (!fp) {
> +		fprintf(stderr, "Error - could not open file '%s': %s\n",
> +			fpath, strerror(errno));
> +		return EXIT_FAILURE;
> +	}
> +
> +	if (fscanf(fp, "%ms %*s %hu %*s %*d %*s %*d", &vdev, vid) != 2)

this is ok, but as I suggested in chan I think you can force the shape of the
strings you want to skip, e.g. "%ms VID: %hu". In this way you are sure to be
matching the line you were looking for, no? I think we can assume that the file
you are reading is not going to change anymore, so I leave to you the decision
of which format string to use.

> +		return EXIT_FAILURE;
> +
> +	*base_dev = NULL;
> +	while (getline(&line_ptr, &len, fp) != -1) {

should line_ptr be initialised to NULL before invoking getline for the first
time? From the getline man:

       If  *lineptr  is NULL, then getline() will allocate a buffer for storing
       the line

therefore it is better to ensure it is NULL for real the first time.

> +		if (sscanf(line_ptr, "Device: %ms", base_dev) == 1)
> +			break;
> +	}
> +	fclose(fp);
> +	free(line_ptr);
> +	/* handle base device not found case */
> +	if (*base_dev == NULL)

I know in the rest of the code does something different, but we should stick to
one stile (at least in the very same function), so please use (!*base_dev) as
you have done for fp above.

> +		return EXIT_FAILURE;
> +
> +	return EXIT_SUCCESS;
> +}
> +
>  int handle_sys_setting(char *mesh_iface, int setting, int argc, char **argv)
>  {
>  	int optchar, res = EXIT_FAILURE;
>  	char *path_buff;
>  	const char **ptr;
> +	unsigned short vid;
> +	char *base_dev;
> +	ssize_t path_length;
>  
>  	while ((optchar = getopt(argc, argv, "h")) != -1) {
>  		switch (optchar) {
> @@ -388,10 +441,40 @@ int handle_sys_setting(char *mesh_iface, int setting, int argc, char **argv)
>  		}
>  	}
>  
> -	path_buff = malloc(PATH_BUFF_LEN);
> -	snprintf(path_buff, PATH_BUFF_LEN, SYS_BATIF_PATH_FMT, mesh_iface);
> -	path_buff[PATH_BUFF_LEN - 1] = '\0';
> +	path_length = strlen(SYS_BATIF_PATH_FMT) + strlen(mesh_iface) + 1;
> +	path_buff = malloc(path_length);
> +	if (!path_buff) {
> +		fprintf(stderr, "Error - could not allocate path buffer: out of memory ?\n");
> +		return EXIT_FAILURE;
> +	}
> +	snprintf(path_buff, path_length, SYS_BATIF_PATH_FMT, mesh_iface);

How is this change above related to your patch?
Did you introduce this change in this version?
If you want to change this, please send another patch (and maybe fix all the
other spots where this behaviour has been replicated).

>  
> +	if (access(path_buff, F_OK) != 0) {
> +		if (errno == ENOENT) {
> +			/* path does not exist, no lan interface: check vlan */
> +			if (get_basedev_vid(mesh_iface, &base_dev, &vid) == EXIT_SUCCESS) {
> +				/* free for previous allocation */
> +				free(path_buff);
> +				path_length = strlen(SYS_VLAN_PATH)
> +					+ strlen(base_dev)
> +					+ VLAN_ID_MAX_LEN + 1;

freestyle indentation again?

> +				path_buff = malloc(path_length);

I think you can squeeze the free+malloc with one invocation to realloc()?

> +				if (!path_buff) {
> +					fprintf(stderr, "Error - could not allocate path buffer: out of memory ?\n");
> +					return EXIT_FAILURE;
> +				}
> +				snprintf(path_buff, path_length, SYS_VLAN_PATH,
> +					base_dev, vid);
> +			} else {
> +				return EXIT_FAILURE;
> +			}

This is not a mistake, but I think it is a good suggestion for you and your next
patches: In case like this, where the else branch contains a single instruction
that is a return, I'd rather suggest to invert the condition in the if and
simplify the code and the indentation.
Now you have

if (A == x) {
	bla;
	bla;
	bla;
} else {
	return B;
}

what about:

if (A != x)
	return B;

bla;
bla;
bla;

This is a very frequent pattern that you will find in the kernel module. It is
used to avoid indentation to grow up too much and so it makes the code easier
to read. Don't you think?

> +		} else if (errno == ENOTDIR) {
> +			/* not a directory, something wrong here */
> +			fprintf(stderr, "Error - expected directory at '%s'\n",
> +				path_buff);
> +			return EXIT_FAILURE;
> +		}
> +	}
>  	if (argc == 1) {
>  		res = read_file(path_buff, (char *)batctl_settings[setting].sysfs_name,
>  				NO_FLAGS, 0, 0, 0);


Cheers,

-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]