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

On 07/05/2011 03:04 PM, Matthias Bolte wrote: 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. 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. 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; I probably would do s/path/directory/, although it's not critical to this patch. Same shortcut question as for virFileMakePathHelper. At this point, why don't we just call virFileMakePathHelper, rather than... ...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. -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org