[libvirt] [PATCH] Properly report failure to create raw storage volume files
We were previously checking for a return < 0 from virFileOperation(), but that function returns a standard errno, which is 0 on success, or some small positive number on failure. The result was that we wouldn't report failures to create storage volume files; instead they would appear to be created, but then would vanish as soon as a pool-refresh was done (or cause some later error as soon as someone tried to access the volume). The other uses of virFileOperation() were already properly checking for != 0. --- src/storage/storage_backend.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index aba8937..9792eed 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -362,7 +362,7 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, createRawFileOpHook, &hdata, VIR_FILE_OP_FORCE_PERMS | (pool->def->type == VIR_STORAGE_POOL_NETFS - ? VIR_FILE_OP_AS_UID : 0))) < 0) { + ? VIR_FILE_OP_AS_UID : 0))) != 0) { virReportSystemError(createstat, _("cannot create path '%s'"), vol->target.path); -- 1.7.1.1
On Tue, Jul 13, 2010 at 03:00:33AM -0400, Laine Stump wrote:
We were previously checking for a return < 0 from virFileOperation(), but that function returns a standard errno, which is 0 on success, or some small positive number on failure. The result was that we wouldn't report failures to create storage volume files; instead they would appear to be created, but then would vanish as soon as a pool-refresh was done (or cause some later error as soon as someone tried to access the volume).
The other uses of virFileOperation() were already properly checking for != 0. --- src/storage/storage_backend.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index aba8937..9792eed 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -362,7 +362,7 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, createRawFileOpHook, &hdata, VIR_FILE_OP_FORCE_PERMS | (pool->def->type == VIR_STORAGE_POOL_NETFS - ? VIR_FILE_OP_AS_UID : 0))) < 0) { + ? VIR_FILE_OP_AS_UID : 0))) != 0) { virReportSystemError(createstat, _("cannot create path '%s'"), vol->target.path);
Ah, good caatch, ACK ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Tue, Jul 13, 2010 at 03:00:33AM -0400, Laine Stump wrote:
We were previously checking for a return < 0 from virFileOperation(), but that function returns a standard errno, which is 0 on success, or some small positive number on failure. The result was that we wouldn't report failures to create storage volume files; instead they would appear to be created, but then would vanish as soon as a pool-refresh was done (or cause some later error as soon as someone tried to access the volume).
Urgh, functions that return positive numbers for errors are just asking for trouble. If you see an 'int' returning function everyone expects it to return < 0 on error, even if you document otherwise.
The other uses of virFileOperation() were already properly checking for != 0. --- src/storage/storage_backend.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index aba8937..9792eed 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -362,7 +362,7 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, createRawFileOpHook, &hdata, VIR_FILE_OP_FORCE_PERMS | (pool->def->type == VIR_STORAGE_POOL_NETFS - ? VIR_FILE_OP_AS_UID : 0))) < 0) { + ? VIR_FILE_OP_AS_UID : 0))) != 0) { virReportSystemError(createstat, _("cannot create path '%s'"), vol->target.path);
I'd rather we changed virFileOperation to return '-errno' so all error return codes are negative, and then fixup other callers Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
These patches started out as a more "correct" fix for the patch https://www.redhat.com/archives/libvir-list/2010-July/msg00257.html (and they supercede that patch). Daniel Berrange suggested that it would be better to switch the function virFileOperation over to use the preferred convention of returning -errno on failure, rather than errno. While I was doing that, I also changed virDirCreate for consistency. During this process, I found one bug (in patch 4/4) and made a slight improvement to another function's return so that virFileOperation's return would always be -errno (rather than sometimes being a simple -1). (patch 3/4)
virFileOperation previously returned 0 on success, or the value of errno on failure. Although there are other functions in libvirt that use this convention, the preferred (and more common) convention is to return 0 on success and -errno (or simply -1 in some cases) on failure. This way the check for failure is always (ret < 0). * src/util/util.c - change virFileOperation and virFileOperationNoFork to return -errno on failure. * src/storage/storage_backend.c, src/qemu/qemu_driver.c - change the hook functions passed to virFileOperation to return -errno on failure. --- src/qemu/qemu_driver.c | 19 ++++++++++--------- src/storage/storage_backend.c | 17 ++++++++--------- src/util/util.c | 37 ++++++++++++++++++++----------------- 3 files changed, 38 insertions(+), 35 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2eb254e..4818be3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4946,12 +4946,13 @@ struct fileOpHookData { struct qemud_save_header *header; }; +/* return -errno on failure, or 0 on success */ static int qemudDomainSaveFileOpHook(int fd, void *data) { struct fileOpHookData *hdata = data; int ret = 0; if (safewrite(fd, hdata->header, sizeof(*hdata->header)) != sizeof(*hdata->header)) { - ret = errno; + ret = -errno; qemuReportError(VIR_ERR_OPERATION_FAILED, _("failed to write header to domain save file '%s'"), hdata->path); @@ -4959,7 +4960,7 @@ static int qemudDomainSaveFileOpHook(int fd, void *data) { } if (safewrite(fd, hdata->xml, hdata->header->xml_len) != hdata->header->xml_len) { - ret = errno; + ret = -errno; qemuReportError(VIR_ERR_OPERATION_FAILED, _("failed to write xml to '%s'"), hdata->path); goto endjob; @@ -5095,7 +5096,7 @@ static int qemudDomainSaveFlag(virDomainPtr dom, const char *path, virReportSystemError(errno, _("unable to open %s"), path); goto endjob; } - if (qemudDomainSaveFileOpHook(fd, &hdata) != 0) { + if (qemudDomainSaveFileOpHook(fd, &hdata) < 0) { close(fd); goto endjob; } @@ -5108,7 +5109,7 @@ static int qemudDomainSaveFlag(virDomainPtr dom, const char *path, S_IRUSR|S_IWUSR, getuid(), getgid(), qemudDomainSaveFileOpHook, &hdata, - 0)) != 0) { + 0)) < 0) { /* If we failed as root, and the error was permission-denied (EACCES), assume it's on a network-connected share where root access is restricted (eg, root-squashed NFS). If the @@ -5116,9 +5117,9 @@ static int qemudDomainSaveFlag(virDomainPtr dom, const char *path, bypass security driver shenanigans, and retry the operation after doing setuid to qemu user */ - if ((rc != EACCES) || + if ((rc != -EACCES) || driver->user == getuid()) { - virReportSystemError(rc, _("Failed to create domain save file '%s'"), + virReportSystemError(-rc, _("Failed to create domain save file '%s'"), path); goto endjob; } @@ -5142,7 +5143,7 @@ static int qemudDomainSaveFlag(virDomainPtr dom, const char *path, case 0: default: /* local file - log the error returned by virFileOperation */ - virReportSystemError(rc, + virReportSystemError(-rc, _("Failed to create domain save file '%s'"), path); goto endjob; @@ -5156,8 +5157,8 @@ static int qemudDomainSaveFlag(virDomainPtr dom, const char *path, S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP, driver->user, driver->group, qemudDomainSaveFileOpHook, &hdata, - VIR_FILE_OP_AS_UID)) != 0) { - virReportSystemError(rc, _("Error from child process creating '%s'"), + VIR_FILE_OP_AS_UID)) < 0) { + virReportSystemError(-rc, _("Error from child process creating '%s'"), path); goto endjob; } diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 730ca7b..23adea7 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -276,7 +276,7 @@ static int createRawFileOpHook(int fd, void *data) { /* Seek to the final size, so the capacity is available upfront * for progress reporting */ if (ftruncate(fd, hdata->vol->capacity) < 0) { - ret = errno; + ret = -errno; virReportSystemError(errno, _("cannot extend file '%s'"), hdata->vol->target.path); @@ -286,10 +286,9 @@ static int createRawFileOpHook(int fd, void *data) { remain = hdata->vol->allocation; if (hdata->inputvol) { - int res = virStorageBackendCopyToFD(hdata->vol, hdata->inputvol, - fd, &remain, 1); - if (res < 0) { - ret = -res; + ret = virStorageBackendCopyToFD(hdata->vol, hdata->inputvol, + fd, &remain, 1); + if (ret < 0) { goto cleanup; } } @@ -308,7 +307,7 @@ static int createRawFileOpHook(int fd, void *data) { bytes = remain; if (safezero(fd, 0, hdata->vol->allocation - remain, bytes) != 0) { - ret = errno; + ret = -errno; virReportSystemError(errno, _("cannot fill file '%s'"), hdata->vol->target.path); goto cleanup; @@ -317,7 +316,7 @@ static int createRawFileOpHook(int fd, void *data) { } } else { /* No progress bars to be shown */ if (safezero(fd, 0, 0, remain) != 0) { - ret = errno; + ret = -errno; virReportSystemError(errno, _("cannot fill file '%s'"), hdata->vol->target.path); goto cleanup; @@ -327,7 +326,7 @@ static int createRawFileOpHook(int fd, void *data) { } if (fsync(fd) < 0) { - ret = errno; + ret = -errno; virReportSystemError(errno, _("cannot sync data to file '%s'"), hdata->vol->target.path); goto cleanup; @@ -365,7 +364,7 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, VIR_FILE_OP_FORCE_PERMS | (pool->def->type == VIR_STORAGE_POOL_NETFS ? VIR_FILE_OP_AS_UID : 0))) < 0) { - virReportSystemError(createstat, + virReportSystemError(-createstat, _("cannot create path '%s'"), vol->target.path); goto cleanup; diff --git a/src/util/util.c b/src/util/util.c index a04c515..ee5dd5e 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -1267,6 +1267,7 @@ int virFileExists(const char *path) } # ifndef WIN32 +/* return -errno on failure, or 0 on success */ static int virFileOperationNoFork(const char *path, int openflags, mode_t mode, uid_t uid, gid_t gid, virFileOperationHook hook, void *hookdata, @@ -1276,26 +1277,26 @@ static int virFileOperationNoFork(const char *path, int openflags, mode_t mode, struct stat st; if ((fd = open(path, openflags, mode)) < 0) { - ret = errno; + ret = -errno; virReportSystemError(errno, _("failed to create file '%s'"), path); goto error; } if (fstat(fd, &st) == -1) { - ret = errno; + ret = -errno; virReportSystemError(errno, _("stat of '%s' failed"), path); goto error; } if (((st.st_uid != uid) || (st.st_gid != gid)) && (fchown(fd, uid, gid) < 0)) { - ret = errno; + ret = -errno; virReportSystemError(errno, _("cannot chown '%s' to (%u, %u)"), path, (unsigned int) uid, (unsigned int) gid); goto error; } if ((flags & VIR_FILE_OP_FORCE_PERMS) && (fchmod(fd, mode) < 0)) { - ret = errno; + ret = -errno; virReportSystemError(errno, _("cannot set mode of '%s' to %04o"), path, mode); @@ -1305,7 +1306,7 @@ static int virFileOperationNoFork(const char *path, int openflags, mode_t mode, goto error; } if (close(fd) < 0) { - ret = errno; + ret = -errno; virReportSystemError(errno, _("failed to close new file '%s'"), path); fd = -1; @@ -1356,6 +1357,7 @@ error: return ret; } +/* return -errno on failure, or 0 on success */ int virFileOperation(const char *path, int openflags, mode_t mode, uid_t uid, gid_t gid, virFileOperationHook hook, void *hookdata, @@ -1380,7 +1382,7 @@ int virFileOperation(const char *path, int openflags, mode_t mode, int forkRet = virFork(&pid); if (pid < 0) { - ret = errno; + ret = -errno; return ret; } @@ -1389,14 +1391,14 @@ int virFileOperation(const char *path, int openflags, mode_t mode, while ((waitret = waitpid(pid, &status, 0) == -1) && (errno == EINTR)); if (waitret == -1) { - ret = errno; + ret = -errno; virReportSystemError(errno, _("failed to wait for child creating '%s'"), path); goto parenterror; } - ret = WEXITSTATUS(status); - if (!WIFEXITED(status) || (ret == EACCES)) { + ret = -WEXITSTATUS(status); + if (!WIFEXITED(status) || (ret == -EACCES)) { /* fall back to the simpler method, which works better in * some cases */ return virFileOperationNoFork(path, openflags, mode, uid, gid, @@ -1417,22 +1419,22 @@ parenterror: /* set desired uid/gid, then attempt to create the file */ if ((gid != 0) && (setgid(gid) != 0)) { - ret = errno; + ret = -errno; virReportSystemError(errno, _("cannot set gid %u creating '%s'"), (unsigned int) gid, path); goto childerror; } if ((uid != 0) && (setuid(uid) != 0)) { - ret = errno; + ret = -errno; virReportSystemError(errno, _("cannot set uid %u creating '%s'"), (unsigned int) uid, path); goto childerror; } if ((fd = open(path, openflags, mode)) < 0) { - ret = errno; - if (ret != EACCES) { + ret = -errno; + if (ret != -EACCES) { /* in case of EACCES, the parent will retry */ virReportSystemError(errno, _("child failed to create file '%s'"), @@ -1441,20 +1443,20 @@ parenterror: goto childerror; } if (fstat(fd, &st) == -1) { - ret = errno; + ret = -errno; virReportSystemError(errno, _("stat of '%s' failed"), path); goto childerror; } if ((st.st_gid != gid) && (fchown(fd, -1, gid) < 0)) { - ret = errno; + ret = -errno; virReportSystemError(errno, _("cannot chown '%s' to (%u, %u)"), path, (unsigned int) uid, (unsigned int) gid); goto childerror; } if ((flags & VIR_FILE_OP_FORCE_PERMS) && (fchmod(fd, mode) < 0)) { - ret = errno; + ret = -errno; virReportSystemError(errno, _("cannot set mode of '%s' to %04o"), path, mode); @@ -1464,7 +1466,7 @@ parenterror: goto childerror; } if (close(fd) < 0) { - ret = errno; + ret = -errno; virReportSystemError(errno, _("child failed to close new file '%s'"), path); goto childerror; @@ -1576,6 +1578,7 @@ childerror: # else /* WIN32 */ +/* return -errno on failure, or 0 on success */ int virFileOperation(const char *path ATTRIBUTE_UNUSED, int openflags ATTRIBUTE_UNUSED, mode_t mode ATTRIBUTE_UNUSED, -- 1.7.1.1
On Mon, Jul 19, 2010 at 09:21:52PM -0400, Laine Stump wrote:
virFileOperation previously returned 0 on success, or the value of errno on failure. Although there are other functions in libvirt that use this convention, the preferred (and more common) convention is to return 0 on success and -errno (or simply -1 in some cases) on failure. This way the check for failure is always (ret < 0).
* src/util/util.c - change virFileOperation and virFileOperationNoFork to return -errno on failure.
* src/storage/storage_backend.c, src/qemu/qemu_driver.c - change the hook functions passed to virFileOperation to return -errno on failure.
ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 07/21/2010 11:11 AM, Daniel Veillard wrote:
On Mon, Jul 19, 2010 at 09:21:52PM -0400, Laine Stump wrote:
virFileOperation previously returned 0 on success, or the value of errno on failure. Although there are other functions in libvirt that use this convention, the preferred (and more common) convention is to return 0 on success and -errno (or simply -1 in some cases) on failure. This way the check for failure is always (ret< 0).
* src/util/util.c - change virFileOperation and virFileOperationNoFork to return -errno on failure.
* src/storage/storage_backend.c, src/qemu/qemu_driver.c - change the hook functions passed to virFileOperation to return -errno on failure. ACK,
Pushed. Thanks!
Previously virStorageBackendCopyToFD would simply return -1 on error. This made the error return from one of its callers inconsistent (createRawFileOpHook is supposed to return -errno, but if virStorageBackendCopyToFD failed, createRawFileOpHook would just return -1). Since there is a useful errno in every case of error return from virStorageBackendCopyToFD, and since the other uses of that function ignore the return code (beyond simply checking to see if it is < 0), this is a safe change. --- src/storage/storage_backend.c | 9 +++++++-- 1 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 23adea7..5b61bba 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -116,13 +116,14 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol, { int inputfd = -1; int amtread = -1; - int ret = -1; + int ret = 0; unsigned long long remain; size_t bytes = 1024 * 1024; char zerobuf[512]; char *buf = NULL; if ((inputfd = open(inputvol->target.path, O_RDONLY)) < 0) { + ret = -errno; virReportSystemError(errno, _("could not open input path '%s'"), inputvol->target.path); @@ -132,6 +133,7 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol, bzero(&zerobuf, sizeof(zerobuf)); if (VIR_ALLOC_N(buf, bytes) < 0) { + ret = -errno; virReportOOMError(); goto cleanup; } @@ -145,6 +147,7 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol, bytes = remain; if ((amtread = saferead(inputfd, buf, bytes)) < 0) { + ret = -errno; virReportSystemError(errno, _("failed reading from file '%s'"), inputvol->target.path); @@ -161,12 +164,14 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol, if (is_dest_file && memcmp(buf+offset, zerobuf, interval) == 0) { if (lseek(fd, interval, SEEK_CUR) < 0) { + ret = -errno; virReportSystemError(errno, _("cannot extend file '%s'"), vol->target.path); goto cleanup; } } else if (safewrite(fd, buf+offset, interval) < 0) { + ret = -errno; virReportSystemError(errno, _("failed writing to file '%s'"), vol->target.path); @@ -177,6 +182,7 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol, } if (inputfd != -1 && close(inputfd) < 0) { + ret = -errno; virReportSystemError(errno, _("cannot close file '%s'"), inputvol->target.path); @@ -185,7 +191,6 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol, inputfd = -1; *total -= remain; - ret = 0; cleanup: if (inputfd != -1) -- 1.7.1.1
On Mon, Jul 19, 2010 at 09:21:53PM -0400, Laine Stump wrote:
Previously virStorageBackendCopyToFD would simply return -1 on error. This made the error return from one of its callers inconsistent (createRawFileOpHook is supposed to return -errno, but if virStorageBackendCopyToFD failed, createRawFileOpHook would just return -1). Since there is a useful errno in every case of error return from virStorageBackendCopyToFD, and since the other uses of that function ignore the return code (beyond simply checking to see if it is < 0), this is a safe change. --- src/storage/storage_backend.c | 9 +++++++-- 1 files changed, 7 insertions(+), 2 deletions(-)
ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 07/21/2010 11:12 AM, Daniel Veillard wrote:
On Mon, Jul 19, 2010 at 09:21:53PM -0400, Laine Stump wrote:
Previously virStorageBackendCopyToFD would simply return -1 on error. This made the error return from one of its callers inconsistent (createRawFileOpHook is supposed to return -errno, but if virStorageBackendCopyToFD failed, createRawFileOpHook would just return -1). Since there is a useful errno in every case of error return from virStorageBackendCopyToFD, and since the other uses of that function ignore the return code (beyond simply checking to see if it is< 0), this is a safe change. --- src/storage/storage_backend.c | 9 +++++++-- 1 files changed, 7 insertions(+), 2 deletions(-)
ACK,
Pushed. Thanks!
virDirCreate also previously returned 0 on success and errno on failure. This makes it fit the recommended convention of returning 0 on success, -errno (ie a negative number) on failure. --- src/storage/storage_backend_fs.c | 8 ++++---- src/util/util.c | 32 +++++++++++++++++--------------- 2 files changed, 21 insertions(+), 19 deletions(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index ffb0071..0761bd8 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -567,8 +567,8 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED, VIR_DIR_CREATE_FORCE_PERMS | VIR_DIR_CREATE_ALLOW_EXIST | (pool->def->type == VIR_STORAGE_POOL_NETFS - ? VIR_DIR_CREATE_AS_UID : 0)) != 0)) { - virReportSystemError(err, _("cannot create path '%s'"), + ? VIR_DIR_CREATE_AS_UID : 0)) < 0)) { + virReportSystemError(-err, _("cannot create path '%s'"), pool->def->target.path); goto error; } @@ -793,8 +793,8 @@ static int createFileDir(virConnectPtr conn ATTRIBUTE_UNUSED, uid, gid, VIR_DIR_CREATE_FORCE_PERMS | (pool->def->type == VIR_STORAGE_POOL_NETFS - ? VIR_DIR_CREATE_AS_UID : 0))) != 0) { - virReportSystemError(err, _("cannot create path '%s'"), + ? VIR_DIR_CREATE_AS_UID : 0))) < 0) { + virReportSystemError(-err, _("cannot create path '%s'"), vol->target.path); return -1; } diff --git a/src/util/util.c b/src/util/util.c index ee5dd5e..8f2a17e 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -1319,6 +1319,7 @@ error: return ret; } +/* return -errno on failure, or 0 on success */ static int virDirCreateNoFork(const char *path, mode_t mode, uid_t uid, gid_t gid, unsigned int flags) { int ret = 0; @@ -1327,27 +1328,27 @@ static int virDirCreateNoFork(const char *path, mode_t mode, uid_t uid, gid_t gi if ((mkdir(path, mode) < 0) && !((errno == EEXIST) && (flags & VIR_DIR_CREATE_ALLOW_EXIST))) { - ret = errno; + ret = -errno; virReportSystemError(errno, _("failed to create directory '%s'"), path); goto error; } if (stat(path, &st) == -1) { - ret = errno; + ret = -errno; virReportSystemError(errno, _("stat of '%s' failed"), path); goto error; } if (((st.st_uid != uid) || (st.st_gid != gid)) && (chown(path, uid, gid) < 0)) { - ret = errno; + ret = -errno; virReportSystemError(errno, _("cannot chown '%s' to (%u, %u)"), path, (unsigned int) uid, (unsigned int) gid); goto error; } if ((flags & VIR_DIR_CREATE_FORCE_PERMS) && (chmod(path, mode) < 0)) { - ret = errno; + ret = -errno; virReportSystemError(errno, _("cannot set mode of '%s' to %04o"), path, mode); @@ -1476,6 +1477,7 @@ childerror: } +/* return -errno on failure, or 0 on success */ int virDirCreate(const char *path, mode_t mode, uid_t uid, gid_t gid, unsigned int flags) { struct stat st; @@ -1493,7 +1495,7 @@ int virDirCreate(const char *path, mode_t mode, int forkRet = virFork(&pid); if (pid < 0) { - ret = errno; + ret = -errno; return ret; } @@ -1501,19 +1503,19 @@ int virDirCreate(const char *path, mode_t mode, /* wait for child to complete, and retrieve its exit code */ while ((waitret = waitpid(pid, &status, 0) == -1) && (errno == EINTR)); if (waitret == -1) { - ret = errno; + ret = -errno; virReportSystemError(errno, _("failed to wait for child creating '%s'"), path); goto parenterror; } - ret = WEXITSTATUS(status); - if (!WIFEXITED(status) || (ret == EACCES)) { + ret = -WEXITSTATUS(status); + if (!WIFEXITED(status) || (ret == -EACCES)) { /* fall back to the simpler method, which works better in * some cases */ return virDirCreateNoFork(path, mode, uid, gid, flags); } - if (ret != 0) { + if (ret < 0) { goto parenterror; } parenterror: @@ -1530,20 +1532,20 @@ parenterror: /* set desired uid/gid, then attempt to create the directory */ if ((gid != 0) && (setgid(gid) != 0)) { - ret = errno; + ret = -errno; virReportSystemError(errno, _("cannot set gid %u creating '%s'"), (unsigned int) gid, path); goto childerror; } if ((uid != 0) && (setuid(uid) != 0)) { - ret = errno; + ret = -errno; virReportSystemError(errno, _("cannot set uid %u creating '%s'"), (unsigned int) uid, path); goto childerror; } if (mkdir(path, mode) < 0) { - ret = errno; - if (ret != EACCES) { + ret = -errno; + if (ret != -EACCES) { /* in case of EACCES, the parent will retry */ virReportSystemError(errno, _("child failed to create directory '%s'"), path); @@ -1553,13 +1555,13 @@ parenterror: /* check if group was set properly by creating after * setgid. If not, try doing it with chown */ if (stat(path, &st) == -1) { - ret = errno; + ret = -errno; virReportSystemError(errno, _("stat of '%s' failed"), path); goto childerror; } if ((st.st_gid != gid) && (chown(path, -1, gid) < 0)) { - ret = errno; + ret = -errno; virReportSystemError(errno, _("cannot chown '%s' to group %u"), path, (unsigned int) gid); -- 1.7.1.1
On Mon, Jul 19, 2010 at 09:21:54PM -0400, Laine Stump wrote:
virDirCreate also previously returned 0 on success and errno on failure. This makes it fit the recommended convention of returning 0 on success, -errno (ie a negative number) on failure. --- src/storage/storage_backend_fs.c | 8 ++++---- src/util/util.c | 32 +++++++++++++++++--------------- 2 files changed, 21 insertions(+), 19 deletions(-)
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index ffb0071..0761bd8 100644
ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 07/21/2010 11:17 AM, Daniel Veillard wrote:
On Mon, Jul 19, 2010 at 09:21:54PM -0400, Laine Stump wrote:
virDirCreate also previously returned 0 on success and errno on failure. This makes it fit the recommended convention of returning 0 on success, -errno (ie a negative number) on failure. --- src/storage/storage_backend_fs.c | 8 ++++---- src/util/util.c | 32 +++++++++++++++++--------------- 2 files changed, 21 insertions(+), 19 deletions(-)
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index ffb0071..0761bd8 100644 ACK,
Pushed. Thanks!
One error exit in virStorageBackendCreateBlockFrom was setting the return value to errno. The convention for volume build functions is to return 0 on success or -1 on failure. Not only was it not necessary to set the return value (it defaults to -1, and is set to 0 when everything has been successfully completed), in the case that some caller were checking for < 0 rather than != 0, they would incorrectly believe that it completed successfully. --- src/storage/storage_backend.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 5b61bba..d989743 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -232,7 +232,6 @@ virStorageBackendCreateBlockFrom(virConnectPtr conn ATTRIBUTE_UNUSED, } if (fstat(fd, &st) == -1) { - ret = errno; virReportSystemError(errno, _("stat of '%s' failed"), vol->target.path); goto cleanup; -- 1.7.1.1
On Mon, Jul 19, 2010 at 09:21:55PM -0400, Laine Stump wrote:
One error exit in virStorageBackendCreateBlockFrom was setting the return value to errno. The convention for volume build functions is to return 0 on success or -1 on failure. Not only was it not necessary to set the return value (it defaults to -1, and is set to 0 when everything has been successfully completed), in the case that some caller were checking for < 0 rather than != 0, they would incorrectly believe that it completed successfully. --- src/storage/storage_backend.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 5b61bba..d989743 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -232,7 +232,6 @@ virStorageBackendCreateBlockFrom(virConnectPtr conn ATTRIBUTE_UNUSED, }
if (fstat(fd, &st) == -1) { - ret = errno; virReportSystemError(errno, _("stat of '%s' failed"), vol->target.path); goto cleanup;
ACK, but it seems to me ret being initialized to -1, maybe it's better to be consistent and on all error do a ret = -errno; before virReportSystemError and the goto cleanup. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 07/21/2010 09:21 AM, Daniel Veillard wrote:
ACK, but it seems to me ret being initialized to -1, maybe it's better to be consistent and on all error do a ret = -errno; before virReportSystemError and the goto cleanup.
If you have the convention of returning -errno, then you must not return -1 unless you meant to report EPERM. I agree with Laine's approach of initializing to 0 in this case, since you don't want to leak an unintended -1 if that was not the real error. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 07/21/2010 11:37 AM, Eric Blake wrote:
On 07/21/2010 09:21 AM, Daniel Veillard wrote:
ACK, but it seems to me ret being initialized to -1, maybe it's better to be consistent and on all error do a ret = -errno; before virReportSystemError and the goto cleanup. If you have the convention of returning -errno, then you must not return -1 unless you meant to report EPERM. I agree with Laine's approach of initializing to 0 in this case, since you don't want to leak an unintended -1 if that was not the real error.
In the case of this function, the patch isn't intended to make the function return -errno (like the others in this series), it was just to make it consistent; this was a bug that I coincidentally noticed while changing the other functions from "return positive errno on failure" to "return -errno on failure". I think DV was wondering out loud if we should change the convention of this function (from "return -1 on error") as well. In order to do that, I would have to 1) change all other functions that are called interchangeably with this function (ie, anything assigned to create_func in storage_backend_fs.c, assigned to .buildVolFrom, or a couple other places), and 2) assure that all places any of these are called are okay with a return that is merely < 0, rather than explicitly -1. (and we would also need to make sure that there was a reasonable errno to return in every error case; for example in some functions the error is caused by a configuration problem, not by some system failure). Instead of pushing this change right away, I'll go through all those functions to see if such a change is possible.
On 07/21/2010 02:28 PM, Laine Stump wrote:
On 07/21/2010 11:37 AM, Eric Blake wrote:
On 07/21/2010 09:21 AM, Daniel Veillard wrote:
ACK, but it seems to me ret being initialized to -1, maybe it's better to be consistent and on all error do a ret = -errno; before virReportSystemError and the goto cleanup. If you have the convention of returning -errno, then you must not return -1 unless you meant to report EPERM. I agree with Laine's approach of initializing to 0 in this case, since you don't want to leak an unintended -1 if that was not the real error.
In the case of this function, the patch isn't intended to make the function return -errno (like the others in this series), it was just to make it consistent; this was a bug that I coincidentally noticed while changing the other functions from "return positive errno on failure" to "return -errno on failure". I
think DV was
wondering out loud if we should change the convention of this function (from "return -1 on error") as well.
In order to do that, I would have to 1) change all other functions that are called interchangeably with this function (ie, anything assigned to create_func in storage_backend_fs.c, assigned to .buildVolFrom, or a couple other places), and 2) assure that all places any of these are called are okay with a return that is merely < 0, rather than explicitly -1. (and we would also need to make sure that there was a reasonable errno to return in every error case; for example in some functions the error is caused by a configuration problem, not by some system failure).
Instead of pushing this change right away, I'll go through all those functions to see if such a change is possible.
I looked at this more, and saw that this function is just one of "many" (probably a couple dozen) functions that are used by the storage driver and pointed to by members of virStorageBackend tables. All of these functions consistently return 0 on success and -1 on error. If I were going to change this one function (virStorageBackendCreateBlockFrom), I should then change any function that could be set as a .buildFrom member of virStorageBackend, and if I did that, I should change all functions that could be any of the members of virStorageBackend. The problem is that in some cases, there is no appropriate errno to return, and none of the callers expect that anyway. So in the end, I've decided to leave this function returning -1 on failure, and 0 on success (and am pushing the patch as-is, since it eliminates something that violates that convention).
participants (4)
-
Daniel P. Berrange -
Daniel Veillard -
Eric Blake -
Laine Stump