Fix example code in comment. libxl_event_check() can return
ERROR_NOT_READY; LIBXL_NOT_READY does not exist.
Signed-off-by: Euan Harris <euan.harris@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
libxl_event_register_callbacks cannot reasonably be called while libxl
is busy (has outstanding operations and/or enabled events).
This is because the previous spec implied (although not entirely
clearly) that event hooks would not be called for existing fd and
timeout interests. There is thus no way to reliably ensure that libxl
would get told about fds and timeouts which it became interested in
beforehand.
So there have to be no such fds or timeouts, which means that the
callbacks must only be registered or changed when the ctx is idle.
Document this restriction, and enforce it with a pair of asserts.
(It would be nicer, perhaps, to say that the application may not call
libxl_osevent_register_hooks other than right after creating the ctx.
But there are existing callers, including libvirt, who do it later -
even after doing major operations such as domain creation.)
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Tested-by: Ian Campbell <ian.campbell@citrix.com>
Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
libxl_postfork_child_noexec would nestedly reaquire the non-recursive
"no_forking" mutex: atfork_lock uses it, as does sigchld_user_remove.
The result on Linux is that the process always deadlocks before
returning from this function.
This is used by xl's console child. So, the ultimate effect is that
xl with pygrub does not manage to connect to the pygrub console.
This behaviour was reported by Michael Young in Xen 4.4.0 RC5.
Also, the use of sigchld_user_remove in libxl_postfork_child_noexec is
not correct with SIGCHLD sharing. libxl_postfork_child_noexec is
documented to suffice if called only on one ctx. So deregistering the
ctx it's called on is not sufficient. Instead, we need a new approach
which discards the whole sigchld_user list and unconditionally removes
our SIGCHLD handler if we had one.
Prompted by this, clarify the semantics of
libxl_postfork_child_noexec. Specifically, expand on the meaning of
"quickly" by explaining what operations are not permitted; and
document the fact that the function doesn't reclaim the resources in
the ctxs.
And add a comment in libxl_postfork_child_noexec explaining the
internal concurrency situation.
This is an important bugfix. IMO the bug is a blocker for Xen 4.4.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Reported-by: M A Young <m.a.young@durham.ac.uk>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
Acked-by: Ian Campbell <Ian.Campbell@citrix.com>
Release-acked-by: George Dunlap <george.dunlap@eu.citrix.com>
(cherry picked from commit 5be1e95318)
Previously, an application which had multiple libxl ctxs in multiple
threads, would have to itself plumb SIGCHLD through to each ctx.
Instead, permit multiple libxl ctxs to all share the SIGCHLD handler.
We keep a list of all the ctxs which are interested in SIGCHLD and
notify all of their self-pipes.
In more detail:
* sigchld_owner, the ctx* of the SIGCHLD owner, is replaced by
sigchld_users, a list of SIGCHLD users.
* Each ctx keeps track of whether it is on the users list, so that
libxl__sigchld_needed and libxl__sigchld_notneeded now instead of
idempotently installing and removing the handler, idempotently add
or remove the ctx from the list.
We ensure that we always have the SIGCHLD handler installed
iff the sigchld_users list is nonempty. To make this a bit
easier we make sigchld_installhandler_core and
sigchld_removehandler_core idempotent.
Specifically, the call sites for sigchld_installhandler_core and
sigchld_removehandler_core are updated to manipulate sigchld_users
and only call the install or remove functions as applicable.
* In the signal handler we walk the list of SIGCHLD users and write
to each of their self-pipes. That means that we need to arrange to
defer SIGCHLD when we are manipulating the list (to avoid the
signal handler interrupting our list manipulation); this is quite
tiresome to arrange.
The code as written will, on the first installation of the SIGCHLD
handler, firstly install the real handler, then immediately replace
it with the deferral handler. Doing it this way makes the code
clearer as it makes the SIGCHLD deferral machinery much more
self-contained (and hence easier to reason about).
* The first part of libxl__sigchld_notneeded is broken out into a new
function sigchld_user_remove (which is also needed during for
postfork). And of course that first part of the function is now
rather different, as explained above.
* sigchld_installhandler_core no longer takes the gc argument,
because it now deals with SIGCHLD for all ctxs.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Jim Fehlig <jfehlig@suse.com>
Cc: Ian Campbell <Ian.Campbell@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
v3: Include bugfixes from "Fixup SIGCHLD sharing" patch:
* Use a mutex for defer_sigchld, to guard against concurrency
between the thread calling defer_sigchld and an instance of the
primary signal handler on another thread.
* libxl_sigchld_owner_libxl_always is incompatible with SIGCHLD
sharing. Document this correctly.
Fix "have have" error in comment.
Move removal of newly unused variables to previous patch.
v2.1: Provide feature test macro LIBXL_HAVE_SIGCHLD_SHARING
Applications exist which want to use libxl in an event-driven mode but
which do not integrate child termination into their event system, but
instead reap all their own children synchronously.
In such an application libxl must own SIGCHLD but avoid reaping any
children that don't belong to libxl.
Provide libxl_sigchld_owner_libxl_always_selective_reap which has this
behaviour.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Jim Fehlig <jfehlig@suse.com>
Cc: Ian Campbell <Ian.Campbell@citrix.com>
Acked-by: Ian Campbell <Ian.Campbell@citrix.com>
---
v2: Document the new mode in the big "Subprocess handling" comment.
Applications exist which don't keep track of all their child processes
in a manner suitable for coherent dispatch of their termination. In
such a situation, nothing in the whole process may call wait, or
waitpid(-1,,). Doing so reaps processes belonging to other parts of
the application and there is then no way to deliver the exit status to
the right place.
To facilitate this, provide a facility for such an application to ask
libxl to call waitpid on each of its children individually.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Jim Fehlig <jfehlig@suse.com>
Cc: Ian Campbell <Ian.Campbell@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
libxl_sigchld_owner_libxl ought to have been mentioned in the list of
options for chldowner. Since it's the default, move the description
of the its behaviour into the description of that option.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Jim Fehlig <jfehlig@suse.com>
Cc: Ian Campbell <Ian.Campbell@citrix.com>
Acked-by: Ian Campbell <Ian.Campbell@citrix.com>
Clarify that libxl_sigchld_owner_libxl causes libxl to reap all the
process's children, and clarify the wording of the description of
libxl_sigchld_owner_libxl_always.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Jim Fehlig <jfehlig@suse.com>
Cc: Ian Campbell <Ian.Campbell@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Update the comment to clarify that libxl_sigchld_owner_libxl_always
implies having only one libxl_ctx, and that
libxl_sigchld_owner_mainloop requires one call to
libxl_childproc_exited per ctx.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
The event is supposed to become owned, and therefore freed, by the application
and the const prevents this.
Unfortunately there is no way to remove the const without breaking existing
callers. The best we can do is use the LIBXL_API_VERSION provisions to remove
the const for callers who wish only to support the 4.3 API and newer.
Callers who wish to support 4.2 will need to live with casting away the const.
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Because there is not necessarily any lock held at the point the
application (eg, libvirt) calls libxl_osevent_occurred_timeout, in a
multithreaded program those calls may be arbitrarily delayed in
relation to other activities within the program.
Specifically this means when ->timeout_deregister returns, libxl does
not know whether it can safely dispose of the for_libxl value or
whether it needs to retain it in case of an in-progress call to
_occurred_timeout.
The interface could be fixed by requiring the application to make a
new call into libxl to say that the deregistration was complete.
However that new call would have to be threaded through the
application's event loop; this is complicated and some application
authors are likely not to implement it properly. Furthermore the
easiest way to implement this facility in most event loops is to queue
up a time event for "now".
Shortcut all of this by having libxl always call timeout_modify
setting abs={0,0} (ie, ASAP) instead of timeout_deregister. This will
cause the application to call _occurred_timeout. When processing this
calldown we see that we were no longer actually interested and simply
throw it away.
Additionally, there is a race between _occurred_timeout and
->timeout_modify. If libxl ever adjusts the deadline for a timeout
the application may already be in the process of calling _occurred, in
which case the situation with for_app's lifetime becomes very
complicated. Therefore abolish libxl__ev_time_modify_{abs,rel} (which
have no callers) and promise to the application only ever to call
->timeout_modify with abs=={0,0}. The application still needs to cope
with ->timeout_modify racing with its internal function which calls
_occurred_timeout. Document this.
This is a forwards-compatible change for applications using the libxl
API, and will hopefully eliminate these races in callback-supplying
applications (such as libvirt) without the need for corresponding
changes to the application. (It is possible that this might expose
bugs in applications, though, as previously libxl would never call
libxl_osevent_hooks->timeout_modify and now it never calls
->timeout_deregister).
For clarity, fold the body of time_register_finite into its one
remaining call site. This makes the semantics of ev->infinite
slightly clearer.
Cc: Bamvor Jian Zhang <bjzhang@suse.com>
Cc: Ian Campbell <Ian.Campbell@citrix.com>
Tested-by: Jim Fehlig <jfehlig@suse.com>
Acked-by: Jim Fehlig <jfehlig@suse.com>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Campbell <ian.campbell@citrix.com>
* libxl may indeed register more than one callback for the same fd,
with some restrictions. The allowable range of responses to this by
the application means that this should pose no problems for users.
But the documentation comment should be fixed.
* Document the relaxed synchronicity semantics of the fd_modify
registration callback.
* A couple of comments referred to old names for functions.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
libxl_internal.h says:
* Functions using LIBXL__INIT_EGC may *not* generally be called from
* within libxl, because libxl__egc_cleanup may call back into the
* application. ...
and
* ... [Functions which take an ao_how] MAY NOT
* be called from inside libxl, because they can cause reentrancy
* callbacks.
However, this was not enforced. Particularly the latter restriction
is easy to overlook, especially since during the transition period to
the new event system we have bent this rule a couple of times, and the
bad pattern simply involves passing 0 or NULL for the ao_how.
So use the compiler to enforce this property, as follows:
- Mark all functions which take a libxl_asyncop_how, or which
use EGC_INIT or LIBXL__INIT_EGC, with a new annotation
LIBXL_EXTERNAL_CALLERS_ONLY in the public header.
- Change the documentation comment for asynch operations and egcs to
say that this should always be done.
- Arrange that if libxl.h is included via libxl_internal.h,
LIBXL_EXTERNAL_CALLERS_ONLY expands to __attribute__((warning(...))),
which generates a message like this:
libxl.c:1772: warning: call to 'libxl_device_disk_remove'
declared with attribute warning:
may not be called from within libxl
Otherwise, the annotation expands to nothing, so external
callers are unaffected.
- Forbid inclusion of both libxl.h and libxl_internal.h unless
libxl_internal.h came first, so that the above check doesn't have
any loopholes. Files which include libxl_internal.h should not
include libxl.h as well.
This is enforced explicitly using #error. However, in practice
with the current tree it just changes the error message when this
mistake is made; otherwise we would carry on to immediately
following #define which would cause the compiler to complain that
LIBXL_EXTERNAL_CALLERS_ONLY was redefined. Then the developer
might be tempted to add a #ifndef which would be wrong - it would
leave the affected translation unit unprotected by the new
enforcement regime. So let's be explicit.
- Fix the one source of files which violate the above principle, the
output from the idl compiler, by removing the redundant inclusion
of libxl.h from the output.
Also introduce a new script "check-libxl-api-rules" which contains
some ad-hoc regexps to spot and complain when libxl.h contains
functions which mention libxl_asyncop_how but not
LIBXL_EXTERNAL_CALLERS_ONLY. This isn't a full C parser but is likely
to get the common cases right and err on the side of complaining.
While we are here, the invocation of perl for the bsd queue.h seddery
to $(PERL).
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Roger Pau Monne <roger.pau@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Committed-by: Ian Campbell <ian.campbell@citrix.com>
SIGCHLD or not; if it does, it has to tell us about deaths of our
children.
Currently there are no callers in libxl which use these facilities.
All code in libxl which forks needs to be converted and libxl_fork
needse to be be abolished.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Committed-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
We introduce a new "carefd" concept, which relates to fds that we care
about not being inherited by long-lived children.
As yet we do not use this anywhere in libxl. Until all locations in
libxl which make such fds are converted, libxl__postfork may not work
entirely properly. If these locations do not use O_CLOEXEC (or use
calls for which there is no O_CLOEXEC) then multithreaded programs may
not work properly.
This introduces a new API call libxl_postfork_child_noexec which must
be called by applications which make long-running non-execing
children. Add the appropriate call to xl's postfork function.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Committed-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
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>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Committed-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
We provide a new set of functions and related structures
libxl_osevent_*
which are to be used by event-driven applications to receive
information from libxl about which fds libxl is interested in, and
what timeouts libxl is waiting for, and to pass back to libxl
information about which fds are readable/writeable etc., and which
timeouts have occurred. Ie, low-level events.
In this patch, this new machinery is still all unused. Callers will
appear in the next patch in the series, which introduces a new API for
applications to receive high-level events about actual domains etc.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Committed-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Ian Jackson
Ian Jackson
Euan Harris
Ian Campbell
Christoph Egger