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 */

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.

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.