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

[Antonio Quartulli <ordex@autistici.org>]

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

On Thu, Aug 29, 2013 at 08:43:34PM +0200, Marco Dalla Torre wrote:
> 
> *sorry for messing up format in previous message, re-sending*

This message should not be here, otherwise it will get included in the commit
messages. Any out of band message should go immediately after the ---.

> 
> Allow the batctl tool to take advantage of changes from commit
> e4ff5c153dab054a6cd1c4132f87bc5e77127456 ("add sys framework for VLAN")
> recently added to batman-adv, so that users can execute commands in
> a per VLAN fashion.
> 
> If no directory entry corresponding to the user-selected device is found
> at the standard location for non VLAN interfaces
> (/sys/class/net/${base_device}), 'batctl' now looks into directory:
>      /sys/devices/virtual/net/${base_device}/mesh/vlan${vid}
> Where:
>     -${base_device}: the batman device on top of which the VLAN is sitting
>     -${device}: the device interface for the VLAN,

${device} ? I don't see it in the text.

>     -${vid}: the identifier assigned to the VLAN.
> 
> Information on VLAN devices (base device, vid) necessary to construct the
> directory path is acquired by parsing /proc/net/vlan/${device}.

Ah it is here..what about moving the description below?

> 
> If the user-selected command is not supported by the VLAN, an appropriate
> error is shown.
> 
> Signed-off-by: Marco Dalla Torre <marco.dallato@gmail.com>
> ---
>  functions.c  |  3 +++
>  main.c       |  2 +-
>  man/batctl.8 |  2 +-
>  sys.c        | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 56 insertions(+), 2 deletions(-)
> 
> diff --git a/sys.c b/sys.c
> index b1d7ea8..00d2ed1 100644
> --- a/sys.c
> +++ b/sys.c
> @@ -371,6 +371,37 @@ static void settings_usage(int setting)
>  	fprintf(stderr, " \t -h print this help\n");
>  }
>  
> +int get_basedev_vid(char *mesh_iface, char **base_dev, unsigned short *vid)
> +{
> +	char *vdev;
> +	char line[100];
> +	const char path[] = "/proc/net/vlan/";
> +	int size = sizeof(path) + sizeof(mesh_iface);
> +	FILE *fp = NULL;
> +
> +	char *fpath = malloc(size);
> +	strcpy(fpath, path);
> +	/* prepare path file path: /proc/net/vlan/$mesh_iface*/
> +	strcat(fpath, mesh_iface);

I think I asked you before: why don't you use a unique (and more clear) snprintf
here rather than one strcpy and one strcat ?

> +
> +	fp = fopen(fpath, "r");
> +	if (fp == NULL)
> +		return 1;
> +
> +	if (fscanf(fp, "%ms %*s %hu %*s %*d %*s %*d", &vdev, vid) == 0)
> +		return 0;
> +
> +	while (fgets(line, sizeof(line), fp) != NULL) {
> +		if (sscanf(line, "Device: %ms", base_dev) == 1)
> +			break;
> +	}
> +	/* handle base device not found case */
> +	if (*base_dev == NULL)
> +		return 0;
> +
> +	return 1;
> +}

The return value of this function is not clear: you return 1 both on error (if
fp == NULL) and on success (at the end). I'd suggest you fix this and to rather
use -1 for error and 0 on success (like standard unix processes/c functions).

Please, provide a comment on top of this function describing what it does, what
do its parameters mean and what it returns. For example of the style to use you
can have a look at most of the functions in the batman-adv kernel module (this
is the so called kernel-doc). It makes much easier to understand functions.


> +
>  int handle_sys_setting(char *mesh_iface, int setting, int argc, char **argv)
>  {
>  	int optchar, res = EXIT_FAILURE;
> @@ -392,6 +423,26 @@ int handle_sys_setting(char *mesh_iface, int setting, int argc, char **argv)
>  	snprintf(path_buff, PATH_BUFF_LEN, SYS_BATIF_PATH_FMT, mesh_iface);
>  	path_buff[PATH_BUFF_LEN - 1] = '\0';
>  
> +	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;
> +			if (get_basedev_vid(mesh_iface, &base_dev, &vid) == 1) {
> +				free(path_buff);
> +				char sys_vlan_path[] =
> +					"/sys/devices/virtual/net/%s/mesh/vlan%d/";
> +				int size = sizeof(sys_vlan_path) + sizeof(base_dev);
> +				path_buff = malloc(size);
> +				sprintf(path_buff, sys_vlan_path, base_dev, vid);
> +			}
> +		} else if (errno == ENOTDIR) {
> +			/* not a directory, something wrong here */
> +			fprintf(stderr, "Error - expected directory at '%s'\n",
> +				path_buff);

I'd rather change this message to something different: ENOTDIR tells you that
something inside the pathname is not a dire while it was supposed to be such.
E.g. /a/b/c/d -> ENOTDIR is set if one of a, b or c is NOT a directory. d is the
file you want to access so that is not checked. This is what "man access" says.


Thanks!

-- 
Antonio Quartulli

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

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