Re: [PATCH v3 5/5] libxl: add options to enable/disable emulated devices

[Wei Liu <wei.liu2@citrix.com>]

On Thu, Jan 21, 2016 at 11:01:43AM +0100, Roger Pau Monné wrote:
> El 21/01/16 a les 10.39, Ian Campbell ha escrit:
> > On Wed, 2016-01-20 at 19:33 +0100, Roger Pau Monné wrote:
> >> El 20/01/16 a les 14.01, Ian Campbell ha escrit:
> >>> On Wed, 2016-01-20 at 12:57 +0100, Roger Pau Monne wrote:
> >>>> Allow enabling or disabling emulated devices from the libxl domain
> >>>> configuration file. For HVM guests with a device model all the
> >>>> emulated
> >>>> devices are enabled. For HVM guests without a device model no devices
> >>>> are
> >>>> enabled by default, although they can be enabled using the options
> >>>> provided.
> >>>> The arbiter of whether a combination is posible or not is always Xen,
> >>>> libxl
> >>>> doesn't do any kind of check.
> >>>>
> >>>> This set of options is also propagated inside of the libxl migration
> >>>> record
> >>>> as part of the contents of the libxl_domain_build_info struct.
> >>>
> >>> ... and this is the real motivation for this change, not actually
> >>> allowing
> >>> users to control all this AIUI.
> >>>
> >>> Did you check that the fields updated using libxl_defbool_setdefault
> >>> are
> >>> actually updated in the JSON copy and therefore propagated to the other
> >>> side of a migration as specific values and not as "pick a default"? I
> >>> think
> >>> we don't want these changing on migration. I think/hope all this was
> >>> automatically handled by the work Wei did in the last release cycle.
> >>
> >> No, values populated by the {build/create}_info_setdefault functions are
> >> not propagated, OTOH values manually set by the user in the config file
> >> are indeed propagated. Do we have any guarantee that _setdefault is
> >> always going to behave in the same way?
> > 
> > No, part of the purpose of defbool and the other "do the default" values is
> > that we can evolve things over time.
> >
> >> If we don't have that guarantee I think this is already a bug, and we
> >> should call _setdefault before sending the domain info to the other end.
> >> In fact I have a patch that does exactly that, but I'm unsure if it's
> >> needed because I don't know the policy regarding default values in libxl.
> > 
> > Wei, isn't this (turning the defaults into concrete values) supposed to be
> > taken care of by the JSON mangling which you added?
> 
> Heh, I think you mean the JSON mangling added by Wei. In order to 
> propagate the values filled by default in libxl_domain_config I had to 
> add the following patch, which basically calls the _setdefault 
> functions before converting the domain_config into JSON. I'm planning 
> to make it part of this series in the next iteration:

The requirement of recording decision made in libxl and pass that to the
receiving end is not new. We had the same problem for uuid, disk and
some other things.

The first way of doing it is to update JSON before it is sent -- see
libxl.c:libxl_retrieve_domain_configuration. It uses the stashed JSON
file as template and pull in various bits from hypervisor and xenstore.
Your need of recording what emulated devices are available fits here.
You just need to provide a way to retrieve those bits in that function.

Another way of doing it is to update the stashed JSON template before it
is saved. See libxl_internal.c:libxl__update_domain_configuration. I
think this might be easier than the first way of doing it.

You should not export the _setdefault function to xl because it is a
layer violation.

Hope this clarify things.

Wei.