Quoting Oren Laadan (orenl@xxxxxxxxxxxxxxx): > > > Serge E. Hallyn wrote: > > Quoting Oren Laadan (orenl@xxxxxxxxxxxxxxx): > >> From: Dan Smith <danms@xxxxxxxxxx> > >> > >> Changes: > >> - Update to match UTS changes > >> > >> Signed-off-by: Dan Smith <danms@xxxxxxxxxx> > >> Signed-off-by: Oren Laadan <orenl@xxxxxxxxxxxxxxx> > > > > Acked-by: Serge Hallyn <serue@xxxxxxxxxx> > > > > However... > > > >> + if (!!ipc_ns ^ !(flags & CLONE_NEWIPC)) > >> + return -EINVAL; > > > > Every time I see this I have to think about whether it is right or not. > > I'm not sure whether it's worth commenting (at each such meme) that > > CLONE_NEWIPC only needed to be set the first time we ran across that > > ipcns, or whether it's indicative that there is a simpler way the code > > could be done. But if it just took me a twice-over to see that it's > > right, when I'd already confirmed that with the CLONE_NEWUTS version > > last week, then a fresh reviewer will be cursing your name... > > This is a consistency check: ensure that the state of the objhash as > reflected in the value of ipc_ns is consistent with the flags. I know what it is, but coding it this way is inconsiderate to everyone trying to review the code. The mind doesn't do well with 'not' to begin with, and you've got three of them plus and xor here in one line, in the flow of a function. (Sure, you can argue !! is not two nots :) So while you're trying to read the flow of the function you always stop there. That impedes review. (I'm not trying to be criticial or beat a dead horse, just trying to explain precisely what it is that is hard to review - since in the end the person reviewing the code when there's a subtle bug in 2 years is the one the code should be designed for) > I'll add a comment. Thanks. But actually I think it's better if you move that into an appropriately named little static inline helper (commented). Then the reader can verify it once separately, and then ignore how it works while verifying the surrounding code. Not just here but, other places where you do this. thanks, -serge _______________________________________________ Containers mailing list Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/containers
