Bug 43905 - [kqueue] [patch] kqueues: EV_SET(kevp++, ...) is non-intuitive in sys/event.h
Summary: [kqueue] [patch] kqueues: EV_SET(kevp++, ...) is non-intuitive in sys/event.h
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: unspecified
Hardware: Any Any
: Normal Affects Only Me
Assignee: David Bright
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2002-10-11 02:40 UTC by Lamont Granquist
Modified: 2018-07-11 16:41 UTC (History)
1 user (show)

See Also:
dab: mfc-stable10+
dab: mfc-stable11+


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Lamont Granquist 2002-10-11 02:40:02 UTC
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);
}
Comment 1 Mark Linimon freebsd_committer freebsd_triage 2006-06-18 06:49:43 UTC
Responsible Changed
From-To: freebsd-bugs->jmg

jmg is willing to look at the kqueue PRs.
Comment 2 Jilles Tjoelker freebsd_committer 2014-04-13 15:33:09 UTC
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
Comment 3 Eitan Adler freebsd_committer freebsd_triage 2017-12-31 07:59:49 UTC
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
Comment 4 commit-hook freebsd_committer 2018-06-28 17:01:50 UTC
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
Comment 5 commit-hook freebsd_committer 2018-07-11 14:50:19 UTC
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
Comment 6 commit-hook freebsd_committer 2018-07-11 14:57:29 UTC
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