Date: Sun, 6 Apr 2014 17:16:49 +0200 From: Ed Schouten <ed@80386.nl> To: Konstantin Belousov <kostikbel@gmail.com> Cc: Robert Watson <rwatson@freebsd.org>, freebsd-arch@freebsd.org Subject: Re: [Patch] kqueue(2) <-> procdesc(4): EVFILT_PROCDESC Message-ID: <CAJOYFBDJy-qJku6Q-hFbh4OzCsqDR8Y=TAYjk6g_W7q9z7pJnA@mail.gmail.com> In-Reply-To: <20140405153621.GH21331@kib.kiev.ua> References: <CAJOYFBBBtjRh66YLwgTwRFCXv4SRMD5zgq_bq1UZZvSMKJ9Crw@mail.gmail.com> <20140405153621.GH21331@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi there, First of all, thanks for the quick review. Well appreciated! On 5 April 2014 17:36, Konstantin Belousov <kostikbel@gmail.com> wrote: > procdesc_kqops_event() should mimic the filt_proc() much more closely > than you do. In particular: > - I do not see why do you check for NOTE_EXIT in kqops_event(). > If enforcing NOTE_EXIT anywhere, procdesc_kqfilter() is more natural place > to put the temporal restriction. I do think that signalling and execing > are equally useful as exiting notifications, but this is for future. > - The whole structure of filt_proc(), which copies masked sfflags to > fflags is easier to extend later. It seems that just repeating > most of the code from filt_proc() would do what needed. My plan was initially to mimick filt_proc() almost entirely and just use KNOTE_LOCKED(..., NOTE_EXIT) just like EVFILT_PROC does, but unfortunately this doesn't seem possible, because if we went down this path, there would be no way for us to activate the knote in procdesc_kqfilter() immediately, as KNOTE_ACTIVATE() is not available outside of kern_event.c. This is why I just changed the code, so that procdesc_kqops_event() is almost literally a copy of filt_proc(), which the difference that it tests against PDF_EXITED instead of using the hint. While there, I made some style fixes to filt_proc(). Thoughts? http://80386.nl/pub/kqueue-evfilt-procdesc.txt (refresh) > I think you must do knlist_clear() before knlist_destroy(). If I understand kqueue internals properly, I am pretty certain that knlist_clear() is not needed in this case, for two reasons: - knlist_clear man page entry: "This function must not be used when f_isfd is set in struct filterops, as the td argument of fdrop() will be NULL." Other file descriptor types also don't seem to call this function. - struct procdesc is only deallocated if the file descriptor is closed. This means that the knlist is guaranteed to be empty at this point in time. knlist_destroy() also has an assertion for this, which is good. -- Ed Schouten <ed@80386.nl>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJOYFBDJy-qJku6Q-hFbh4OzCsqDR8Y=TAYjk6g_W7q9z7pJnA>