Linux Ethernet Bridging - Nov 2018 - [Bridge] [PATCH net-next 1/2] net: bridge: add support for user-controlled bool options

> +/* br_boolopt_toggle - change user-controlled boolean option
> + *
> + * @br: bridge device
> + * @opt: id of the option to change
> + * @on: new option value
> + *
> + * Changes the value of the respective boolean option to @on taking care
of
> + * any internal option value mapping and configuration.
> + */
> +int br_boolopt_toggle(struct net_bridge *br, enum br_boolopt_id opt, bool
on)
> +{
> +	int err = -ENOENT;
> +
> +	switch (opt) {
> +	default:
> +		break;
> +	}
> +
> +	return err;
> +}
> +
> +int br_boolopt_multi_toggle(struct net_bridge *br,
> +			    struct br_boolopt_multi *bm)
> +{
> +	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);
> +		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;
> +		}
An old kernel with a new iproute2 might partially succeed in toggling
some low bits, but then silently ignore a high bit that is not
supported by the kernel. As a user, i want to know the kernel does not
support an option i'm trying to toggle.

Can you walk all the bits in the u32 from the MSB to the LSB? That
should avoid this problem.

	Andrew

On 22/11/2018 17:49, Andrew Lunn wrote:
>> +/* br_boolopt_toggle - change user-controlled boolean option
>> + *
>> + * @br: bridge device
>> + * @opt: id of the option to change
>> + * @on: new option value
>> + *
>> + * Changes the value of the respective boolean option to @on taking
care of
>> + * any internal option value mapping and configuration.
>> + */
>> +int br_boolopt_toggle(struct net_bridge *br, enum br_boolopt_id opt,
bool on)
>> +{
>> +	int err = -ENOENT;
>> +
>> +	switch (opt) {
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return err;
>> +}
>> +
> 
> 
>> +int br_boolopt_multi_toggle(struct net_bridge *br,
>> +			    struct br_boolopt_multi *bm)
>> +{
>> +	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);
>> +		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;
>> +		}
> 
> An old kernel with a new iproute2 might partially succeed in toggling
> some low bits, but then silently ignore a high bit that is not
> supported by the kernel. As a user, i want to know the kernel does not
> support an option i'm trying to toggle.
> 
That is already how netlink option setting works, if the option isn't
supported
it is silently ignored. I tried to stay as close to the current behaviour as
possible.
It also applies to partial option changes, some options will be changed until an
error
is encountered.
> Can you walk all the bits in the u32 from the MSB to the LSB? That
> should avoid this problem.
> 
I did wonder about this and left it as netlink option behaviour but
I can leave the walk as is and just check if the highest order set bit >=
BR_BOOLOPT_MAX
and err out with ENOTSUPP for example. Will reconsider for v2.
> 	Andrew
>