WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] [PATCH 3 of 9] libxl: signal caller if domain already de

To: Ian Campbell <ian.campbell@xxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH 3 of 9] libxl: signal caller if domain already destroyed on domain death event
From: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
Date: Mon, 26 Jul 2010 15:32:56 +0100
Cc: Ian, Campbell <Ian.Campbell@xxxxxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>
Delivery-date: Mon, 26 Jul 2010 07:34:34 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <1e0b63948031587b958e.1280141807@xxxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <1e0b63948031587b958e.1280141807@xxxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Alpine 2.00 (DEB 1167 2008-08-23)
On Mon, 26 Jul 2010, Ian Campbell wrote:
> # HG changeset patch
> # User Ian Campbell <ian.campbell@xxxxxxxxxx>
> # Date 1280140562 -3600
> # Node ID 1e0b63948031587b958ea307410d19c7b2be9614
> # Parent  f6300d42a667cf6a1a02fc065ecd9eaea0e10ecc
> libxl: signal caller if domain already destroyed on domain death event
> 
> Currently libxl_event_get_domain_death_info returns 0 if the event was
> not a domain death event and 1 if it was but does not infom the user
> if someone else has already cleaned up the domain, which means the
> caller must replicate some of the logic from within libxl.
> 
> Instead have the libxl_event_get_XXX_info functions required that the
> event is of the right type (the caller must have recently switched on
> event->type anyway).
> 
> This allows the return codes to be used in an event specific way and
> we take advantage of this by returning an error from
> libxl_event_get_domain_death_info if the domain is not dying.
> 
> Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> 
> diff -r f6300d42a667 -r 1e0b63948031 tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c     Mon Jul 26 11:36:02 2010 +0100
> +++ b/tools/libxl/libxl.c     Mon Jul 26 11:36:02 2010 +0100
> @@ -721,54 +721,54 @@ int libxl_free_waiter(libxl_waiter *wait
>      return 0;
>  }
>  
> +/*
> + * Returns:
> + *  - 0 if the domain is dead and there is no cleanup to be done. e.g 
> because someone else has already done it.
> + *  - 1 if the domain is dead and there is cleanup to be done.
> + *
> + * Can return error if the domain exists and is still running
> + */
>  int libxl_event_get_domain_death_info(struct libxl_ctx *ctx, uint32_t domid, 
> libxl_event *event, struct libxl_dominfo *info)
>  {
> -    int rc = 0, ret;
> -    if (event && event->type == LIBXL_EVENT_DOMAIN_DEATH) {
> -        ret = libxl_domain_info(ctx, info, domid);
> +    if (libxl_domain_info(ctx, info, domid) < 0)
> +        return 0;
>  
> -        if (ret == 0 && info->domid == domid) {
> -            if (info->running || (!info->shutdown && !info->dying && 
> !info->crashed))
> -                    goto out;
> -                rc = 1;
> -                goto out;
> -        }
> -        memset(info, 0, sizeof(*info));
> -        rc = 1;
> -        goto out;
> -    }
> -out:
> -    return rc;
> +    if (info->running || (!info->shutdown && !info->dying && !info->crashed))
> +        return ERROR_INVAL;
> +
> +    return 1;
>  }
>  
> +/*
> + * Returns true and fills *disk if the caller should eject the disk
> + */
>  int libxl_event_get_disk_eject_info(struct libxl_ctx *ctx, uint32_t domid, 
> libxl_event *event, libxl_device_disk *disk)
>  {
> -    if (event && event->type == LIBXL_EVENT_DISK_EJECT) {
> -        char *path;
> -        char *backend;
> -        char *value = libxl_xs_read(ctx, XBT_NULL, event->path);
> +    char *path;
> +    char *backend;
> +    char *value;
>  
> -        if (!value || strcmp(value,  "eject"))
> -            return 0;
> +    value = libxl_xs_read(ctx, XBT_NULL, event->path);
>  
> -        path = strdup(event->path);
> -        path[strlen(path) - 6] = '\0';
> -        backend = libxl_xs_read(ctx, XBT_NULL, libxl_sprintf(ctx, 
> "%s/backend", path));
> +    if (!value || strcmp(value,  "eject"))
> +        return 0;
>  
> -        disk->backend_domid = 0;
> -        disk->domid = domid;
> -        disk->physpath = NULL;
> -        disk->phystype = 0;
> -        /* this value is returned to the user: do not free right away */
> -        disk->virtpath = libxl_xs_read(ctx, XBT_NULL, libxl_sprintf(ctx, 
> "%s/dev", backend));
> -        disk->unpluggable = 1;
> -        disk->readwrite = 0;
> -        disk->is_cdrom = 1;
> +    path = strdup(event->path);
> +    path[strlen(path) - 6] = '\0';
> +    backend = libxl_xs_read(ctx, XBT_NULL, libxl_sprintf(ctx, "%s/backend", 
> path));
>  
> -        free(path);
> -        return 1;
> -    }
> -    return 0;
> +    disk->backend_domid = 0;
> +    disk->domid = domid;
> +    disk->physpath = NULL;
> +    disk->phystype = 0;
> +    /* this value is returned to the user: do not free right away */
> +    disk->virtpath = libxl_xs_read(ctx, XBT_NULL, libxl_sprintf(ctx, 
> "%s/dev", backend));
> +    disk->unpluggable = 1;
> +    disk->readwrite = 0;
> +    disk->is_cdrom = 1;
> +
> +    free(path);
> +    return 1;
>  }
>  
>  static int libxl_destroy_device_model(struct libxl_ctx *ctx, uint32_t domid)
> diff -r f6300d42a667 -r 1e0b63948031 tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c        Mon Jul 26 11:36:02 2010 +0100
> +++ b/tools/libxl/xl_cmdimpl.c        Mon Jul 26 11:36:02 2010 +0100
> @@ -1338,7 +1338,6 @@ start:
>          struct libxl_dominfo info;
>          libxl_event event;
>          libxl_device_disk disk;
> -        memset(&info, 0x00, sizeof(xc_domaininfo_t));
>  
>          FD_ZERO(&rfds);
>          FD_SET(fd, &rfds);
> @@ -1349,12 +1348,17 @@ start:
>          libxl_get_event(&ctx, &event);
>          switch (event.type) {
>              case LIBXL_EVENT_DOMAIN_DEATH:
> -                if (libxl_event_get_domain_death_info(&ctx, domid, &event, 
> &info)) {
> -                    LOG("Domain %d is dead", domid);
> -                    if (info.crashed || info.dying || (info.shutdown && 
> info.shutdown_reason != SHUTDOWN_suspend)) {
> +                ret = libxl_event_get_domain_death_info(&ctx, domid, &event, 
> &info);
> +
> +                if (ret < 0) continue;
> +
> +                LOG("Domain %d is dead", domid);
> +
> +                if (ret) {
> +                    if (info.shutdown_reason != SHUTDOWN_suspend) {
>                          LOG("Domain %d needs to be clean: destroying the 
> domain", domid);
>                          libxl_domain_destroy(&ctx, domid, 0);
> -                        if (info.shutdown && info.shutdown_reason == 
> SHUTDOWN_reboot) {
> +                        if (info.shutdown_reason == SHUTDOWN_reboot) {

Isn't still possible to get here and have info.shutdown == 0 (and even
info.dying == 0, after reading the fourth patch)?
If so, the previous test is probably clearer.

The rest of the series looks fine.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel

<Prev in Thread] Current Thread [Next in Thread>