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. :) 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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
