2011/7/6 Eric Blake <eblake@xxxxxxxxxx>:
> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list