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 forvlans\n", fname);
return;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 herefprintf(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,