Nikolay Aleksandrov
2018-Nov-25 08:12 UTC
[Bridge] [PATCH net-next v2 1/3] net: bridge: add support for user-controlled bool options
On 24/11/2018 18:46, nikolay at cumulusnetworks.com wrote:> On 24 November 2018 18:25:41 EET, Andrew Lunn <andrew at lunn.ch> wrote: >> On Sat, Nov 24, 2018 at 06:18:33PM +0200, nikolay at cumulusnetworks.com >> wrote: >>> On 24 November 2018 18:10:41 EET, Andrew Lunn <andrew at lunn.ch> wrote: >>>>> +int br_boolopt_toggle(struct net_bridge *br, enum br_boolopt_id >> opt, >>>> bool on, >>>>> + struct netlink_ext_ack *extack) >>>>> +{ >>>>> + switch (opt) { >>>>> + default: >>>>> + /* shouldn't be called with unsupported options */ >>>>> + WARN_ON(1); >>>>> + break; >>>> >>>> So you return 0 here, meaning the br_debug() lower down will not >>>> happen. Maybe return -EOPNOTSUPP? >>>> >>> >>> No, the idea here is that some option in the future might return an >> error. >>> This function cannot be called with unsupported option thus the warn. >> >> >> O.K, i was trying to make it easier to see which option caused it to >> happen. >> >>>>> + } >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>> >>>>> +int br_boolopt_multi_toggle(struct net_bridge *br, >>>>> + struct br_boolopt_multi *bm, >>>>> + struct netlink_ext_ack *extack) >>>>> +{ >>>>> + unsigned long bitmap = bm->optmask; >>>>> + int err = 0; >>>>> + int opt_id; >>>>> + >>>>> + for_each_set_bit(opt_id, &bitmap, BR_BOOLOPT_MAX) { >>>>> + bool on = !!(bm->optval & BIT(opt_id)); >>>>> + >>>>> + err = br_boolopt_toggle(br, opt_id, on, extack); >>>>> + if (err) { >>>>> + br_debug(br, "boolopt multi-toggle error: option: %d current: >> %d >>>> new: %d error: %d\n", >>>>> + opt_id, br_boolopt_get(br, opt_id), on, err); >>>>> + break; >>>>> + } >>>>> + } >>>> >>>> Does the semantics of extack allow you to return something even when >>>> there is no error? If there are bits > BR_BOOLOPT_MAX you could >> return >>>> 0, but also add a warning in extack that some bits where not >> supported >>>> by this kernel. >>> >>> If we return 0 there's no reason to check extack. >> >> Well, the caller can check to see if extack is present, even on >> success. This is extack, not extnack after all... >> > > Evenif it's possible to return it without an error (I need to confirm that), the real problem is extack doesn't support > format strings, i. e. we can't say which bit is missing which makes it useless in this case IMO. >One more thing I forgot to mention is the case with newer iproute2 and older kernel without these patches, then the whole boolopt option will be ignored without setting extact which makes it inconsistent if user-space would rely on that to check if options were set. I think the best way to go if one wants to check for support is to dump the boolopts if they're present, then optmask could be used to see what will be set (or was set). That would be reliable in all cases.>> Andrew >
Stephen Hemminger
2018-Nov-26 17:39 UTC
[Bridge] [PATCH net-next v2 1/3] net: bridge: add support for user-controlled bool options
On Sun, 25 Nov 2018 10:12:45 +0200 Nikolay Aleksandrov <nikolay at cumulusnetworks.com> wrote:> On 24/11/2018 18:46, nikolay at cumulusnetworks.com wrote: > > On 24 November 2018 18:25:41 EET, Andrew Lunn <andrew at lunn.ch> wrote: > >> On Sat, Nov 24, 2018 at 06:18:33PM +0200, nikolay at cumulusnetworks.com > >> wrote: > >>> On 24 November 2018 18:10:41 EET, Andrew Lunn <andrew at lunn.ch> wrote: > >>>>> +int br_boolopt_toggle(struct net_bridge *br, enum br_boolopt_id > >> opt, > >>>> bool on, > >>>>> + struct netlink_ext_ack *extack) > >>>>> +{ > >>>>> + switch (opt) { > >>>>> + default: > >>>>> + /* shouldn't be called with unsupported options */ > >>>>> + WARN_ON(1); > >>>>> + break; > >>>> > >>>> So you return 0 here, meaning the br_debug() lower down will not > >>>> happen. Maybe return -EOPNOTSUPP? > >>>> > >>> > >>> No, the idea here is that some option in the future might return an > >> error. > >>> This function cannot be called with unsupported option thus the warn.>Please don't implement some part of the API until it is used (YAGNI). If do this kind of "someday will come" design the code will end up littered with dead ends.