[Devel] Re: [RFC v14-rc3][PATCH 36/36] Stub implementation of IPC namespace c/r

[Serge E. Hallyn]

Quoting Oren Laadan (orenl at cs.columbia.edu):
> 
> 
> Serge E. Hallyn wrote:
> > Quoting Oren Laadan (orenl at cs.columbia.edu):
> >> From: Dan Smith <danms at us.ibm.com>
> >>
> >> Changes:
> >>  - Update to match UTS changes
> >>
> >> Signed-off-by: Dan Smith <danms at us.ibm.com>
> >> Signed-off-by: Oren Laadan <orenl at cs.columbia.edu>
> > 
> > Acked-by: Serge Hallyn <serue at us.ibm.com>
> > 
> > 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 at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers