On Wed, Jan 18, 2023 at 05:37:37PM +0100, Claudio Jeker wrote:
> On Wed, Jan 18, 2023 at 05:18:58PM +0100, Theo Buehler wrote:
> > On Wed, Jan 18, 2023 at 02:46:19PM +0100, Claudio Jeker wrote:
> > > This is the next step in vstate cleanup.
> > > Since the vstate is now part of struct filterstate use that information
> > > instead of passing an explicit vstate to the various update functions.
> >
> > It took me a moment to understand that rde_filterstate_clean() does not
> > zero the vstate, so the changes in rde_update.c preserve behavior.
> > That's not super intuitive to me, but if that's bound to stay this way,
>
> Uhm, now I'm a bit confused. Where is the vstate used after calling
> rde_filterstate_clean()? I did not spot that issue in rde_update.c
> I think we could zero everyting in the filterstate during clean. Just to
> be sure.
It is super confusing. For example in up_generate_addpath() it starts
with:
rde_filterstate_prep(&state, new);
After this, state.vstate == prefix_roa_vstate(new).
if (rde_filter(rules, peer, prefix_peer(new), &addr,
prefixlen, &state) == ACTION_DENY) {
rde_filterstate_clean(&state);
continue;
}
if (up_enforce_open_policy(peer, &state)) {
rde_filterstate_clean(&state);
continue;
}
/* from here on we know this is an update */
p = prefix_adjout_get(peer, new->path_id_tx, &addr, prefixlen);
up_prep_adjout(peer, &state, addr.aid);
None of the above code touches state.vstate.
prefix_adjout_update(p, peer, &state, &addr,
prefixlen, new->path_id_tx);
and here you dropped the prefix_roa_vstate(new) argument that is now
taken from state, which is correct.
In up_generate_update() I think there's something wrong.
Again, the vstate is copied from new at the start
rde_filterstate_prep(&state, new);
then new may or may not change over the next few lines, and the two
rde_filterstate_clean(&state) before prefix_adjout_update() don't
change state's vstate, and state.vstate may or may not match
prefix_adjout_update(new).
