On Wed, Jan 24, 2024 at 05:13:45PM +0100, Phil Sutter wrote:
> On Wed, Jan 24, 2024 at 04:42:16PM +0100, Florian Westphal wrote:
> > Phil Sutter <phil@xxxxxx> wrote:
> > > While you correctly hate the game instead of its player, you probably
> > > hate the wrong game: The code above indeed is confusing. Maybe one
> > > should move that monotonicity check into libxtables which should
> > > simplify it quite a bit. I'll have a look. :)
> >
> > Something IS broken. Still not working on FC 39 test machine
> > even after fresh clone.
> >
> > On a "working" VM:
> > export XTABLES_LIBDIR=$(pwd)/extensions
> > iptables/xtables-nft-multi ebtables -A INPUT --stp-root-cost 1
> > have 1 32765
> >
> > @@ -150,7 +151,9 @@ static void brstp_parse(struct xt_option_call *cb)
> > RANGE_ASSIGN("root-prio", root_prio, cb->val.u16_range);
> > break;
> > case O_RCOST:
> > + fprintf(stderr, "have %u %u\n", cb->val.u32_range[0], cb->val.u32_range[1]);
> >
> > I can't even figure out where the correct max value is supposed to be set.
> >
> > Varying the input:
> >
> > xtables-nft-multi ebtables -A INPUT --stp-root-cost 1
> > have 1 32764
> >
> > Looks to me as if the upper value is undefined.
> >
> > Other users of *RC versions handle it in .parse, e.g. libxt_length.
> > No idea how this is working.
>
> In xtopt_parse_mint(), there is:
>
> | const uintmax_t lmax = xtopt_max_by_type(entry->type);
> | [...]
> | if (*arg == '\0' || *arg == sep) {
> | /* Default range components when field not spec'd. */
> | end = (char *)arg;
> | value = (cb->nvals == 1) ? lmax : 0;
>
> But that branch appears to be dead code. So this is indeed a bug and a
> specific build may or may not hit it as your experience shows. I'll see
> how xtopt_parse_mint() can be fixed.
The big elucidation was the code is called only for ranges and somehow I
managed to miss the point that your sample command doesn't contain a
range in the first place.
So while I still think it makes sense to have the 'low <= high' check
done by the parser, I applied your patch for now as it indeed fixes that
bug in libebt_stp extension parser. Sorry for all the confusion I must
have caused. :(
Meanwhile I've added test cases for ranges in various formats which
uncovered quite a few things to fix.
Thanks, Phil
