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

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

[Marco Dalla Torre]

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

Version 2 - code style fixes

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,
     -${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}.

If the user-selected command is not supported by the VLAN, an appropriate
error is shown.
---
  functions.c |  3 +++
  sys.c       | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
  2 files changed, 54 insertions(+)

diff --git a/functions.c b/functions.c
index cc05a48..ed010ea 100644
--- a/functions.c
+++ b/functions.c
@@ -135,6 +135,9 @@ static void file_open_problem_dbg(char *dir, char 
*fname, char *full_path)
  			fprintf(stderr, "Error - the folder '/sys/' was not found on the 
system\n");
  			fprintf(stderr, "Please make sure that the sys filesystem is 
properly mounted\n");
  			return;
+		} else if (strstr(dir, "/sys/devices/virtual/")) {
+			fprintf(stderr, "The selected feature '%s' is not supported for 
vlans\n", fname);
+			return;
  		}
  	}
  diff --git a/sys.c b/sys.c
index b1d7ea8..942ead0 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);
+
+	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;
+}
+
  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);
+			}
+		}
+		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);
-- 
1.8.3.2


[-- Attachment #2: Attached Message Part --]
[-- Type: text/plain, Size: 0 bytes --]

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

[Antonio Quartulli]

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

On Tue, Aug 27, 2013 at 02:29:40PM +0200, Marco Dalla Torre wrote:
> Version 2 - code style fixes

Next time, specify the version of the patch in the subject, e.g. [PATCHv2].

> 
> 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,
>      -${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}.
> 
> If the user-selected command is not supported by the VLAN, an appropriate
> error is shown.

good explanation of how it works, thanks, but in the commit message you should
state in a few lines what this change is bringing.
It is not clear what the new feature is from what you wrote. Don't you think the
same?

Then a Signed-off-by line is missing. The patch cannot be accepted without it :)

> ---
>   functions.c |  3 +++
>   sys.c       | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 54 insertions(+)
> 
> diff --git a/functions.c b/functions.c
> index cc05a48..ed010ea 100644
> --- a/functions.c
> +++ b/functions.c
> @@ -135,6 +135,9 @@ static void file_open_problem_dbg(char *dir, char 
> *fname, char *full_path)
>   			fprintf(stderr, "Error - the folder '/sys/' was not found on the 
> system\n");
>   			fprintf(stderr, "Please make sure that the sys filesystem is 
> properly mounted\n");
>   			return;
> +		} else if (strstr(dir, "/sys/devices/virtual/")) {
> +			fprintf(stderr, "The selected feature '%s' is not supported for 
> vlans\n", fname);
> +			return;
>   		}
>   	}
>   diff --git a/sys.c b/sys.c
> index b1d7ea8..942ead0 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)

please put a short comment on top of the function explaining what it does and
what it returns. This way it is easier to re-use it in the future.

