| Message ID | 1366404770-28523-3-git-send-email-vyasevic@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
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
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
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) {
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(-)