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

Antonio Quartulli
2013-08-28T15:05:23Z

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