6:32 a.m.
1, Report an backing missing error on guest boot if its disks have
broken backing file chain
2, List volume even if its diskchain is broken
Guannan Ren(4)
[PATCH 1/4] qemu: refactor qemuDomainCheckDiskPresence for only disk
[PATCH 2/4] qemu: report error if disk backing files doesn't exist
[PATCH 3/4] qemu: check presence of each disk in chain
[PATCH 4/4] storage: list volumes even if its diskchain is broken
src/qemu/qemu_domain.c | 108
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------------
src/qemu/qemu_process.c | 7 -------
src/storage/storage_backend_fs.c | 4 +++-
src/util/virstoragefile.c | 23 +++++++++++++++--------
tests/virstoragetest.c | 16 ++++++++--------
5 files changed, 90 insertions(+), 68 deletions(-)
6:32 a.m.
New subject: [libvirt] [PATCH 1/4] qemu: refactor qemuDomainCheckDiskPresence for only disk presence check
Refactor this function to make it focus on disk presence checking,
including diskchain checking, and not only for CDROM and Floppy.
This change is good for the following patches.
---
src/qemu/qemu_domain.c | 99 ++++++++++++++++++++++++++++++--------------------
1 file changed, 59 insertions(+), 40 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 06efe14..1fa1a5f 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1989,6 +1989,61 @@ cleanup:
virObjectUnref(cfg);
}
+static int
+qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainDiskDefPtr disk,
+ int startupPolicy,
+ bool cold_boot)
+{
+ char uuid[VIR_UUID_STRING_BUFLEN];
+ virDomainEventPtr event = NULL;
+
+ virUUIDFormat(vm->def->uuid, uuid);
+
+ switch ((enum virDomainStartupPolicy) startupPolicy) {
+ case VIR_DOMAIN_STARTUP_POLICY_OPTIONAL:
+ break;
+
+ case VIR_DOMAIN_STARTUP_POLICY_MANDATORY:
+ virReportSystemError(errno,
+ _("cannot access file '%s'"),
+ disk->src);
+ goto error;
+ break;
+
+ case VIR_DOMAIN_STARTUP_POLICY_REQUISITE:
+ if (cold_boot) {
+ virReportSystemError(errno,
+ _("cannot access file '%s'"),
+ disk->src);
+ goto error;
+ }
+ break;
+
+ case VIR_DOMAIN_STARTUP_POLICY_DEFAULT:
+ case VIR_DOMAIN_STARTUP_POLICY_LAST:
+ /* this should never happen */
+ break;
+ }
+
+ VIR_DEBUG("Dropping disk '%s' on domain '%s' (UUID '%s')
"
+ "due to inaccessible source '%s'",
+ disk->dst, vm->def->name, uuid, disk->src);
+
+ event = virDomainEventDiskChangeNewFromObj(vm, disk->src, NULL,
disk->info.alias,
+
VIR_DOMAIN_EVENT_DISK_CHANGE_MISSING_ON_START);
+ if (event)
+ qemuDomainEventQueue(driver, event);
+
+ VIR_FREE(disk->src);
+
+ return 0;
+
+error:
+ return -1;
+}
+
int
qemuDomainCheckDiskPresence(virQEMUDriverPtr driver,
virDomainObjPtr vm,
@@ -1997,12 +2052,8 @@ qemuDomainCheckDiskPresence(virQEMUDriverPtr driver,
int ret = -1;
size_t i;
virDomainDiskDefPtr disk;
- char uuid[VIR_UUID_STRING_BUFLEN];
- virDomainEventPtr event = NULL;
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
- virUUIDFormat(vm->def->uuid, uuid);
-
for (i = 0; i < vm->def->ndisks; i++) {
disk = vm->def->disks[i];
@@ -2016,42 +2067,10 @@ qemuDomainCheckDiskPresence(virQEMUDriverPtr driver,
continue;
}
- switch ((enum virDomainStartupPolicy) disk->startupPolicy) {
- case VIR_DOMAIN_STARTUP_POLICY_OPTIONAL:
- break;
-
- case VIR_DOMAIN_STARTUP_POLICY_MANDATORY:
- virReportSystemError(errno,
- _("cannot access file '%s'"),
- disk->src);
- goto cleanup;
- break;
-
- case VIR_DOMAIN_STARTUP_POLICY_REQUISITE:
- if (cold_boot) {
- virReportSystemError(errno,
- _("cannot access file '%s'"),
- disk->src);
- goto cleanup;
- }
- break;
-
- case VIR_DOMAIN_STARTUP_POLICY_DEFAULT:
- case VIR_DOMAIN_STARTUP_POLICY_LAST:
- /* this should never happen */
- break;
- }
-
- VIR_DEBUG("Dropping disk '%s' on domain '%s' (UUID
'%s') "
- "due to inaccessible source '%s'",
- disk->dst, vm->def->name, uuid, disk->src);
-
- event = virDomainEventDiskChangeNewFromObj(vm, disk->src, NULL,
disk->info.alias,
-
VIR_DOMAIN_EVENT_DISK_CHANGE_MISSING_ON_START);
- if (event)
- qemuDomainEventQueue(driver, event);
-
- VIR_FREE(disk->src);
+ if (qemuDomainCheckDiskStartupPolicy(driver, vm, disk,
+ disk->startupPolicy,
+ cold_boot) < 0)
+ goto cleanup;
}
ret = 0;
--
1.8.3.1
9:17 a.m.
New subject: [libvirt] [PATCH 1/4] qemu: refactor qemuDomainCheckDiskPresence for only disk presence check
On 07/18/2013 01:32 PM, Guannan Ren wrote:
Refactor this function to make it focus on disk presence checking,
including diskchain checking, and not only for CDROM and Floppy.
This change is good for the following patches.
---
src/qemu/qemu_domain.c | 99 ++++++++++++++++++++++++++++++--------------------
1 file changed, 59 insertions(+), 40 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 06efe14..1fa1a5f 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1989,6 +1989,61 @@ cleanup:
virObjectUnref(cfg);
}
+static int
+qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainDiskDefPtr disk,
+ int startupPolicy,
No need for this parameter as you have it in the disk already.
ACK with that fixed, clean code movement.
Martin
6:32 a.m.
New subject: [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
9:17 a.m.
New subject: [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
7:43 a.m.
New subject: [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.
6:32 a.m.
New subject: [libvirt] [PATCH 3/4] qemu: check presence of each disk in chain
For disk with startupPolicy support, such as cdrom and floppy
when its chain is broken, the startup policy will apply,
otherwise, report error on chain issue.
Force to collect diskchain metadata when qemu process start
everytime.
---
src/qemu/qemu_domain.c | 19 ++++++++++---------
src/qemu/qemu_process.c | 7 -------
2 files changed, 10 insertions(+), 16 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 1fa1a5f..c4adbaa 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2057,20 +2057,21 @@ qemuDomainCheckDiskPresence(virQEMUDriverPtr driver,
for (i = 0; i < vm->def->ndisks; i++) {
disk = vm->def->disks[i];
- if (!disk->startupPolicy || !disk->src)
+ if (!disk->src)
continue;
- if (virFileAccessibleAs(disk->src, F_OK,
- cfg->user,
- cfg->group) >= 0) {
- /* disk accessible */
+ if (qemuDomainDetermineDiskChain(driver,
+ disk, true) >= 0)
continue;
+
+ if (disk->startupPolicy) {
+ if (qemuDomainCheckDiskStartupPolicy(driver, vm, disk,
+ disk->startupPolicy,
+ cold_boot) >= 0)
+ continue;
}
- if (qemuDomainCheckDiskStartupPolicy(driver, vm, disk,
- disk->startupPolicy,
- cold_boot) < 0)
- goto cleanup;
+ goto cleanup;
}
ret = 0;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 3d5e8f6..f495267 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3562,17 +3562,10 @@ int qemuProcessStart(virConnectPtr conn,
if (qemuAssignDeviceAliases(vm->def, priv->qemuCaps) < 0)
goto cleanup;
- VIR_DEBUG("Checking for CDROM and floppy presence");
if (qemuDomainCheckDiskPresence(driver, vm,
flags & VIR_QEMU_PROCESS_START_COLD) < 0)
goto cleanup;
- for (i = 0; i < vm->def->ndisks; i++) {
- if (qemuDomainDetermineDiskChain(driver, vm->def->disks[i],
- false) < 0)
- goto cleanup;
- }
-
/* Get the advisory nodeset from numad if 'placement' of
* either <vcpu> or <numatune> is 'auto'.
*/
--
1.8.3.1
9:17 a.m.
New subject: [libvirt] [PATCH 3/4] qemu: check presence of each disk in chain
On 07/18/2013 01:32 PM, Guannan Ren wrote:
For disk with startupPolicy support, such as cdrom and floppy
when its chain is broken, the startup policy will apply,
otherwise, report error on chain issue.
Force to collect diskchain metadata when qemu process start
everytime.
---
src/qemu/qemu_domain.c | 19 ++++++++++---------
src/qemu/qemu_process.c | 7 -------
2 files changed, 10 insertions(+), 16 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 1fa1a5f..c4adbaa 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2057,20 +2057,21 @@ qemuDomainCheckDiskPresence(virQEMUDriverPtr driver,
for (i = 0; i < vm->def->ndisks; i++) {
disk = vm->def->disks[i];
- if (!disk->startupPolicy || !disk->src)
+ if (!disk->src)
continue;
- if (virFileAccessibleAs(disk->src, F_OK,
- cfg->user,
- cfg->group) >= 0) {
- /* disk accessible */
+ if (qemuDomainDetermineDiskChain(driver,
+ disk, true) >= 0)
continue;
As mentioned in [PATCH 2/4], this should be changed to check whether the
backing chain is broken or not.
+
+ if (disk->startupPolicy) {
+ if (qemuDomainCheckDiskStartupPolicy(driver, vm, disk,
+ disk->startupPolicy,
+ cold_boot) >= 0)
+ continue;
}
- if (qemuDomainCheckDiskStartupPolicy(driver, vm, disk,
- disk->startupPolicy,
- cold_boot) < 0)
- goto cleanup;
+ goto cleanup;
}
ret = 0;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 3d5e8f6..f495267 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3562,17 +3562,10 @@ int qemuProcessStart(virConnectPtr conn,
if (qemuAssignDeviceAliases(vm->def, priv->qemuCaps) < 0)
goto cleanup;
- VIR_DEBUG("Checking for CDROM and floppy presence");
Changed debug message could be used in qemuDomainCheckDiskPresence(),
even for every disk, so it is easier to find why we were browsing
through files on the host.
Other than that the patch looks fine, we'll see how it looks after 2/4
is fixed.
Martin
6:32 a.m.
New subject: [libvirt] [PATCH 4/4] storage: list volumes even if its diskchain is broken
---
src/storage/storage_backend_fs.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index d305b06..c6c019d 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -94,7 +94,9 @@ virStorageBackendProbeTarget(virStorageVolTargetPtr target,
if (!(meta = virStorageFileGetMetadataFromFD(target->path, fd,
target->format))) {
- ret = -1;
+ /* list volume even if its diskchain is broken */
+ virResetLastError();
+ ret = -3;
goto error;
}
--
1.8.3.1
4173
days inactive
4181
days old
8 comments
2 participants
participants (2)
-
Guannan Ren -
Martin Kletzander