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

On Fri, May 20, 2016 at 04:32:05PM -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@redhat.com>

I suggest folding the following in for better conformance to
CodingStyle.md.

With those changes, I'm happy with this patch but I don't want to apply
it until we come to some kind of consensus on patch 2.

Acked-by: Ben Pfaff <blp@ovn.org>

--8<--------------------------cut here-------------------------->8--

Ben Pfaff <blp@ovn.org> writes:

> On Fri, May 20, 2016 at 04:32:05PM -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@redhat.com>
>
> I suggest folding the following in for better conformance to
> CodingStyle.md.

D'oh!  I'm trying to undo 15 years of one particular c++ coding style,
so apologies that I keep messing these up.

> With those changes, I'm happy with this patch but I don't want to apply
> it until we come to some kind of consensus on patch 2.

Agreed.

Thanks for the review, Ben!

-Aaron

> Acked-by: Ben Pfaff <blp@ovn.org>
>
> --8<--------------------------cut here-------------------------->8--
>
> diff --git a/lib/chutil-unix.c b/lib/chutil-unix.c
> index 208ce6f..b61352a 100644
> --- a/lib/chutil-unix.c
> +++ b/lib/chutil-unix.c
> @@ -15,16 +15,18 @@
>   */
>  
>  #include <config.h>
> +
> +#include "chutil.h"
> +
>  #include <errno.h>
> -#include <inttypes.h>
>  #include <grp.h>
> +#include <inttypes.h>
>  #include <pwd.h>
>  #include <stdlib.h>
>  #include <string.h>
>  #include <sys/stat.h>
>  #include <unistd.h>
>  
> -#include "chutil.h"
>  #include "daemon.h"
>  #include "util.h"
>  #include "openvswitch/vlog.h"
> @@ -46,9 +48,8 @@ VLOG_DEFINE_THIS_MODULE(chutil_unix);
>  
>  #define SUID_MODES  (S_ISUID | S_ISGID)
>  
> -/* Convert a chown-style string to uid/gid; supports numeric arguments
> - * as well as usernames.
> - */
> +/* Convert a chown-style string to uid/gid; supports numeric arguments as well
> + * as usernames.  Returns 0 on success, otherwise a positve errno value. */
>  int
>  ovs_strtousr(const char *user_spec, uid_t *uid, char **user, gid_t *gid,
>               bool validate_user_group)
> @@ -229,7 +230,7 @@ chmod_getmode(const char *mode, mode_t oldmode)
>                  actors_mask = USR_MODES;
>              }
>              while (*mode) {
> -                switch(*mode++) {
> +                switch (*mode++) {
>                  case 'r':
>                      perms_mask |= READ_MODES;
>                      break;
> @@ -265,8 +266,8 @@ actions:
>  }
>  
>  /* Changes the mode of a file to the mode specified.  Accepts chmod style
> - * comma-separated strings.
> - */
> + * comma-separated strings.  Returns 0 on success, otherwise a positve errno
> + * value. */
>  int
>  ovs_chmod(const char *path, const char *mode)
>  {
> @@ -307,8 +308,8 @@ ovs_chmod(const char *path, const char *mode)
>  }
>  
>  /* Changes the ownership of a file to the mode specified.  Accepts chown style
> - * user:group strings.
> - */
> + * user:group strings.  Returns 0 on success, otherwise a positve errno
> + * value. */
>  int
>  ovs_chown(const char *path, const char *owner)
>  {
> diff --git a/tests/test-chutil.c b/tests/test-chutil.c
> index 9057220..4465437 100644
> --- a/tests/test-chutil.c
> +++ b/tests/test-chutil.c
> @@ -60,7 +60,8 @@ with_temp_file(int (*fn)(const char *pathname))
>  }
>  
>  static int
> -run_chmod_bad_parsing(const char *pathname) {
> +run_chmod_bad_parsing(const char *pathname)
> +{
>      static char users[] = "bcdefhijklmnpqrstvwxyz";
>      static char perms[] = "abcdefghijklmnopqtuvyz";
>      static char actions[] = "~`!@#$%^&*()_";
> @@ -72,7 +73,7 @@ run_chmod_bad_parsing(const char *pathname) {
>          return -1;
>      }
>  
> -    for (itest = users; itest != users+strlen(users); ++itest) {
> +    for (itest = users; itest != users + strlen(users); ++itest) {
>          char buf[256] = {0};
>          mode_t testmode;
>          snprintf(buf, sizeof(buf), "%c+rwx", *itest);
> @@ -83,7 +84,7 @@ run_chmod_bad_parsing(const char *pathname) {
>          }
>      }
>  
> -    for (itest = perms; itest != perms+strlen(perms); ++itest) {
> +    for (itest = perms; itest != perms + strlen(perms); ++itest) {
>          char buf[256] = {0};
>          mode_t testmode;
>          snprintf(buf, sizeof(buf), "u+%c", *itest);
> @@ -94,7 +95,7 @@ run_chmod_bad_parsing(const char *pathname) {
>          }
>      }
>  
> -    for (itest = actions; itest != actions+strlen(actions); ++itest) {
> +    for (itest = actions; itest != actions + strlen(actions); ++itest) {
>          char buf[256] = {0};
>          mode_t testmode;
>          snprintf(buf, sizeof(buf), "u%crw", *itest);
> @@ -108,7 +109,7 @@ run_chmod_bad_parsing(const char *pathname) {
>      return 0;
>  }
>  
> -//skip suid and sgid for now
> +/* Skip suid and sgid for now. */
>  static int
>  run_chmod_str_successes(const char *pathname)
>  {
> @@ -194,7 +195,7 @@ run_chmod_numeric_successes(const char *pathname)
>  static int
>  run_ovs_strtouser_successes(void)
>  {
> -    // seems this is the only user:group combination to exist?
> +    /* seems this is the only user:group combination to exist? */
>      const char *ugparses[] = {"root:", "root:root", "0:0", "0:", "nobody:",
>                                "root", "nobody"};
>      size_t iugstr;
> @@ -214,8 +215,8 @@ run_ovs_strtouser_successes(void)
>  static int
>  run_ovs_strtouser_failures(void)
>  {
> -    // if any of these are successful, you have a poorly configured system
> -    // so this test 'failing' is the least of your worries
> +    /* If any of these are successful, you have a poorly configured system so
> +     * this test 'failing' is the least of your worries. */
>      const char *ugparses[] = {"nobody:root", "THISUSERBETTERNOTEXSIST:",
>                                ":THISGROUPBETTERNOTEXIST"};
>      size_t iugstr;