Re: [Xen-devel] [PATCH v1 03/10] libxl: store a copy of vanilla domain configuration when creating domain

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, Jul 24, 2014 at 07:52:46PM +0100, Ian Jackson wrote:
> Wei Liu writes ("[PATCH v1 03/10] libxl: store a copy of vanilla domain 
> configuration when creating domain"):
> > This patch utilises new "libxl-json" file and stores a copy of user
> > supplied domain configuration in JSON form. This copy will be used as
> > template.
> > 
> > There're two save operations in total. The template config is first
> > saved right after the domain gets its UUID and domain id. Then after the
> > domain creation succeeds, the relevant bits touched by libxl are
> > synchronised to the stored config.
> 
> I think there is a potential race with domain destruction here:
> 
>     Task 1                                 Task 2
>     Creating the domain                    Trying to shut down
> 
>       actually create domain
>                                              observe domid
>                                              start domain destruction
>                                              delete all userdata
>                                              destroy domain
>       store the userdata
>         *** forbidden state created: userdata exists but domain doesn't
>         *** userdata has been leaked
>       [ would now bomb out ]
> 
> This race actually exists in libxl_userdata_store so it is my fault.
> Sorry!
> 

No worries. I can fix this.

> I think this should be fixed as follows:
> 
>   * domain destruction should take a lock across deleting the
>     userdata and destroying the domain in Xen
> 
>   * libxl_userdata_store should take take the lock,
>     check domain exists, store userdata, unlock lock.
> 
> This would be a separate lock to the one you introduce, I think.
> Your lock covers a lot of xenstore processing.  But perhaps we can
> reuse some of the fcntl and lockfile massaging stuff.
> 

Why do we need a second lock? The one I introduced in previous patch is
used to serialise access to user data. Isn't that just what we want in
this case?

> 
> > +    /* At this point we've got domid and UUID, store configuration */
> > +    ret = libxl__set_domain_configuration(gc, domid, &d_config_saved);
> 
> This libxl__set_domain_configuration is the only thing which creates
> the libxl-json userdata ?  If so I think there is no 
> 

-EPARSE. :-(

> > +    libxl_domain_config_dispose(&d_config_saved);
> > +    if (ret) {
> 
> Please use the idempotent error-handling style.  This dispose needs to
> be in the `error_out' section.  You will need to call _init at the top
> of the function.  I think there's a separate success exit path in this
> function so I think you do need two calls to dispose.
> 

Will fix this.

> > +static void update_domain_config(libxl__gc *gc,
> > +                                 libxl_domain_config *dst,
> > +                                 const libxl_domain_config *src)
> > +{
> 
> I think it would be a good idea to move this domain configuration
> update stuff into a file of its own rather than interleaving it with
> the (supposedly chronologically-ordered) domain creation logic.
> 

No problem.

> > +    /* update network interface information */
> > +    int i;
> > +
> > +    for (i = 0; i < src->num_nics; i++)
> > +        libxl__update_config_nic(gc, &dst->nics[i], &src->nics[i]);
> 
> Does creating the config early, and then updating it, not mean that
> there will be a window during domain creation when configuration
> exists but lacks these updates ?
> 
> I think that might be a (theoretically application-visible) bug.
> 

Yes, there's such windows.

Ian C suggested I add lock during creation, which I think should fix
this problem.

> > +/* update the saved domain configuration with a callback function,
> > + * which is responsible to pull in useful fields from src.
> > + */
> > +typedef void (update_callback)(libxl__gc *, libxl_domain_config *,
> > +                               const libxl_domain_config *);
> 
> libxl coding style has no ` ' before the `*'.
> 

Ack.

> > +static int libxl__update_domain_configuration(libxl__gc *gc,
> > +                                              uint32_t domid,
> > +                                              update_callback callback,
> > +                                              const libxl_domain_config 
> > *src)
> ...
> > +    libxl_domain_config_init(&d_config_saved);
> > +
> > +    rc = libxl__get_domain_configuration(gc, domid, &d_config_saved);
> > +
> > +    if (rc) {
> 
> Spurious blank line.
> 

Fixed.

> > +        LOG(ERROR, "cannot get domain configuration: %d", rc);
> 
> Doesn't libxl__get_domain_configuration log a message already ?  I
> think so, in which case it's probably not ideal to log again.
> 

Yes. I agree.

> To help with (a) not spewing lots of messages for a single error and
> (b) writing code uncluttered by unnecessary logging calls, it can be
> helpful to mention in the doc comments for functions whether they log
> failures.
> 
> If they do you can replace this whole block with a one-line
>        if (rc) goto out;
> 
> > +    callback(gc, &d_config_saved, src);
> 
> Callback should probably have the ability to fail.
> 

I guess you mean I should write

   if (callback(XXX))

This of course can be done.

> > +    rc = libxl__set_domain_configuration(gc, domid, &d_config_saved);
> > +
> > +    if (rc)
> > +        LOG(ERROR, "cannot set domain configuration: %d", rc);
> 
> 1. Spurious blank line.  2. See above re logging.   3. This falling
> through into the end of the function is unpleasant - please make
> things regular.
> 
> The end of most functions should have something like:
>        rc = 0;
>     out:
> 
> > +    libxl_domain_config_dispose(&d_config_saved);
> > +
> > +out:
> > +    return rc;
> 
> This seems to leak d_config_saved on error paths.
> 

Will fix this.

> > @@ -1354,6 +1424,12 @@ 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) {
> > +        rc = libxl__update_domain_configuration(gc, dcs->guest_domid,
> > +                                                update_domain_config,
> > +                                                d_config);
> > +    }
> 
> Oh dear.  I see this function already has a mad error handling style
> which you are following.  I won't ask you to fix it.
> 

Luckily it's very short at the moment.

Wei.

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