netdev - Re: [RFC][PATCH -mm take5 5/7] switch function of netpoll

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a781481a0706131004q722bb409tcd4e1d13102bfb8c@mail.gmail.com>
Date:	Wed, 13 Jun 2007 22:34:07 +0530
From:	"Satyam Sharma" <satyam.sharma@...il.com>
To:	"Keiichi KII" <k-keiichi@...jp.nec.com>
Cc:	"Matt Mackall" <mpm@...enic.com>,
	"Andrew Morton" <akpm@...ux-foundation.org>,
	"David Miller" <davem@...emloft.net>, linux-kernel@...r.kernel.org,
	netdev@...r.kernel.org
Subject: Re: [RFC][PATCH -mm take5 5/7] switch function of netpoll

Hi Keiichi,

On 6/13/07, Keiichi KII <k-keiichi@...jp.nec.com> wrote:
> From: Keiichi KII <k-keiichi@...jp.nec.com>
>
> This patch contains switch function of netpoll.
>
> If "enabled" attribute of certain port is '1', this port is used
> and the configurations of this port are unable to change.
>
> If "enabled" attribute of certain port is '0', this port isn't used
> and the configurations of this port are able to change.
>
> -+- /sys/class/misc/
> |-+- netconsole/
>   |-+- port1/
>   | |--- id          [r--r--r--]  id
>   | |--- enabled     [rw-r--r--]  0: disable 1: enable, writable
>   | ...
>   |--- port2/
>   ...
>
> Signed-off-by: Keiichi KII <k-keiichi@...jp.nec.com>
> Signed-off-by: Takayoshi Kochi <t-kochi@...jp.nec.com>
> ---
> Index: mm/drivers/net/netconsole.c
> ===================================================================
> --- mm.orig/drivers/net/netconsole.c
> +++ mm/drivers/net/netconsole.c
> @@ -71,6 +71,7 @@ struct netconsole_target {
>         struct list_head list;
>         struct kobject obj;
>         int id;
> +       int enabled;
>         struct netpoll np;
>  };

I really think you need to document/comment the struct members.
The newly introduced "enabled" for example, is serving a dual-purpose
in your code here (like you mention in the Changelog). It's used not
only to enable/disable a target, but also to ensure atomicity when
changing the (multiple) configurable parameters of a given target ...
the first purpose is self-evident from the name, but the second is
non-obvious. A sysfs node is also a userspace interface, so some
explicit mention of this is required.

Satyam
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html