Re: [libvirt] [PATCH] Fix return value semantic of virFileMakePath

On 07/05/2011 03:04 PM, Matthias Bolte wrote: > Some callers expected virFileMakePath to set errno, some expected > it to return an errno value. Unify this to return 0 on success and > -1 on error. Set errno to report detailed error information. > > Also Make virFileMakePath report an error when stat fails with an > errno different from ENOENT. When I first read this, I thought that you meant that virFileMakePath would use virReportSystemError, but only on that particular failure path.  But on reading the patch, I see that you merely meant that this patch now guarantees that virFileMakePath returns -1 for errno different than ENOENT right away, rather than wasting time calling strdup and eventually mkdir and possibly having the errno from mkdir be less specific than the errno from the initial failed stat.  So maybe reword this as: Also optimize virFileMakePath if stat fails with an errno different from ENOENT.

> @@ -429,8 +429,8 @@ static int lxcContainerMountBasicFS(virDomainFSDefPtr root) >          } >      } > > -    if ((rc = virFileMakePath("/dev/pts") != 0)) { > -        virReportSystemError(rc, "%s", > +    if (virFileMakePath("/dev/pts") < 0) { > +        virReportSystemError(errno, "%s", Nice bug fix, by the way.  Matthias had to point out to me on IRC that the parenthesis were wrong, and that virFileMakePAth("/dev/pts") != 0 ended up making rc only 0 or 1, rather than 0 or an errno value, making for a misleading virReportSystemError output pre-patch.

> +++ b/src/util/util.c > @@ -1010,66 +1010,80 @@ int virDirCreate(const char *path ATTRIBUTE_UNUSED, >  } >  #endif /* WIN32 */ > > -static int virFileMakePathHelper(char *path) { > +static int virFileMakePathHelper(char *path) > +{ >      struct stat st; >      char *p = NULL; > -    int err; > >      if (stat(path, &st) >= 0) >          return 0; > +    else if (errno != ENOENT) > +        return -1; The 'else' isn't strictly necessary.  And as long as we are taking shortcuts, should we use this instead? if (stat(path, &st) >= 0) {    if (S_ISDIR(st.st_mode))        return 0;    errno = ENOTDIR;    return -1; } if (errno != ENOENT)    return -1; > +/** > + * Creates the given path with mode 0777 if it's not already existing I probably would do s/path/directory/, although it's not critical to this patch.

> + * completely. > + * > + * Returns 0 on success, or -1 if an error occurred (in which case, errno > + * is set appropriately). > + */ >  int virFileMakePath(const char *path) >  { > +    int ret = -1; >      struct stat st; >      char *parent = NULL; >      char *p; > -    int err = 0; > >      if (stat(path, &st) >= 0) > +        return 0; > +    else if (errno != ENOENT) >          goto cleanup; Same shortcut question as for virFileMakePathHelper. > >      if ((parent = strdup(path)) == NULL) { > -        err = ENOMEM; > +        errno = ENOMEM; >          goto cleanup; >      } At this point, why don't we just call virFileMakePathHelper, rather than... > >      if ((p = strrchr(parent, '/')) == NULL) { > -        err = EINVAL; > +        errno = EINVAL; >          goto cleanup; >      } > >      if (p != parent) { >          *p = '\0'; > -        if ((err = virFileMakePathHelper(parent)) != 0) { > + > +        if (virFileMakePathHelper(parent) < 0) >              goto cleanup; > -        } >      } > > -    if (mkdir(path, 0777) < 0 && errno != EEXIST) { > -        err = errno; > +    if (mkdir(path, 0777) < 0 && errno != EEXIST) >          goto cleanup; > -    } ...copying the same code for the strrchr, recursion, and mkdir ourselves?  For that matter, even the initial stat() could be skipped, and have this function be _just_ the strdup and call into the recursive helper. On the other hand, that could be a cleanup for a separate patch; what you have here is minimally invasive for fixing the problem at hand, so: ACK.