Apropos of an observation by Roger that it is too easy to violate the
restriction on reentering libxl and in particular too easy to
start multiple nested aos on a single thread, which is forbidden:
DO NOT APPLY THIS PATCH.
It contains -Wno-error. Without that it would break the build.
Subject [PATCH] libxl: enforce prohibitions of internal callers
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.
In the tree I am using as a base at the time of writing, this new
restriction catches three errors: two in libxl_device_disk_remove and
one in libxl__device_disk_local_detach. To avoid entirely breaking my
build I have also done this:
- Temporarily change -Werror to -Wno-error in the libxl Makefile.
This patch should not be applied in this form.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Roger Pau Monne <roger.pau@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/Makefile | 2 +-
tools/libxl/gentypes.py | 1 -
tools/libxl/libxl.h | 34 +++++++++++++++++++++++++---------
tools/libxl/libxl_event.h | 21 ++++++++++++++-------
tools/libxl/libxl_internal.h | 14 ++++++++++----
5 files changed, 50 insertions(+), 22 deletions(-)
diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index e11354f..5eeb639 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -11,7 +11,7 @@ MINOR = 0
XLUMAJOR = 1.0
XLUMINOR = 0
-CFLAGS += -Werror -Wno-format-zero-length -Wmissing-declarations \
+CFLAGS += -Wno-error -Wno-format-zero-length -Wmissing-declarations \
-Wno-declaration-after-statement -Wformat-nonliteral
CFLAGS += -I. -fPIC
diff --git a/tools/libxl/gentypes.py b/tools/libxl/gentypes.py
index 3c561ba..6e83b21 100644
--- a/tools/libxl/gentypes.py
+++ b/tools/libxl/gentypes.py
@@ -341,7 +341,6 @@ if __name__ == ''__main__'':
#include <stdlib.h>
#include <string.h>
-#include "libxl.h"
#include "libxl_internal.h"
#define LIBXL_DTOR_POISON 0xa5
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 4f1f4fd..2195043 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -266,6 +266,13 @@
#endif
#endif
+/* Functions annotated with LIBXL_EXTERNAL_CALLERS_ONLY may not be
+ * called from within libxl itself. Callers outside libxl, who
+ * do not #include libxl_internal.h, are fine. */
+#ifndef LIBXL_EXTERNAL_CALLERS_ONLY
+#define LIBXL_EXTERNAL_CALLERS_ONLY /* disappears for callers outside libxl */
+#endif
+
typedef uint8_t libxl_mac[6];
#define LIBXL_MAC_FMT "%02hhx:%02hhx:%02hhx:%02hhx:%02hhx:%02hhx"
#define LIBXL_MAC_FMTLEN ((2*6)+5) /* 6 hex bytes plus 5 colons */
@@ -495,11 +502,13 @@ int libxl_ctx_free(libxl_ctx *ctx /* 0 is OK */);
int libxl_domain_create_new(libxl_ctx *ctx, libxl_domain_config *d_config,
uint32_t *domid,
const libxl_asyncop_how *ao_how,
- const libxl_asyncprogress_how *aop_console_how);
+ const libxl_asyncprogress_how *aop_console_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
int libxl_domain_create_restore(libxl_ctx *ctx, libxl_domain_config *d_config,
uint32_t *domid, int restore_fd,
const libxl_asyncop_how *ao_how,
- const libxl_asyncprogress_how
*aop_console_how);
+ const libxl_asyncprogress_how *aop_console_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
/* A progress report will be made via ao_console_how, of type
* domain_create_console_available, when the domain''s primary
* console is available and can be connected to.
@@ -510,7 +519,8 @@ void libxl_domain_config_dispose(libxl_domain_config
*d_config);
int libxl_domain_suspend(libxl_ctx *ctx, uint32_t domid, int fd,
int flags, /* LIBXL_SUSPEND_* */
- const libxl_asyncop_how *ao_how);
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
#define LIBXL_SUSPEND_DEBUG 1
#define LIBXL_SUSPEND_LIVE 2
@@ -522,7 +532,8 @@ int libxl_domain_resume(libxl_ctx *ctx, uint32_t domid, int
suspend_cancel);
int libxl_domain_remus_start(libxl_ctx *ctx, libxl_domain_remus_info *info,
uint32_t domid, int send_fd, int recv_fd,
- const libxl_asyncop_how *ao_how);
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
int libxl_domain_shutdown(libxl_ctx *ctx, uint32_t domid);
int libxl_domain_reboot(libxl_ctx *ctx, uint32_t domid);
@@ -544,7 +555,8 @@ int libxl_domain_unpause(libxl_ctx *ctx, uint32_t domid);
int libxl_domain_core_dump(libxl_ctx *ctx, uint32_t domid,
const char *filename,
- const libxl_asyncop_how *ao_how);
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid, uint32_t
target_memkb);
int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid, int32_t
target_memkb, int relative, int enforce);
@@ -640,7 +652,8 @@ void libxl_vminfo_list_free(libxl_vminfo *list, int nr);
int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid, libxl_device_disk
*disk);
int libxl_device_disk_remove(libxl_ctx *ctx, uint32_t domid,
libxl_device_disk *disk,
- const libxl_asyncop_how *ao_how);
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
int libxl_device_disk_destroy(libxl_ctx *ctx, uint32_t domid,
libxl_device_disk *disk);
@@ -658,7 +671,8 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid,
libxl_device_disk *disk);
int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic
*nic);
int libxl_device_nic_remove(libxl_ctx *ctx, uint32_t domid,
libxl_device_nic *nic,
- const libxl_asyncop_how *ao_how);
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
int libxl_device_nic_destroy(libxl_ctx *ctx, uint32_t domid, libxl_device_nic
*nic);
libxl_device_nic *libxl_device_nic_list(libxl_ctx *ctx, uint32_t domid, int
*num);
@@ -669,14 +683,16 @@ int libxl_device_nic_getinfo(libxl_ctx *ctx, uint32_t
domid,
int libxl_device_vkb_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vkb
*vkb);
int libxl_device_vkb_remove(libxl_ctx *ctx, uint32_t domid,
libxl_device_vkb *vkb,
- const libxl_asyncop_how *ao_how);
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
int libxl_device_vkb_destroy(libxl_ctx *ctx, uint32_t domid, libxl_device_vkb
*vkb);
/* Framebuffer */
int libxl_device_vfb_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vfb
*vfb);
int libxl_device_vfb_remove(libxl_ctx *ctx, uint32_t domid,
libxl_device_vfb *vfb,
- const libxl_asyncop_how *ao_how);
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
int libxl_device_vfb_destroy(libxl_ctx *ctx, uint32_t domid, libxl_device_vfb
*vfb);
/* PCI Passthrough */
diff --git a/tools/libxl/libxl_event.h b/tools/libxl/libxl_event.h
index 713d96d..3344bc8 100644
--- a/tools/libxl/libxl_event.h
+++ b/tools/libxl/libxl_event.h
@@ -37,7 +37,8 @@ typedef int libxl_event_predicate(const libxl_event*, void
*user);
int libxl_event_check(libxl_ctx *ctx, libxl_event **event_r,
uint64_t typemask,
- libxl_event_predicate *predicate, void *predicate_user);
+ libxl_event_predicate *predicate, void *predicate_user)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
/* 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
@@ -48,7 +49,8 @@ int libxl_event_check(libxl_ctx *ctx, libxl_event **event_r,
int libxl_event_wait(libxl_ctx *ctx, libxl_event **event_r,
uint64_t typemask,
- libxl_event_predicate *predicate, void *predicate_user);
+ libxl_event_predicate *predicate, void *predicate_user)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
/* 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
@@ -256,7 +258,8 @@ struct pollfd;
*/
int libxl_osevent_beforepoll(libxl_ctx *ctx, int *nfds_io,
struct pollfd *fds, int *timeout_upd,
- struct timeval now);
+ struct timeval now)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
/* nfds and fds[0..nfds] must be from the most recent call to
* _beforepoll, as modified by poll. (It is therefore not possible
@@ -271,7 +274,8 @@ int libxl_osevent_beforepoll(libxl_ctx *ctx, int *nfds_io,
* libxl_event_check.
*/
void libxl_osevent_afterpoll(libxl_ctx *ctx, int nfds, const struct pollfd
*fds,
- struct timeval now);
+ struct timeval now)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
typedef struct libxl_osevent_hooks {
@@ -357,14 +361,16 @@ void libxl_osevent_register_hooks(libxl_ctx *ctx,
*/
void libxl_osevent_occurred_fd(libxl_ctx *ctx, void *for_libxl,
- int fd, short events, short revents);
+ int fd, short events, short revents)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
/* Implicitly, on entry to this function the timeout has been
* deregistered. If _occurred_timeout is called, libxl will not
* call timeout_deregister; if it wants to requeue the timeout it
* will call timeout_register again.
*/
-void libxl_osevent_occurred_timeout(libxl_ctx *ctx, void *for_libxl);
+void libxl_osevent_occurred_timeout(libxl_ctx *ctx, void *for_libxl)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
/*======================================================================*/
@@ -506,7 +512,8 @@ void libxl_childproc_setmode(libxl_ctx *ctx, const
libxl_childproc_hooks *hooks,
* certainly need to use the self-pipe trick (or a working pselect or
* ppoll) to implement this.
*/
-int libxl_childproc_reaped(libxl_ctx *ctx, pid_t, int status);
+int libxl_childproc_reaped(libxl_ctx *ctx, pid_t, int status)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
/*
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 6e6e5fd..cd507db 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -52,6 +52,12 @@
#include <xen/io/xenbus.h>
+#ifdef LIBXL_H
+# error libxl.h should be included via libxl_internal.h, not separately
+#endif
+#define LIBXL_EXTERNAL_CALLERS_ONLY \
+ __attribute__((warning("may not be called from within libxl")))
+
#include "libxl.h"
#include "_libxl_paths.h"
#include "_libxl_save_msgs_callout.h"
@@ -1534,10 +1540,10 @@ libxl__device_model_version_running(libxl__gc *gc,
uint32_t domid);
*
* Functions using LIBXL__INIT_EGC may *not* generally be called from
* within libxl, because libxl__egc_cleanup may call back into the
- * application. This should be documented near the function
- * prototype(s) for callers of LIBXL__INIT_EGC and EGC_INIT. You
- * should in any case not find it necessary to call egc-creators from
- * within libxl.
+ * application. This should be enforced by declaring all such
+ * functions in libxl.h or libxl_event.h with
+ * LIBXL_EXTERNAL_CALLERS_ONLY. You should in any case not find it
+ * necessary to call egc-creators from within libxl.
*
* The callbacks must all take place with the ctx unlocked because
* the application is entitled to reenter libxl from them. This
--
tg: (0f31282..) t/xen/xl.external-callers-only (depends on:
t/xen/xl.ao.progress-ignored-free-event)