[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH] tools/libxc: fix error handling in xc_mem_paging_load


  • To: xen-devel@xxxxxxxxxxxxxxxxxxx
  • From: "Andres Lagar-Cavilla" <andres@xxxxxxxxxxxxxxxx>
  • Date: Thu, 26 Jan 2012 02:54:43 -0800
  • Cc: olaf@xxxxxxxxx, tim@xxxxxxx, ian.campbell@xxxxxxxxxx
  • Delivery-date: Thu, 26 Jan 2012 10:55:18 +0000
  • Domainkey-signature: a=rsa-sha1; c=nofws; d=lagarcavilla.org; h=message-id :in-reply-to:references:date:subject:from:to:cc:reply-to :mime-version:content-type:content-transfer-encoding; q=dns; s= lagarcavilla.org; b=YVOuoQaX55hkCJxnz3Xq97XjT78AKoiHOLvTeU3z6E/N uXq625wiOjLAq5Xc0zL8a51ROewB8Xr76BO663wzVY6grv+cd8BasrZ6sg8O5/s3 nuc2tgSJJPzGY4F3e9w3rraGRGeiY00TbFeRFdaUnntE7ugfQ+tAgh9yndJmnjY=
  • List-id: Xen developer discussion <xen-devel.lists.xensource.com>

> Date: Wed, 18 Jan 2012 18:15:24 +0100
> From: Olaf Hering <olaf@xxxxxxxxx>
> To: xen-devel@xxxxxxxxxxxxxxxxxxx
> Subject: [Xen-devel] [PATCH] tools/libxc: fix error handling in
>       xc_mem_paging_load
> Message-ID: <1821b0e1ce16d0015542.1326906924@xxxxxxxxxxxx>
> Content-Type: text/plain; charset="us-ascii"
>
> # HG changeset patch
> # User Olaf Hering <olaf@xxxxxxxxx>
> # Date 1326906775 -3600
> # Node ID 1821b0e1ce16d0015542d4164505d97f130f09d7
> # Parent  15ab61865ecbd146f6ce65fbea5bf49bfd9c6cb1
> tools/libxc: fix error handling in xc_mem_paging_load
>
> xc_mem_paging_load() does not pass errors in errno and the actual errno
> from xc_mem_event_control() is overwritten by munlock().
> xenpaging_populate_page() needs to check errno, but with the switch to
> xc_mem_paging_load() it could not receive ENOMEM anymore.
>
> Update xc_mem_paging_load() to return -1 and preserve errno during
> munlock().
>
> Signed-off-by: Olaf Hering <olaf@xxxxxxxxx>

Bump on this one. It's a simple and necessary fix.
And:
Acked-by: Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>

