Bug 43905
| Summary: | [kqueue] [patch] kqueues: EV_SET(kevp++, ...) is non-intuitive in sys/event.h | ||
|---|---|---|---|
| Product: | Base System | Reporter: | Lamont Granquist <lamont> |
| Component: | kern | Assignee: | David Bright <dab> |
| Status: | Closed FIXED | ||
| Severity: | Affects Only Me | CC: | dab |
| Priority: | Normal | Flags: | dab:
mfc-stable11+
dab: mfc-stable10+ |
| Version: | Unspecified | ||
| Hardware: | Any | ||
| OS: | Any | ||
Responsible Changed From-To: freebsd-bugs->jmg jmg is willing to look at the kqueue PRs. In FreeBSD PR kern/43905, you wrote:
> EV_SET() macro call references the first argument 6 times making calls
> like EV_SET(kevp++, [...]); not work as-expected and violates
> principle of least-surprise.
SVN r110241, Sun Feb 2 2003, fixed this using a temporary variable, but
this may cause problems if an identifier 'kevp' is in scope.
As an alternative to the inline function, C99 compound literals and
designated initializers allow doing this cleanly using a macro:
Index: sys/sys/event.h
===================================================================
--- sys/sys/event.h (revision 264352)
+++ sys/sys/event.h (working copy)
@@ -45,14 +45,15 @@
#define EVFILT_SENDFILE (-12) /* attached to sendfile requests */
#define EVFILT_SYSCOUNT 12
-#define EV_SET(kevp_, a, b, c, d, e, f) do { \
- struct kevent *kevp = (kevp_); \
- (kevp)->ident = (a); \
- (kevp)->filter = (b); \
- (kevp)->flags = (c); \
- (kevp)->fflags = (d); \
- (kevp)->data = (e); \
- (kevp)->udata = (f); \
+#define EV_SET(kevp, a, b, c, d, e, f) do { \
+ *(kevp) = (struct kevent){ \
+ .ident = (a), \
+ .filter = (b), \
+ .flags = (c), \
+ .fflags = (d), \
+ .data = (e), \
+ .udata = (f) \
+ }; \
} while(0)
struct kevent {
--
Jilles Tjoelker
For bugs matching the following criteria: Status: In Progress Changed: (is less than) 2014-06-01 Reset to default assignee and clear in-progress tags. Mail being skipped A commit references this bug: Author: dab Date: Thu Jun 28 17:01:05 UTC 2018 New revision: 335765 URL: https://svnweb.freebsd.org/changeset/base/335765 Log: Remove potential identifier conflict in the EV_SET macro. PR43905 pointed out a problem with the EV_SET macro if the passed struct kevent pointer were specified with an expression with side effects (e.g., "kevp++"). This was fixed in rS110241, but by using a local block that defined an internal variable (named "kevp") to get the pointer value once. This worked, but could cause issues if an existing variable named "kevp" is in scope. To avoid that issue, jilles@ pointed out that "C99 compound literals and designated initializers allow doing this cleanly using a macro". This change incorporates that suggestion, essentially verbatim from jilles@ comment on PR43905, except retaining the old definition for pre-C99 or non-STDC (e.g., C++) compilers. PR: 43905 Submitted by: Jilles Tjoelker (jilles@) Reported by: Lamont Granquist <lamont@scriptkiddie.org> Reviewed by: jmg (no comments), jilles MFC after: 1 week Sponsored by: Dell EMC Differential Revision: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=43905 Changes: head/sys/sys/event.h A commit references this bug: Author: dab Date: Wed Jul 11 14:50:07 UTC 2018 New revision: 336198 URL: https://svnweb.freebsd.org/changeset/base/336198 Log: MFC r335765, r335776, r336186: Remove potential identifier conflict in the EV_SET macro. PR43905 pointed out a problem with the EV_SET macro if the passed struct kevent pointer were specified with an expression with side effects (e.g., "kevp++"). This was fixed in rS110241, but by using a local block that defined an internal variable (named "kevp") to get the pointer value once. This worked, but could cause issues if an existing variable named "kevp" is in scope. To avoid that issue, jilles@ pointed out that "C99 compound literals and designated initializers allow doing this cleanly using a macro". This change incorporates that suggestion, essentially verbatim from jilles@ comment on PR43905, except retaining the old definition for pre-C99 or non-STDC (e.g., C++) compilers. PR: 43905 Submitted by: Jilles Tjoelker (jilles@) Reported by: Lamont Granquist <lamont@scriptkiddie.org> Sponsored by: Dell EMC Changes: _U stable/11/ stable/11/sys/sys/event.h A commit references this bug: Author: dab Date: Wed Jul 11 14:56:38 UTC 2018 New revision: 336200 URL: https://svnweb.freebsd.org/changeset/base/336200 Log: MFC r335765, r335776, r336186: Remove potential identifier conflict in the EV_SET macro. PR43905 pointed out a problem with the EV_SET macro if the passed struct kevent pointer were specified with an expression with side effects (e.g., "kevp++"). This was fixed in rS110241, but by using a local block that defined an internal variable (named "kevp") to get the pointer value once. This worked, but could cause issues if an existing variable named "kevp" is in scope. To avoid that issue, jilles@ pointed out that "C99 compound literals and designated initializers allow doing this cleanly using a macro". This change incorporates that suggestion, essentially verbatim from jilles@ comment on PR43905, except retaining the old definition for pre-C99 or non-STDC (e.g., C++) compilers. PR: 43905 Submitted by: Jilles Tjoelker (jilles@) Reported by: Lamont Granquist <lamont@scriptkiddie.org> Sponsored by: Dell EMC Changes: _U stable/10/ stable/10/sys/sys/event.h |
EV_SET() macro call references the first argument 6 times making calls like EV_SET(kevp++, [...]); not work as-expected and violates principle of least-surprise. Fix: best i can suggest is to convert to an inline function: static __inline void EV_SET(struct kevent *kevp, const uintptr_t ident, const short filter, const u_short flags, const u_int fflags, const intptr_t data, void * const udata) { (kevp)->ident = ident; (kevp)->filter = filter; (kevp)->flags = flags; (kevp)->fflags = fflags; (kevp)->data = data; (kevp)->udata = udata; } You could introduce a temporary variable in the EV_SET macro, but things get interesting if there's a namespace collision between that temporary variable and the first argument to EV_SET(). How-To-Repeat: #include <sys/types.h> #include <sys/event.h> #include <stdio.h> #include <stdlib.h> int main(void) { int i; struct kevent *kevp = (struct kevent *) malloc(sizeof(struct kevent) * 100); for(i = 0; i < 7; i++) { printf("%d %p\n", i, kevp + i); } printf("%p\n", kevp); EV_SET(kevp++, 0, EVFILT_READ, EV_ADD, 0, 0, 0); printf("%p\n", kevp); exit(0); }