util: fix virFileOpenAs return value and resulting error logs

This resolves:

     https://bugzilla.redhat.com/show_bug.cgi?id=851411
     https://bugzilla.redhat.com/show_bug.cgi?id=955500

The first problem was that virFileOpenAs was returning fd (-1) in one
of the error cases rather than ret (-errno), so the caller thought
that the error was EPERM rather than ENOENT.

The second problem was that some log messages in the general purpose
qemuOpenFile() function would always say "Failed to create" even if
the caller hadn't included O_CREAT (i.e. they were trying to open an
existing file).

This fixes virFileOpenAs to jump down to the error return (which
returns ret instead of fd) in the previously mentioned incorrect
failure case of virFileOpenAs(), removes all error logging from
virFileOpenAs() (since the callers report it), and modifies
qemuOpenFile to appropriately use "open" or "create" in its log
messages.

NB: I seriously considered removing logging from all callers of
virFileOpenAs(), but there is at least one case where the caller
doesn't want virFileOpenAs() to log any errors, because it's just
going to try again (qemuOpenFile()). We can't simply make a silent
variation of virFileOpenAs() though, because qemuOpenFile() can't make
the decision about whether or not it wants to retry until after
virFileOpenAs() has already returned an error code.

Likewise, I also considered changing virFileOpenAs() to return -1 with
errno set on return, and may still do that, but only as a separate
patch, as it obscures the intent of this patch too much.

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 99d2dc4a4d4d4536aecb36f8c8de0719563bb374..b39e0052ef007908d0dab49ddce946104e82bbdb 100644 (file)
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -1,5 +1,5 @@
 /*---------------------------------------------------------------------------*/
