Re: [Xen-devel] [PATCH V5 28/32] libxl: store up-to-date domain configuration as we create domain

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, May 20, 2014 at 03:12:18PM +0100, Ian Campbell wrote:
> On Tue, 2014-05-13 at 22:54 +0100, Wei Liu wrote:
> > This patch utilizes "libxl-json" format and helper functions introduced
> > in previous patch to store up-to-do domain configuration when creating a
> > domain.
> > 
> > Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> 
> This seems to end up saving the domain config 2 or even 3 times. I think
> we need an explanation as to why that is.
> 

Twice (see below).

> Also we need to know what the invariants are, what must (not) change
> either before or after each sequencing point, what must be in each
> update etc.
> 

The first save is the vanilla copy, that's what the user provides.

Then udpate_domain_config is called to pull in everything that may get
changed by libxl to the vanilla copy. What's pulled in is documented in
update_domain_config.

The second save saves the updated version.

> Can you not save the raw version at the start (before anything has
> touched it) and then do the domid, uuid, mac, vtpm etc all at the end in
> domcreate_complete? (Don't you do the uuid twice? The comments mention
> it twice)
>  

I don't follow. I think this is what being done at the moment. The
vanilla domain configuration is saved at the beginning of domain
creation then updated when domain creation completes. It's exactly what
you suggested.

Re UUID, no. Domain UUID and UUID for devices are different things.

> > +static void update_domain_config(libxl_ctx *ctx,
> 
> Internal functions should take a libxl__gc not a ctx.
> 

OK.

> > @@ -1366,6 +1400,25 @@ static void domcreate_complete(libxl__egc *egc,
> >      if (!rc && d_config->b_info.exec_ssidref)
> >          rc = xc_flask_relabel_domain(CTX->xch, dcs->guest_domid, 
> > d_config->b_info.exec_ssidref);
> >  
> > +    if (!rc) {
> > +        libxl_domain_config d_config_saved;
> > +
> > +        libxl_domain_config_init(&d_config_saved);
> > +        rc = libxl_load_domain_configuration(CTX, dcs->guest_domid,
> > +                                             &d_config_saved);
> > +        if (rc) {
> > +            LOG(ERROR, "cannot load domain configuration: %d", rc);
> > +            goto out;
> > +        }
> > +        update_domain_config(CTX, &d_config_saved, d_config);
> > +        rc = libxl_store_domain_configuration(CTX, dcs->guest_domid,
> > +                                              &d_config_saved);
> > +        if (rc)
> > +            LOG(ERROR, "cannot store domain configuration: %d", rc);
> > +        libxl_domain_config_dispose(&d_config_saved);
> 
> Unless you are sure this is the only place this will be needed A helper
> of the form libxl__domain_config_update(gc, &update_domain_config, data)
> might be useful. i.e. read, call a callback, write. data is a void *
> passed to the update_domain_config callback.
> 

This form might be useful in later patch as well. I will see what I can
do.

Wei.

> Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel