Re: [GIT PULL] apparmor changes for v6.2

[John Johansen]

On 12/14/22 13:54, Linus Torvalds wrote:
> On Wed, Dec 14, 2022 at 10:36 AM John Johansen
> <john.johansen@xxxxxxxxxxxxx> wrote:
> > John Johansen (45):
> >         apparmor: make unpack_array return a trianary value
>
> John, this is unacceptable.

ack, sorry.

> I noticed it due to the conflict, but this really is garbage.
>
> First off, the word is "ternary" (or possibly "tristate").
>
> Secondly, we don't do types like this
>
>      #define tri int

ack

> and even if we did, that's a *horrible* name not just for a type, but
> for a #define.
>
> Finally, what the heck is "TRI_TRUE/TRI_NONE/TRI_FALSE"? WTF?
>
> It looks like it is used in one single place - the return value for
> "unpack_array()" (now renamed "aa_unpack_array()"), and the TRI_FALSE
> case is basically an error case for an invalid case.
>
> And TRI_NONE is just a *different* failure case ("name does not exist"
> vs "data is invalid").
>
> And then, to make matters worse, ABSOLUTELY NOBODY CARES ABOUT THE
> DIFFERENCE. All real users just want to see TRI_TRUE (for "success"),
> and anything else is an error anyway.
right, the end goal being not two invalid cases but a case of this
is optional but if not present some default data needs to be tied
in. This can be represented different ways, and using the int as
you suggest below seems like the right way to go.

It also looks like I kicked out the following patch that used this
mess, for further revision and sadly didn't drop this one as well.

> Yes, yes, there's that one KUNIT test, which wants to actually see
> that TRI_FALSE because it's testing that array-out-of-bounds case. It
> also - for some unfathomable reason - seems to then want to see some
> particular pointer values in that invalid data after the failure,
> which seems bogus, but whatever.
>
> In other words, that type is badly done and mis-named to start with,
> but then the different ternary values themselves are confusingly
> mis-named too in ways that make no sense.
>
> And to cap it all off, NOBODY CARES about those horrid things anyway.
>
> Anyway, I started out doing the mindless conflict resolution, but then
> I just couldn't deal with that 'tri' type. There were just too many
> things wrong with it for me to accept it, and I felt dirty for just
> editing it.
>
> Then I tried out just making it a
>
>       typedef enum { TRI_TRUE/TRI_NONE/TRI_FALSE } ternary_t;
>
> which fixes some of the syntactic issues.
>
> But the whole naming confusion of the values and how NONE-vs-FALSE
> wasn't actually a useful distinction anyway made me just axe it
> completely.

okay

> I'm honestly baffled by why you didn't just make it return the size or
> a negative error code, like is the norm. The size is limited to 16
> bits anyway, so returning an 'int' with a negative error would have
> been very natural.

indeed, tbh I have no clue why. As you say the int type fits right
in with existing kernel code, and doesn't have any range problems.

> But just to keep the pattern with some of the other users, and
> minimize my surgery, I made it just return 'bool'.
okay

> I'm sorry to do all that surgery on it, but I just couldn't stomach
> doing anything else.

understandable

> The resulting merge diff is fairly messy, and to make matters worse I
> can't actually *test* any of this. But the code looks more palatable

I will make sure it gets run through all the testing

> to me, and I did try to be careful about my surgery.
as always, I have no worries about that

> Apologies if I broke something,

none, needed. You did what you needed to do. If needed I will follow-up
with a patch.