-/*  Copyright (C) 2006-2012 Red Hat, Inc.
+/*  Copyright (C) 2006-2013 Red Hat, Inc.
  *  Copyright (C) 2011-2013 SUSE LINUX Products GmbH, Nuernberg, Germany.
  *  Copyright (C) 2011 Univention GmbH.
  *
@@ -550,8 +550,8 @@ libxlSaveImageOpen(libxlDriverPrivatePtr driver, const char *from,
     char *xml = NULL;
 
     if ((fd = virFileOpenAs(from, O_RDONLY, 0, -1, -1, 0)) < 0) {
-        virReportError(VIR_ERR_OPERATION_FAILED,
-                       "%s", _("cannot read domain image"));
+        virReportSystemError(-fd,
+                             _("Failed to open domain image file '%s'"), from);
         goto error;
     }
 

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 6a23573b9c073b1d2b669daa80b932e2261ed95b..3eb4915f83066a2b214a2c96f6021c3979c4da20 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2569,10 +2569,9 @@ qemuOpenFile(virQEMUDriverPtr driver, const char *path, int oflags,
 
     /* First try creating the file as root */
     if (!is_reg) {
-        fd = open(path, oflags & ~O_CREAT);
-        if (fd < 0) {
-            virReportSystemError(errno, _("unable to open %s"), path);
-            goto cleanup;
+        if ((fd = open(path, oflags & ~O_CREAT)) < 0) {
+            fd = -errno;
+            goto error;
         }
     } else {
         if ((fd = virFileOpenAs(path, oflags, S_IRUSR | S_IWUSR, uid, gid,
@@ -2583,36 +2582,30 @@ qemuOpenFile(virQEMUDriverPtr driver, const char *path, int oflags,
                qemu user (cfg->user) is non-root, just set a flag to
                bypass security driver shenanigans, and retry the operation
                after doing setuid to qemu user */
-            if ((fd != -EACCES && fd != -EPERM) ||
-                cfg->user == getuid()) {
-                virReportSystemError(-fd,
-                                     _("Failed to create file '%s'"),
-                                     path);
-                goto cleanup;
-            }
+            if ((fd != -EACCES && fd != -EPERM) || cfg->user == getuid())
+                goto error;
 
             /* On Linux we can also verify the FS-type of the directory. */
             switch (path_shared) {
                 case 1:
-                   /* it was on a network share, so we'll continue
-                    * as outlined above
-                    */
-                   break;
+                    /* it was on a network share, so we'll continue
+                     * as outlined above
+                     */
+                    break;
 
                 case -1:
-                   virReportSystemError(errno,
-                                        _("Failed to create file "
-                                          "'%s': couldn't determine fs type"),
-                                        path);
-                   goto cleanup;
+                    virReportSystemError(-fd, oflags & O_CREAT
+                                         ? _("Failed to create file "
+                                             "'%s': couldn't determine fs type")
+                                         : _("Failed to open file "
+                                             "'%s': couldn't determine fs type"),
+                                         path);
+                    goto cleanup;
 
                 case 0:
                 default:
-                   /* local file - log the error returned by virFileOpenAs */
-                   virReportSystemError(-fd,
-                                        _("Failed to create file '%s'"),
-                                        path);
-                   goto cleanup;
+                    /* local file - log the error returned by virFileOpenAs */
+                    goto error;
             }
 
             /* Retry creating the file as cfg->user */
@@ -2621,8 +2614,9 @@ qemuOpenFile(virQEMUDriverPtr driver, const char *path, int oflags,
                                     S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP,
                                     cfg->user, cfg->group,
                                     vfoflags | VIR_FILE_OPEN_FORK)) < 0) {
-                virReportSystemError(-fd,
-                                   _("Error from child process creating '%s'"),
+                virReportSystemError(-fd, oflags & O_CREAT
+                                     ? _("Error from child process creating '%s'")
+                                     : _("Error from child process opening '%s'"),
                                      path);
                 goto cleanup;
             }
@@ -2641,6 +2635,13 @@ cleanup:
         *bypassSecurityDriver = bypass_security;
     virObjectUnref(cfg);
     return fd;
+
+error:
+    virReportSystemError(-fd, oflags & O_CREAT
+                         ? _("Failed to create file '%s'")
+                         : _("Failed to open file '%s'"),
+                         path);
+    goto cleanup;
 }
 
 /* Helper function to execute a migration to file with a correct save header

diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 7a728e0f3d3cc8a4d2c6b643f5c25881e48e8e38..01a404e81d0655c1a24b7804abec9acd85aee188 100644 (file)
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -405,7 +405,7 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED,
                             vol->target.perms.gid,
                             operation_flags)) < 0) {
         virReportSystemError(-fd,
-                             _("cannot create path '%s'"),
+                             _("Failed to create file '%s'"),
                              vol->target.path);
         goto cleanup;
     }

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index c7bb85a86d4ec15e7850a243ea9b49df63fbdea1..0c433978a7e93ecbc611b1df2bdbf08f11c929df 100644 (file)
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -893,7 +893,7 @@ virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid)
     int fd, ret;
 
     if ((fd = virFileOpenAs(path, O_RDONLY, 0, uid, gid, 0)) < 0) {
-        virReportSystemError(errno, _("cannot open file '%s'"), path);
+        virReportSystemError(-fd, _("Failed to open file '%s'"), path);
         return -1;
     }
 
@@ -952,7 +952,7 @@ virStorageFileGetMetadataRecurse(const char *path, const char *directory,
         return NULL;
 
     if ((fd = virFileOpenAs(path, O_RDONLY, 0, uid, gid, 0)) < 0) {
-        virReportSystemError(-fd, _("cannot open file '%s'"), path);
+        virReportSystemError(-fd, _("Failed to open file '%s'"), path);
         return NULL;
     }
 

diff --git a/src/util/virutil.c b/src/util/virutil.c
index de1937c46074acf62dee1e3722aba77716e178bc..6a4bc14902571bd9ca00c804091bd07ab55d1e0c 100644 (file)
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -973,9 +973,11 @@ virFileOpenForked(const char *path, int openflags, mode_t mode,
  * uid:gid before returning (even if it already existed with a
  * different owner). If @flags includes VIR_FILE_OPEN_FORCE_MODE,
  * ensure it has those permissions before returning (again, even if
- * the file already existed with different permissions).  The return
- * value (if non-negative) is the file descriptor, left open.  Returns
- * -errno on failure.  */
+ * the file already existed with different permissions).
+ *
+ * The return value (if non-negative) is the file descriptor, left
+ * open.  Returns -errno on failure.
+ */
 int
 virFileOpenAs(const char *path, int openflags, mode_t mode,
               uid_t uid, gid_t gid, unsigned int flags)
@@ -999,6 +1001,8 @@ virFileOpenAs(const char *path, int openflags, mode_t mode,
 
         if ((fd = open(path, openflags, mode)) < 0) {
             ret = -errno;
+            if (!(flags & VIR_FILE_OPEN_FORK))
+                goto error;
         } else {
             ret = virFileOpenForceOwnerMode(path, fd, mode, uid, gid, flags);
             if (ret < 0)
@@ -1024,45 +1028,26 @@ virFileOpenAs(const char *path, int openflags, mode_t mode,
 
             /* On Linux we can also verify the FS-type of the
              * directory.  (this is a NOP on other platforms). */
-            switch (virStorageFileIsSharedFS(path)) {
-            case 1:
-                /* it was on a network share, so we'll re-try */
-                break;
-            case -1:
-                /* failure detecting fstype */
-                virReportSystemError(errno, _("couldn't determine fs type "
-                                              "of mount containing '%s'"), path);
-                goto error;
-            case 0:
-            default:
-                /* file isn't on a recognized network FS */
+            if (virStorageFileIsSharedFS(path) <= 0)
                 goto error;
-            }
         }
 
         /* passed all prerequisites - retry the open w/fork+setuid */
         if ((fd = virFileOpenForked(path, openflags, mode, uid, gid, flags)) < 0) {
             ret = fd;
-            fd = -1;
             goto error;
         }
     }
 
     /* File is successfully opened */
-
     return fd;
 
 error:
-    if (fd < 0) {
-        /* whoever failed the open last has already set ret = -errno */
-        virReportSystemError(-ret, openflags & O_CREAT
-                             ? _("failed to create file '%s'")
-                             : _("failed to open file '%s'"),
-                             path);
-    } else {
+    if (fd >= 0) {
         /* some other failure after the open succeeded */
         VIR_FORCE_CLOSE(fd);
     }
+    /* whoever failed the open last has already set ret = -errno */
     return ret;
 }