| Message ID | 20201215163603.21700-5-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tools/xenstore: support live update for xenstored | expand |
On 15/12/2020 16:35, Juergen Gross wrote: > Signed-off-by: Juergen Gross <jgross@suse.com> > Reviewed-by: Wei Liu <wl@xen.org> > Reviewed-by: Julien Grall <jgrall@amazon.com> > --- > V7: > - new patch > > V8: > - some minor comments by Julien Grall addressed > > Signed-off-by: Juergen Gross <jgross@suse.com> Various of your patches still have double SoB. (Just as a note to be careful to anyone committing...) > diff --git a/tools/include/xenevtchn.h b/tools/include/xenevtchn.h > index 91821ee56d..dadc46ea36 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_open_fd() 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). > */ More of the comment block than this needs adjusting in light of the exec() changes. > -/* 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 open_flags); > > +/* Flag XENEVTCHN_NO_CLOEXEC is ignored by xenevtchn_open_fd(). */ > +xenevtchn_handle *xenevtchn_open_fd(struct xentoollog_logger *logger, > + int fd, unsigned open_flags); > + I spotted this before, but didn't have time to reply. This isn't "open fd". It is "construct a xenevtchn_handle object around an already-open fd". As such, open_flags appears bogus because at no point are we making an open() call. (I'd argue that it irrespective of other things, it wants naming xenevtchn_fdopen() for API familiarity.) However, the root of the problem is actually the ambiguity in the name. These are not flags to the open() system call, but general flags for xenevtchn. Therefore, I recommend a prep patch which renames open_flags to just flags, and while at it, changes to unsigned int rather than a naked "unsigned" type. There are no API/ABI implications for this, but it will help massively with code clarity. I'd also possibly go as far as to say that plumbing 'flags' down into osdep ought to split out into a separate patch. There is also a wild mix of coding styles even within the hunks here. Additionally, something in core.c should check for unknown flags and reject them them with EINVAL. It was buggy that this wasn't done before, and really needs to be implemented before we start having cases where people might plausibly pass something other than 0. ~Andrew P.S. if you don't fancy doing all of this, my brain could do with a break from the complicated work, and I can see about organising this cleanup.
On 15.12.20 19:09, Andrew Cooper wrote: > On 15/12/2020 16:35, Juergen Gross wrote: >> Signed-off-by: Juergen Gross <jgross@suse.com> >> Reviewed-by: Wei Liu <wl@xen.org> >> Reviewed-by: Julien Grall <jgrall@amazon.com> >> --- >> V7: >> - new patch >> >> V8: >> - some minor comments by Julien Grall addressed >> >> Signed-off-by: Juergen Gross <jgross@suse.com> > > Various of your patches still have double SoB. (Just as a note to be > careful to anyone committing...) Yeah, this is annoying. They are all after the first "---" mark, so they shouldn't end up in git AFAICS. Why git is adding those duplicates I don't know. I had this problem before, but some git update made it disappear. Now it is back, but not for all patches. :-( > >> diff --git a/tools/include/xenevtchn.h b/tools/include/xenevtchn.h >> index 91821ee56d..dadc46ea36 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_open_fd() 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). >> */ > > More of the comment block than this needs adjusting in light of the > exec() changes. > >> -/* 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 open_flags); >> >> +/* Flag XENEVTCHN_NO_CLOEXEC is ignored by xenevtchn_open_fd(). */ >> +xenevtchn_handle *xenevtchn_open_fd(struct xentoollog_logger *logger, >> + int fd, unsigned open_flags); >> + > > I spotted this before, but didn't have time to reply. > > This isn't "open fd". It is "construct a xenevtchn_handle object around > an already-open fd". As such, open_flags appears bogus because at no > point are we making an open() call. (I'd argue that it irrespective of > other things, it wants naming xenevtchn_fdopen() for API familiarity.) Okay. > > However, the root of the problem is actually the ambiguity in the name. > These are not flags to the open() system call, but general flags for > xenevtchn. > > Therefore, I recommend a prep patch which renames open_flags to just > flags, and while at it, changes to unsigned int rather than a naked > "unsigned" type. There are no API/ABI implications for this, but it > will help massively with code clarity. Okay. > > I'd also possibly go as far as to say that plumbing 'flags' down into > osdep ought to split out into a separate patch. There is also a wild > mix of coding styles even within the hunks here. Fine with me. > > Additionally, something in core.c should check for unknown flags and > reject them them with EINVAL. It was buggy that this wasn't done > before, and really needs to be implemented before we start having cases > where people might plausibly pass something other than 0. Are you sure this is safe? I'm not arguing against it, but we considered to do that and didn't dare to. > > ~Andrew > > P.S. if you don't fancy doing all of this, my brain could do with a > break from the complicated work, and I can see about organising this > cleanup. > I'm fine doing it. I'm sure you'll find some other no-brainer to work on :-) Juergen
On 16/12/2020 06:06, Jürgen Groß wrote: > On 15.12.20 19:09, Andrew Cooper wrote: >> >> Additionally, something in core.c should check for unknown flags and >> reject them them with EINVAL. It was buggy that this wasn't done >> before, and really needs to be implemented before we start having cases >> where people might plausibly pass something other than 0. > > Are you sure this is safe? I'm not arguing against it, but we considered > to do that and didn't dare to. Well - you're already breaking things by adding meaning to bit 0 where it was previously ignored. But fundamentally - any caller passing non-zero to begin with is buggy, and it will be less bad to fix up our input validation and given them a clean EINVAL now. The alternative is no error and some weird side effect when we implement whichever bit they were settings. ~Andrew
On 16.12.20 12:22, Andrew Cooper wrote: > On 16/12/2020 06:06, Jürgen Groß wrote: >> On 15.12.20 19:09, Andrew Cooper wrote: >>> >>> Additionally, something in core.c should check for unknown flags and >>> reject them them with EINVAL. It was buggy that this wasn't done >>> before, and really needs to be implemented before we start having cases >>> where people might plausibly pass something other than 0. >> >> Are you sure this is safe? I'm not arguing against it, but we considered >> to do that and didn't dare to. > > Well - you're already breaking things by adding meaning to bit 0 where > it was previously ignored. Fair enough. > But fundamentally - any caller passing non-zero to begin with is buggy, > and it will be less bad to fix up our input validation and given them a > clean EINVAL now. Fine with me. > The alternative is no error and some weird side effect when we implement > whichever bit they were settings. Hmm, yes. I'll add the check. Juergen
diff --git a/tools/include/xenevtchn.h b/tools/include/xenevtchn.h index 91821ee56d..dadc46ea36 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_open_fd() 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 open_flags); +/* Flag XENEVTCHN_NO_CLOEXEC is ignored by xenevtchn_open_fd(). */ +xenevtchn_handle *xenevtchn_open_fd(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 aff6ecfaa0..fa1e44b6ea 100644 --- a/tools/libs/evtchn/core.c +++ b/tools/libs/evtchn/core.c @@ -13,6 +13,7 @@ * License along with this library; If not, see <http://www.gnu.org/licenses/>. */ +#include <errno.h> #include <unistd.h> #include <stdlib.h> @@ -28,10 +29,9 @@ 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 open_flags) +static xenevtchn_handle *xenevtchn_alloc_handle(xentoollog_logger *logger) { xenevtchn_handle *xce = malloc(sizeof(*xce)); - int rc; if (!xce) return NULL; @@ -49,19 +49,48 @@ xenevtchn_handle *xenevtchn_open(xentoollog_logger *logger, unsigned open_flags) if (!xce->logger) goto err; } - rc = osdep_evtchn_open(xce); - if ( rc < 0 ) goto err; + return xce; + +err: + xenevtchn_close(xce); + return NULL; +} + +xenevtchn_handle *xenevtchn_open(xentoollog_logger *logger, unsigned open_flags) +{ + xenevtchn_handle *xce = xenevtchn_alloc_handle(logger); + int rc; + + if (!xce) return NULL; + + rc = osdep_evtchn_open(xce, open_flags); + 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); + xenevtchn_close(xce); return NULL; } +xenevtchn_handle *xenevtchn_open_fd(struct xentoollog_logger *logger, + int fd, unsigned open_flags) +{ + xenevtchn_handle *xce; + + if (open_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 6564ed4c44..635c10f09f 100644 --- a/tools/libs/evtchn/freebsd.c +++ b/tools/libs/evtchn/freebsd.c @@ -31,9 +31,14 @@ #define EVTCHN_DEV "/dev/xen/evtchn" -int osdep_evtchn_open(xenevtchn_handle *xce) +int osdep_evtchn_open(xenevtchn_handle *xce, unsigned int open_flags) { - int fd = open(EVTCHN_DEV, O_RDWR|O_CLOEXEC); + int flags = O_RDWR; + int fd; + + if ( !(open_flags & XENEVTCHN_NO_CLOEXEC) ) + flags |= O_CLOEXEC; + fd = open(EVTCHN_DEV, flags); if ( fd == -1 ) return -1; xce->fd = fd; diff --git a/tools/libs/evtchn/libxenevtchn.map b/tools/libs/evtchn/libxenevtchn.map index 33a38f953a..722fa026f9 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_open_fd; +} VERS_1.1; diff --git a/tools/libs/evtchn/linux.c b/tools/libs/evtchn/linux.c index 17e64aea32..2297488f88 100644 --- a/tools/libs/evtchn/linux.c +++ b/tools/libs/evtchn/linux.c @@ -34,9 +34,14 @@ #define O_CLOEXEC 0 #endif -int osdep_evtchn_open(xenevtchn_handle *xce) +int osdep_evtchn_open(xenevtchn_handle *xce, unsigned int open_flags) { - int fd = open("/dev/xen/evtchn", O_RDWR|O_CLOEXEC); + int flags = O_RDWR; + int fd; + + if ( !(open_flags & XENEVTCHN_NO_CLOEXEC) ) + flags |= O_CLOEXEC; + fd = open("/dev/xen/evtchn", flags); if ( fd == -1 ) return -1; xce->fd = fd; diff --git a/tools/libs/evtchn/minios.c b/tools/libs/evtchn/minios.c index 9cd7636fc5..f5db021747 100644 --- a/tools/libs/evtchn/minios.c +++ b/tools/libs/evtchn/minios.c @@ -63,7 +63,11 @@ static void port_dealloc(struct evtchn_port_info *port_info) { free(port_info); } -int osdep_evtchn_open(xenevtchn_handle *xce) +/* + * 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 open_flags) { int fd = alloc_fd(FTYPE_EVTCHN); if ( fd == -1 ) diff --git a/tools/libs/evtchn/netbsd.c b/tools/libs/evtchn/netbsd.c index 8b8545d2f9..7c73d1c599 100644 --- a/tools/libs/evtchn/netbsd.c +++ b/tools/libs/evtchn/netbsd.c @@ -31,7 +31,7 @@ #define EVTCHN_DEV_NAME "/dev/xenevt" -int osdep_evtchn_open(xenevtchn_handle *xce) +int osdep_evtchn_open(xenevtchn_handle *xce, unsigned int open_flags) { int fd = open(EVTCHN_DEV_NAME, O_NONBLOCK|O_RDWR); if ( fd == -1 ) diff --git a/tools/libs/evtchn/private.h b/tools/libs/evtchn/private.h index 31e595bea2..bcac2a191d 100644 --- a/tools/libs/evtchn/private.h +++ b/tools/libs/evtchn/private.h @@ -14,7 +14,7 @@ struct xenevtchn_handle { Xentoolcore__Active_Handle tc_ah; }; -int osdep_evtchn_open(xenevtchn_handle *xce); +int osdep_evtchn_open(xenevtchn_handle *xce, unsigned int open_flags); int osdep_evtchn_close(xenevtchn_handle *xce); int osdep_evtchn_restrict(xenevtchn_handle *xce, domid_t domid); diff --git a/tools/libs/evtchn/solaris.c b/tools/libs/evtchn/solaris.c index dd41f62a24..7e22f7906a 100644 --- a/tools/libs/evtchn/solaris.c +++ b/tools/libs/evtchn/solaris.c @@ -29,7 +29,7 @@ #include "private.h" -int osdep_evtchn_open(xenevtchn_handle *xce) +int osdep_evtchn_open(xenevtchn_handle *xce, unsigned int open_flags) { int fd;