Change virFileOperation to return -errno (ie < 0) on error.

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.

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 6c7852882fc22aa85554b7c2ab497a19000d3a20..c96788b4a9d19089621ed58844a207c2b0042bd8 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4992,12 +4992,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);
@@ -5005,7 +5006,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;
@@ -5141,7 +5142,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;
         }
@@ -5154,7 +5155,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
@@ -5162,9 +5163,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;
             }
@@ -5188,7 +5189,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;
@@ -5202,8 +5203,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 730ca7b52fca959b9eead94a4650d41e663b21b4..23adea7899578678071c6d0c1c412b2cbf64fe28 100644 (file)
--- 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 a04c515414def4e229e926a4ec8e88ca817304d8..ee5dd5ea72fda964eb021d7da4bdf13670188a15 100644 (file)
--- 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,