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

Marek Lindner
2013-09-04T06:48:07Z

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