|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 05/15] libxl: change libxl__ao_device_remove to libxl__ao_device
Roger Pau Monne writes ("[PATCH v2 05/15] libxl: change libxl__ao_device_remove
to libxl__ao_device"):
> Introduce a new structure to track state of device backends, that will
> be used in following patches on this series.
>
> This structure if used for both device creation and device
> destruction and removes libxl__ao_device_remove.
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index ae5527b..b62110a 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1802,6 +1795,44 @@ _hidden void
> libxl__bootloader_init(libxl__bootloader_state *bl);
...
> +struct libxl__ao_device {
> + /* filled in by user */
> + libxl__ao *ao;
> + /* action being performed */
> + libxl__device_action action;
> + libxl__device *dev;
> + int force;
> + libxl__device_callback *callback;
> + /* private for implementation */
> + /* State in which the device is */
> + int active;
> + int rc;
> + libxl__ev_devstate ds;
> + void *base;
> +};
Re your comment, don't you mean "state of the operation" rather than
"state of the device" ?
I would prefer some reformatting to make the scope of comments like
"filled in by user" clearer. Perhaps a blank line before "private for
implementation". Or perhaps moving all the small comments to after
the variables.
TBH I'm not sure what the comment "action being performed" is for.
After all the variable is called "action" so it's obvious.
> +/* Arranges that dev will be removed to the guest, and the
> + * hotplug scripts will be executed (if necessary). When
> + * this is done (or an error happens), the callback in
> + * aorm->callback will be called.
> + */
> +_hidden void libxl__initiate_device_remove(libxl__egc *egc,
> + libxl__ao_device *aorm);
> +
Does this really only do removals ? If so where is the addition
function and what is the "action" parameter for ?
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 2a09192..b13de4f 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1271,6 +1271,26 @@ int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t
> domid, int autopass)
>
>
> /******************************************************************************/
>
> +/* generic callback for devices that only need to set ao_complete */
> +static void libxl__device_cb(libxl__egc *egc, libxl__ao_device *aorm)
Maybe this should have a more descriptive name ?
device_addrm_aocomplete
depending on whether it's for removal only or for addition too.
You are still using the parameter name "aorm" for something which is
not only a removal operation. "aodev" or something.
> int libxl_device_nic_destroy(libxl_ctx *ctx, uint32_t domid,
> - libxl_device_nic *nic)
> + libxl_device_nic *nic,
> + const libxl_asyncop_how *ao_how)
> {
> - GC_INIT(ctx);
> - libxl__device device;
> + AO_CREATE(ctx, domid, ao_how);
> + libxl__device *device;
> + libxl__ao_device *aorm;
> int rc;
>
> - rc = libxl__device_from_nic(gc, domid, nic, &device);
> + GCNEW(device);
> + rc = libxl__device_from_nic(gc, domid, nic, device);
> if (rc != 0) goto out;
>
> - rc = libxl__device_destroy(gc, &device);
> + GCNEW(aorm);
> + libxl__init_ao_device(aorm, ao, NULL);
> + aorm->action = DEVICE_DISCONNECT;
> + aorm->force = 1;
> + aorm->dev = device;
> + aorm->callback = libxl__device_cb;
> + libxl__initiate_device_remove(egc, aorm);
> +
Is there some way to make these functions less repetitive ?
We have {nic,disk,vkb,vfb}_{remove,destroy} all of which have
essentially identical innards.
I have some suggestions, in descending order of my own preference.
How about writing the common code once in a macro:
1 #define DEFINE_DEVICE_REMOVE(type, removedestroy, force) \
1 int libxl_device_##type##_##removedestroy(libxl_ctx *ctx, \
1 uint32_t domid, libxl_device_##type *nic, \
1 const libxl_asyncop_how *ao_how) \
1 { \
1 AO_CREATE etc. etc. \
1 rc = libxl__device_from_##type( etc. etc. ) \
1 etc. etc. \
1 }
8 DEFINE_DEVICE_REMOVE(nic, remove, 0)
. DEFINE_DEVICE_REMOVE(nic, destroy, 1)
. DEFINE_DEVICE_REMOVE(disk, remove, 0)
. DEFINE_DEVICE_REMOVE(disk, destroy, 1)
. DEFINE_DEVICE_REMOVE(vkb, remove, 0)
. DEFINE_DEVICE_REMOVE(vkb, destroy, 1)
. DEFINE_DEVICE_REMOVE(vfb, remove, 0)
. DEFINE_DEVICE_REMOVE(vfb, destroy, 1)
(Numbers on the left show how many instances of formulaic code there
are.)
Or writing the common code once in a function which takes an
appropriate function pointer type for the conversion:
1 int device_remove(libxl_ao *ao, uint32_t domid, int force,
1 void *libxl_foo_device,
1 int (*libxl_device_from_foo_device)
1 (libxl__gc *gc, uint32_t domid,
1 void *libxl_foo_device, libxl__device **device))
1 {
1 AO_CREATE etc. etc.
4 static int device_from_nic_void(libxl__gc *gc, uint32_t domid,
4 void *libxl_foo_device, libxl__device **device)
4 { return libxl__device_from_nic(gc, domid, libxl_foo_device, device); }
8 int libxl_device_nic_remove(libxl_ctx *ctx, uint32_t domid,
8 libxl_device_nic *nic,
8 const libxl_asyncop_how *ao_how)
8 { return device_remove(ctx, domid, 0, nic, device_from_nic_void); }
. int libxl_device_nic_destroy(libxl_ctx *ctx, uint32_t domid,
. libxl_device_nic *nic,
. const libxl_asyncop_how *ao_how)
. { return device_remove(ctx, domid, 1, nic, device_from_nic_void); }
Or combining the above:
1 int device_remove(libxl_ao *ao, uint32_t domid, int force,
1 void *libxl_foo_device,
1 int (*libxl_device_from_foo_device)
1 (libxl__gc *gc, uint32_t domid,
1 void *libxl_foo_device, libxl__device **device))
1 {
1 AO_CREATE etc. etc.
1 #define DEFINE_DEVICE_REMOVE(type) \
4 static int device_from_##type##_void(libxl__gc *gc, uint32_t domid, \
4 void *libxl_foo_device, libxl__device **device) \
4 { return libxl__device_from_##type(gc, domid, \
4 libxl_foo_device, device); } \
\
2 int libxl_device_##type##_remove(libxl_ctx *ctx, uint32_t domid, \
2 libxl_device_##type *thing, \
2 const libxl_asyncop_how *ao_how) \
2 { return device_remove(ctx, domid, 0, thing, \
2 device_from_##type##_void); } \
\
. int libxl_device_##type##_destroy(libxl_ctx *ctx, uint32_t domid, \
. libxl_device_##type *thing, \
. const libxl_asyncop_how *ao_how) \
. { return device_remove(ctx, domid, 1, thing, \
. device_from_##type##_void); }
4 DEVICE_DEVICE_REMOVE(nic)
. DEVICE_DEVICE_REMOVE(disk)
. DEVICE_DEVICE_REMOVE(vfb)
. DEVICE_DEVICE_REMOVE(vkb)
Or at least reducing the number of copies from 8 to 4 like this:
4 int nic_remove(libxl_ctx *ctx, uint32_t domid,
4 libxl__device_nic *nic, int force,
4 const libxl_asyncop_how *ao_how);
8 int libxl_device_nic_remove(libxl_ctx *ctx, uint32_t domid,
8 libxl_device_nic *nic,
8 const libxl_asyncop_how *ao_how)
8 { return nic_remove(ctx, domid, nic, 0, ao_how); }
. int libxl_device_nic_destroy(libxl_ctx *ctx, uint32_t domid,
. libxl_device_nic *nic,
. const libxl_asyncop_how *ao_how)
. { return nic_remove(ctx, domid, nic, 1, ao_how); }
Or at least reducing the size of the octuplicated code like this:
1 int device_remove(libxl_ao *ao, uint32_t domid,
1 libxl__device *device, int force)
1 {
1 ...
1 GCNEW(aodev);
1 libxl__init_ao_device(aodev, ao, NULL);
1 aodev->action = DEVICE_DISCONNECT;
1 aodev->force = 1;
1 aodev->dev = device;
1 aodev->callback = libxl__device_cb;
1 libxl__initiate_device_addremove(egc, aodev);
1 etc. etc.
8 int libxl_device_nic_remove(libxl_ctx *ctx, uint32_t domid,
8 libxl_device_nic *nic,
8 const libxl_asyncop_how *ao_how)
8 {
8 AO_CREATE(ctx, domid, ao_how);
8 int rc;
8 libxl__device *device;
8
8 rc = libxl__device_from_nic(gc, domid, nic, &device);
8 if (rc) return AO_ABORT(rc);
8
8 device_remove(ao, domid, device, 0);
8 return AO_INPROGRESS;
8 }
?
> +/* Device AO operations */
>
> -typedef struct {
> - libxl__ao *ao;
> - libxl__ev_devstate ds;
> -} libxl__ao_device_remove;
> +void libxl__init_ao_device(libxl__ao_device *aorm, libxl__ao *ao,
> + libxl__ao_device **base)
> +{
> + aorm->ao = ao;
> + aorm->active = 1;
> + aorm->rc = 0;
> + aorm->base = base;
> +}
I think a function "init" should set "active" to 0, surely ?
Since the operation has not in fact been started. So perhaps this
should just be a memset.
> +retry_transaction:
> + t = xs_transaction_start(ctx->xsh);
> + if (aorm->force)
> + libxl__xs_path_cleanup(gc, t,
> + libxl__device_frontend_path(gc, aorm->dev));
> + state = libxl__xs_read(gc, t, state_path);
> if (!state)
> goto out_ok;
If you're reworking this, you should check for errors other than
ENOENT here.
But actually, I think all of this is done for you by
libxl__ev_devstate_wait. You don't need to pre-check the state path
at all.
Also you seem to only do the _path_cleanup thing (removing newly empty
parent directories) in the force case, if I'm not mistaken. I think
it would be better to make the force case simply skip the device wait
and then reuse the rest of the code path if possible.
> + if (rc == ERROR_TIMEDOUT && aorm->action == DEVICE_DISCONNECT &&
> + !aorm->force) {
> + aorm->force = 1;
> + libxl__initiate_device_remove(egc, aorm);
> + return;
> + }
I like this approach. Economical.
> + /* Some devices (vkbd) fail to disconnect properly,
> + * but we shouldn't alarm the user if it's during
> + * domain destruction.
> + */
Perhaps we should have a separate flag which controls error reporting
so that a user-requested graceful device disconnect gets full error
logging ?
Thanks,
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |