[libvirt] [PATCH] Revert "Error out on missing machine type in machine configs"

[Cole Robinson]

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