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