> +{
> +	char *vdev;
> +	char line[100];
> +	const char path[] = "/proc/net/vlan/";
> +	int size = sizeof(path) + sizeof(mesh_iface);

be careful here: mesh_iface is a char*, sizeof(mesh_iface) will give you the
size of the pointer, not the length of the string. Did you mean strlen() ?
sizeof() works as you expected only for fixed size arrays (e.g. path[]).

> +	FILE *fp = NULL;

This doe snot need to be initialised...(it's just a minor remark..)

> +
> +	char *fpath = malloc(size);
> +	strcpy(fpath, path);
> +	/* prepare path file path: /proc/net/vlan/$mesh_iface*/
> +	strcat(fpath, mesh_iface);

why not directly using one call to sprintf() to compose the fpath? I think it is
also easier to read, no?

> +
> +	fp = fopen(fpath, "r");
> +	if (fp == NULL)
> +		return 1;

Is this a "success" or a "failure"? At the beginning I thought it was a failure,
but then I saw you return 1 also at the very end, which is a success...
what about returning 0 on success and -1 on failure and keep this behaviour all
over? or is there a particular reason for returning 1 here?

> +
> +	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

Use C standard comments please: /* like this */

> +	if (*base_dev == NULL)

I know there are some spots in the code where this has been done already....but
don't you think it is clearer if you do transform the check above in:

if (!*base_dev)

? (this applies to the fp check above too).

> +		return 0;
> +
> +	return 1;
> +}
> +
>   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) {

explicitly check for < 0 not !=. The man says that -1 is returned in case of
error. In the future it could be possible that positive values are used for
other purposes, so better to literally stick to what the man says.


> +		if (errno == ENOENT) {
> +			// path does not exist, no lan interface: check vlan

comment style

> +			unsigned short vid = 0;
> +			char *base_dev = NULL;

please declare all the variables at the top of the function. This helps to avoid
erroneous variable hiding in future changes.

> +			if (get_basedev_vid(mesh_iface, &base_dev, &vid) == 1) {

to reduce the indentation here you could revert the condition and then change
the value of res and "goto out;" if true.

In this way the code following can have one tab less. This is an hint to reduce
indentation and make the code less difficult to read (this is something we were
discussing offline)

> +				free(path_buff);
> +				char sys_vlan_path[] = "/sys/devices/virtual/net/%s/mesh/vlan%d/";
> +				int size = sizeof(sys_vlan_path) + sizeof(base_dev);

same for these vars (move to the top)

You can also define constants like this using a define at the top of the file. This
helps to easily change them (if needed) later and make the code nicer :)

> +				path_buff = malloc(size);
> +				sprintf(path_buff, sys_vlan_path, base_dev, vid);
> +			}
> +		}
> +		if (errno == ENOTDIR) {
> +			// not a directory, something wrong here
> +			fprintf(stderr, "Error - expected directory at '%s'\n",
> +				path_buff);
> +			return EXIT_FAILURE;

you have to goto out here, otherwise we have a memory leak due to path_buff
being not free'd. I'd suggest to change res and goto out;

> +		}
> +	}
>   	if (argc == 1) {
>   		res = read_file(path_buff, (char *)batctl_settings[setting].sysfs_name,
>   				NO_FLAGS, 0, 0, 0);
> -- 
> 1.8.3.2
> 


Nice job so far :-) For being your first patch this is for sure a good one!

Cheers,


-- 
Antonio Quartulli

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

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

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

[Marek Lindner]

On Tuesday, August 27, 2013 20:29:40 Marco Dalla Torre wrote:
> Version 2 - code style fixes
> 
> 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,
>      -${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}.
> 
> If the user-selected command is not supported by the VLAN, an appropriate
> error is shown.
> ---
>   functions.c |  3 +++
>   sys.c       | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 54 insertions(+)

Please don't forget the man page update.

Thanks,
Marek

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

[Marco Dalla Torre]

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,
     -${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}.

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/functions.c b/functions.c
index cc05a48..ed010ea 100644

--- a/functions.c

+++ b/functions.c

@@ -135,7 +135,10 @@

static·void·file_open_problem_dbg(char·*dir,·char·*fname,·char·*full_path)
» » » 
fprintf(stderr,·"Error·-·the·folder·'/sys/'·was·not·found·on·the·system\n"); 

» » » 
fprintf(stderr,·"Please·make·sure·that·the·sys·filesystem·is·properly·mounted\n"); 

» » » return;
+» » }·else·if·(strstr(dir,·"/sys/devices/virtual/"))·{
+» » » 
fprintf(stderr,·"The·selected·feature·'%s'·is·not·supported·for·vlans\n",·fname); 

+» » » return;
» » }
» }

=======================================

diff --git a/main.c b/main.c
index 24d42fb..bac37ca 100644

--- a/main.c

+++ b/main.c

@@ -49,8 +49,8 @@

void·print_usage(void)
» 
fprintf(stderr,·"Usage:·batctl·[options]·command|debug·table·[parameters]\n"); 

» fprintf(stderr,·"options:\n");
-» fprintf(stderr,·"·\t-m·mesh·interface·(default·'bat0')\n");
+» 
fprintf(stderr,·"·\t-m·mesh·interface·or·mesh-based·VLAN·interface·(default·'bat0')\n"); 

» 
fprintf(stderr,·"·\t-h·print·this·help·(or·'batctl·<command|debug·table>·-h'·for·the·parameter·help)\n"); 

