| Message ID | 20210114153803.2591-6-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tools/xenstore: support live update for xenstored | expand |
On 14/01/2021 15:37, Juergen Gross wrote: > diff --git a/tools/include/xenevtchn.h b/tools/include/xenevtchn.h > index 3e9b6e7323..b6dd8f3186 100644 > --- a/tools/include/xenevtchn.h > +++ b/tools/include/xenevtchn.h > @@ -64,11 +64,25 @@ struct xentoollog_logger; > * > * Calling xenevtchn_close() is the only safe operation on a > * xenevtchn_handle which has been inherited. > + * > + * Setting XENEVTCHN_NO_CLOEXEC allows to keep the file descriptor used > + * for the event channel driver open across exec(2). In order to be able > + * to use that file descriptor the new binary activated via exec(2) has > + * to call xenevtchn_fdopen() with that file descriptor as parameter in > + * order to associate it with a new handle. The file descriptor can be > + * obtained via xenevtchn_fd() before calling exec(2). > */ Earlier commentary in this block is already wrong (refer to gnttab, and making what appear to be false claims), and/or made stale by this change. How about: /* * Opens the evtchn device node. Return a handle to the event channel * driver, or NULL on failure, in which case errno will be set * appropriately. * * On fork(2): * * After fork, a child process must not use any opened evtchn handle * inherited from their parent. This includes operations such as * poll() on the underlying file descriptor. Calling xenevtchn_close() * is the only safe operation on a xenevtchn_handle which has been * inherited. * * The child must open a new handle if they want to interact with * evtchn. * * On exec(2): * * Wherever possible, the device node will be opened with O_CLOEXEC, * so it is not inherited by the subsequent program. * * However the XENEVTCHN_NO_CLOEXEC flag may be used to avoid opening * the device node with O_CLOEXEC. This is intended for use by * daemons which support a self-reexec method of updating themselves. * * In this case, the updated daemon should pass the underlying file * descriptor it inherited to xenevtchn_fdopen() to reconstruct the * library handle. */ which I think is somewhat more concise? > -/* Currently no flags are defined */ > + > +/* Don't set O_CLOEXEC when opening event channel driver node. */ > +#define XENEVTCHN_NO_CLOEXEC 0x01 Do we really want an byte-looking constant? Wouldn't (1 << 0) be a more normal way of writing this? > + > xenevtchn_handle *xenevtchn_open(struct xentoollog_logger *logger, > unsigned int flags); > > +/* Flag XENEVTCHN_NO_CLOEXEC is ignored by xenevtchn_fdopen(). */ > +xenevtchn_handle *xenevtchn_fdopen(struct xentoollog_logger *logger, > + int fd, unsigned open_flags); True, but see below... > + > /* > * Close a handle previously allocated with xenevtchn_open(). xenevtchn_{,fd}open(), now. > diff --git a/tools/libs/evtchn/core.c b/tools/libs/evtchn/core.c > index c069d5da71..f2ab27384b 100644 > --- a/tools/libs/evtchn/core.c > +++ b/tools/libs/evtchn/core.c > > +xenevtchn_handle *xenevtchn_fdopen(struct xentoollog_logger *logger, > + int fd, unsigned int flags) > +{ > + xenevtchn_handle *xce; > + > + if ( flags & ~XENEVTCHN_NO_CLOEXEC ) > + { > + errno = EINVAL; > + return NULL; > + } Do we really want to tolerate XENEVTCHN_NO_CLOEXEC here? I'd suggest rejecting it, because nothing good can come of a caller thinking it has avoided setting O_CLOEXEC when in fact it hasn't. > diff --git a/tools/libs/evtchn/freebsd.c b/tools/libs/evtchn/freebsd.c > index bb601f350f..ed2baf3c95 100644 > --- a/tools/libs/evtchn/freebsd.c > +++ b/tools/libs/evtchn/freebsd.c > @@ -33,8 +33,12 @@ > > int osdep_evtchn_open(xenevtchn_handle *xce, unsigned int flags) > { > - int fd = open(EVTCHN_DEV, O_RDWR|O_CLOEXEC); > + int open_flags = O_RDWR; > + int fd; > > + if ( !(flags & XENEVTCHN_NO_CLOEXEC) ) > + open_flags |= O_CLOEXEC; As we're now consistently using hypervisor style, we ought to have an extra newline here and in equivalent positions. If you're happy with the suggestions, I'm happy folding them on commit, to avoid yet another posting. ~Andrew
On 15.01.21 02:01, Andrew Cooper wrote: > On 14/01/2021 15:37, Juergen Gross wrote: >> diff --git a/tools/include/xenevtchn.h b/tools/include/xenevtchn.h >> index 3e9b6e7323..b6dd8f3186 100644 >> --- a/tools/include/xenevtchn.h >> +++ b/tools/include/xenevtchn.h >> @@ -64,11 +64,25 @@ struct xentoollog_logger; >> * >> * Calling xenevtchn_close() is the only safe operation on a >> * xenevtchn_handle which has been inherited. >> + * >> + * Setting XENEVTCHN_NO_CLOEXEC allows to keep the file descriptor used >> + * for the event channel driver open across exec(2). In order to be able >> + * to use that file descriptor the new binary activated via exec(2) has >> + * to call xenevtchn_fdopen() with that file descriptor as parameter in >> + * order to associate it with a new handle. The file descriptor can be >> + * obtained via xenevtchn_fd() before calling exec(2). >> */ > > Earlier commentary in this block is already wrong (refer to gnttab, and > making what appear to be false claims), and/or made stale by this change. > > How about: > > /* > * Opens the evtchn device node. Return a handle to the event channel > * driver, or NULL on failure, in which case errno will be set > * appropriately. > * > * On fork(2): > * > * After fork, a child process must not use any opened evtchn handle > * inherited from their parent. This includes operations such as > * poll() on the underlying file descriptor. Calling xenevtchn_close() > * is the only safe operation on a xenevtchn_handle which has been > * inherited. > * > * The child must open a new handle if they want to interact with > * evtchn. > * > * On exec(2): > * > * Wherever possible, the device node will be opened with O_CLOEXEC, > * so it is not inherited by the subsequent program. > * > * However the XENEVTCHN_NO_CLOEXEC flag may be used to avoid opening > * the device node with O_CLOEXEC. This is intended for use by > * daemons which support a self-reexec method of updating themselves. > * > * In this case, the updated daemon should pass the underlying file > * descriptor it inherited to xenevtchn_fdopen() to reconstruct the > * library handle. > */ > > which I think is somewhat more concise? Yes, I'll change it. > > >> -/* Currently no flags are defined */ >> + >> +/* Don't set O_CLOEXEC when opening event channel driver node. */ >> +#define XENEVTCHN_NO_CLOEXEC 0x01 > > Do we really want an byte-looking constant? Wouldn't (1 << 0) be a more > normal way of writing this? Fine with me. > >> + >> xenevtchn_handle *xenevtchn_open(struct xentoollog_logger *logger, >> unsigned int flags); >> >> +/* Flag XENEVTCHN_NO_CLOEXEC is ignored by xenevtchn_fdopen(). */ >> +xenevtchn_handle *xenevtchn_fdopen(struct xentoollog_logger *logger, >> + int fd, unsigned open_flags); > > True, but see below... > >> + >> /* >> * Close a handle previously allocated with xenevtchn_open(). > > xenevtchn_{,fd}open(), now. Ah, right. > >> diff --git a/tools/libs/evtchn/core.c b/tools/libs/evtchn/core.c >> index c069d5da71..f2ab27384b 100644 >> --- a/tools/libs/evtchn/core.c >> +++ b/tools/libs/evtchn/core.c >> >> +xenevtchn_handle *xenevtchn_fdopen(struct xentoollog_logger *logger, >> + int fd, unsigned int flags) >> +{ >> + xenevtchn_handle *xce; >> + >> + if ( flags & ~XENEVTCHN_NO_CLOEXEC ) >> + { >> + errno = EINVAL; >> + return NULL; >> + } > > Do we really want to tolerate XENEVTCHN_NO_CLOEXEC here? I'd suggest > rejecting it, because nothing good can come of a caller thinking it has > avoided setting O_CLOEXEC when in fact it hasn't. Fine with me. > >> diff --git a/tools/libs/evtchn/freebsd.c b/tools/libs/evtchn/freebsd.c >> index bb601f350f..ed2baf3c95 100644 >> --- a/tools/libs/evtchn/freebsd.c >> +++ b/tools/libs/evtchn/freebsd.c >> @@ -33,8 +33,12 @@ >> >> int osdep_evtchn_open(xenevtchn_handle *xce, unsigned int flags) >> { >> - int fd = open(EVTCHN_DEV, O_RDWR|O_CLOEXEC); >> + int open_flags = O_RDWR; >> + int fd; >> >> + if ( !(flags & XENEVTCHN_NO_CLOEXEC) ) >> + open_flags |= O_CLOEXEC; > > As we're now consistently using hypervisor style, we ought to have an > extra newline here and in equivalent positions. Yes. > > If you're happy with the suggestions, I'm happy folding them on commit, > to avoid yet another posting. I'll post v12 today. Juergen
diff --git a/tools/include/xenevtchn.h b/tools/include/xenevtchn.h index 3e9b6e7323..b6dd8f3186 100644 --- a/tools/include/xenevtchn.h +++ b/tools/include/xenevtchn.h @@ -64,11 +64,25 @@ struct xentoollog_logger; * * Calling xenevtchn_close() is the only safe operation on a * xenevtchn_handle which has been inherited. + * + * Setting XENEVTCHN_NO_CLOEXEC allows to keep the file descriptor used + * for the event channel driver open across exec(2). In order to be able + * to use that file descriptor the new binary activated via exec(2) has + * to call xenevtchn_fdopen() with that file descriptor as parameter in + * order to associate it with a new handle. The file descriptor can be + * obtained via xenevtchn_fd() before calling exec(2). */ -/* Currently no flags are defined */ + +/* Don't set O_CLOEXEC when opening event channel driver node. */ +#define XENEVTCHN_NO_CLOEXEC 0x01 + xenevtchn_handle *xenevtchn_open(struct xentoollog_logger *logger, unsigned int flags); +/* Flag XENEVTCHN_NO_CLOEXEC is ignored by xenevtchn_fdopen(). */ +xenevtchn_handle *xenevtchn_fdopen(struct xentoollog_logger *logger, + int fd, unsigned open_flags); + /* * Close a handle previously allocated with xenevtchn_open(). */ diff --git a/tools/libs/evtchn/Makefile b/tools/libs/evtchn/Makefile index ad01a17b3d..b8c37b5b97 100644 --- a/tools/libs/evtchn/Makefile +++ b/tools/libs/evtchn/Makefile @@ -2,7 +2,7 @@ XEN_ROOT = $(CURDIR)/../../.. include $(XEN_ROOT)/tools/Rules.mk MAJOR = 1 -MINOR = 1 +MINOR = 2 SRCS-y += core.c SRCS-$(CONFIG_Linux) += linux.c diff --git a/tools/libs/evtchn/core.c b/tools/libs/evtchn/core.c index c069d5da71..f2ab27384b 100644 --- a/tools/libs/evtchn/core.c +++ b/tools/libs/evtchn/core.c @@ -30,18 +30,10 @@ static int all_restrict_cb(Xentoolcore__Active_Handle *ah, domid_t domid) return xenevtchn_restrict(xce, domid); } -xenevtchn_handle *xenevtchn_open(xentoollog_logger *logger, unsigned int flags) +static xenevtchn_handle *xenevtchn_alloc_handle(xentoollog_logger *logger) { - xenevtchn_handle *xce; - int rc; - - if ( flags ) - { - errno = EINVAL; - return NULL; - } + xenevtchn_handle *xce = malloc(sizeof(*xce)); - xce = malloc(sizeof(*xce)); if ( !xce ) return NULL; @@ -60,21 +52,59 @@ xenevtchn_handle *xenevtchn_open(xentoollog_logger *logger, unsigned int flags) goto err; } + return xce; + +err: + xenevtchn_close(xce); + return NULL; +} + +xenevtchn_handle *xenevtchn_open(xentoollog_logger *logger, unsigned int flags) +{ + xenevtchn_handle *xce; + int rc; + + if ( flags & ~XENEVTCHN_NO_CLOEXEC ) + { + errno = EINVAL; + return NULL; + } + + xce = xenevtchn_alloc_handle(logger); + if ( !xce ) + return NULL; + rc = osdep_evtchn_open(xce, flags); - if ( rc < 0 ) + if ( rc < 0 ) goto err; return xce; - err: - xentoolcore__deregister_active_handle(&xce->tc_ah); - osdep_evtchn_close(xce); - xtl_logger_destroy(xce->logger_tofree); - free(xce); - +err: + xenevtchn_close(xce); return NULL; } +xenevtchn_handle *xenevtchn_fdopen(struct xentoollog_logger *logger, + int fd, unsigned int flags) +{ + xenevtchn_handle *xce; + + if ( flags & ~XENEVTCHN_NO_CLOEXEC ) + { + errno = EINVAL; + return NULL; + } + + xce = xenevtchn_alloc_handle(logger); + if ( !xce ) + return NULL; + + xce->fd = fd; + + return xce; +} + int xenevtchn_close(xenevtchn_handle *xce) { int rc; diff --git a/tools/libs/evtchn/freebsd.c b/tools/libs/evtchn/freebsd.c index bb601f350f..ed2baf3c95 100644 --- a/tools/libs/evtchn/freebsd.c +++ b/tools/libs/evtchn/freebsd.c @@ -33,8 +33,12 @@ int osdep_evtchn_open(xenevtchn_handle *xce, unsigned int flags) { - int fd = open(EVTCHN_DEV, O_RDWR|O_CLOEXEC); + int open_flags = O_RDWR; + int fd; + if ( !(flags & XENEVTCHN_NO_CLOEXEC) ) + open_flags |= O_CLOEXEC; + fd = open(EVTCHN_DEV, open_flags); if ( fd == -1 ) return -1; diff --git a/tools/libs/evtchn/libxenevtchn.map b/tools/libs/evtchn/libxenevtchn.map index 33a38f953a..4c180ea65d 100644 --- a/tools/libs/evtchn/libxenevtchn.map +++ b/tools/libs/evtchn/libxenevtchn.map @@ -21,3 +21,7 @@ VERS_1.1 { global: xenevtchn_restrict; } VERS_1.0; +VERS_1.2 { + global: + xenevtchn_fdopen; +} VERS_1.1; diff --git a/tools/libs/evtchn/linux.c b/tools/libs/evtchn/linux.c index 62adc0e574..60bb75a791 100644 --- a/tools/libs/evtchn/linux.c +++ b/tools/libs/evtchn/linux.c @@ -36,8 +36,12 @@ int osdep_evtchn_open(xenevtchn_handle *xce, unsigned int flags) { - int fd = open("/dev/xen/evtchn", O_RDWR|O_CLOEXEC); + int open_flags = O_RDWR; + int fd; + if ( !(flags & XENEVTCHN_NO_CLOEXEC) ) + open_flags |= O_CLOEXEC; + fd = open("/dev/xen/evtchn", open_flags); if ( fd == -1 ) return -1; diff --git a/tools/libs/evtchn/minios.c b/tools/libs/evtchn/minios.c index 47c153c268..5728991cb8 100644 --- a/tools/libs/evtchn/minios.c +++ b/tools/libs/evtchn/minios.c @@ -69,6 +69,10 @@ static void port_dealloc(struct evtchn_port_info *port_info) free(port_info); } +/* + * XENEVTCHN_NO_CLOEXEC is being ignored, as there is no exec() call supported + * in Mini-OS. + */ int osdep_evtchn_open(xenevtchn_handle *xce, unsigned int flags) { int fd = alloc_fd(FTYPE_EVTCHN);