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

[Aaron Conole]

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. :)  Also, thanks so
much for the review, Ben.

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

+1

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

okay, I can just drop it.

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

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

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

Okay, yep you got the idea.  I'll rewrite it so that it looks like:

/* This is a case where no result was returned, but an error was not
   signaled by the get*ent api */
if (!e && !res) {
    e = ENOENT;
}

if (e != 0) {
    ...
}

I'll also clean up the error message.

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

> 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?

ack.

> 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?

Yes, at least according to the manpages.

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

Okay.

> In the tests, please write the { that opens a function on a line by
> itself.
d'oh!  Will do.