Olaf Hering
2012-Jan-18 17:15 UTC
[PATCH] tools/libxc: fix error handling in xc_mem_paging_load
# HG changeset patch
# User Olaf Hering <olaf@aepfle.de>
# 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@aepfle.de>
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;
}
Andres Lagar-Cavilla
2012-Jan-18 18:23 UTC
Re: [PATCH] tools/libxc: fix error handling in xc_mem_paging_load
> Date: Wed, 18 Jan 2012 18:15:24 +0100 > From: Olaf Hering <olaf@aepfle.de> > To: xen-devel@lists.xensource.com > Subject: [Xen-devel] [PATCH] tools/libxc: fix error handling in > xc_mem_paging_load > Message-ID: <1821b0e1ce16d0015542.1326906924@probook.site> > Content-Type: text/plain; charset="us-ascii" > > # HG changeset patch > # User Olaf Hering <olaf@aepfle.de> > # 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@aepfle.de>Acked-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>> > 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@eu.citrix.com> > To: Ian Campbell <Ian.Campbell@citrix.com> > Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>, > "xen-devel@lists.xensource.com" <xen-devel@lists.xensource.com> > Subject: Re: [Xen-devel] [PATCH 13/18] xenstored: support running in > minios stubdom > Message-ID: <20246.64955.717774.909586@mariner.uk.xensource.com> > 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@citrix.com> > To: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: "xen-devel@lists.xensource.com" <xen-devel@lists.xensource.com> > Subject: Re: [Xen-devel] [PATCH 3/9] libxl: New event generation API > Message-ID: <1326908004.14689.296.camel@zakaz.uk.xensource.com> > 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@eu.citrix.com> >> --- >> 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@lists.xensource.com > http://lists.xensource.com/xen-devel > > > End of Xen-devel Digest, Vol 83, Issue 288 > ****************************************** >
Andres Lagar-Cavilla
2012-Jan-26 10:54 UTC
Re: [PATCH] tools/libxc: fix error handling in xc_mem_paging_load
> Date: Wed, 18 Jan 2012 18:15:24 +0100 > From: Olaf Hering <olaf@aepfle.de> > To: xen-devel@lists.xensource.com > Subject: [Xen-devel] [PATCH] tools/libxc: fix error handling in > xc_mem_paging_load > Message-ID: <1821b0e1ce16d0015542.1326906924@probook.site> > Content-Type: text/plain; charset="us-ascii" > > # HG changeset patch > # User Olaf Hering <olaf@aepfle.de> > # 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@aepfle.de>Bump on this one. It''s a simple and necessary fix. And: Acked-by: Andres Lagar-Cavilla <andres@lagarcavilla.org> 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@eu.citrix.com> > To: Ian Campbell <Ian.Campbell@citrix.com> > Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>, > "xen-devel@lists.xensource.com" <xen-devel@lists.xensource.com> > Subject: Re: [Xen-devel] [PATCH 13/18] xenstored: support running in > minios stubdom > Message-ID: <20246.64955.717774.909586@mariner.uk.xensource.com> > 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@citrix.com> > To: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: "xen-devel@lists.xensource.com" <xen-devel@lists.xensource.com> > Subject: Re: [Xen-devel] [PATCH 3/9] libxl: New event generation API > Message-ID: <1326908004.14689.296.camel@zakaz.uk.xensource.com> > 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@eu.citrix.com> >> --- >> 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@lists.xensource.com > http://lists.xensource.com/xen-devel > > > End of Xen-devel Digest, Vol 83, Issue 288 > ****************************************** >