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

[Ján Tomko]

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