Re: [libvirt] [PATCH 2/4] qemu: report error if disk backing files doesn't exist

26 Jul 2013

On 07/25/2013 10:17 PM, Martin Kletzander wrote:
...
On 07/18/2013 01:32 PM, Guannan Ren wrote:
s/doesn't/don't/ in $SUBJ
...
Adding virFileAccessibleAs() to check if the backing file described in
disk meta exist in real path. If not, report error. The uid and gid
arguments don't take effect on F_OK mode for access, so use gid and gid
of current process.
This patch doesn't break anything new, but thanks to the
getuid()/getgid(), it leaves the previous problem in the code.  Even
though F_OK doesn't need uid/gid to check whether the file exists, root
may not have permissions for upper directories on NFS storage for
example, so ...
...
---
  src/util/virstoragefile.c | 23 +++++++++++++++--------
  tests/virstoragetest.c    | 16 ++++++++--------
  2 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index cb6df91..b678eb8 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -572,6 +572,13 @@ virFindBackingFile(const char *start, bool start_is_dir, const char *path,
          goto cleanup;
      }
+    if (virFileAccessibleAs(combined, F_OK, getuid(), getgid()) < 0) {
+        virReportSystemError(errno,
+                             _("Backing file '%s' does not exist"),
+                             combined);
+        goto cleanup;
+    }
+
      if (!(*canonical = canonicalize_file_name(combined))) {
          virReportSystemError(errno,
                               _("Can't canonicalize path '%s'"), path);
... this hunk does make the code report better errors, but in the future
it should canonicalize the filename using root/qemu/domain users.
ACK to this hunk, with the error reworded (e.g. "Cannot access backing
file %s") and, of course, commit message changed appropriately, but ...
...
@@ -857,14 +864,10 @@ virStorageFileGetMetadataInternal(const char *path,
                                         !!directory, backing,
                                         &meta->directory,
                                         &meta->backingStore) < 0) {
-                    /* the backing file is (currently) unavailable, treat this
-                     * file as standalone:
-                     * backingStoreRaw is kept to mark broken image chains */
-                    meta->backingStoreIsFile = false;
-                    backingFormat = VIR_STORAGE_FILE_NONE;
-                    VIR_WARN("Backing file '%s' of image '%s' is missing.",
-                             meta->backingStoreRaw, path);
-
+                    VIR_FREE(backing);
+                    VIR_ERROR(_("Backing file '%s' of image '%s' is missing."),
+                              meta->backingStoreRaw, path);
+                    goto cleanup;
To fix a pre-existing error, we should (instead of this change) just add
virResetLastError() here as the error is used somewhere else in the code
and should be kept in virFindBackingFile().  Having it in the logs seems
OK to me.
It makes sense, but it seems that the tests/virstoragetest.c testcase is 
using the
last error in checking warning flag, see:
     if (data->flags & EXP_WARN) {
         if (!virGetLastError()) {
             fprintf(stderr, "call should have warned\n");
             goto cleanup;
         }
         virResetLastError();

It is not serious issue without calling virResetLastError() here.

    