6:13 a.m.
Patch 1/2 is just a cleanup; patch 2/2 fixes the actual issue.
Michal Privoznik (2):
virDomainDiskDefForeachPath: Prefer virStorageSourceIsLocalStorage
Introduce and use virDomainDiskZeroSource
src/conf/domain_conf.c | 27 ++++++++++++++++++++++++---
src/conf/domain_conf.h | 1 +
src/libvirt_private.syms | 1 +
src/qemu/qemu_domain.c | 2 +-
src/qemu/qemu_process.c | 2 +-
src/vmx/vmx.c | 6 +++---
src/xenconfig/xen_xm.c | 2 +-
7 files changed, 32 insertions(+), 9 deletions(-)
--
2.10.2
6:13 a.m.
New subject: [libvirt] [PATCH 1/2] virDomainDiskDefForeachPath: Prefer virStorageSourceIsLocalStorage
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1b0a55b..01553b5 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -25342,10 +25342,8 @@ virDomainDiskDefForeachPath(virDomainDiskDefPtr disk,
}
for (tmp = disk->src; tmp; tmp = tmp->backingStore) {
- int actualType = virStorageSourceGetActualType(tmp);
/* execute the callback only for local storage */
- if (actualType != VIR_STORAGE_TYPE_NETWORK &&
- actualType != VIR_STORAGE_TYPE_VOLUME &&
+ if (virStorageSourceIsLocalStorage(tmp) &&
tmp->path) {
if (iter(disk, tmp->path, depth, opaque) < 0)
goto cleanup;
--
2.10.2
6:48 a.m.
New subject: [libvirt] [PATCH 1/2] virDomainDiskDefForeachPath: Prefer virStorageSourceIsLocalStorage
On Fri, Mar 31, 2017 at 01:13:50PM +0200, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
ACK and safe for freeze.
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1b0a55b..01553b5 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -25342,10 +25342,8 @@ virDomainDiskDefForeachPath(virDomainDiskDefPtr disk,
}
for (tmp = disk->src; tmp; tmp = tmp->backingStore) {
- int actualType = virStorageSourceGetActualType(tmp);
/* execute the callback only for local storage */
- if (actualType != VIR_STORAGE_TYPE_NETWORK &&
- actualType != VIR_STORAGE_TYPE_VOLUME &&
+ if (virStorageSourceIsLocalStorage(tmp) &&
tmp->path) {
if (iter(disk, tmp->path, depth, opaque) < 0)
goto cleanup;
--
2.10.2
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
6:13 a.m.
New subject: [libvirt] [PATCH 2/2] Introduce and use virDomainDiskZeroSource
Currently, if we want to zero out disk source (e,g, due to
startupPolicy when starting up a domain) we use
virDomainDiskSetSource(disk, NULL). This works well for file
based storage (storage type file, dir, or block). But it doesn't
work at all for other types like volume and network.
So imagine that you have a domain that has a CDROM configured
which source is a volume from an inactive pool. Because it is
startupPolicy='optional', the CDROM is empty when the domain
starts. However, the source element is not cleared out in the
status XML and thus when the daemon restarts and tries to
reconnect to the domain it refreshes the disks (which fails - the
storage pool is still not running) and thus the domain is killed.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 23 +++++++++++++++++++++++
src/conf/domain_conf.h | 1 +
src/libvirt_private.syms | 1 +
src/qemu/qemu_domain.c | 2 +-
src/qemu/qemu_process.c | 2 +-
src/vmx/vmx.c | 6 +++---
src/xenconfig/xen_xm.c | 2 +-
7 files changed, 31 insertions(+), 6 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 01553b5..a60a456 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1719,6 +1719,29 @@ virDomainDiskSetSource(virDomainDiskDefPtr def, const char *src)
}
+void
+virDomainDiskZeroSource(virDomainDiskDefPtr def)
+{
+ switch ((virStorageType) def->src->type) {
+ case VIR_STORAGE_TYPE_DIR:
+ case VIR_STORAGE_TYPE_FILE:
+ case VIR_STORAGE_TYPE_BLOCK:
+ VIR_FREE(def->src->path);
+ break;
+ case VIR_STORAGE_TYPE_NETWORK:
+ VIR_FREE(def->src->volume);
+ break;
+ case VIR_STORAGE_TYPE_VOLUME:
+ virStorageSourcePoolDefFree(def->src->srcpool);
+ def->src->srcpool = NULL;
+ break;
+ case VIR_STORAGE_TYPE_NONE:
+ case VIR_STORAGE_TYPE_LAST:
+ break;
+ }
+}
+
+
const char *
virDomainDiskGetDriver(virDomainDiskDefPtr def)
{
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 47eaace..05b544c 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2590,6 +2590,7 @@ void virDomainDiskSetType(virDomainDiskDefPtr def, int type);
const char *virDomainDiskGetSource(virDomainDiskDef const *def);
int virDomainDiskSetSource(virDomainDiskDefPtr def, const char *src)
ATTRIBUTE_RETURN_CHECK;
+void virDomainDiskZeroSource(virDomainDiskDefPtr def);
const char *virDomainDiskGetDriver(virDomainDiskDefPtr def);
int virDomainDiskSetDriver(virDomainDiskDefPtr def, const char *name)
ATTRIBUTE_RETURN_CHECK;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index b551cb8..60c90d1 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -324,6 +324,7 @@ virDomainDiskSetDriver;
virDomainDiskSetFormat;
virDomainDiskSetSource;
virDomainDiskSetType;
+virDomainDiskZeroSource;
virDomainFSDefFree;
virDomainFSDefNew;
virDomainFSIndexByName;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 589eb18..a50b3aa 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4908,7 +4908,7 @@ qemuDomainCheckRemoveOptionalDisk(virQEMUDriverPtr driver,
event = virDomainEventDiskChangeNewFromObj(vm, src, NULL,
disk->info.alias,
VIR_DOMAIN_EVENT_DISK_CHANGE_MISSING_ON_START);
- ignore_value(virDomainDiskSetSource(disk, NULL));
+ virDomainDiskZeroSource(disk);
/* keeping the old startup policy would be invalid for new images */
disk->startupPolicy = VIR_DOMAIN_STARTUP_POLICY_DEFAULT;
} else {
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index a20beb1..6be0b51 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -6804,7 +6804,7 @@ qemuProcessRefreshDisks(virQEMUDriverPtr driver,
if (info->removable) {
if (info->empty)
- ignore_value(virDomainDiskSetSource(disk, NULL));
+ virDomainDiskZeroSource(disk);
if (info->tray) {
if (info->tray_open)
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index 31af2e9..5c833b8 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -2283,7 +2283,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt,
virConfPtr con
virDomainDiskSetType(*def, VIR_STORAGE_TYPE_BLOCK);
if (STRCASEEQ(fileName, "auto detect")) {
- ignore_value(virDomainDiskSetSource(*def, NULL));
+ virDomainDiskZeroSource(*def);
(*def)->startupPolicy = VIR_DOMAIN_STARTUP_POLICY_OPTIONAL;
} else if (virDomainDiskSetSource(*def, fileName) < 0) {
goto cleanup;
@@ -2294,7 +2294,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt,
virConfPtr con
virDomainDiskSetType(*def, VIR_STORAGE_TYPE_BLOCK);
if (STRCASEEQ(fileName, "auto detect")) {
- ignore_value(virDomainDiskSetSource(*def, NULL));
+ virDomainDiskZeroSource(*def);
(*def)->startupPolicy = VIR_DOMAIN_STARTUP_POLICY_OPTIONAL;
} else if (virDomainDiskSetSource(*def, fileName) < 0) {
goto cleanup;
@@ -2326,7 +2326,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt,
virConfPtr con
}
virDomainDiskSetType(*def, VIR_STORAGE_TYPE_FILE);
- ignore_value(virDomainDiskSetSource(*def, NULL));
+ virDomainDiskZeroSource(*def);
} else {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Invalid or not yet handled value '%s' "
diff --git a/src/xenconfig/xen_xm.c b/src/xenconfig/xen_xm.c
index 8ef68bb..327cb65 100644
--- a/src/xenconfig/xen_xm.c
+++ b/src/xenconfig/xen_xm.c
@@ -140,7 +140,7 @@ xenParseXMDisk(virConfPtr conf, virDomainDefPtr def)
if (offset == head) {
/* No source file given, eg CDROM with no media */
- ignore_value(virDomainDiskSetSource(disk, NULL));
+ virDomainDiskZeroSource(disk);
} else {
if (VIR_STRNDUP(tmp, head, offset - head) < 0)
goto cleanup;
--
2.10.2
7:10 a.m.
New subject: [libvirt] [PATCH 2/2] Introduce and use virDomainDiskZeroSource
On Fri, Mar 31, 2017 at 01:13:51PM +0200, Michal Privoznik wrote:
Currently, if we want to zero out disk source (e,g, due to
startupPolicy when starting up a domain) we use
virDomainDiskSetSource(disk, NULL). This works well for file
based storage (storage type file, dir, or block). But it doesn't
work at all for other types like volume and network.
So imagine that you have a domain that has a CDROM configured
which source is a volume from an inactive pool. Because it is
startupPolicy='optional', the CDROM is empty when the domain
starts. However, the source element is not cleared out in the
status XML and thus when the daemon restarts and tries to
reconnect to the domain it refreshes the disks (which fails - the
storage pool is still not running) and thus the domain is killed.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 23 +++++++++++++++++++++++
src/conf/domain_conf.h | 1 +
src/libvirt_private.syms | 1 +
src/qemu/qemu_domain.c | 2 +-
src/qemu/qemu_process.c | 2 +-
src/vmx/vmx.c | 6 +++---
src/xenconfig/xen_xm.c | 2 +-
7 files changed, 31 insertions(+), 6 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 01553b5..a60a456 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1719,6 +1719,29 @@ virDomainDiskSetSource(virDomainDiskDefPtr def, const char *src)
}
+void
+virDomainDiskZeroSource(virDomainDiskDefPtr def)
+{
+ switch ((virStorageType) def->src->type) {
+ case VIR_STORAGE_TYPE_DIR:
+ case VIR_STORAGE_TYPE_FILE:
+ case VIR_STORAGE_TYPE_BLOCK:
+ VIR_FREE(def->src->path);
+ break;
+ case VIR_STORAGE_TYPE_NETWORK:
+ VIR_FREE(def->src->volume);
+ break;
+ case VIR_STORAGE_TYPE_VOLUME:
+ virStorageSourcePoolDefFree(def->src->srcpool);
+ def->src->srcpool = NULL;
+ break;
+ case VIR_STORAGE_TYPE_NONE:
+ case VIR_STORAGE_TYPE_LAST:
+ break;
+ }
+}
+
+
Won't this break on migration? I feel like we're losing information
here. Can't we just check the sourceOptional (or whatever the name is)
when reconnecting to such domains?
Anyway, that's a discussion we can have later on. This patch is not
changing the behaviour, just fixing part of it. So ACK to that, however
since I don't feel that very knowledgeable and confident in the XEN and
VMX areas.
But ...
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index 31af2e9..5c833b8 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -2283,7 +2283,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt,
virConfPtr con
virDomainDiskSetType(*def, VIR_STORAGE_TYPE_BLOCK);
if (STRCASEEQ(fileName, "auto detect")) {
- ignore_value(virDomainDiskSetSource(*def, NULL));
+ virDomainDiskZeroSource(*def);
(*def)->startupPolicy = VIR_DOMAIN_STARTUP_POLICY_OPTIONAL;
} else if (virDomainDiskSetSource(*def, fileName) < 0) {
goto cleanup;
@@ -2294,7 +2294,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt,
virConfPtr con
virDomainDiskSetType(*def, VIR_STORAGE_TYPE_BLOCK);
if (STRCASEEQ(fileName, "auto detect")) {
- ignore_value(virDomainDiskSetSource(*def, NULL));
+ virDomainDiskZeroSource(*def);
(*def)->startupPolicy = VIR_DOMAIN_STARTUP_POLICY_OPTIONAL;
} else if (virDomainDiskSetSource(*def, fileName) < 0) {
goto cleanup;
@@ -2326,7 +2326,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt,
virConfPtr con
}
virDomainDiskSetType(*def, VIR_STORAGE_TYPE_FILE);
- ignore_value(virDomainDiskSetSource(*def, NULL));
+ virDomainDiskZeroSource(*def);
} else {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Invalid or not yet handled value '%s' "
looking at the code, and it's even visible just from the context in
these hunks, you're only changing the behaviour for
VIR_STORAGE_TYPE_BLOCK in VMX (i.e. not changing the behaviour), ...
diff --git a/src/xenconfig/xen_xm.c b/src/xenconfig/xen_xm.c
index 8ef68bb..327cb65 100644
--- a/src/xenconfig/xen_xm.c
+++ b/src/xenconfig/xen_xm.c
@@ -140,7 +140,7 @@ xenParseXMDisk(virConfPtr conf, virDomainDefPtr def)
if (offset == head) {
/* No source file given, eg CDROM with no media */
- ignore_value(virDomainDiskSetSource(disk, NULL));
+ virDomainDiskZeroSource(disk);
} else {
... and here you clear the source of a disk object that was just created
and has no source set yet. So this part of the condition can be removed.
I wanted to say that this could be cleaned even more, so for now just
change the 'else' to 'if (offset != head)', and that's ACK and safe
for
freeze.
if (VIR_STRNDUP(tmp, head, offset - head) < 0)
goto cleanup;
--
2.10.2
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
7:18 a.m.
New subject: [libvirt] [PATCH 2/2] Introduce and use virDomainDiskZeroSource
On Fri, Mar 31, 2017 at 13:13:51 +0200, Michal Privoznik wrote:
Currently, if we want to zero out disk source (e,g, due to
startupPolicy when starting up a domain) we use
virDomainDiskSetSource(disk, NULL). This works well for file
based storage (storage type file, dir, or block). But it doesn't
work at all for other types like volume and network.
So imagine that you have a domain that has a CDROM configured
which source is a volume from an inactive pool. Because it is
startupPolicy='optional', the CDROM is empty when the domain
starts. However, the source element is not cleared out in the
status XML and thus when the daemon restarts and tries to
reconnect to the domain it refreshes the disks (which fails - the
storage pool is still not running) and thus the domain is killed.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 23 +++++++++++++++++++++++
src/conf/domain_conf.h | 1 +
src/libvirt_private.syms | 1 +
src/qemu/qemu_domain.c | 2 +-
src/qemu/qemu_process.c | 2 +-
src/vmx/vmx.c | 6 +++---
src/xenconfig/xen_xm.c | 2 +-
7 files changed, 31 insertions(+), 6 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 01553b5..a60a456 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1719,6 +1719,29 @@ virDomainDiskSetSource(virDomainDiskDefPtr def, const char *src)
}
+void
+virDomainDiskZeroSource(virDomainDiskDefPtr def)
virDomainDiskEmptySource perhaps?
+{
+ switch ((virStorageType) def->src->type) {
+ case VIR_STORAGE_TYPE_DIR:
+ case VIR_STORAGE_TYPE_FILE:
+ case VIR_STORAGE_TYPE_BLOCK:
+ VIR_FREE(def->src->path);
+ break;
+ case VIR_STORAGE_TYPE_NETWORK:
+ VIR_FREE(def->src->volume);
This certainly is not enough to clear network disks. You also need to
clear the hosts and the path component. Resetting the protocol also
would be desired.
+ break;
+ case VIR_STORAGE_TYPE_VOLUME:
+ virStorageSourcePoolDefFree(def->src->srcpool);
+ def->src->srcpool = NULL;
+ break;
+ case VIR_STORAGE_TYPE_NONE:
+ case VIR_STORAGE_TYPE_LAST:
+ break;
+ }
You also should reset the disk type. I'm not sure though wether setting
it to "NONE" is a good idea. "FILE" might be safer.
All of this needs to clear the backing chain too.
+}
+
+
const char *
virDomainDiskGetDriver(virDomainDiskDefPtr def)
{
Usage looks good.
8:03 a.m.
New subject: [libvirt] [PATCH 2/2] Introduce and use virDomainDiskZeroSource
On 03/31/2017 02:18 PM, Peter Krempa wrote:
On Fri, Mar 31, 2017 at 13:13:51 +0200, Michal Privoznik wrote:
> Currently, if we want to zero out disk source (e,g, due to
> startupPolicy when starting up a domain) we use
> virDomainDiskSetSource(disk, NULL). This works well for file
> based storage (storage type file, dir, or block). But it doesn't
> work at all for other types like volume and network.
>
> So imagine that you have a domain that has a CDROM configured
> which source is a volume from an inactive pool. Because it is
> startupPolicy='optional', the CDROM is empty when the domain
> starts. However, the source element is not cleared out in the
> status XML and thus when the daemon restarts and tries to
> reconnect to the domain it refreshes the disks (which fails - the
> storage pool is still not running) and thus the domain is killed.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/conf/domain_conf.c | 23 +++++++++++++++++++++++
> src/conf/domain_conf.h | 1 +
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_domain.c | 2 +-
> src/qemu/qemu_process.c | 2 +-
> src/vmx/vmx.c | 6 +++---
> src/xenconfig/xen_xm.c | 2 +-
> 7 files changed, 31 insertions(+), 6 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 01553b5..a60a456 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1719,6 +1719,29 @@ virDomainDiskSetSource(virDomainDiskDefPtr def, const char
*src)
> }
>
>
> +void
> +virDomainDiskZeroSource(virDomainDiskDefPtr def)
virDomainDiskEmptySource perhaps?
> +{
> + switch ((virStorageType) def->src->type) {
> + case VIR_STORAGE_TYPE_DIR:
> + case VIR_STORAGE_TYPE_FILE:
> + case VIR_STORAGE_TYPE_BLOCK:
> + VIR_FREE(def->src->path);
> + break;
> + case VIR_STORAGE_TYPE_NETWORK:
> + VIR_FREE(def->src->volume);
This certainly is not enough to clear network disks. You also need to
clear the hosts and the path component. Resetting the protocol also
would be desired.
Fair enough.
> + break;
> + case VIR_STORAGE_TYPE_VOLUME:
> + virStorageSourcePoolDefFree(def->src->srcpool);
> + def->src->srcpool = NULL;
> + break;
> + case VIR_STORAGE_TYPE_NONE:
> + case VIR_STORAGE_TYPE_LAST:
> + break;
> + }
You also should reset the disk type. I'm not sure though wether setting
it to "NONE" is a good idea. "FILE" might be safer.
This doesn't sound fair. If the original disk was type of volume, why it
should all of a sudden be reported as type of file?
If we are afraid of other areas of the code failing or being unprepared
for, I say have them fail so that they can be fixed.
Michal
8:23 a.m.
New subject: [libvirt] [PATCH 2/2] Introduce and use virDomainDiskZeroSource
On Fri, Mar 31, 2017 at 15:03:35 +0200, Michal Privoznik wrote:
On 03/31/2017 02:18 PM, Peter Krempa wrote:
> On Fri, Mar 31, 2017 at 13:13:51 +0200, Michal Privoznik wrote:
> > Currently, if we want to zero out disk source (e,g, due to
> > startupPolicy when starting up a domain) we use
> > virDomainDiskSetSource(disk, NULL). This works well for file
> > based storage (storage type file, dir, or block). But it doesn't
> > work at all for other types like volume and network.
> >
> > So imagine that you have a domain that has a CDROM configured
> > which source is a volume from an inactive pool. Because it is
> > startupPolicy='optional', the CDROM is empty when the domain
> > starts. However, the source element is not cleared out in the
> > status XML and thus when the daemon restarts and tries to
> > reconnect to the domain it refreshes the disks (which fails - the
> > storage pool is still not running) and thus the domain is killed.
> >
> > Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> > ---
> > src/conf/domain_conf.c | 23 +++++++++++++++++++++++
> > src/conf/domain_conf.h | 1 +
> > src/libvirt_private.syms | 1 +
> > src/qemu/qemu_domain.c | 2 +-
> > src/qemu/qemu_process.c | 2 +-
> > src/vmx/vmx.c | 6 +++---
> > src/xenconfig/xen_xm.c | 2 +-
> > 7 files changed, 31 insertions(+), 6 deletions(-)
[...]
> > + break;
> > + case VIR_STORAGE_TYPE_VOLUME:
> > + virStorageSourcePoolDefFree(def->src->srcpool);
> > + def->src->srcpool = NULL;
> > + break;
> > + case VIR_STORAGE_TYPE_NONE:
> > + case VIR_STORAGE_TYPE_LAST:
> > + break;
> > + }
>
> You also should reset the disk type. I'm not sure though wether setting
> it to "NONE" is a good idea. "FILE" might be safer.
This doesn't sound fair. If the original disk was type of volume, why it
should all of a sudden be reported as type of file?
Well, yes. As said it's the safest option. E.g. the following XML which
would be a result of the code above does not pass schema validation:
<disk type='network' device='cdrom'>
<driver name='qemu' type='raw'/>
<target dev='hda' bus='ide'/>
<readonly/>
<address type='drive' controller='0' bus='0'
target='0' unit='0'/>
</disk>
Is it fault of the schema being broken? Perhaps yes. I didn't really
think about it.
Also. Since the disk is empty the source really does not mater. The need
to keep is a historical artifact which will require a lot of
refactoring.
If we are afraid of other areas of the code failing or being
unprepared for,
I say have them fail so that they can be fixed.
I'm not sure whether this approach is correct. You are saying we should
introduce regressions?
The problem here is that while libvirt models the "type" as being part
of the disk, it in fact is part of the source itself. The qemu driver
will probably be mostly prepared for this, but I'd rather not risk it.
Certaily not during the freeze, such shenanigans are certainly NOT safe
for the freeze.
8:37 a.m.
New subject: [libvirt] [PATCH 2/2] Introduce and use virDomainDiskZeroSource
On 03/31/2017 03:23 PM, Peter Krempa wrote:
On Fri, Mar 31, 2017 at 15:03:35 +0200, Michal Privoznik wrote:
> On 03/31/2017 02:18 PM, Peter Krempa wrote:
>> On Fri, Mar 31, 2017 at 13:13:51 +0200, Michal Privoznik wrote:
>>> Currently, if we want to zero out disk source (e,g, due to
>>> startupPolicy when starting up a domain) we use
>>> virDomainDiskSetSource(disk, NULL). This works well for file
>>> based storage (storage type file, dir, or block). But it doesn't
>>> work at all for other types like volume and network.
>>>
>>> So imagine that you have a domain that has a CDROM configured
>>> which source is a volume from an inactive pool. Because it is
>>> startupPolicy='optional', the CDROM is empty when the domain
>>> starts. However, the source element is not cleared out in the
>>> status XML and thus when the daemon restarts and tries to
>>> reconnect to the domain it refreshes the disks (which fails - the
>>> storage pool is still not running) and thus the domain is killed.
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
>>> ---
>>> src/conf/domain_conf.c | 23 +++++++++++++++++++++++
>>> src/conf/domain_conf.h | 1 +
>>> src/libvirt_private.syms | 1 +
>>> src/qemu/qemu_domain.c | 2 +-
>>> src/qemu/qemu_process.c | 2 +-
>>> src/vmx/vmx.c | 6 +++---
>>> src/xenconfig/xen_xm.c | 2 +-
>>> 7 files changed, 31 insertions(+), 6 deletions(-)
[...]
>>> + break;
>>> + case VIR_STORAGE_TYPE_VOLUME:
>>> + virStorageSourcePoolDefFree(def->src->srcpool);
>>> + def->src->srcpool = NULL;
>>> + break;
>>> + case VIR_STORAGE_TYPE_NONE:
>>> + case VIR_STORAGE_TYPE_LAST:
>>> + break;
>>> + }
>>
>> You also should reset the disk type. I'm not sure though wether setting
>> it to "NONE" is a good idea. "FILE" might be safer.
>
> This doesn't sound fair. If the original disk was type of volume, why it
> should all of a sudden be reported as type of file?
Well, yes. As said it's the safest option. E.g. the following XML which
would be a result of the code above does not pass schema validation:
<disk type='network' device='cdrom'>
<driver name='qemu' type='raw'/>
<target dev='hda' bus='ide'/>
<readonly/>
<address type='drive' controller='0' bus='0'
target='0' unit='0'/>
</disk>
Is it fault of the schema being broken? Perhaps yes. I didn't really
think about it.
Yes, it's the schema which is broken. Our schema should accept that any
disk with device='cdrom' can lack <source/> element regardless of its
type. The same goes for floppy disks.
Also. Since the disk is empty the source really does not mater. The need
to keep is a historical artifact which will require a lot of
refactoring.
> If we are afraid of other areas of the code failing or being unprepared for,
> I say have them fail so that they can be fixed.
I'm not sure whether this approach is correct. You are saying we should
introduce regressions?
No. If they are broken now, that is not what I call a regression.
The problem here is that while libvirt models the "type" as being part
of the disk, it in fact is part of the source itself. The qemu driver
will probably be mostly prepared for this, but I'd rather not risk it.
Certaily not during the freeze, such shenanigans are certainly NOT safe
for the freeze.
Fair enough. I don't care that much about this. I care more about fixing
the bug and getting the fix in for the release. If adding the one line
is a requisite I can live with that. Will post v2 soon then.
BTW: just to be clear - you're okay with 1/2 being pushed as is, right?
Michal
9:02 a.m.
New subject: [libvirt] [PATCH v2 2/2] Introduce and use virDomainDiskEmptySource
Currently, if we want to zero out disk source (e,g, due to
startupPolicy when starting up a domain) we use
virDomainDiskSetSource(disk, NULL). This works well for file
based storage (storage type file, dir, or block). But it doesn't
work at all for other types like volume and network.
So imagine that you have a domain that has a CDROM configured
which source is a volume from an inactive pool. Because it is
startupPolicy='optional', the CDROM is empty when the domain
starts. However, the source element is not cleared out in the
status XML and thus when the daemon restarts and tries to
reconnect to the domain it refreshes the disks (which fails - the
storage pool is still not running) and thus the domain is killed.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
diff to v1:
- Free more struct entries
- Set disk type
- Call the function less frequently
src/conf/domain_conf.c | 33 +++++++++++++++++++++++++++++++++
src/conf/domain_conf.h | 1 +
src/libvirt_private.syms | 1 +
src/qemu/qemu_domain.c | 2 +-
src/qemu/qemu_process.c | 2 +-
5 files changed, 37 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 01553b5..f1f0b56 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1719,6 +1719,39 @@ virDomainDiskSetSource(virDomainDiskDefPtr def, const char *src)
}
+void
+virDomainDiskEmptySource(virDomainDiskDefPtr def)
+{
+ virStorageSourcePtr src = def->src;
+
+ switch ((virStorageType) src->type) {
+ case VIR_STORAGE_TYPE_DIR:
+ case VIR_STORAGE_TYPE_FILE:
+ case VIR_STORAGE_TYPE_BLOCK:
+ VIR_FREE(src->path);
+ break;
+ case VIR_STORAGE_TYPE_NETWORK:
+ VIR_FREE(src->path);
+ VIR_FREE(src->volume);
+ virStorageNetHostDefFree(src->nhosts, src->hosts);
+ src->nhosts = 0;
+ src->hosts = NULL;
+ src->protocol = VIR_STORAGE_NET_PROTOCOL_NONE;
+ break;
+ case VIR_STORAGE_TYPE_VOLUME:
+ virStorageSourcePoolDefFree(src->srcpool);
+ src->srcpool = NULL;
+ break;
+ case VIR_STORAGE_TYPE_NONE:
+ case VIR_STORAGE_TYPE_LAST:
+ break;
+ }
+
+ virStorageSourceBackingStoreClear(src);
+ src->type = VIR_STORAGE_TYPE_FILE;
+}
+
+
const char *
virDomainDiskGetDriver(virDomainDiskDefPtr def)
{
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 47eaace..26c0e6b 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2590,6 +2590,7 @@ void virDomainDiskSetType(virDomainDiskDefPtr def, int type);
const char *virDomainDiskGetSource(virDomainDiskDef const *def);
int virDomainDiskSetSource(virDomainDiskDefPtr def, const char *src)
ATTRIBUTE_RETURN_CHECK;
+void virDomainDiskEmptySource(virDomainDiskDefPtr def);
const char *virDomainDiskGetDriver(virDomainDiskDefPtr def);
int virDomainDiskSetDriver(virDomainDiskDefPtr def, const char *name)
ATTRIBUTE_RETURN_CHECK;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index b551cb8..92083e5 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -299,6 +299,7 @@ virDomainDiskDetectZeroesTypeFromString;
virDomainDiskDetectZeroesTypeToString;
virDomainDiskDeviceTypeToString;
virDomainDiskDiscardTypeToString;
+virDomainDiskEmptySource;
virDomainDiskErrorPolicyTypeFromString;
virDomainDiskErrorPolicyTypeToString;
virDomainDiskFindByBusAndDst;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 589eb18..b733505 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4908,7 +4908,7 @@ qemuDomainCheckRemoveOptionalDisk(virQEMUDriverPtr driver,
event = virDomainEventDiskChangeNewFromObj(vm, src, NULL,
disk->info.alias,
VIR_DOMAIN_EVENT_DISK_CHANGE_MISSING_ON_START);
- ignore_value(virDomainDiskSetSource(disk, NULL));
+ virDomainDiskEmptySource(disk);
/* keeping the old startup policy would be invalid for new images */
disk->startupPolicy = VIR_DOMAIN_STARTUP_POLICY_DEFAULT;
} else {
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index a20beb1..e450d06 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -6804,7 +6804,7 @@ qemuProcessRefreshDisks(virQEMUDriverPtr driver,
if (info->removable) {
if (info->empty)
- ignore_value(virDomainDiskSetSource(disk, NULL));
+ virDomainDiskEmptySource(disk);
if (info->tray) {
if (info->tray_open)
--
2.10.2
9:17 a.m.
New subject: [libvirt] [PATCH v2 2/2] Introduce and use virDomainDiskEmptySource
On Fri, Mar 31, 2017 at 16:02:14 +0200, Michal Privoznik wrote:
Currently, if we want to zero out disk source (e,g, due to
startupPolicy when starting up a domain) we use
virDomainDiskSetSource(disk, NULL). This works well for file
based storage (storage type file, dir, or block). But it doesn't
work at all for other types like volume and network.
So imagine that you have a domain that has a CDROM configured
which source is a volume from an inactive pool. Because it is
startupPolicy='optional', the CDROM is empty when the domain
starts. However, the source element is not cleared out in the
status XML and thus when the daemon restarts and tries to
reconnect to the domain it refreshes the disks (which fails - the
storage pool is still not running) and thus the domain is killed.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
diff to v1:
- Free more struct entries
- Set disk type
- Call the function less frequently
src/conf/domain_conf.c | 33 +++++++++++++++++++++++++++++++++
src/conf/domain_conf.h | 1 +
src/libvirt_private.syms | 1 +
src/qemu/qemu_domain.c | 2 +-
src/qemu/qemu_process.c | 2 +-
5 files changed, 37 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 01553b5..f1f0b56 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1719,6 +1719,39 @@ virDomainDiskSetSource(virDomainDiskDefPtr def, const char *src)
}
+void
+virDomainDiskEmptySource(virDomainDiskDefPtr def)
+{
+ virStorageSourcePtr src = def->src;
+
+ switch ((virStorageType) src->type) {
+ case VIR_STORAGE_TYPE_DIR:
+ case VIR_STORAGE_TYPE_FILE:
+ case VIR_STORAGE_TYPE_BLOCK:
+ VIR_FREE(src->path);
+ break;
+ case VIR_STORAGE_TYPE_NETWORK:
+ VIR_FREE(src->path);
+ VIR_FREE(src->volume);
+ virStorageNetHostDefFree(src->nhosts, src->hosts);
+ src->nhosts = 0;
+ src->hosts = NULL;
+ src->protocol = VIR_STORAGE_NET_PROTOCOL_NONE;
+ break;
+ case VIR_STORAGE_TYPE_VOLUME:
+ virStorageSourcePoolDefFree(src->srcpool);
+ src->srcpool = NULL;
+ break;
+ case VIR_STORAGE_TYPE_NONE:
+ case VIR_STORAGE_TYPE_LAST:
+ break;
+ }
+
+ virStorageSourceBackingStoreClear(src);
I think that instead of all the above you should just call
virStorageSourceClear(). It also clears the format of the volume,
seclabels, encryption data and all other stuff which is relevant only
for the image itself.
+ src->type = VIR_STORAGE_TYPE_FILE;
And then fix the type.
+}
+
+
const char *
virDomainDiskGetDriver(virDomainDiskDefPtr def)
{
ACK with the change above. I think this should be safe for freeze.
Peter
9:32 a.m.
New subject: [libvirt] [PATCH v2 2/2] Introduce and use virDomainDiskEmptySource
On 03/31/2017 04:17 PM, Peter Krempa wrote:
On Fri, Mar 31, 2017 at 16:02:14 +0200, Michal Privoznik wrote:
> Currently, if we want to zero out disk source (e,g, due to
> startupPolicy when starting up a domain) we use
> virDomainDiskSetSource(disk, NULL). This works well for file
> based storage (storage type file, dir, or block). But it doesn't
> work at all for other types like volume and network.
>
> So imagine that you have a domain that has a CDROM configured
> which source is a volume from an inactive pool. Because it is
> startupPolicy='optional', the CDROM is empty when the domain
> starts. However, the source element is not cleared out in the
> status XML and thus when the daemon restarts and tries to
> reconnect to the domain it refreshes the disks (which fails - the
> storage pool is still not running) and thus the domain is killed.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
>
> diff to v1:
> - Free more struct entries
> - Set disk type
> - Call the function less frequently
>
> src/conf/domain_conf.c | 33 +++++++++++++++++++++++++++++++++
> src/conf/domain_conf.h | 1 +
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_domain.c | 2 +-
> src/qemu/qemu_process.c | 2 +-
> 5 files changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 01553b5..f1f0b56 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1719,6 +1719,39 @@ virDomainDiskSetSource(virDomainDiskDefPtr def, const char
*src)
> }
>
>
> +void
> +virDomainDiskEmptySource(virDomainDiskDefPtr def)
> +{
> + virStorageSourcePtr src = def->src;
> +
> + switch ((virStorageType) src->type) {
> + case VIR_STORAGE_TYPE_DIR:
> + case VIR_STORAGE_TYPE_FILE:
> + case VIR_STORAGE_TYPE_BLOCK:
> + VIR_FREE(src->path);
> + break;
> + case VIR_STORAGE_TYPE_NETWORK:
> + VIR_FREE(src->path);
> + VIR_FREE(src->volume);
> + virStorageNetHostDefFree(src->nhosts, src->hosts);
> + src->nhosts = 0;
> + src->hosts = NULL;
> + src->protocol = VIR_STORAGE_NET_PROTOCOL_NONE;
> + break;
> + case VIR_STORAGE_TYPE_VOLUME:
> + virStorageSourcePoolDefFree(src->srcpool);
> + src->srcpool = NULL;
> + break;
> + case VIR_STORAGE_TYPE_NONE:
> + case VIR_STORAGE_TYPE_LAST:
> + break;
> + }
> +
> + virStorageSourceBackingStoreClear(src);
I think that instead of all the above you should just call
virStorageSourceClear(). It also clears the format of the volume,
seclabels, encryption data and all other stuff which is relevant only
for the image itself.
> + src->type = VIR_STORAGE_TYPE_FILE;
And then fix the type.
> +}
> +
> +
> const char *
> virDomainDiskGetDriver(virDomainDiskDefPtr def)
> {
ACK with the change above. I think this should be safe for freeze.
Yeah, I think too, but lets not push it. This is a very narrow use case
so our users will not suffer. I'll push it after the release. But thank
you anyway.
Michal
2823
days inactive
2823
days old
11 comments
3 participants
participants (3)
-
Martin Kletzander -
Michal Privoznik -
Peter Krempa