Re: [Xen-devel] [PATCH v9 06/17] libxl: convert libxl__device_disk_local_attach to an async op

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Roger Pau Monne writes ("[PATCH v9 06/17] libxl: convert 
libxl__device_disk_local_attach to an async op"):
> This will be needed in future patches, when libxl__device_disk_add
> becomes async also. Create a new status structure that defines the
> local attach of a disk device and use it in
> libxl__device_disk_local_attach.

Thanks.


> +/*----- local disk attach: attach a disk locally to run the bootloader 
> -----*/
> +
> +typedef struct libxl__disk_local_state libxl__disk_local_state;
> +typedef void libxl__disk_local_state_callback(libxl__egc*,
> +                                              libxl__disk_local_state*,
> +                                              int rc);
> +

> +/* A libxl__disk_local_state may be in the following states:
> + * Undefined, Idle, Attaching, Attached, Detaching, Detached.

What is the difference between Detached and Idle ?

> + */
> +struct libxl__disk_local_state {
> +    /* filled by the user */
> +    libxl__ao *ao;
> +    const libxl_device_disk *in_disk;
> +    libxl_device_disk disk;
> +    const char *blkdev_start;
> +    libxl__disk_local_state_callback *callback;
> +    /* filled by libxl__device_disk_local_initiate_attach */
> +    char *diskpath;
> +    /* private for implementation of local detach */
> +    libxl__ao_device aodev;
> +};
> +
> +/* Initializes the state of a libxl__disk_local_state to init,
                                                            Idle
> + * can be called multiple times.
> + * State Undefined -> Idle
> + */
> +_hidden void libxl__device_disk_local_init(libxl__disk_local_state *dls,
> +                                           libxl__ao *ao,
> +                                           libxl_device_disk *disk,
> +                                           const char *blkdev_start,
> +                                           libxl__disk_local_state_callback
> +                                           *callback);

I don't think an _init function should take an ao and a bunch of
parameters to fill in like that.  Mostly elsewhere init functions just
make sure that attempts to free are safe no-ops.  IMO callers should
fill in themselves the fields which the doc comment says they should
fill in themselves.

> +/* Make a disk available in this (the control) domain. Always calls
> + * dls->callback when finished.
> + * State Idle -> Attached
> + */

Surely state Idle -> Attaching.

> +_hidden void libxl__device_disk_local_initiate_attach(libxl__egc *egc,
> +                                                libxl__disk_local_state 
> *dls);

You need to say what the state is on entry to the callback.  The
sensible thing would be:
   /* State on entry to the callback is:
    *    Attached if rc==0
    *    Idle if rc!=0
    */
but I infer from your user in libxl_bootloader.c that if rc!=0 the dls
is left in some kind of half-attached error state which isn't
documented above.

> +/* Disconnects a disk device form the control domain. If the passed
> + * dls is not attached (or has already been detached),
> + * libxl__device_disk_local_initiate_detach will just call the callback
> + * directly.
> + * State Idle/Attached -> Idle
> + */
> +_hidden void libxl__device_disk_local_initiate_detach(libxl__egc *egc,
> +                                                libxl__disk_local_state 
> *dls);
> +

You mean state Idle/Attached -> Detaching, and
    /* State on entry to the callback is Idle. */
    
Ian.


> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
...
> @@ -2191,6 +2202,7 @@ char * libxl__device_disk_local_attach(libxl__gc *gc,
>              default:
>                  LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
>                             "unrecognized disk format: %d", disk->format);
> +                rc = ERROR_FAIL;
>                  break;

Why `break' and not `goto out' ?  (Here and later.)

Perhaps this would be clear from context.

> +void libxl__device_disk_local_initiate_detach(libxl__egc *egc,
> +                                     libxl__disk_local_state *dls)
...
>      switch (disk->backend) {
>          case LIBXL_DISK_BACKEND_QDISK:
>              if (disk->vdev != NULL) {
> -                libxl_device_disk_remove(gc->owner, LIBXL_TOOLSTACK_DOMID,
...
> +                return;
>              }
> -            break;
>          default:

This deserves:
  +            /* disk->vdev == NULL; fall through */
           default:
or something.  Particularly since the next comment is then not true,
or at least not applicable:

>              /*
>               * Nothing to do for PHYSTYPE_PHY.
>               * For other device types assume that the blktap2 process is
>               * needed by the soon to be started domain and do nothing.
>               */
> -            break;
> +            dls->callback(egc, dls, rc);



> -    if (bl->diskpath) {
> -        libxl__device_disk_local_detach(gc, &bl->localdisk);
> -        free(bl->diskpath);
> -        bl->diskpath = 0;
> -    }
>      libxl__domaindeathcheck_stop(gc,&bl->deathcheck);
>      libxl__datacopier_kill(&bl->keystrokes);
>      libxl__datacopier_kill(&bl->display);
> @@ -249,10 +245,35 @@ static void bootloader_setpaths(libxl__gc *gc, 
> libxl__bootloader_state *bl)
>      bl->outputpath = GCSPRINTF(XEN_RUN_DIR "/bootloader.%"PRIu32".out", 
> domid);
>  }
> 
> +/* Callbacks */
> +
> +static void bootloader_finished_cb(libxl__egc *egc,
> +                                   libxl__disk_local_state *dls,
> +                                   int rc);
> +
>  static void bootloader_callback(libxl__egc *egc, libxl__bootloader_state *bl,
>                                  int rc)
>  {
>      bootloader_cleanup(egc, bl);
> +
> +    bl->dls.callback = bootloader_finished_cb;
> +    libxl__device_disk_local_initiate_detach(egc, &bl->dls);
> +}

This detaches the disk while we're still using it, AFAICT.
The dls needs to survive completion of the bootloader.

> +static void bootloader_finished_cb(libxl__egc *egc,
> +                                   libxl__disk_local_state *dls,
> +                                   int rc)
> +{
> +    STATE_AO_GC(dls->ao);
> +    libxl__bootloader_state *bl = CONTAINER_OF(dls, *bl, dls);
> +
> +    free(bl->dls.diskpath);
> +    bl->dls.diskpath = 0;

Surely that isn't right ?  We should let the local detach machinery
free this.  The doc comment in the dls struct definition doesn't say
we're to mess with it like this.

> +static void bootloader_disk_attached_cb(libxl__egc *egc,
> +                                        libxl__disk_local_state *dls,
> +                                        int rc)
> +{
> +    STATE_AO_GC(dls->ao);
> +    libxl__bootloader_state *bl = CONTAINER_OF(dls, *bl, dls);
> +    const libxl_domain_build_info *info = bl->info;
> +    const char *bootloader;
> +
> +    if (rc) {
> +        LOG(ERROR, "failed to attach local disk for bootloader execution");
> +        dls->callback = bootloader_disk_failed_cb;
> +        libxl__device_disk_local_initiate_detach(egc, dls);
> +        return;

So in line with my comments about the possible states of a dls, I
think this recovery should be done by the local attach machinery, not
left to the caller like this.

If you do want to insist that if the attach callback is given an error
you need to give a name to the implied Error state of the dls.

If you do as I suggest you can do away with the separate
bootloader_disk_failed_cb; it becomes buried in the attach/detach
machinery which probably doesn't even need a new function for it.


Thanks,
Ian.

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