4:35 a.m.
v3: https://www.redhat.com/archives/libvir-list/2013-June/thread.html
v3 to v4: Rebase
v2 to v3: Not only check disk source, startupPolicy should work if any
backing file is missing. The commit 039a3283 break the limition
of contiguous device boot orders, so I remove my previous patch
about it.
v1 to v2: Added relax schema for disk of block and dir type
Removed original patch 3/5.
The set of patches is trying to add 'startupPolicy' attribute support
to the source element of hard disks. Policy levels are using the
mandatory, requisite, optional levels as originally documented.
For the 'optional' policy, there is a little difference from CDROM and
Floppy which only drop its source path, for disks, if missing, the
checking function will drop their definitions, because qemu doesn't
allow missing source path for hard disk.
Guannan Ren
[PATCH v3 1/4] conf: add startupPolicy attribute for harddisk
[PATCH v3 2/4] storage: report error rather than warning if backing files doesn't
exist
[PATCH v3 3/4] qemu: drop disk with 'optional' startupPolicy if it is not
present
[PATCH v3 4/4] security: restore DAC for every disk file in disk chain
docs/schemas/domaincommon.rng | 6 ++++++
include/libvirt/libvirt.h.in | 1 +
src/conf/domain_conf.c | 20 +++++++++++++-------
src/qemu/qemu_domain.c | 119
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------------------------
src/qemu/qemu_process.c | 7 -------
src/security/security_dac.c | 15 ++++++++++++++-
src/util/virstoragefile.c | 23 +++++++++++++++--------
tests/virstoragetest.c | 16 ++++++++--------
8 files changed, 130 insertions(+), 77 deletions(-)
4:35 a.m.
New subject: [libvirt] [PATCH v4 1/4] conf: add startupPolicy attribute for harddisk
Add startupPolicy attribute policy for harddisk with type "file",
"block" and "dir". The "network" type disk is still not
supported.
---
docs/schemas/domaincommon.rng | 6 ++++++
src/conf/domain_conf.c | 20 +++++++++++++-------
2 files changed, 19 insertions(+), 7 deletions(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index cf82878..483f068 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -1093,6 +1093,9 @@
<ref name="absFilePath"/>
</attribute>
<optional>
+ <ref name="startupPolicy"/>
+ </optional>
+ <optional>
<ref name='devSeclabel'/>
</optional>
</element>
@@ -1110,6 +1113,9 @@
<attribute name="dir">
<ref name="absFilePath"/>
</attribute>
+ <optional>
+ <ref name="startupPolicy"/>
+ </optional>
<empty/>
</element>
</optional>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 011de71..1040b40 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4739,7 +4739,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
switch (def->type) {
case VIR_DOMAIN_DISK_TYPE_FILE:
source = virXMLPropString(cur, "file");
- startupPolicy = virXMLPropString(cur, "startupPolicy");
break;
case VIR_DOMAIN_DISK_TYPE_BLOCK:
source = virXMLPropString(cur, "dev");
@@ -4828,7 +4827,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
case VIR_DOMAIN_DISK_TYPE_VOLUME:
if (virDomainDiskSourcePoolDefParse(cur, def) < 0)
goto error;
- startupPolicy = virXMLPropString(cur, "startupPolicy");
break;
default:
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -4837,6 +4835,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
goto error;
}
+ startupPolicy = virXMLPropString(cur, "startupPolicy");
+
/* People sometimes pass a bogus '' source path
when they mean to omit the source element
completely (e.g. CDROM without media). This is
@@ -5410,11 +5410,10 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
goto error;
}
- if (def->device != VIR_DOMAIN_DISK_DEVICE_CDROM &&
- def->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
+ if (def->type == VIR_DOMAIN_DISK_TYPE_NETWORK) {
virReportError(VIR_ERR_INVALID_ARG,
- _("Setting disk %s is allowed only for "
- "cdrom or floppy"),
+ _("Setting disk %s is not allowed for "
+ "disk of network type"),
startupPolicy);
goto error;
}
@@ -13872,6 +13871,9 @@ virDomainDiskSourceDefFormat(virBufferPtr buf,
case VIR_DOMAIN_DISK_TYPE_BLOCK:
virBufferEscapeString(buf, " <source dev='%s'",
def->src);
+ if (def->startupPolicy)
+ virBufferEscapeString(buf, " startupPolicy='%s'",
+ startupPolicy);
if (def->nseclabels) {
virBufferAddLit(buf, ">\n");
virBufferAdjustIndent(buf, 8);
@@ -13884,8 +13886,12 @@ virDomainDiskSourceDefFormat(virBufferPtr buf,
}
break;
case VIR_DOMAIN_DISK_TYPE_DIR:
- virBufferEscapeString(buf, " <source
dir='%s'/>\n",
+ virBufferEscapeString(buf, " <source dir='%s'",
def->src);
+ if (def->startupPolicy)
+ virBufferEscapeString(buf, " startupPolicy='%s'",
+ startupPolicy);
+ virBufferAddLit(buf, "/>\n");
break;
case VIR_DOMAIN_DISK_TYPE_NETWORK:
virBufferAsprintf(buf, " <source protocol='%s'",
--
1.8.1.4
8:31 a.m.
New subject: [libvirt] [PATCH v4 1/4] conf: add startupPolicy attribute for harddisk
On 07/02/2013 11:35 AM, Guannan Ren wrote:
Add startupPolicy attribute policy for harddisk with type
"file",
"block" and "dir". The "network" type disk is still not
supported.
---
docs/schemas/domaincommon.rng | 6 ++++++
src/conf/domain_conf.c | 20 +++++++++++++-------
2 files changed, 19 insertions(+), 7 deletions(-)
[...]
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 011de71..1040b40 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
[...]
@@ -5410,11 +5410,10 @@
virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
goto error;
}
- if (def->device != VIR_DOMAIN_DISK_DEVICE_CDROM &&
- def->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
+ if (def->type == VIR_DOMAIN_DISK_TYPE_NETWORK) {
virReportError(VIR_ERR_INVALID_ARG,
Pre-existing, but shouldn't this be VIR_ERR_XML_ERROR ?
Otherwise the patch looks ok, but is missing documentation. And in this
case it is important to have it there, I think.
The possibility of specifying startupPolicy should be also documented
and it should be mentioned there that the whole disk will be dropped in
case this attribute is specified and the disk (or any other in it's
backing chain) is not found. I'm trying to stress this out because for
cdrom and floppy disks we can safely drop the source only, it can be
plugged in even with old QEMU, OSes are used to that. But with disks it
changes what is seen from the guest. I believe that's also why
startupPolicy can be specified only for cdrom, floppy and USB hostdevs
for now. This particular functionality calls for potentional problems
IMHO in case it is not properly documented.
Also, 'requisite' for disks would mean that you have to hot-unplug the
disk if it needs to be removed due to migration because otherwise qemu
wouldn't start on the destination, or would it?
The more I'm staring into the code, the more I feel like this is not a
good idea and should be managed from upper application if this behavior
is required for disks.
Martin
10:01 a.m.
New subject: [libvirt] [PATCH v4 1/4] conf: add startupPolicy attribute for harddisk
On 07/15/2013 09:31 PM, Martin Kletzander wrote:
On 07/02/2013 11:35 AM, Guannan Ren wrote:
> Add startupPolicy attribute policy for harddisk with type "file",
> "block" and "dir". The "network" type disk is still not
supported.
> ---
> docs/schemas/domaincommon.rng | 6 ++++++
> src/conf/domain_conf.c | 20 +++++++++++++-------
> 2 files changed, 19 insertions(+), 7 deletions(-)
>
[...]
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 011de71..1040b40 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
[...]
> @@ -5410,11 +5410,10 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
> goto error;
> }
>
> - if (def->device != VIR_DOMAIN_DISK_DEVICE_CDROM &&
> - def->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
> + if (def->type == VIR_DOMAIN_DISK_TYPE_NETWORK) {
> virReportError(VIR_ERR_INVALID_ARG,
Pre-existing, but shouldn't this be VIR_ERR_XML_ERROR ?
Otherwise the patch looks ok, but is missing documentation. And in this
case it is important to have it there, I think.
The possibility of specifying startupPolicy should be also documented
and it should be mentioned there that the whole disk will be dropped in
case this attribute is specified and the disk (or any other in it's
backing chain) is not found. I'm trying to stress this out because for
cdrom and floppy disks we can safely drop the source only, it can be
plugged in even with old QEMU, OSes are used to that. But with disks it
changes what is seen from the guest. I believe that's also why
startupPolicy can be specified only for cdrom, floppy and USB hostdevs
for now. This particular functionality calls for potentional problems
IMHO in case it is not properly documented.
Also, 'requisite' for disks would mean that you have to hot-unplug the
disk if it needs to be removed due to migration because otherwise qemu
wouldn't start on the destination, or would it?
Yeah, the hot-unplugging disk is necessary in the case of 'requisite' if
diskchains checking failed during migration.
The more I'm staring into the code, the more I feel like this is not a
good idea and should be managed from upper application if this behavior
is required for disks.
I am not sure it is upper application's work. libvirt collected
diskchain data for each disk at guest bootup already
so it is easier to remove or hot-unplug a disk if backingchain is broken
from libvirt point of view.
upper application know nothing about backingchain for guest disks.
11:03 p.m.
New subject: [libvirt] [PATCH v4 1/4] conf: add startupPolicy attribute for harddisk
On 07/15/2013 05:01 PM, Guannan Ren wrote:
On 07/15/2013 09:31 PM, Martin Kletzander wrote:
> On 07/02/2013 11:35 AM, Guannan Ren wrote:
>> Add startupPolicy attribute policy for harddisk with type "file",
>> "block" and "dir". The "network" type disk is still
not supported.
>> ---
>> docs/schemas/domaincommon.rng | 6 ++++++
>> src/conf/domain_conf.c | 20 +++++++++++++-------
>> 2 files changed, 19 insertions(+), 7 deletions(-)
>>
> [...]
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 011de71..1040b40 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
> [...]
>> @@ -5410,11 +5410,10 @@
>> virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
>> goto error;
>> }
>> - if (def->device != VIR_DOMAIN_DISK_DEVICE_CDROM &&
>> - def->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
>> + if (def->type == VIR_DOMAIN_DISK_TYPE_NETWORK) {
>> virReportError(VIR_ERR_INVALID_ARG,
> Pre-existing, but shouldn't this be VIR_ERR_XML_ERROR ?
>
> Otherwise the patch looks ok, but is missing documentation. And in this
> case it is important to have it there, I think.
>
> The possibility of specifying startupPolicy should be also documented
> and it should be mentioned there that the whole disk will be dropped in
> case this attribute is specified and the disk (or any other in it's
> backing chain) is not found. I'm trying to stress this out because for
> cdrom and floppy disks we can safely drop the source only, it can be
> plugged in even with old QEMU, OSes are used to that. But with disks it
> changes what is seen from the guest. I believe that's also why
> startupPolicy can be specified only for cdrom, floppy and USB hostdevs
> for now. This particular functionality calls for potentional problems
> IMHO in case it is not properly documented.
>
> Also, 'requisite' for disks would mean that you have to hot-unplug the
> disk if it needs to be removed due to migration because otherwise qemu
> wouldn't start on the destination, or would it?
Yeah, the hot-unplugging disk is necessary in the case of 'requisite' if
diskchains checking failed during migration.
>
> The more I'm staring into the code, the more I feel like this is not a
> good idea and should be managed from upper application if this behavior
> is required for disks.
I am not sure it is upper application's work. libvirt collected
diskchain data for each disk at guest bootup already
so it is easier to remove or hot-unplug a disk if backingchain is broken
from libvirt point of view.
upper application know nothing about backingchain for guest disks.
I haven't explained myself correctly here. In the last paragraph I was
talking about the whole 'startupPolicy for harddisk' approach, not only
about removing the disk on migration.
Martin
4:35 a.m.
New subject: [libvirt] [PATCH v4 2/4] storage: report error rather than warning if backing files doesn't exist
If one of backing files for disk source doesn't exist, the guest will not
be able to find and use the disk even though the disk still exists in
guest xml definition. So reporting an error make more sense.
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 have so much meannings for F_OK, so give 0 for them.
---
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 27aa4fe..cb61e5b 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -580,6 +580,13 @@ virFindBackingFile(const char *start, bool start_is_dir, const char
*path,
goto cleanup;
}
+ if (virFileAccessibleAs(combined, F_OK, 0, 0) < 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);
@@ -870,14 +877,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_ERROR(_("Backing file '%s' of image '%s' is
missing."),
+ meta->backingStoreRaw, path);
+ VIR_FREE(backing);
+ goto cleanup;
}
}
VIR_FREE(backing);
@@ -1062,6 +1065,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 fef4b37..fb5da5c 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -483,10 +483,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);
@@ -498,10 +498,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.1.4
8:31 a.m.
New subject: [libvirt] [PATCH v4 2/4] storage: report error rather than warning if backing files doesn't exist
s/doesn't/don't/ in $SUBJ
On 07/02/2013 11:35 AM, Guannan Ren wrote:
If one of backing files for disk source doesn't exist, the guest
will not
be able to find and use the disk even though the disk still exists in
guest xml definition. So reporting an error make more sense.
Adding virFileAccessibleAs() to check if the backing file described in
disk meta exist in real path. If not, report error. the uid and gid
s/exist/exists/; s/the/The/
arguments don't have so much meannings for F_OK, so give 0 for
them.
s/meannings/meaning/
---
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 27aa4fe..cb61e5b 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -580,6 +580,13 @@ virFindBackingFile(const char *start, bool start_is_dir, const char
*path,
goto cleanup;
}
+ if (virFileAccessibleAs(combined, F_OK, 0, 0) < 0) {
+ virReportSystemError(errno,
+ _("Backing file '%s' does not exist"),
+ combined);
+ goto cleanup;
+ }
+
Pre-existing, but this nad other errors will be shadowed by the error in
qemuDomainBlockCommit. The existence of the file is already checked by:
if (!(*canonical = canonicalize_file_name(combined))) {
this ^^ line, so no need to call virFileAccessibleAs().
virReportSystemError(errno,
_("Can't canonicalize path '%s'"),
path);
@@ -870,14 +877,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_ERROR(_("Backing file '%s' of image '%s' is
missing."),
+ meta->backingStoreRaw, path);
+ VIR_FREE(backing);
+ goto cleanup;
And then you report one more error here, even though the function may
fail because of something else. I'd guess that's why it was a warning.
No idea why it skipped the errors, though.
The errors should be fixed, but this patch just adds more confusion to
the error reporting.
Martin
10:10 a.m.
New subject: [libvirt] [PATCH v4 2/4] storage: report error rather than warning if backing files doesn't exist
On 07/15/2013 09:31 PM, Martin Kletzander wrote:
s/doesn't/don't/ in $SUBJ
On 07/02/2013 11:35 AM, Guannan Ren wrote:
> If one of backing files for disk source doesn't exist, the guest will not
> be able to find and use the disk even though the disk still exists in
> guest xml definition. So reporting an error make more sense.
>
> Adding virFileAccessibleAs() to check if the backing file described in
> disk meta exist in real path. If not, report error. the uid and gid
s/exist/exists/; s/the/The/
> arguments don't have so much meannings for F_OK, so give 0 for them.
s/meannings/meaning/
> ---
> 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 27aa4fe..cb61e5b 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -580,6 +580,13 @@ virFindBackingFile(const char *start, bool start_is_dir, const
char *path,
> goto cleanup;
> }
>
> + if (virFileAccessibleAs(combined, F_OK, 0, 0) < 0) {
> + virReportSystemError(errno,
> + _("Backing file '%s' does not
exist"),
> + combined);
> + goto cleanup;
> + }
> +
Pre-existing, but this nad other errors will be shadowed by the error in
qemuDomainBlockCommit. The existence of the file is already checked by:
> if (!(*canonical = canonicalize_file_name(combined))) {
this ^^ line, so no need to call virFileAccessibleAs().
Calling virFileAccessibleAs() here is for more clear error message.
> virReportSystemError(errno,
> _("Can't canonicalize path
'%s'"), path);
> @@ -870,14 +877,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_ERROR(_("Backing file '%s' of image
'%s' is missing."),
> + meta->backingStoreRaw, path);
> + VIR_FREE(backing);
> + goto cleanup;
And then you report one more error here, even though the function may
fail because of something else. I'd guess that's why it was a warning.
No idea why it skipped the errors, though.
The errors should be fixed, but this patch just adds more confusion to
the error reporting.
no, VIR_ERROR just adds one more error log item, the actual raised error
is still there not being shadowed.
Guannan
10:31 a.m.
New subject: [libvirt] [PATCH v4 2/4] storage: report error rather than warning if backing files doesn't exist
On 07/02/2013 11:35 AM, Guannan Ren wrote:
If one of backing files for disk source doesn't exist, the guest
will not
be able to find and use the disk even though the disk still exists in
guest xml definition. So reporting an error make more sense.
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 have so much meannings for F_OK, so give 0 for them.
---
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 27aa4fe..cb61e5b 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -870,14 +877,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_ERROR(_("Backing file '%s' of image '%s' is
missing."),
+ meta->backingStoreRaw, path);
+ VIR_FREE(backing);
+ goto cleanup;
}
}
VIR_FREE(backing);
This change means you won't be able to start the pool if one of the files is
missing a backing file. I've forwarded a patch [1] from/for [2] that ignores
missing files on pool start and there is a bug [3] requesting that we ignore
other files as well. I feel like this is going in the other direction.
Wouldn't it be enough to check for them on domain start-up and leave the pool
running even if one of those volumes doesn't have an existing backing file?
Jan
[1] https://www.redhat.com/archives/libvir-list/2013-July/msg00635.html
[2] https://bugzilla.redhat.com/show_bug.cgi?id=977706
[3] https://bugzilla.redhat.com/show_bug.cgi?id=710866
1:14 a.m.
New subject: [libvirt] [PATCH v4 2/4] storage: report error rather than warning if backing files doesn't exist
On 07/15/2013 05:31 PM, Ján Tomko wrote:
On 07/02/2013 11:35 AM, Guannan Ren wrote:
> If one of backing files for disk source doesn't exist, the guest will not
> be able to find and use the disk even though the disk still exists in
> guest xml definition. So reporting an error make more sense.
>
> 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 have so much meannings for F_OK, so give 0 for them.
> ---
> 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 27aa4fe..cb61e5b 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -870,14 +877,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_ERROR(_("Backing file '%s' of image
'%s' is missing."),
> + meta->backingStoreRaw, path);
> + VIR_FREE(backing);
> + goto cleanup;
> }
> }
> VIR_FREE(backing);
This change means you won't be able to start the pool if one of the files is
missing a backing file. I've forwarded a patch [1] from/for [2] that ignores
missing files on pool start and there is a bug [3] requesting that we ignore
other files as well. I feel like this is going in the other direction.
Wouldn't it be enough to check for them on domain start-up and leave the pool
running even if one of those volumes doesn't have an existing backing file?
How about making it configurable for the pool? There are definitely
some users who want the pool to reflect actual info after pool-refresh.
3:40 a.m.
New subject: [libvirt] [PATCH v4 2/4] storage: report error rather than warning if backing files doesn't exist
On Tue, Jul 16, 2013 at 08:14:54AM +0200, Martin Kletzander wrote:
On 07/15/2013 05:31 PM, Ján Tomko wrote:
> On 07/02/2013 11:35 AM, Guannan Ren wrote:
>> If one of backing files for disk source doesn't exist, the guest will not
>> be able to find and use the disk even though the disk still exists in
>> guest xml definition. So reporting an error make more sense.
>>
>> 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 have so much meannings for F_OK, so give 0 for them.
>> ---
>> 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 27aa4fe..cb61e5b 100644
>> --- a/src/util/virstoragefile.c
>> +++ b/src/util/virstoragefile.c
>
>> @@ -870,14 +877,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_ERROR(_("Backing file '%s' of image
'%s' is missing."),
>> + meta->backingStoreRaw, path);
>> + VIR_FREE(backing);
>> + goto cleanup;
>> }
>> }
>> VIR_FREE(backing);
>
> This change means you won't be able to start the pool if one of the files is
> missing a backing file. I've forwarded a patch [1] from/for [2] that ignores
> missing files on pool start and there is a bug [3] requesting that we ignore
> other files as well. I feel like this is going in the other direction.
>
> Wouldn't it be enough to check for them on domain start-up and leave the pool
> running even if one of those volumes doesn't have an existing backing file?
>
How about making it configurable for the pool? There are definitely
some users who want the pool to reflect actual info after pool-refresh.
I don't think this needs to be configurable. The pool should show *every*
single file, regardless of whether the file has a broken backing file.
We shouldn't be trying to second guess what the user wants to do with a
image with broken backing file. Just expose as much info as we have and
let them deal with the problem.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
3:47 a.m.
New subject: [libvirt] [PATCH v4 2/4] storage: report error rather than warning if backing files doesn't exist
On 07/16/2013 10:40 AM, Daniel P. Berrange wrote:
On Tue, Jul 16, 2013 at 08:14:54AM +0200, Martin Kletzander wrote:
> On 07/15/2013 05:31 PM, Ján Tomko wrote:
>> On 07/02/2013 11:35 AM, Guannan Ren wrote:
>>> If one of backing files for disk source doesn't exist, the guest will
not
>>> be able to find and use the disk even though the disk still exists in
>>> guest xml definition. So reporting an error make more sense.
>>>
>>> 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 have so much meannings for F_OK, so give 0 for them.
>>> ---
>>> 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 27aa4fe..cb61e5b 100644
>>> --- a/src/util/virstoragefile.c
>>> +++ b/src/util/virstoragefile.c
>>
>>> @@ -870,14 +877,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_ERROR(_("Backing file '%s' of image
'%s' is missing."),
>>> + meta->backingStoreRaw, path);
>>> + VIR_FREE(backing);
>>> + goto cleanup;
>>> }
>>> }
>>> VIR_FREE(backing);
>>
>> This change means you won't be able to start the pool if one of the files is
>> missing a backing file. I've forwarded a patch [1] from/for [2] that ignores
>> missing files on pool start and there is a bug [3] requesting that we ignore
>> other files as well. I feel like this is going in the other direction.
>>
>> Wouldn't it be enough to check for them on domain start-up and leave the
pool
>> running even if one of those volumes doesn't have an existing backing file?
>>
>
> How about making it configurable for the pool? There are definitely
> some users who want the pool to reflect actual info after pool-refresh.
I don't think this needs to be configurable. The pool should show *every*
single file, regardless of whether the file has a broken backing file.
We shouldn't be trying to second guess what the user wants to do with a
image with broken backing file. Just expose as much info as we have and
let them deal with the problem.
I was thinking about the case that was mentioned in Jan's link to
bugzilla, where they wanted to keep even deleted volumes. Since I
disagree with that for normal pool, we could make it configurable for
such use-cases (although it looks more like invalid usage of our pools
in that BZ).
Martin
4:08 a.m.
New subject: [libvirt] [PATCH v4 2/4] storage: report error rather than warning if backing files doesn't exist
On Tue, Jul 16, 2013 at 10:47:54AM +0200, Martin Kletzander wrote:
On 07/16/2013 10:40 AM, Daniel P. Berrange wrote:
> On Tue, Jul 16, 2013 at 08:14:54AM +0200, Martin Kletzander wrote:
>> On 07/15/2013 05:31 PM, Ján Tomko wrote:
>>> On 07/02/2013 11:35 AM, Guannan Ren wrote:
>>>> If one of backing files for disk source doesn't exist, the guest
will not
>>>> be able to find and use the disk even though the disk still exists in
>>>> guest xml definition. So reporting an error make more sense.
>>>>
>>>> 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 have so much meannings for F_OK, so give 0 for
them.
>>>> ---
>>>> 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 27aa4fe..cb61e5b 100644
>>>> --- a/src/util/virstoragefile.c
>>>> +++ b/src/util/virstoragefile.c
>>>
>>>> @@ -870,14 +877,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_ERROR(_("Backing file '%s' of
image '%s' is missing."),
>>>> + meta->backingStoreRaw, path);
>>>> + VIR_FREE(backing);
>>>> + goto cleanup;
>>>> }
>>>> }
>>>> VIR_FREE(backing);
>>>
>>> This change means you won't be able to start the pool if one of the
files is
>>> missing a backing file. I've forwarded a patch [1] from/for [2] that
ignores
>>> missing files on pool start and there is a bug [3] requesting that we
ignore
>>> other files as well. I feel like this is going in the other direction.
>>>
>>> Wouldn't it be enough to check for them on domain start-up and leave the
pool
>>> running even if one of those volumes doesn't have an existing backing
file?
>>>
>>
>> How about making it configurable for the pool? There are definitely
>> some users who want the pool to reflect actual info after pool-refresh.
>
> I don't think this needs to be configurable. The pool should show *every*
> single file, regardless of whether the file has a broken backing file.
> We shouldn't be trying to second guess what the user wants to do with a
> image with broken backing file. Just expose as much info as we have and
> let them deal with the problem.
>
I was thinking about the case that was mentioned in Jan's link to
bugzilla, where they wanted to keep even deleted volumes. Since I
disagree with that for normal pool, we could make it configurable for
such use-cases (although it looks more like invalid usage of our pools
in that BZ).
Which BZ are you referring to ? I read BZ 977706 to be about not causing
errors during pool-refresh if a 2nd node has deleted the volume while
the first node is in the middle of refresh. This isn't about keeping
deleted volumes visible.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
6:57 a.m.
New subject: [libvirt] [PATCH v4 2/4] storage: report error rather than warning if backing files doesn't exist
On 07/16/2013 11:08 AM, Daniel P. Berrange wrote:
[...]
Which BZ are you referring to ? I read BZ 977706 to be about not
causing
errors during pool-refresh if a 2nd node has deleted the volume while
the first node is in the middle of refresh. This isn't about keeping
deleted volumes visible.
Misread the BZ, the problem is the pool gets delete, not the volume, sorry.
Martin
9:08 p.m.
New subject: [libvirt] [PATCH v4 2/4] storage: report error rather than warning if backing files doesn't exist
On 07/16/2013 04:40 PM, Daniel P. Berrange wrote:
On Tue, Jul 16, 2013 at 08:14:54AM +0200, Martin Kletzander wrote:
> On 07/15/2013 05:31 PM, Ján Tomko wrote:
>> On 07/02/2013 11:35 AM, Guannan Ren wrote:
>>> If one of backing files for disk source doesn't exist, the guest will
not
>>> be able to find and use the disk even though the disk still exists in
>>> guest xml definition. So reporting an error make more sense.
>>>
>>> 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 have so much meannings for F_OK, so give 0 for them.
>>> ---
>>> 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 27aa4fe..cb61e5b 100644
>>> --- a/src/util/virstoragefile.c
>>> +++ b/src/util/virstoragefile.c
>>> @@ -870,14 +877,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_ERROR(_("Backing file '%s' of image
'%s' is missing."),
>>> + meta->backingStoreRaw, path);
>>> + VIR_FREE(backing);
>>> + goto cleanup;
>>> }
>>> }
>>> VIR_FREE(backing);
>> This change means you won't be able to start the pool if one of the files is
>> missing a backing file. I've forwarded a patch [1] from/for [2] that ignores
>> missing files on pool start and there is a bug [3] requesting that we ignore
>> other files as well. I feel like this is going in the other direction.
>>
>> Wouldn't it be enough to check for them on domain start-up and leave the
pool
>> running even if one of those volumes doesn't have an existing backing file?
>>
> How about making it configurable for the pool? There are definitely
> some users who want the pool to reflect actual info after pool-refresh.
I don't think this needs to be configurable. The pool should show *every*
single file, regardless of whether the file has a broken backing file.
We shouldn't be trying to second guess what the user wants to do with a
image with broken backing file. Just expose as much info as we have and
let them deal with the problem.
Daniel,
How about for guest cold bootup in this situation where one of its disks
has a broken file
we should throw an error out or give a warn in the log as what it is
doing currently.
4:41 a.m.
New subject: [libvirt] [PATCH v4 2/4] storage: report error rather than warning if backing files doesn't exist
On Wed, Jul 17, 2013 at 10:08:55AM +0800, Guannan Ren wrote:
On 07/16/2013 04:40 PM, Daniel P. Berrange wrote:
>On Tue, Jul 16, 2013 at 08:14:54AM +0200, Martin Kletzander wrote:
>>On 07/15/2013 05:31 PM, Ján Tomko wrote:
>>>On 07/02/2013 11:35 AM, Guannan Ren wrote:
>>>>If one of backing files for disk source doesn't exist, the guest will
not
>>>>be able to find and use the disk even though the disk still exists in
>>>>guest xml definition. So reporting an error make more sense.
>>>>
>>>>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 have so much meannings for F_OK, so give 0 for them.
>>>>---
>>>> 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 27aa4fe..cb61e5b 100644
>>>>--- a/src/util/virstoragefile.c
>>>>+++ b/src/util/virstoragefile.c
>>>>@@ -870,14 +877,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_ERROR(_("Backing file '%s' of image
'%s' is missing."),
>>>>+ meta->backingStoreRaw, path);
>>>>+ VIR_FREE(backing);
>>>>+ goto cleanup;
>>>> }
>>>> }
>>>> VIR_FREE(backing);
>>>This change means you won't be able to start the pool if one of the files
is
>>>missing a backing file. I've forwarded a patch [1] from/for [2] that
ignores
>>>missing files on pool start and there is a bug [3] requesting that we ignore
>>>other files as well. I feel like this is going in the other direction.
>>>
>>>Wouldn't it be enough to check for them on domain start-up and leave the
pool
>>>running even if one of those volumes doesn't have an existing backing
file?
>>>
>>How about making it configurable for the pool? There are definitely
>>some users who want the pool to reflect actual info after pool-refresh.
>I don't think this needs to be configurable. The pool should show *every*
>single file, regardless of whether the file has a broken backing file.
>We shouldn't be trying to second guess what the user wants to do with a
>image with broken backing file. Just expose as much info as we have and
>let them deal with the problem.
>
Daniel,
How about for guest cold bootup in this situation where one of its
disks has a broken file
we should throw an error out or give a warn in the log as what it is
doing currently.
If we can't access the file on guest boot, then we need to raise errors,
since it wouldn't be possible to set labelling or permissions. Of course
we have to make sure we don't give bogus errors in the case of root
squash NFS too. We need to consider storage pool enumeration separately
from guest bootup though. Both have specific semantics and we shouldn't
force them to behave the same way - the API needs to serve both their
needs.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
4:50 a.m.
New subject: [libvirt] [PATCH v4 2/4] storage: report error rather than warning if backing files doesn't exist
On 07/17/2013 05:41 PM, Daniel P. Berrange wrote:
On Wed, Jul 17, 2013 at 10:08:55AM +0800, Guannan Ren wrote:
> On 07/16/2013 04:40 PM, Daniel P. Berrange wrote:
>> On Tue, Jul 16, 2013 at 08:14:54AM +0200, Martin Kletzander wrote:
>>> On 07/15/2013 05:31 PM, Ján Tomko wrote:
>>>> On 07/02/2013 11:35 AM, Guannan Ren wrote:
>>>>> If one of backing files for disk source doesn't exist, the guest
will not
>>>>> be able to find and use the disk even though the disk still exists
in
>>>>> guest xml definition. So reporting an error make more sense.
>>>>>
>>>>> 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 have so much meannings for F_OK, so give 0 for
them.
>>>>> ---
>>>>> 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 27aa4fe..cb61e5b 100644
>>>>> --- a/src/util/virstoragefile.c
>>>>> +++ b/src/util/virstoragefile.c
>>>>> @@ -870,14 +877,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_ERROR(_("Backing file '%s' of
image '%s' is missing."),
>>>>> + meta->backingStoreRaw, path);
>>>>> + VIR_FREE(backing);
>>>>> + goto cleanup;
>>>>> }
>>>>> }
>>>>> VIR_FREE(backing);
>>>> This change means you won't be able to start the pool if one of the
files is
>>>> missing a backing file. I've forwarded a patch [1] from/for [2] that
ignores
>>>> missing files on pool start and there is a bug [3] requesting that we
ignore
>>>> other files as well. I feel like this is going in the other direction.
>>>>
>>>> Wouldn't it be enough to check for them on domain start-up and leave
the pool
>>>> running even if one of those volumes doesn't have an existing backing
file?
>>>>
>>> How about making it configurable for the pool? There are definitely
>>> some users who want the pool to reflect actual info after pool-refresh.
>> I don't think this needs to be configurable. The pool should show *every*
>> single file, regardless of whether the file has a broken backing file.
>> We shouldn't be trying to second guess what the user wants to do with a
>> image with broken backing file. Just expose as much info as we have and
>> let them deal with the problem.
>>
> Daniel,
>
> How about for guest cold bootup in this situation where one of its
> disks has a broken file
> we should throw an error out or give a warn in the log as what it is
> doing currently.
If we can't access the file on guest boot, then we need to raise errors,
since it wouldn't be possible to set labelling or permissions. Of course
we have to make sure we don't give bogus errors in the case of root
squash NFS too. We need to consider storage pool enumeration separately
from guest bootup though. Both have specific semantics and we shouldn't
force them to behave the same way - the API needs to serve both their
needs.
Hi Daniel,
About the root squash NFS issue, currently, we use access(disk_path,
F_OK) to check each disk file
in a diskchain. It is okay to boot up guest for disk whose backing files
are located in NFS shared folder with root_squash. The disk chain will
not be reported as broken in my test.
It seems that qemu needs only the read permission for disk backing files
and all changes on disk in furture will write to current disk file itself.
So I think in this case libvirt doesn't need to do checking on NFS
shared folder. Do you think it is right?
Guannan
10:40 p.m.
New subject: [libvirt] [PATCH v4 2/4] storage: report error rather than warning if backing files doesn't exist
On 07/15/2013 11:31 PM, Ján Tomko wrote:
On 07/02/2013 11:35 AM, Guannan Ren wrote:
> If one of backing files for disk source doesn't exist, the guest will not
> be able to find and use the disk even though the disk still exists in
> guest xml definition. So reporting an error make more sense.
>
> 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 have so much meannings for F_OK, so give 0 for them.
> ---
> 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 27aa4fe..cb61e5b 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -870,14 +877,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_ERROR(_("Backing file '%s' of image
'%s' is missing."),
> + meta->backingStoreRaw, path);
> + VIR_FREE(backing);
> + goto cleanup;
> }
> }
> VIR_FREE(backing);
This change means you won't be able to start the pool if one of the files is
missing a backing file. I've forwarded a patch [1] from/for [2] that ignores
missing files on pool start and there is a bug [3] requesting that we ignore
other files as well. I feel like this is going in the other direction.
Wouldn't it be enough to check for them on domain start-up and leave the pool
running even if one of those volumes doesn't have an existing backing file?
Jan
[1] https://www.redhat.com/archives/libvir-list/2013-July/msg00635.html
[2] https://bugzilla.redhat.com/show_bug.cgi?id=977706
[3] https://bugzilla.redhat.com/show_bug.cgi?id=710866
As what you pointed out is close to my issue, I chose to propose a patch
on bug 977706 first
https://www.redhat.com/archives/libvir-list/2013-July/msg01026.html.
Guannan
4:35 a.m.
New subject: [libvirt] [PATCH v4 3/4] qemu: drop disk with 'optional' startupPolicy if it is not present
Go through disks of guest, if one disk doesn't exist or its backing
chain is broken, with 'optional' startupPolicy, for CDROM and Floppy
we only discard its source path definition in xml, for disks we drop
it from disk list and free it.
Originally, for these disks without startupPolicy attribute, libvirt
ignore the presence checking, this patch make startupPolicy 'mandatory'
as default.
---
include/libvirt/libvirt.h.in | 1 +
src/qemu/qemu_domain.c | 119 ++++++++++++++++++++++++++-----------------
src/qemu/qemu_process.c | 7 ---
3 files changed, 74 insertions(+), 53 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index b791125..5459eb4 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -4699,6 +4699,7 @@ typedef void (*virConnectDomainEventBlockJobCallback)(virConnectPtr
conn,
*/
typedef enum {
VIR_DOMAIN_EVENT_DISK_CHANGE_MISSING_ON_START = 0, /* oldSrcPath is set */
+ VIR_DOMAIN_EVENT_HARDDISK_DROP_MISSING_ON_START = 1,
#ifdef VIR_ENUM_SENTINELS
VIR_DOMAIN_EVENT_DISK_CHANGE_LAST
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 8d79066..d54d477 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2006,75 +2006,102 @@ cleanup:
virObjectUnref(cfg);
}
+static int
+qemuDomainCheckRemoveOptionalDisk(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainDiskDefPtr disk)
+{
+ char uuid[VIR_UUID_STRING_BUFLEN];
+ virDomainEventPtr event = NULL;
+ virDomainDiskDefPtr del_disk = NULL;
+
+ virUUIDFormat(vm->def->uuid, uuid);
+
+ VIR_DEBUG("Dropping disk '%s' on domain '%s' (UUID '%s')
"
+ "due to inaccessible source '%s'",
+ disk->dst, vm->def->name, uuid, disk->src);
+
+ if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM ||
+ disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
+
+ event = virDomainEventDiskChangeNewFromObj(vm, disk->src, NULL,
+ disk->info.alias,
+
VIR_DOMAIN_EVENT_DISK_CHANGE_MISSING_ON_START);
+
+ VIR_FREE(disk->src);
+ } else {
+ event = virDomainEventDiskChangeNewFromObj(vm, disk->src, NULL,
+ disk->info.alias,
+
VIR_DOMAIN_EVENT_HARDDISK_DROP_MISSING_ON_START);
+
+ if (!(del_disk = virDomainDiskRemoveByName(vm->def, disk->src))) {
+ virReportError(VIR_ERR_INVALID_ARG,
+ _("no source device %s"), disk->src);
+ return -1;
+ }
+ virDomainDiskDefFree(del_disk);
+ }
+
+ if (event)
+ qemuDomainEventQueue(driver, event);
+
+ return 0;
+}
+
int
qemuDomainCheckDiskPresence(virQEMUDriverPtr driver,
virDomainObjPtr vm,
bool cold_boot)
{
int ret = -1;
- int i;
+ 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];
+ size_t count = vm->def->ndisks;
+ size_t nextdisk = 0;
- if (!disk->startupPolicy || !disk->src)
- continue;
+ for (i = 0; i < count; i++) {
+ disk = vm->def->disks[nextdisk];
- if (virFileAccessibleAs(disk->src, F_OK,
- cfg->user,
- cfg->group) >= 0) {
- /* disk accessible */
+ if (qemuDomainDetermineDiskChain(driver, disk, false) == 0) {
+ nextdisk++;
continue;
}
switch ((enum virDomainStartupPolicy) disk->startupPolicy) {
- case VIR_DOMAIN_STARTUP_POLICY_OPTIONAL:
- break;
+ 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_REQUISITE:
+ if (cold_boot) {
+ goto error;
+ }
+ break;
+
+ case VIR_DOMAIN_STARTUP_POLICY_MANDATORY:
+ case VIR_DOMAIN_STARTUP_POLICY_DEFAULT:
+ goto error;
- case VIR_DOMAIN_STARTUP_POLICY_DEFAULT:
- case VIR_DOMAIN_STARTUP_POLICY_LAST:
- /* this should never happen */
- break;
+ 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);
+ virResetLastError();
- event = virDomainEventDiskChangeNewFromObj(vm, disk->src, NULL,
disk->info.alias,
-
VIR_DOMAIN_EVENT_DISK_CHANGE_MISSING_ON_START);
- if (event)
- qemuDomainEventQueue(driver, event);
+ /* For cdrom or floppy, we only remove its source definition,
+ * so, the nextdisk need to point to next disk in next loop.
+ */
+ if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM ||
+ disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY)
+ nextdisk++;
- VIR_FREE(disk->src);
+ if (qemuDomainCheckRemoveOptionalDisk(driver, vm, disk) < 0)
+ goto error;
}
ret = 0;
-cleanup:
- virObjectUnref(cfg);
+error:
return ret;
}
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index ac5ffcf..4672f7f 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3543,17 +3543,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.1.4
4:35 a.m.
New subject: [libvirt] [PATCH v4 4/4] security: restore DAC for every disk file in disk chain
---
src/security/security_dac.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 0d6defc..577db8e 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -335,6 +335,16 @@ err:
return rc;
}
+static int
+virSecurityDACRestoreDiskSecurityFileLabel(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED,
+ const char *path,
+ size_t depth ATTRIBUTE_UNUSED,
+ void *opaque ATTRIBUTE_UNUSED)
+{
+ return virSecurityDACRestoreSecurityFileLabel(path);
+}
+
+
static int
virSecurityDACSetSecurityFileLabel(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED,
@@ -424,7 +434,10 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr
mgr,
}
}
- return virSecurityDACRestoreSecurityFileLabel(disk->src);
+ return virDomainDiskDefForeachPath(disk,
+ false,
+ virSecurityDACRestoreDiskSecurityFileLabel,
+ NULL);
}
--
1.8.1.4
8:30 a.m.
On 07/02/2013 05:35 PM, Guannan Ren wrote:
v3:
https://www.redhat.com/archives/libvir-list/2013-June/thread.html
v3 to v4: Rebase
v2 to v3: Not only check disk source, startupPolicy should work if any
backing file is missing. The commit 039a3283 break the limition
of contiguous device boot orders, so I remove my previous patch
about it.
v1 to v2: Added relax schema for disk of block and dir type
Removed original patch 3/5.
The set of patches is trying to add 'startupPolicy' attribute support
to the source element of hard disks. Policy levels are using the
mandatory, requisite, optional levels as originally documented.
For the 'optional' policy, there is a little difference from CDROM and
Floppy which only drop its source path, for disks, if missing, the
checking function will drop their definitions, because qemu doesn't
allow missing source path for hard disk.
Are we going to support this feature. I think this is a good feature for
two reasons:
1, currently, we only check the disk presence for floppy and CDROM
device which is not enough.
For disks with backing files, if one of its backing file is missing,
libvirt doesn't check anything, the qemu
will throw an unclear error message as follows:
A guest with a disk image /var/lib/libvirt/images/snapshot/snap2.qcow
# qemu-img info --backing-chain /var/lib/libvirt/images/snapshot/snap2.qcow
image: /var/lib/libvirt/images/snapshot/snap2.qcow
file format: qcow2
virtual size: 10G (10737418240 bytes)
disk size: 19M
cluster_size: 65536
backing file: /var/lib/libvirt/images/snapshot/snap1.qcow
backing file format: qcow2
image: /var/lib/libvirt/images/snapshot/snap1.qcow
file format: qcow2
virtual size: 10G (10737418240 bytes)
disk size: 196K
cluster_size: 65536
backing file: /var/lib/libvirt/images/fedora18.img
backing file format: raw
if we change snap1.qcow to *snap1.qcow.back* , then bootup the guest
The qemu will throw an error:
qemu-system-x86_64: -drive
file=/var/lib/libvirt/images/snapshot/snap2.qcow,if=none,id=drive-virtio-disk0,format=qcow2:
could not open disk image /var/lib/libvirt/images/snapshot/snap2.qcow:
No such file or directory
Actually, snap2.qcow is there, one of its backing file snap1.qcow is
missing. So we need to check the presence of all files in disk chains.
With this patch, libvirt will throw an error:
error: Backing file '/var/lib/libvirt/images/snapshot/snap1.qcow' does
not exist: No such file or directory
2, Adding 'startupPolicy' attribute for guest disk is useful for guests
which use the disk from storage-centric production environment.
Guest uses the the block disk assigned by Fibre channel or FCoE or ISCSI
storage. The storage administrator can adjust the disk assignment
between guests, if guest disk with startupPolicy value of optional,
guest still can boot up after its assigned disks are missing.
So I think it is good to add this flexibility to disk configuration for
guests. Any thoughts?
Guannan
4113
days inactive
4161
days old
20 comments
4 participants
participants (4)
-
Daniel P. Berrange -
Guannan Ren -
Ján Tomko -
Martin Kletzander