8:21 p.m.
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