» fprintf(stderr,·"·\t-v·print·version\n");
» fprintf(stderr,·"\n");

=======================================

diff --git a/man/batctl.8 b/man/batctl.8
index d04385b..7fd324d 100644

--- a/man/batctl.8

+++ b/man/batctl.8

@@ -42,8 +42,8 @@

behaviour·or·using·the·B.A.T.M.A.N.·advanced·protocol.
.SH·OPTIONS
.TP
.I·\fBoptions:
-\-m·····specify·mesh·interface·(default·'bat0')
+\-m·····specify·mesh·interface·or·a·mesh-based·VLAN·interface·(default·'bat0') 

.br
\-h·····print·general·batctl·help
.br

=======================================

diff --git a/sys.c b/sys.c
index b1d7ea8..00d2ed1 100644

--- a/sys.c

+++ b/sys.c

@@ -371,7 +371,38 @@

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);
+
+» 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;
+}
+
int·handle_sys_setting(char·*mesh_iface,·int·setting,·int·argc,·char·**argv) 

{
» int·optchar,·res·=·EXIT_FAILURE;

@@ -392,8 +423,27 @@

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);
+» » » return·EXIT_FAILURE;
+» » }
+» }
» if·(argc·==·1)·{
» » res·=·read_file(path_buff,·(char·*)batctl_settings[setting].sysfs_name,
» » » » NO_FLAGS,·0,·0,·0);
--·

1.8.3.2

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

[Marco Dalla Torre]

*sorry for messing up format in previous message, re-sending*

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,
    -${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}.

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/functions.c b/functions.c
index cc05a48..ed010ea 100644
--- a/functions.c
+++ b/functions.c
@@ -135,6 +135,9 @@ static void file_open_problem_dbg(char *dir, char *fname, char *full_path)
 			fprintf(stderr, "Error - the folder '/sys/' was not found on the system\n");
 			fprintf(stderr, "Please make sure that the sys filesystem is properly mounted\n");
 			return;
+		} else if (strstr(dir, "/sys/devices/virtual/")) {
+			fprintf(stderr, "The selected feature '%s' is not supported for vlans\n", fname);
+			return;
 		}
 	}
 
diff --git a/main.c b/main.c
index 24d42fb..bac37ca 100644
--- a/main.c
+++ b/main.c
@@ -49,7 +49,7 @@ void print_usage(void)
 
 	fprintf(stderr, "Usage: batctl [options] command|debug table [parameters]\n");
 	fprintf(stderr, "options:\n");
-	fprintf(stderr, " \t-m mesh interface (default 'bat0')\n");
+	fprintf(stderr, " \t-m mesh interface or mesh-based VLAN interface (default 'bat0')\n");
 	fprintf(stderr, " \t-h print this help (or 'batctl <command|debug table> -h' for the parameter help)\n");
 	fprintf(stderr, " \t-v print version\n");
 	fprintf(stderr, "\n");
diff --git a/man/batctl.8 b/man/batctl.8
index d04385b..7fd324d 100644
--- a/man/batctl.8
+++ b/man/batctl.8
@@ -42,7 +42,7 @@ behaviour or using the B.A.T.M.A.N. advanced protocol.
 .SH OPTIONS
 .TP
 .I \fBoptions:
-\-m     specify mesh interface (default 'bat0')
+\-m     specify mesh interface or a mesh-based VLAN interface (default 'bat0')
 .br
 \-h     print general batctl help
 .br
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);
+
+	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;
+}
+
 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);
+			return EXIT_FAILURE;
+		}
+	}
 	if (argc == 1) {
 		res = read_file(path_buff, (char *)batctl_settings[setting].sysfs_name,
 				NO_FLAGS, 0, 0, 0);
-- 
1.8.3.2

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

[Antonio Quartulli]

[-- 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 --]

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

[Marco Dalla Torre]

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/${device}/mesh/), 'batctl' now looks into directory:
     /sys/devices/virtual/net/${base_device}/mesh/vlan${vid}
Information on VLAN devices (base device, vid) necessary to construct the
directory path is acquired by parsing /proc/net/vlan/${device}.
Where:
    -${base_device}: the batman device on top of which the VLAN is sitting
    -${device}: the device interface for the VLAN,
    -${vid}: the identifier assigned to the VLAN.

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 |  4 ++--
 sys.c        | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 64 insertions(+), 3 deletions(-)

