Re: [PATCH] fix restore: xenstore entries left when restore failed

[Ian Campbell <Ian.Campbell@citrix.com>]

On Wed, 2014-11-19 at 15:12 +0800, Chunyan Liu wrote:
> While running libvirt-tck domain/102-broken-save-restore.t
> test (save domain, corrupt saved file by truncate the last 512k,
> then restore), found that restore domain failed, but domain
> related xenstore entries still exist in xenstore.
> 
> Add a patch to clear xenstore entries in this case.
> 
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> ---
>  tools/libxl/libxl.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index de23fec..447840d 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1525,6 +1525,54 @@ static void devices_destroy_cb(libxl__egc *egc,
>                                 libxl__devices_remove_state *drs,
>                                 int rc);
>  
> +static void libxl_clear_xs_entry(libxl__gc *gc, uint32_t domid)

This seems to duplicate a bunch of stuff already done by
libxl__destroy_domid and libxl__devices_destroy etc. I think rather than
duplicating this libxl__destroy_domid should be made to Do The Right
Thing by trying to clean up any remnants of the domid even if it doesn't
currently exist.

Alternatively perhaps the real bug is in the error path of the restore
functionality, which isn't calling the correct unwind path. Ian, any
thoughts?

Ian.

> +    const char *dom_path, *vm_path;
> +    char *path;
> +    unsigned int num_kinds, num_dev_xsentries;
> +    char **kinds = NULL, **devs = NULL;
> +    int i, j;
> +
> +    /* remove libxl path */
> +    libxl__xs_rm_checked(gc, XBT_NULL, libxl__xs_libxl_path(gc, domid));
> +
> +    dom_path = libxl__xs_get_dompath(gc, domid);
> +    if (!dom_path)
> +        return;
> +
> +    /* remove backend entries */
> +    path = GCSPRINTF("%s/device", dom_path);
> +    kinds = libxl__xs_directory(gc, XBT_NULL, path, &num_kinds);
> +    if (kinds && num_kinds) {
> +        for (i = 0; i < num_kinds; i++) {
> +            path = GCSPRINTF("%s/device/%s", dom_path, kinds[i]);
> +            devs = libxl__xs_directory(gc, XBT_NULL, path, &num_dev_xsentries);
> +            if (!devs)
> +                continue;
> +            for (j = 0; j < num_dev_xsentries; j++) {
> +                path = GCSPRINTF("%s/device/%s/%s/backend",
> +                                 dom_path, kinds[i], devs[j]);
> +                path = libxl__xs_read(gc, XBT_NULL, path);
> +                if (path)
> +                    libxl__xs_rm_checked(gc, XBT_NULL, path);
> +            }
> +        }
> +    }
> +
> +    path = GCSPRINTF("%s/console/backend", dom_path);
> +    path = libxl__xs_read(gc, XBT_NULL, path);
> +    if (path)
> +        libxl__xs_rm_checked(gc, XBT_NULL, path);
> +
> +    /* remove vm path */
> +    vm_path = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/vm", dom_path));
> +    if (vm_path)
> +        libxl__xs_rm_checked(gc, XBT_NULL, vm_path);
> +
> +    /* remove dom path */
> +    libxl__xs_rm_checked(gc, XBT_NULL, dom_path);
> +}
> +
>  void libxl__destroy_domid(libxl__egc *egc, libxl__destroy_domid_state *dis)
>  {
>      STATE_AO_GC(dis->ao);
> @@ -1540,6 +1588,10 @@ void libxl__destroy_domid(libxl__egc *egc, libxl__destroy_domid_state *dis)
>          break;
>      case ERROR_INVAL:
>          LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "non-existant domain %d", domid);
> +        /* domain may not started successfully but some xenstore entries
> +         * might be created already in earlier stage. We need to clear
> +         * those entries. */
> +        libxl_clear_xs_entry(gc, domid);
>      default:
>          goto out;
>      }