[ovs-dev] [ovs-dev, 1/2] chutil: introduce a new change-utils lib

Ben Pfaff blp at ovn.org
Fri May 20 02:46:33 UTC 2016


On Thu, May 19, 2016 at 02:27:38PM -0400, Aaron Conole wrote:
> Ben Pfaff <blp at 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 <aconole at 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.



More information about the dev mailing list