Re: [Xen-devel] [PATCH v12 01/17] libxl: fix removal of secondary consoles

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2012-07-25 at 12:19 +0100, Roger Pau Monne wrote:
> Secondary consoles are processed by libxl with the rest of the
> devices by calling libxl__initiate_device_remove that waits for the
> device to reach state 6 before procceeding with the removal.
> 
> When libxl is destroying the console devices, Qemu is already dead or
> dying, and xenconsoled completely ignores the state backend entry for
> console devices, since it performs the cleanup based on the result of
> reads/writes to the tty.
> 
> Since we don't want to execute hotplug scripts for consoles, leave the
> behaviour as it was previously, and just nuke the backend/frontend
> xenstore entries by calling libxl__device_destroy.
> 
> Report: http://markmail.org/message/yqgppcsdip6tnmh6
> 
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Reported-by: Ian Campbell <ian.campbell@xxxxxxxxxxxxx>
> Signed-off-by: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> ---
>  tools/libxl/libxl_device.c |   12 +++++++++++-
>  1 files changed, 11 insertions(+), 1 deletions(-)
> 
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index a94beab..2120cfb 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -81,6 +81,8 @@ static int libxl__num_devices(libxl__gc *gc, uint32_t domid)
>      for (i = 0; i < num_kinds; i++) {
>          if (libxl__device_kind_from_string(kinds[i], &kind))
>              continue;
> +        if (kind == LIBXL__DEVICE_KIND_CONSOLE)
> +            continue;
>  
>          path = GCSPRINTF("/local/domain/%d/device/%s", domid, kinds[i]);
>          devs = libxl__xs_directory(gc, XBT_NULL, path, &num_devs);
> @@ -512,10 +514,18 @@ void libxl__devices_destroy(libxl__egc *egc, 
> libxl__devices_remove_state *drs)
>              path = libxl__xs_read(gc, XBT_NULL, path);
>              GCNEW(dev);
>              if (path && libxl__parse_backend_path(gc, path, dev) == 0) {
> -                aodev = &aodevs->array[numdev];
>                  dev->domid = domid;
>                  dev->kind = kind;
>                  dev->devid = atoi(devs[j]);
> +                if (dev->backend_kind == LIBXL__DEVICE_KIND_CONSOLE) {
> +                    /* Currently console devices can be destroyed
> +                     * synchronously by just removing xenstore entries,
> +                     * this is what libxl__device_destroy does.
> +                     */
> +                    libxl__device_destroy(gc, dev);
> +                    continue;
> +                }
> +                aodev = &aodevs->array[numdev];

I don't think this is worth a respin but perhaps as a thought for a
future cleanup having the special case extent into both
libxl__num_devices and libxl__devices_destroy is potentially confusing.

Much clearer would be to leave num_devices alone and always numdev++
even when applying the special case.

I'd probably write it as:

if (path && libxl__parse_backend_path(gc, path, dev) == 0) {
    ...
        switch (dev->backend_kind) {
        case LIBXL__DEVICE_KIND_CONSOLE:
                /* comment */
                libxl_device_destroy...
                break;
        default:
                aodev = ...
                aodev->action = ...
                aodev->dev = dev;
                ...
                break
        }
        numdev++;
}






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