Re: Regression in xen-unstable due to commit 3802ecbaa9eb36

From: Wei Liu <wei.liu2@citrix.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Juergen Gross <jgross@suse.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	Wei Liu <wei.liu2@citrix.com>, Olaf Hering <ohering@suse.com>,
	xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: Regression in xen-unstable due to commit 3802ecbaa9eb36
Date: Thu, 16 May 2019 13:42:30 +0100	[thread overview]
Message-ID: <20190516124230.GC2798@zion.uk.xensource.com> (raw)
In-Reply-To: <20190516114255.7t6re7yot6wp3mom@Air-de-Roger>

On Thu, May 16, 2019 at 01:42:55PM +0200, Roger Pau Monné wrote:
> On Thu, May 16, 2019 at 12:24:50PM +0100, Wei Liu wrote:
> > On Thu, May 16, 2019 at 12:57:35PM +0200, Olaf Hering wrote:
> > > Am Thu, 16 May 2019 12:45:40 +0200
> > > schrieb Roger Pau Monné <roger.pau@citrix.com>:
> > > 
> > > > Having a field in build_info with a default value that depends on
> > > > fields outside of build_info is problematic, since not all callers of
> > > > libxl__domain_build_info_setdefault have access to libxl_domain_config.
> > > 
> > > One option would be a new API that gets a libxl_domain_config and which
> > > calls libxl__domain_set_device_model, libxl__domain_create_info_setdefault
> > > and libxl__domain_build_info_setdefault. To me it looks like create_domain
> > > can not build a proper d_config all on its own, it just has not enough info.
> > 
> > If you're talking about adding a new _public_ API:
> > 
> > The problem with this approach is that it doesn't help existing libxl
> > users. They will need to be fixed by calling this new API.
> > 
> > Will it work if 1) you make libxl__domain_set_device_model idempotent
> > and 2) call it from within libxl__domain_build_info_setdefault (which
> > basically restores the original code path before your patch)?
> 
> Calling libxl__domain_set_device_model from
> libxl__domain_build_info_setdefault would require passing
> domain_config to libxl__domain_build_info_setdefault, which gets back
> to my proposal.
> 

Oh I see what you were getting at.

I have merely been looking at the one patch that introduced this
regression which didn't use any field in d_config. But in the subsequent
patch d_config was used.

> In order to know if a PV or PVH domain requires a device model (for
> the PV backends) libxl needs to look at the contents of domain_config
> because that's where the list of devices is stored.
> 
> Another option would be to differentiate between device model and PV
> backend. In the PV/PVH case QEMU is not acting as a device model but
> rather as a PV backend, hence it could be signaled using a different
> field, that could live in domain_config instead of
> domain_build_info?

I think this is generally a good idea. Not sure how much code churn is
going to be though.

Wei.

> 
> Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel