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

[Ben Pfaff]

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.

I wish we could just use the appropriate code from gnulib for this but,
alas, licenses.

I'm not sure that it's worth adding a check or a fallback for lstat
given that chutil-unix.c is only built on unixlike systems (also, OVS
already uses lstat elsewhere on unixlikes without checking).

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().

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.

However, the logic here seems slightly wrong to me anyway:
    if (e != 0 || !res) {
        if (!res) e = ENOENT;
        VLOG_ERR("Failed to retrieve user pwentry (%s), aborting.",
                 ovs_strerror(e));
        goto release;
    }
I guess the logic is supposed to be that, if there was no error but no
result was returned, assume ENOENT.  But for that wouldn't one write
something more like this:
        if (!e) {
            e = ENOENT;
        }
or even ovs_strerror(e ? e : ENOENT).

Similar comments for the group parsing code in the same function.

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);

It looks like chmod_getmode() doesn't allow multiple symbolic modes,
e.g. u+rw,g+r, or nonincremental modes, e.g. o=rwx.  I don't think it
would be difficult to add those, can you do it?

In ovs_chmod() and ovs_chown(), often it doesn't read well to
interpolate error strings into messages directly, because you end up
with things like
        ovs_chmod: stat error No such file or directory for file nonexistent
so ordinarily we'd separate them grammatically, e.g.:
        ovs_chmod: stat error for file nonexistent (No such file or directory)

It's odd that ovs_chmod() and ovs_chown() set errno to 0 initially, is
that necessary?

The header guard in chutil.h should come first, instead of after the
#includes.

In the tests, please write the { that opens a function on a line by
itself.