Thanks,
Andres
>
> diff -r 15ab61865ecb -r 1821b0e1ce16 tools/libxc/xc_mem_paging.c
> --- a/tools/libxc/xc_mem_paging.c
> +++ b/tools/libxc/xc_mem_paging.c
> @@ -68,23 +68,28 @@ int xc_mem_paging_prep(xc_interface *xch
>  int xc_mem_paging_load(xc_interface *xch, domid_t domain_id,
>                                  unsigned long gfn, void *buffer)
>  {
> -    int rc;
> +    int rc, old_errno;
> +
> +    errno = -EINVAL;
>
>      if ( !buffer )
> -        return -EINVAL;
> +        return -1;
>
>      if ( ((unsigned long) buffer) & (XC_PAGE_SIZE - 1) )
> -        return -EINVAL;
> +        return -1;
>
>      if ( mlock(buffer, XC_PAGE_SIZE) )
> -        return -errno;
> +        return -1;
>
>      rc = xc_mem_event_control(xch, domain_id,
>                                  XEN_DOMCTL_MEM_EVENT_OP_PAGING_PREP,
>                                  XEN_DOMCTL_MEM_EVENT_OP_PAGING,
>                                  buffer, NULL, gfn);
>
> -    (void)munlock(buffer, XC_PAGE_SIZE);
> +    old_errno = errno;
> +    munlock(buffer, XC_PAGE_SIZE);
> +    errno = old_errno;
> +
>      return rc;
>  }
>
>
>
>
> ------------------------------
>
> Message: 4
> Date: Wed, 18 Jan 2012 17:13:31 +0000
> From: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
> To: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
> Cc: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>,
>       "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>
> Subject: Re: [Xen-devel] [PATCH 13/18] xenstored: support running in
>       minios stubdom
> Message-ID: <20246.64955.717774.909586@xxxxxxxxxxxxxxxxxxxxxxxx>
> Content-Type: text/plain; charset="us-ascii"
>
> Ian Campbell writes ("Re: [Xen-devel] [PATCH 13/18] xenstored: support
> running in minios stubdom"):
>> One thing which might help is to provide nop versions of functions
>> instead of idef'ing both the definition and callsite. e.g.
>>  static void write_pidfile(const char *pidfile)
>> +#ifndef __MINIOS__
>>      stuff
>> +#else
>> +    nothing
>> +endif
>
> I would normally prefer:
>
>> +#ifndef __MINIOS__
>>  static void write_pidfile(const char *pidfile)
>>      stuff
>>  }
>> +#else
>> +static void write_pidfile(const char *pidfile)
>> +}
>> +endif
>
> I think this is fairly easy to read; the only hard part is figuring
> out which version is being used, which can often be done by putting
> the relevant bits in a separate file.
>
> Ian.
>
>
>
> ------------------------------
>
> Message: 5
> Date: Wed, 18 Jan 2012 17:33:24 +0000
> From: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
> To: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>
> Subject: Re: [Xen-devel] [PATCH 3/9] libxl: New event generation API
> Message-ID: <1326908004.14689.296.camel@xxxxxxxxxxxxxxxxxxxxxx>
> Content-Type: text/plain; charset="UTF-8"
>
> On Fri, 2012-01-13 at 19:25 +0000, Ian Jackson wrote:
>> Replace the existing API for retrieving high-level events (events
>> about domains, etc.) from libxl with a new one.
>>
>> This changes the definition and semantics of the `libxl_event'
>> structure, and replaces the calls for obtaining information about
>> domain death and disk eject events.
>>
>> This is an incompatible change, sorry.  The alternative was to try to
>> provide both the previous horrid API and the new one, and would also
>> involve never using the name `libxl_event' for the new interface.
>>
>> The new "libxl_event" structure is blacklisted in the ocaml bindings
>> for two reasons:
>>   - It has a field name "type" (which is a keyword in ocaml);
>>     the ocaml idl generator should massage this field name on
>>     output, to "type_" perhaps.
>>   - The ocaml idl generator does not support KeyedUnion.
>>
>> Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
>> ---
>>  tools/libxl/libxl.c            |  329
>> +++++++++++++++++++++++++++++-----------
>>  tools/libxl/libxl.h            |   55 +------
>>  tools/libxl/libxl_event.c      |  236 ++++++++++++++++++++++++++---
>>  tools/libxl/libxl_event.h      |  183 ++++++++++++++++++++++-
>>  tools/libxl/libxl_internal.h   |   77 +++++++++-
>>  tools/libxl/libxl_types.idl    |   34 ++++-
>>  tools/libxl/xl_cmdimpl.c       |  270 +++++++++++++++++++--------------
>>  tools/ocaml/libs/xl/genwrap.py |    1 +
>>  8 files changed, 908 insertions(+), 277 deletions(-)
>>
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> index 413b684..19ff12c 100644
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c
>> @@ -95,6 +115,13 @@ int libxl_ctx_free(libxl_ctx *ctx)
>>
>>      /* Deregister all libxl__ev_KINDs: */
>>
>> +    free_disable_deaths(gc, &CTX->death_list);
>> +    free_disable_deaths(gc, &CTX->death_reported);
>> +
>> +    libxl_evgen_disk_eject *eject;
>> +    while ((eject = LIBXL_LIST_FIRST(&CTX->disk_eject_evgens)))
>> +        libxl__evdisable_disk_eject(gc, eject);
>
> Why a helper for deaths but not ejects?
>
> [...]
>
>> diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
>> index ec66340..621a7cc 100644
>> --- a/tools/libxl/libxl_event.c
>> +++ b/tools/libxl/libxl_event.c
>
>>
>>  /*
>> diff --git a/tools/libxl/libxl_event.h b/tools/libxl/libxl_event.h
>> index 63ef65e..0e83800 100644
>> --- a/tools/libxl/libxl_event.h
>> +++ b/tools/libxl/libxl_event.h
>
>> +#define LIBXL_EVENTMASK_ALL (~(unsigned long)0)
>> +
>> +typedef int libxl_event_predicate(const libxl_event*, void *user);
>> +  /* Return value is 0 if the event is unwanted or non-0 if it is.
>> +   * Predicates are not allowed to fail.
>> +   */
>> +
>> +int libxl_event_check(libxl_ctx *ctx, libxl_event **event_r,
>> +                      unsigned long typemask,
>> +                      libxl_event_predicate *predicate, void
>> *predicate_user);
>> +  /* Searches for an event, already-happened, which matches typemask
>> +   * and predicate.  predicate==0 matches any event.
>> +   * libxl_event_check returns the event, which must then later be
>> +   * freed by the caller using libxl_event_free.
>> +   *
>> +   * Returns ERROR_NOT_READY if no such event has happened.
>> +   */
>> +
>> +int libxl_event_wait(libxl_ctx *ctx, libxl_event **event_r,
>> +                     unsigned long typemask,
>> +                     libxl_event_predicate *predicate, void
>> *predicate_user);
>> +  /* Like libxl_event_check but blocks if no suitable events are
>> +   * available, until some are.  Uses libxl_osevent_beforepoll/
>> +   * _afterpoll so may be inefficient if very many domains are being
>> +   * handled by a single program.
>> +   */
>> +
>> +void libxl_event_free(libxl_ctx *ctx, libxl_event *event);
>> +
>> +
>> +/* Alternatively or additionally, the application may also use this: */
>> +
>> +typedef struct libxl_event_hooks {
>> +    uint64_t event_occurs_mask;
>
> libxl_event_{wait,check} and LIBXL_EVENTMASK_ALL have an unsigned long
> mask. Are they not the same set of bits?
>
> [...]
>
>> + * The user value is returned in the generated events and may be
>> + * used by the caller for whatever it likes.  The type ev_user is
>> + * guaranteed to be an unsigned integer type which is at least
>> + * as big as uint64_t and is also guaranteed to be big enough to
>> + * contain any intptr_t value.
>
> Does anything actually guarantee that sizeof(uint64_t) >
> sizeof(intptr_t)? I'm sure it's true in practice and I'm happy to rely
> on it. Just interested.
>
>> + *[...]
>
>> + * Applications should ensure that they eventually retrieve every
>> + * event using libxl_event_check or libxl_event_wait, since events
>> + * which occur but are not retreived by the application will be queued
>
>                               retrieved
>
>> + * inside libxl indefinitely.  libxl_event_check/_wait may be O(n)
>> + * where n is the number of queued events which do not match the
>> + * criteria specified in the arguments to check/wait.
>> + */
> [...]
>> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
>> index 574dec7..a6dac79 100644
>> --- a/tools/libxl/libxl_types.idl
>> +++ b/tools/libxl/libxl_types.idl
>> @@ -395,3 +390,32 @@ libxl_sched_sedf = Struct("sched_sedf", [
>>      ("extratime", integer),
>>      ("weight", integer),
>>      ], dispose_fn=None)
>> +
>> +libxl_event_type = Enumeration("event_type", [
>> +    (1, "DOMAIN_SHUTDOWN"),
>> +    (2, "DOMAIN_DESTROY"),
>> +    (3, "DISK_EJECT"),
>> +    ])
>> +
>> +libxl_ev_user = UInt(64)
>
> The other option here would be Builtin(...) and an entry in the builtin
> table in the wrapper generator.
>
> Arguably the idl could be improved by causing Number() to have a width
> field. Currently it has a signedness and width is a property of UInt but
> the latter could be pushed up the hierarchy.
>
> You'd still end up with
>       FOO = Number("FOO", width=X)
> which isn't really much better.
>
> Or the ocaml generate could handle Number as the biggest int.
>
> Hrm. None of that seems all that much better than what you have. Chalk
> it up to potential future work.
>
>> +libxl_ev_link = Builtin("ev_link", passby=PASS_BY_REFERENCE,
>> private=True)
>> +
>> +libxl_event = Struct("event",[
>> +    ("link",     libxl_ev_link,0),
>
> This "0" == "const=False" which is the default. I don't think it is
> necessary.
>
> [...]
>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>> index 8c30de1..e292bfc 100644
>> --- a/tools/libxl/xl_cmdimpl.c
>> +++ b/tools/libxl/xl_cmdimpl.c
>
>> @@ -1702,92 +1729,106 @@ start:
>>      }
>>      LOG("Waiting for domain %s (domid %d) to die [pid %ld]",
>>          d_config.c_info.name, domid, (long)getpid());
> [...]
>> +    ret = libxl_evenable_domain_death(ctx, domid, 0, &deathw);
>> +    if (ret) goto out;
>>
> [...]
>> +    if (!diskws) {
>> +        diskws = xmalloc(sizeof(*diskws) * d_config.num_disks);
>
> I didn't see this getting freed on the exit path.
>
>> +        for (i = 0; i < d_config.num_disks; i++)
>> +            diskws[i] = NULL;
>> +    }
>> +    for (i = 0; i < d_config.num_disks; i++) {
>> +        ret = libxl_evenable_disk_eject(ctx, domid,
>> d_config.disks[i].vdev,
>> +                                        0, &diskws[i]);
>> +        if (ret) goto out;
>> +    }
>
> This is all (I think) safe for num_disks == 0 but why waste the effort?
>
> Incidentally we have libxl_device_disk.removable which might be an
> opportunity to optimise? Assuming it is meaningful in that way. I think
> in reality only emulated CD-ROM devices ever generate this event but
> perhaps exposing that in the API overcomplicates things.
>
> [...]
>
> Ian.
>
>
>
>
> ------------------------------
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel
>
>
> End of Xen-devel Digest, Vol 83, Issue 288
> ******************************************
>



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.