|
[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
> 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>
Acked-by: Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>
>
> 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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |