Revert "Error out on missing machine type in machine configs"

Message ID 56CF087F.9040404@redhat.com
State New
Headers show

Commit Message

Cole Robinson Feb. 25, 2016, 1:58 p.m.
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:



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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Comments

Cole Robinson Feb. 25, 2016, 2:13 p.m. | #1
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patch

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)))