Patchwork [v2,net-next,2/6] bridge: make flags sysfs interface a little bit more extensible

login
register
mail settings
Submitter Vlad Yasevich
Date April 19, 2013, 8:52 p.m.
Message ID <1366404770-28523-3-git-send-email-vyasevic@redhat.com>
Download mbox | patch
Permalink /patch/238103/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Vlad Yasevich - April 19, 2013, 8:52 p.m.
Some changes of the flags may need to trigger additional actions.
Make the flag change function generic and have the per-attribute
function call that.

Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
 net/bridge/br_sysfs_if.c |   39 ++++++++++++++++++++++++++++-----------
 1 files changed, 28 insertions(+), 11 deletions(-)
stephen hemminger - April 19, 2013, 8:54 p.m.
On Fri, 19 Apr 2013 16:52:46 -0400
Vlad Yasevich <vyasevic@redhat.com> wrote:

>  
> +static ssize_t show_flag(struct net_bridge_port *p, char *buf,
> +			  unsigned long mask)
> +{
> +	return sprintf(buf, "%d\n", !!(p->flags & mask));
> +}

Flags are bit, show in hex please.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
stephen hemminger - April 19, 2013, 8:55 p.m.
On Fri, 19 Apr 2013 16:52:46 -0400
Vlad Yasevich <vyasevic@redhat.com> wrote:

> +
> +static int store_flag(struct net_bridge_port *p, unsigned long v,
> +		     unsigned long mask)
> +{
> +	unsigned long flags = p->flags;
> +
> +	if (v)
> +		flags |= mask;
> +	else
> +		flags &= ~mask;
> +
> +	if (flags != p->flags) {
> +		p->flags = flags;
> +		br_ifinfo_notify(RTM_NEWLINK, p);
> +	}
> +	return 0;
> +}

I don't want to allow arbitrary flag stores (and shows).
It exposes kernel bits to user space and creates an unintended ABI.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vlad Yasevich - April 19, 2013, 9:33 p.m.
On 04/19/2013 04:55 PM, Stephen Hemminger wrote:
> On Fri, 19 Apr 2013 16:52:46 -0400
> Vlad Yasevich <vyasevic@redhat.com> wrote:
>
>> +
>> +static int store_flag(struct net_bridge_port *p, unsigned long v,
>> +		     unsigned long mask)
>> +{
>> +	unsigned long flags = p->flags;
>> +
>> +	if (v)
>> +		flags |= mask;
>> +	else
>> +		flags &= ~mask;
>> +
>> +	if (flags != p->flags) {
>> +		p->flags = flags;
>> +		br_ifinfo_notify(RTM_NEWLINK, p);
>> +	}
>> +	return 0;
>> +}
>
> I don't want to allow arbitrary flag stores (and shows).
> It exposes kernel bits to user space and creates an unintended ABI.
>

Stephen

This is really no different then what you currently have.  It simply
removed code duplication and allows for slight sensibility.

Right now, the above function is essentially defined for every flag
attributed that you define with BRPORT_ATTR_FLAG().  You essentially 
have store_<flag>_name() function for every flag that replicates the 
above code.

This patch removes this duplication by having each store_<flag>_name()
function call into this new static store_flag() function with the
mask that is coded and expanded at compile time.

So there is no new ABI, there is no arbitrary flag stores, but there is
smaller code and there is ability to extend the flag store behavior to
possibly do something else it if is needed.

-vlad

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vlad Yasevich - April 19, 2013, 9:35 p.m.
On 04/19/2013 04:54 PM, Stephen Hemminger wrote:
> On Fri, 19 Apr 2013 16:52:46 -0400
> Vlad Yasevich <vyasevic@redhat.com> wrote:
>
>>
>> +static ssize_t show_flag(struct net_bridge_port *p, char *buf,
>> +			  unsigned long mask)
>> +{
>> +	return sprintf(buf, "%d\n", !!(p->flags & mask));
>> +}
>
> Flags are bit, show in hex please.
>

I did not change the output format.  In fact, on second look
I can pull this change out as it isn't needed.

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

Patch

diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
index 1f28cd4..606362c 100644
--- a/net/bridge/br_sysfs_if.c
+++ b/net/bridge/br_sysfs_if.c
@@ -25,6 +25,10 @@  struct brport_attribute {
 	ssize_t (*show)(struct net_bridge_port *, char *);
 	int (*store)(struct net_bridge_port *, unsigned long);
 };
+static ssize_t show_flag(struct net_bridge_port *p, char *buf,
+			  unsigned long mask);
+static int store_flag(struct net_bridge_port *p, unsigned long v,
+		     unsigned long mask);
 
 #define BRPORT_ATTR(_name,_mode,_show,_store)		        \
 const struct brport_attribute brport_attr_##_name = { 	        \
@@ -37,24 +41,37 @@  const struct brport_attribute brport_attr_##_name = { 	        \
 #define BRPORT_ATTR_FLAG(_name, _mask)				\
 static ssize_t show_##_name(struct net_bridge_port *p, char *buf) \
 {								\
-	return sprintf(buf, "%d\n", !!(p->flags & _mask));	\
+	return show_flag(p, buf, _mask);			\
 }								\
 static int store_##_name(struct net_bridge_port *p, unsigned long v) \
 {								\
-	unsigned long flags = p->flags;				\
-	if (v)							\
-		flags |= _mask;					\
-	else							\
-		flags &= ~_mask;				\
-	if (flags != p->flags) {				\
-		p->flags = flags;				\
-		br_ifinfo_notify(RTM_NEWLINK, p);		\
-	}							\
-	return 0;						\
+	return store_flag(p, v, _mask);				\
 }								\
 static BRPORT_ATTR(_name, S_IRUGO | S_IWUSR,			\
 		   show_##_name, store_##_name)
 
+static ssize_t show_flag(struct net_bridge_port *p, char *buf,
+			  unsigned long mask)
+{
+	return sprintf(buf, "%d\n", !!(p->flags & mask));
+}
+
+static int store_flag(struct net_bridge_port *p, unsigned long v,
+		     unsigned long mask)
+{
+	unsigned long flags = p->flags;
+
+	if (v)
+		flags |= mask;
+	else
+		flags &= ~mask;
+
+	if (flags != p->flags) {
+		p->flags = flags;
+		br_ifinfo_notify(RTM_NEWLINK, p);
+	}
+	return 0;
+}
 
 static ssize_t show_path_cost(struct net_bridge_port *p, char *buf)
 {