Messages in this thread | ![/](/images/icornerl.gif) | | From | Linus Torvalds <> | Date | Wed, 14 Dec 2022 13:54:04 -0800 | Subject | Re: [GIT PULL] apparmor changes for v6.2 |
| |
On Wed, Dec 14, 2022 at 10:36 AM John Johansen <john.johansen@canonical.com> wrote: > > John Johansen (45): > apparmor: make unpack_array return a trianary value
John, this is unacceptable.
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
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.
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.
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.
But just to keep the pattern with some of the other users, and minimize my surgery, I made it just return 'bool'.
I'm sorry to do all that surgery on it, but I just couldn't stomach doing anything else.
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 to me, and I did try to be careful about my surgery.
Apologies if I broke something,
Linus
| ![\](/images/icornerr.gif) |