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

[Ian Jackson <Ian.Jackson@eu.citrix.com>]

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!

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.


> +    /* 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 

> +    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.

> +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.

> +    /* 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.

> +/* 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 `*'.

> +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.

> +        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.

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.

> +    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.

> @@ -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.

> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index e9d93ac..72d21ad 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -3240,6 +3240,20 @@ int libxl__lock_domain_configuration(libxl__gc *gc, uint32_t domid,
>  int libxl__unlock_domain_configuration(libxl__gc *gc, uint32_t domid,
>                                         int *fd_lock);
>  
> +static inline void libxl__update_config_nic(libxl__gc *gc,
> +                                            libxl_device_nic *dst,
> +                                            libxl_device_nic *src)
> +{
> +    libxl_mac_copy(CTX, &dst->mac, &src->mac);
> +}

Nice that this is so short...

Thanks,
Ian.