|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v9 06/17] libxl: convert libxl__device_disk_local_attach to an async op
Ian Jackson wrote:
> 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 ?
None, I've removed the "Detached" state.
>> + */
>> +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.
Ok, I've removed the libxl__device_disk_local_init function and filled
the struct in the caller's function.
>> +/* Make a disk available in this (the control) domain. Always calls
>> + * dls->callback when finished.
>> + * State Idle -> Attached
>> + */
>
> Surely state Idle -> Attaching.
Fixed
>> +_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.
Yes, this is what the previous code did (I'm not saying it is good or bad).
>> +/* 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. */
>
Fixed.
>
>
>> 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.
This is part of the original code, and I don't think it's a good idea to
add more patches to my series, if we want to get them in for 4.2.
>> +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:
Done.
>> /*
>> * 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.
This is done in the same part of code as it was previously done (in the
past the call was inside bootloader_cleanup, which was the first
function bootloader_callback calls).
Since bootloader_callback is called after bootloader_finished, and that
gets caller when the bootloader process dies, I'm quite sure the
bootloader has already finished at this point.
>> +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.
I've changed that so the free is done inside the callback of
libxl__device_disk_local_initiate_detach.
>> +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.
Ok, I agree this is not optimal, but I would also argue that it has the
same behaviour as the previous code. I will fix this by calling the
detach function in the attach callback if the attach failed.
> 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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |