Re: [libvirt] [PATCH 2/4] qemu: report error if disk backing files doesn't exist
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. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/4] qemu: report error if disk backing files doesn't exist
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. } } VIR_FREE(backing); ... NACK to the rest. As written in the comment you are removing, backingStoreRaw is kept to mark broken chains. And that is the thing we should use to check whether the backing chain is broken, not throw away all the data we've got. @@ -1047,6 +1050,10 @@ virStorageFileGetMetadataRecurse(const char *path, const char *directory, uid, gid, allow_probe, cycle); +if (!ret-backingMeta) { +virStorageFileFreeMetadata(ret); +ret = NULL; +} } return ret; That means you can drop this hunk too, the tests as well, and the detection of broken chains could be added in a separate function (just a cycle checking for that) and called when we want to know whether the chain is broken. Martin -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/4] qemu: report error if disk backing files doesn't exist
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. --- 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); @@ -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; } } VIR_FREE(backing); @@ -1047,6 +1050,10 @@ virStorageFileGetMetadataRecurse(const char *path, const char *directory, uid, gid, allow_probe, cycle); +if (!ret-backingMeta) { +virStorageFileFreeMetadata(ret); +ret = NULL; +} } return ret; diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index a3c59ef..030f81b 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -480,10 +480,10 @@ mymain(void) /* Qcow2 file with missing backing file but specified type */ const testFileData chain9[] = { qcow2_bogus }; TEST_CHAIN(9, qcow2, absqcow2, VIR_STORAGE_FILE_QCOW2, - chain9, EXP_WARN, - chain9, ALLOW_PROBE | EXP_WARN, - chain9, EXP_WARN, - chain9, ALLOW_PROBE | EXP_WARN); + chain9, EXP_FAIL, + chain9, ALLOW_PROBE | EXP_FAIL, + chain9, EXP_FAIL, + chain9, ALLOW_PROBE | EXP_FAIL); /* Rewrite qcow2 to a missing backing file, without backing type */ virCommandFree(cmd); @@ -495,10 +495,10 @@ mymain(void) /* Qcow2 file with missing backing file and no specified type */ const testFileData chain10[] = { qcow2_bogus }; TEST_CHAIN(10, qcow2, absqcow2, VIR_STORAGE_FILE_QCOW2, - chain10, EXP_WARN, - chain10, ALLOW_PROBE | EXP_WARN, - chain10, EXP_WARN, - chain10, ALLOW_PROBE | EXP_WARN); + chain10, EXP_FAIL, + chain10, ALLOW_PROBE | EXP_FAIL, + chain10, EXP_FAIL, + chain10, ALLOW_PROBE | EXP_FAIL); /* Rewrite qcow2 to use an nbd: protocol as backend */ virCommandFree(cmd); -- 1.8.3.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list