diff --git a/functions.c b/functions.c
index cc05a48..ed010ea 100644
--- a/functions.c
+++ b/functions.c
@@ -135,6 +135,9 @@ static void file_open_problem_dbg(char *dir, char *fname, char *full_path)
 			fprintf(stderr, "Error - the folder '/sys/' was not found on the system\n");
 			fprintf(stderr, "Please make sure that the sys filesystem is properly mounted\n");
 			return;
+		} else if (strstr(dir, "/sys/devices/virtual/")) {
+			fprintf(stderr, "The selected feature '%s' is not supported for vlans\n", fname);
+			return;
 		}
 	}
 
diff --git a/main.c b/main.c
index 24d42fb..bac37ca 100644
--- a/main.c
+++ b/main.c
@@ -49,7 +49,7 @@ void print_usage(void)
 
 	fprintf(stderr, "Usage: batctl [options] command|debug table [parameters]\n");
 	fprintf(stderr, "options:\n");
-	fprintf(stderr, " \t-m mesh interface (default 'bat0')\n");
+	fprintf(stderr, " \t-m mesh interface or mesh-based VLAN interface (default 'bat0')\n");
 	fprintf(stderr, " \t-h print this help (or 'batctl <command|debug table> -h' for the parameter help)\n");
 	fprintf(stderr, " \t-v print version\n");
 	fprintf(stderr, "\n");
diff --git a/man/batctl.8 b/man/batctl.8
index d04385b..b45cccf 100644
--- a/man/batctl.8
+++ b/man/batctl.8
@@ -42,7 +42,7 @@ behaviour or using the B.A.T.M.A.N. advanced protocol.
 .SH OPTIONS
 .TP
 .I \fBoptions:
-\-m     specify mesh interface (default 'bat0')
+\-m     specify mesh interface or a mesh-based VLAN interface (default 'bat0')
 .br
 \-h     print general batctl help
 .br
@@ -61,7 +61,7 @@ originator interval. The interval is in units of milliseconds.
 .br
 .IP "\fBap_isolation\fP|\fBap\fP [\fB0\fP|\fB1\fP]"
 If no parameter is given the current ap isolation setting is displayed. Otherwise the parameter is used to enable or
-disable ap isolation.
+disable ap isolation. This command can be used in conjunction with "-m" option to target per VLAN configurations.
 .br
 .IP "\fBbridge_loop_avoidance\fP|\fBbl\fP [\fB0\fP|\fB1\fP]"
 If no parameter is given the current bridge loop avoidance setting is displayed. Otherwise the parameter is used to enable
diff --git a/sys.c b/sys.c
index b1d7ea8..24b64a4 100644
--- a/sys.c
+++ b/sys.c
@@ -371,6 +371,44 @@ static void settings_usage(int setting)
 	fprintf(stderr, " \t -h print this help\n");
 }
 
+/**
+ * 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];
+	const char path[] = "/proc/net/vlan/";
+	int size = strlen(path) + strlen(mesh_iface) + 1;
+	FILE *fp = NULL;
+	char *fpath = malloc(size);
+	/* prepare path file path: /proc/net/vlan/$mesh_iface*/
+	snprintf(fpath, size, "%s%s", path, mesh_iface);
+	fp = fopen(fpath, "r");
+	if (fp == NULL)
+		return -1;
+
+	if (fscanf(fp, "%ms %*s %hu %*s %*d %*s %*d", &vdev, vid) == 0)
+		return -1;
+
+	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 -1;
+
+	return 0;
+}
+
 int handle_sys_setting(char *mesh_iface, int setting, int argc, char **argv)
 {
 	int optchar, res = EXIT_FAILURE;
@@ -392,6 +430,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) == 0) {
+				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);
+			return EXIT_FAILURE;
+		}
+	}
 	if (argc == 1) {
 		res = read_file(path_buff, (char *)batctl_settings[setting].sysfs_name,
 				NO_FLAGS, 0, 0, 0);
-- 
1.8.3.2

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

[Marek Lindner]

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