On Thu, May 19, 2016 at 02:27:38PM -0400, Aaron Conole wrote:
> Ben Pfaff <b...@ovn.org> writes:
>
> > On Tue, May 03, 2016 at 01:26:39PM -0400, Aaron Conole wrote:
> >> It will be useful in the future to be able to set ownership and permissions
> >> on files which Open vSwitch creates. Allowing the specification of such
> >> ownership and permissions using the standard user:group, uog+-rwxs, and
> >> numerical forms commonly associated with those actions.
> >>
> >> This patch introduces a new chutil library, currently with a posix command
> >> implementation. WIN32 support does not exist at this time, but could be
> >> added
> >> in the future.
> >>
> >> As part of this, the daemon-unix.c was refactored to move the ownership
> >> parsing code to the chutil library. A new set of tests was added, and the
> >> chmod side is implemented.
> >>
> >> Signed-off-by: Aaron Conole <acon...@redhat.com>
> >
> > This is very thorough. I am impressed.
>
> You can't take it back, now. It's on the internet. :)
Hahaha.
> > I'm not sure whether it's worth both using sysconf() to check for buffer
> > sizes and dynamically resizing as necessary. The latter is necessary
> > anyhow so it seems like it's enough on its own.
> >
> > An equivalent of enlarge_buffer() is already available as x2nrealloc().
>
> okay. I moved it over from the daemon code, and figured I should
> preserve it. But I'll drop it, and just use the x2nrealloc() code
> instead.
I didn't notice that this was just moved code.
I still think x2nrealloc() is better.
> > Please always write {} around 'if' and 'while' statements, e.g.
> > if (!res) {
> > e = ENOENT;
> > }
> > instead of:
> > if (!res) e = ENOENT;
> > in accordance with CodingStyle.md.
>
> Expect a possible checkpatch.py patch coming soon for this. Oops.
Great, that's a good idea.
> > In chmod_getmode() here, I hadn't observed before that ('0' & 7) == 0
> > but really it's better to write -'0' since that's completely portable
> > and more obviously correct:
> > ret = (ret << 3) | (*mode++ & 0x7);
>
> Sorry, I thought it was clever :). I can switch, but it will make the
> code a bit larger (because it still needs to mask the bits).
Why? To quote another line:
while (*mode >= '0' && *mode <= '7')
ret = (ret << 3) | (*mode++ & 0x7);
Since *mode >= '0' && *mode <= '7', (*mode - '0') >= 0 && (*mode - '0')
<= 7, so adding & 7 will have no additional effect.
> > It's odd that ovs_chmod() and ovs_chown() set errno to 0 initially, is
> > that necessary?
>
> Yes, at least according to the manpages.
Which manpages?
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
