On Mon, Mar 3, 2025 at 2:04 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Mon, 2025-03-03 at 13:38 -0800, Andrii Nakryiko wrote:
> > On Sat, Mar 1, 2025 at 1:45 PM Mykyta Yatsenko
> > <mykyta.yatsenko5@xxxxxxxxx> wrote:
> > >
> > > On 01/03/2025 08:12, Eduard Zingerman wrote:
> > > > On Fri, 2025-02-28 at 17:52 +0000, Mykyta Yatsenko wrote:
> > > >
> > > > [...]
> > > >
> > > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > > > index 9ced1ce2334c..dd2f64903c3b 100644
> > > > > --- a/tools/lib/bpf/libbpf.c
> > > > > +++ b/tools/lib/bpf/libbpf.c
> > > > > @@ -4858,7 +4858,7 @@ bool bpf_map__autocreate(const struct bpf_map *map)
> > > > >
> > > > > int bpf_map__set_autocreate(struct bpf_map *map, bool autocreate)
> > > > > {
> > > > > - if (map->obj->state >= OBJ_LOADED)
> > > > > + if (map->obj->state >= OBJ_PREPARED)
> > > > > return libbpf_err(-EBUSY);
> > > > I looked through logic in patches #1 and #2 and changes look correct.
> > > > Running tests under valgrind does not show issues with this feature.
> > > > The only ask from my side is to consider doing ==/!= comparisons in
> > > > cases like above. E.g. it seems that `map->obj->state != OBJ_OPENED`
> > > > is a bit simpler to understand when reading condition above.
> > > > Or maybe that's just me.
> > > I'm not sure about this one. >= or < checks for state relative to
> > > operand more
> > > flexibly,for example `map->obj->state >= OBJ_PREPARED` is read as
> > > "is the object in at least PREPARED state". Perhaps, if we add more states,
> > > these >,< checks will not require any changes, while ==, != may.
> > > I guess this also depends on what we actually want to check here, is it that
> > > state at least PREPARED or the state is not initial OPENED.
> > > Not a strong opinion, though, happy to flip code to ==, !=.
> >
> > Those steps are logically ordered, so >= and <= makes more sense. If
> > we ever add one extra step somewhere in between existing steps, most
> > checks will stay correct, while with equality a lot of them might need
> > to be adjusted to multiple equalities.
>
> As I said, for me personally it is easier to read "can set autocreate
> only in OPENED state", compared to "can't set autocreate if state is
> PREPARED of higher".
The latter, IMO. PREPARED state is when maps are finalized, so if we
got there or beyond, can't modify maps. For progs the similar step is
LOADED.
> But whatever, I'm not a true C programmer anyway :)
>
> [...]
>
