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

[Matthias Bolte]

2011/7/6 Eric Blake <eblake at redhat.com>:
> 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.

Yes, the word "report" gives the wrong impression, I'll go with your suggestion.

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

This one was hard to spot. I didn't see it in the v1 of this patch
that already touched this line. I only noticed it on the second visit.

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

I'll change that for clarity.

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

Thanks. I'll push this patch with an improved commit message and
function comment and defer the logic improvements to a followup patch.

-- 
Matthias Bolte
http://photron.blogspot.com