[libvirt] [PATCH] Revert "Error out on missing machine type in machine configs"
Revert commit 55e6d8cd9eac7eb2aaa4d221585e9402cf7269d5. It unconditionally required a machine type for all machine types even though qemu is the only emulator using them. Reverting this re-introduces the crash when someone fiddles with libvirt's machine configs with /etc/, but fixes persistent domains for other drivers. Breaks https://bugzilla.redhat.com/show_bug.cgi?id=1267256 again. --- A proper fix would be too invasive for the freeze, we would either * have to revert commit f1a89a8 which relaxed the requirement to execute the emulator * or (partially) revert commits a8b628e and 3d92a00 which reintroduced them to post parse callbacks and keep track of which parser requires the machine type in virDomainDefParserConfig. src/conf/domain_conf.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3b15cb4..79758d4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14851,12 +14851,6 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; } VIR_FREE(capsdata); - } else { - if (!def->os.machine) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Missing machine type")); - goto error; - } } /* Extract domain name */ -- 2.4.10
On 02/25/2016 07:24 AM, Ján Tomko wrote:
Revert commit 55e6d8cd9eac7eb2aaa4d221585e9402cf7269d5.
It unconditionally required a machine type for all machine types even though qemu is the only emulator using them.
Reverting this re-introduces the crash when someone fiddles with libvirt's machine configs with /etc/, but fixes persistent domains for other drivers.
Breaks https://bugzilla.redhat.com/show_bug.cgi?id=1267256 again. --- A proper fix would be too invasive for the freeze, we would either * have to revert commit f1a89a8 which relaxed the requirement to execute the emulator * or (partially) revert commits a8b628e and 3d92a00 which reintroduced them to post parse callbacks and keep track of which parser requires the machine type in virDomainDefParserConfig.
src/conf/domain_conf.c | 6 ------ 1 file changed, 6 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3b15cb4..79758d4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14851,12 +14851,6 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; } VIR_FREE(capsdata); - } else { - if (!def->os.machine) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Missing machine type")); - goto error; - } }
/* Extract domain name */
Why not just move that check to the beginning of the qemu specific qemuDomainDefPostParse? And add a test case that hits the error - Cole
On Thu, Feb 25, 2016 at 07:45:17AM -0500, Cole Robinson wrote:
On 02/25/2016 07:24 AM, Ján Tomko wrote:
Revert commit 55e6d8cd9eac7eb2aaa4d221585e9402cf7269d5.
It unconditionally required a machine type for all machine types even though qemu is the only emulator using them.
Reverting this re-introduces the crash when someone fiddles with libvirt's machine configs with /etc/, but fixes persistent domains for other drivers.
Breaks https://bugzilla.redhat.com/show_bug.cgi?id=1267256 again. --- A proper fix would be too invasive for the freeze, we would either * have to revert commit f1a89a8 which relaxed the requirement to execute the emulator * or (partially) revert commits a8b628e and 3d92a00 which reintroduced them to post parse callbacks and keep track of which parser requires the machine type in virDomainDefParserConfig.
src/conf/domain_conf.c | 6 ------ 1 file changed, 6 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3b15cb4..79758d4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14851,12 +14851,6 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; } VIR_FREE(capsdata); - } else { - if (!def->os.machine) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Missing machine type")); - goto error; - } }
/* Extract domain name */
Why not just move that check to the beginning of the qemu specific qemuDomainDefPostParse? And add a test case that hits the error
That is basically the second option I suggested and this patch is a prerequisite for that. Jan
On 02/25/2016 08:50 AM, Ján Tomko wrote:
On Thu, Feb 25, 2016 at 07:45:17AM -0500, Cole Robinson wrote:
On 02/25/2016 07:24 AM, Ján Tomko wrote:
Revert commit 55e6d8cd9eac7eb2aaa4d221585e9402cf7269d5.
It unconditionally required a machine type for all machine types even though qemu is the only emulator using them.
Reverting this re-introduces the crash when someone fiddles with libvirt's machine configs with /etc/, but fixes persistent domains for other drivers.
Breaks https://bugzilla.redhat.com/show_bug.cgi?id=1267256 again. --- A proper fix would be too invasive for the freeze, we would either * have to revert commit f1a89a8 which relaxed the requirement to execute the emulator * or (partially) revert commits a8b628e and 3d92a00 which reintroduced them to post parse callbacks and keep track of which parser requires the machine type in virDomainDefParserConfig.
src/conf/domain_conf.c | 6 ------ 1 file changed, 6 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3b15cb4..79758d4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14851,12 +14851,6 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; } VIR_FREE(capsdata); - } else { - if (!def->os.machine) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Missing machine type")); - goto error; - } }
/* Extract domain name */
Why not just move that check to the beginning of the qemu specific qemuDomainDefPostParse? And add a test case that hits the error
That is basically the second option I suggested and this patch is a prerequisite for that.
I don't follow... that option mentions reverting patches and you called it invasive. I'm thinking this patch plus: diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c56f9f1..610ffdd 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1307,6 +1307,12 @@ qemuDomainDefPostParse(virDomainDefPtr def, return ret; } + if (!def->os.machine) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing machine type")); + goto error; + } + /* check for emulator and create a default one if needed */ if (!def->emulator && !(def->emulator = virDomainDefGetDefaultEmulator(def, caps))) That should fix the original bug, and the lxc issue John reported, and seems safe for the next release IMO. Or am I missing something and that solution isn't sufficient? Thanks, Cole
On Thu, Feb 25, 2016 at 08:58:23AM -0500, Cole Robinson wrote:
On 02/25/2016 08:50 AM, Ján Tomko wrote:
On Thu, Feb 25, 2016 at 07:45:17AM -0500, Cole Robinson wrote:
On 02/25/2016 07:24 AM, Ján Tomko wrote:
Revert commit 55e6d8cd9eac7eb2aaa4d221585e9402cf7269d5.
It unconditionally required a machine type for all machine types even though qemu is the only emulator using them.
Reverting this re-introduces the crash when someone fiddles with libvirt's machine configs with /etc/, but fixes persistent domains for other drivers.
Breaks https://bugzilla.redhat.com/show_bug.cgi?id=1267256 again. --- A proper fix would be too invasive for the freeze, we would either * have to revert commit f1a89a8 which relaxed the requirement to execute the emulator * or (partially) revert commits a8b628e and 3d92a00 which reintroduced them to post parse callbacks and keep track of which parser requires the machine type in virDomainDefParserConfig.
src/conf/domain_conf.c | 6 ------ 1 file changed, 6 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3b15cb4..79758d4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14851,12 +14851,6 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; } VIR_FREE(capsdata); - } else { - if (!def->os.machine) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Missing machine type")); - goto error; - } }
/* Extract domain name */
Why not just move that check to the beginning of the qemu specific qemuDomainDefPostParse? And add a test case that hits the error
That is basically the second option I suggested and this patch is a prerequisite for that.
I don't follow... that option mentions reverting patches and you called it invasive. I'm thinking this patch plus:
Right, VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS not being useful for qemu driver is an unrelated issue.
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c56f9f1..610ffdd 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1307,6 +1307,12 @@ qemuDomainDefPostParse(virDomainDefPtr def, return ret; }
+ if (!def->os.machine) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing machine type")); + goto error; + } + /* check for emulator and create a default one if needed */ if (!def->emulator && !(def->emulator = virDomainDefGetDefaultEmulator(def, caps)))
That should fix the original bug, and the lxc issue John reported, and seems safe for the next release IMO.
I'll send v2 series with tests. Jan
On 02/25/2016 09:10 AM, Ján Tomko wrote:
On Thu, Feb 25, 2016 at 08:58:23AM -0500, Cole Robinson wrote:
On 02/25/2016 08:50 AM, Ján Tomko wrote:
On Thu, Feb 25, 2016 at 07:45:17AM -0500, Cole Robinson wrote:
On 02/25/2016 07:24 AM, Ján Tomko wrote:
Revert commit 55e6d8cd9eac7eb2aaa4d221585e9402cf7269d5.
It unconditionally required a machine type for all machine types even though qemu is the only emulator using them.
Reverting this re-introduces the crash when someone fiddles with libvirt's machine configs with /etc/, but fixes persistent domains for other drivers.
Breaks https://bugzilla.redhat.com/show_bug.cgi?id=1267256 again. --- A proper fix would be too invasive for the freeze, we would either * have to revert commit f1a89a8 which relaxed the requirement to execute the emulator * or (partially) revert commits a8b628e and 3d92a00 which reintroduced them to post parse callbacks and keep track of which parser requires the machine type in virDomainDefParserConfig.
src/conf/domain_conf.c | 6 ------ 1 file changed, 6 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3b15cb4..79758d4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14851,12 +14851,6 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; } VIR_FREE(capsdata); - } else { - if (!def->os.machine) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Missing machine type")); - goto error; - } }
/* Extract domain name */
Why not just move that check to the beginning of the qemu specific qemuDomainDefPostParse? And add a test case that hits the error
That is basically the second option I suggested and this patch is a prerequisite for that.
I don't follow... that option mentions reverting patches and you called it invasive. I'm thinking this patch plus:
Right, VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS not being useful for qemu driver is an unrelated issue.
Oh I see now, you meant proper fix for the broader problem that qemuDomainDefPostParse mandates certain bits that we attempt to skip with VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS . Gotchya - Cole
participants (2)
-
Cole Robinson -
Ján Tomko