Re: [B.A.T.M.A.N.] [PATCH] Alfred not working within network name space

[Sven Eckelmann <sven@narfation.org>]

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

On Dienstag, 1. November 2016 20:07:24 CET Jean-Jacques Sarton wrote:
> This patch shall allow to use the debugfs as previouly, including
> the check for valify of the interface used.
> For netns the check seem to be implemented within the function
> netlink_query_common().
> For netlink we check if the originators ans transtable_global
> informations are accessible through netlink.
> 
> To: b.a.t.m.a.n@lists.open-mesh.org
> 
> Signed-off-by: Jean-Jacques Sarton <jj.sarton@t-online.de>

Please fix the subject/summary and write a proper commit message
body [1]. Please remove the odd "To: " in the middle of the commit
message body.

The subject of the mail should start with '[PATCH]' (or '[PATCH v2]'
for the second version of the patch and so on). Then you should add
"alfred: " so we easily spot in patchwork for which project the patch
is. After that, you should add a "oneline summary" of what it is
doing and not the problem like in your "Alfred not working within
network name space".

About the commit message body:

 * "This patch" is a bad start for a commit message
 * The first sentence doesn't make a lot of sense because your
   change should add the interface checks via netlink without using
   debugfs.
 * previouly -> previously
 * What is "valify"?
 * netlink_query_common doesn't check the validity of the interface
   but it can be used to check if this interface supports the netlink
   commands to retrieve the global translation table and the originators
   table
 * ans -> and

Maybe you can describe what is currently wrong and then how it gets fixed.

The mail subject ("oneline summary") without the '[PATCH...]' + the
commit message body (till the first occurence of an '---' line) is
later used to describe the change in git.

[...]
> -int batadv_interface_check(const char *mesh_iface)
> +int batadv_interface_check_debugfs(const char *mesh_iface)
>  {
>  	char full_path[MAX_PATH + 1];
>  	FILE *f;
> @@ -166,6 +166,18 @@ int batadv_interface_check(const char *mesh_iface)
>  	return 0;
>  }

Please mark this function static.

> +int batadv_interface_check(const char *mesh_iface)
> +{
> +	int ret = 0;
> +	enable_net_admin_capability(1);
> +	ret = batadv_interface_check_netlink(mesh_iface);
> +	enable_net_admin_capability(0);
> +
> +	if ( ret < 0 )
> +		return batadv_interface_check_debugfs(mesh_iface);
> +	return 0;
> +}
> +

Please use the same whitespace rules like in the rest of the code [2].
For example a newline after the variables block, no extra spaces
inside the parenthesis and a newline after "return batadv_"...

It is also not required to initialize ret when you overwrite the initial
value later without reading the initial value somewhere.

[...]
> +
> +int batadv_interface_check_netlink(const char *mesh_iface)
> +{
> +	struct get_tq_netlink_opts opts = {
> +		.tq = 0,
> +		.found = false,
> +		.query_opts = {
> +			.err = 0,
> +		},
> +	};
> +	int ret = 0;
> +
> +	memset(&opts.mac, 0, ETH_ALEN);
> +
> +	ret = netlink_query_common(mesh_iface,  BATADV_CMD_GET_ORIGINATORS,
> +			           get_tq_netlink_cb, &opts.query_opts);
> +	if (ret < 0)
> +		return ret;
> +
> +	memset(&opts.mac, 0, ETH_ALEN);
> +	ret = netlink_query_common(mesh_iface, BATADV_CMD_GET_TRANSTABLE_GLOBAL,
> +			           get_tq_netlink_cb, &opts.query_opts);
> +
> +	if (ret < 0)
> +		return ret;
> +	return 0;
> +}

See previous mails and the things mentioned above. It is also odd that you use
sometimes 12 spaces (better use 1 tab + 4 spaces for changes to this project).

Kind regards,
	Sven

[1] https://www.kernel.org/doc/Documentation/SubmittingPatches
[2] We try to use a non-strict version of
    https://www.kernel.org/doc/Documentation/CodingStyle

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 801 bytes --]