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.
--
Eric Blake eblake@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list