From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sabrina Dubroca Subject: Re: [PATCH net-next v2 3/3] vxlan: move flag sets to use a helper func Date: Thu, 29 Nov 2018 15:19:36 +0100 Message-ID: <20181129141936.GB20478@bistromath.localdomain> References: <1543444079-29883-1-git-send-email-roopa@cumulusnetworks.com> <1543444079-29883-4-git-send-email-roopa@cumulusnetworks.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Cc: davem@davemloft.net, netdev@vger.kernel.org To: Roopa Prabhu Return-path: Received: from mx1.redhat.com ([209.132.183.28]:49258 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729806AbeK3BZK (ORCPT ); Thu, 29 Nov 2018 20:25:10 -0500 Content-Disposition: inline In-Reply-To: <1543444079-29883-4-git-send-email-roopa@cumulusnetworks.com> Sender: netdev-owner@vger.kernel.org List-ID: 2018-11-28, 14:27:59 -0800, Roopa Prabhu wrote: > +/* Set/clear flags based on attribute */ > +static void vxlan_nl2flag(struct vxlan_config *conf, struct nlattr *tb[], > + int attrtype, unsigned long mask) > +{ > + unsigned long flags; > + > + if (!tb[attrtype]) > + return; > + > + if (nla_get_u8(tb[attrtype])) > + flags = conf->flags | mask; > + else > + flags = conf->flags & ~mask; > + > + conf->flags = flags; > +} > + > static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[], > struct net_device *dev, struct vxlan_config *conf, > bool changelink, struct netlink_ext_ack *extack) > @@ -3458,45 +3475,27 @@ static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[], > if (data[IFLA_VXLAN_TTL]) > conf->ttl = nla_get_u8(data[IFLA_VXLAN_TTL]); > > - if (data[IFLA_VXLAN_TTL_INHERIT]) > - conf->flags |= VXLAN_F_TTL_INHERIT; > + vxlan_nl2flag(conf, data, IFLA_VXLAN_TTL_INHERIT, > + VXLAN_F_TTL_INHERIT); IFLA_VXLAN_TTL_INHERIT is a NLA_FLAG. It doesn't have any u8 data to get. Same for: [IFLA_VXLAN_GBP] = { .type = NLA_FLAG, }, [IFLA_VXLAN_GPE] = { .type = NLA_FLAG, }, [IFLA_VXLAN_REMCSUM_NOPARTIAL] = { .type = NLA_FLAG }, [IFLA_VXLAN_TTL_INHERIT] = { .type = NLA_FLAG }, nit: This patch would have been easier to review if it came first in the series. Converting: if (nla_get_u8(data[IFLA_VXLAN_FOO])) conf->flags |= VXLAN_F_FOO; to this new helper, which in effect does if (nla_get_u8(data[IFLA_VXLAN_FOO])) conf->flags |= VXLAN_F_FOO; else conf->flags &= ~VXLAN_F_FOO; seems to change behavior, but since you're (unless I missed one) only applying it to flags that couldn't be changed before patch 1, it looks fine. -- Sabrina