Date: Fri, 14 Oct 2016 09:23:58 -0700 From: Scott Mitchell <scott.k.mitch1@gmail.com> To: Konstantin Belousov <kostikbel@gmail.com> Cc: Hartmut.Brandt@dlr.de, freebsd-current@freebsd.org, Sepherosa Ziehau <sepherosa@gmail.com>, Adrian Chadd <adrian.chadd@gmail.com> Subject: Re: asio and kqueue (2nd trye) (was: RE: (boost::)asio and kqueue problem) Message-ID: <CAFn2buB8MdQ32BY9_ArQbAMp=UYFgm=mpQ0Ft2HjHvaRDHiRUQ@mail.gmail.com> In-Reply-To: <20161014120333.GB2404@kib.kiev.ua> References: <611243783F62AF48AFB07BC25FA4B1061CE64959@DLREXMBX01.intra.dlr.de> <20161014120333.GB2404@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
Patch generally lgtm ... just 1 nit comment:
+ } else {
+ if (sbavail(&so->so_rcv) >= so->so_rcv.sb_lowat)
+ return 1;
+ }
Collapse the else and the block inside to just make it an `else if`
for less branching.
On Fri, Oct 14, 2016 at 5:03 AM, Konstantin Belousov <kostikbel@gmail.com>
wrote:
> On Fri, Oct 14, 2016 at 09:21:52AM +0000, Hartmut.Brandt@dlr.de wrote:
> > Hi all,
> >
> > here is the 2nd try taking into account the comments I received. Since
> I'm not familiar with the locking in the sockets area I ask somebody with
> that knowledge to check it before I commit it.
>
> I have only style notes, the factual code changes in the patch look good
> to me.
>
> Index: uipc_socket.c
> ===================================================================
> --- uipc_socket.c (revision 307091)
> +++ uipc_socket.c (working copy)
> @@ -159,15 +159,9 @@
> static int filt_soread(struct knote *kn, long hint);
> static void filt_sowdetach(struct knote *kn);
> static int filt_sowrite(struct knote *kn, long hint);
> -static int filt_solisten(struct knote *kn, long hint);
> static int inline hhook_run_socket(struct socket *so, void *hctx, int32_t
> h_id);
> fo_kqfilter_t soo_kqfilter;
>
> -static struct filterops solisten_filtops = {
> - .f_isfd = 1,
> - .f_detach = filt_sordetach,
> - .f_event = filt_solisten,
> -};
> static struct filterops soread_filtops = {
> .f_isfd = 1,
> .f_detach = filt_sordetach,
> @@ -3075,10 +3069,7 @@
>
> switch (kn->kn_filter) {
> case EVFILT_READ:
> - if (so->so_options & SO_ACCEPTCONN)
> - kn->kn_fop = &solisten_filtops;
> - else
> - kn->kn_fop = &soread_filtops;
> + kn->kn_fop = &soread_filtops;
> sb = &so->so_rcv;
> break;
> case EVFILT_WRITE:
> @@ -3282,29 +3273,34 @@
> static int
> filt_soread(struct knote *kn, long hint)
> {
> - struct socket *so;
> + struct socket *so = kn->kn_fp->f_data;
> Style is against mixing declaration and initialization. Please keep the
> next removed line instead.
>
> - so = kn->kn_fp->f_data;
> This one.
>
> - SOCKBUF_LOCK_ASSERT(&so->so_rcv);
> + if (so->so_options & SO_ACCEPTCONN) {
> + kn->kn_data = so->so_qlen;
> + return (!TAILQ_EMPTY(&so->so_comp));
>
> - kn->kn_data = sbavail(&so->so_rcv) - so->so_rcv.sb_ctl;
> - if (so->so_rcv.sb_state & SBS_CANTRCVMORE) {
> - kn->kn_flags |= EV_EOF;
> - kn->kn_fflags = so->so_error;
> - return (1);
> - } else if (so->so_error) /* temporary udp error */
> - return (1);
> + } else {
> You do not need else {} block, 'then' branch ends with return(). If you
> remove else, you do not need additional indent for the old filt_soread()
> function' body.
>
> + SOCKBUF_LOCK_ASSERT(&so->so_rcv);
>
> - if (kn->kn_sfflags & NOTE_LOWAT) {
> - if (kn->kn_data >= kn->kn_sdata)
> - return 1;
> - } else {
> - if (sbavail(&so->so_rcv) >= so->so_rcv.sb_lowat)
> - return 1;
> + kn->kn_data = sbavail(&so->so_rcv) - so->so_rcv.sb_ctl;
> + if (so->so_rcv.sb_state & SBS_CANTRCVMORE) {
> + kn->kn_flags |= EV_EOF;
> + kn->kn_fflags = so->so_error;
> + return (1);
> + } else if (so->so_error) /* temporary udp error */
> + return (1);
> +
> + if (kn->kn_sfflags & NOTE_LOWAT) {
> + if (kn->kn_data >= kn->kn_sdata)
> + return 1;
> return (1);
> since you change the line anyway.
>
> + } else {
> + if (sbavail(&so->so_rcv) >= so->so_rcv.sb_lowat)
> + return 1;
> Same.
>
> + }
> +
> + /* This hook returning non-zero indicates an event, not
> error */
> + return (hhook_run_socket(so, NULL, HHOOK_FILT_SOREAD));
> }
> -
> - /* This hook returning non-zero indicates an event, not error */
> - return (hhook_run_socket(so, NULL, HHOOK_FILT_SOREAD));
> }
>
> static void
> @@ -3346,16 +3342,6 @@
> return (kn->kn_data >= so->so_snd.sb_lowat);
> }
>
> -/*ARGSUSED*/
> -static int
> -filt_solisten(struct knote *kn, long hint)
> -{
> - struct socket *so = kn->kn_fp->f_data;
> -
> - kn->kn_data = so->so_qlen;
> - return (!TAILQ_EMPTY(&so->so_comp));
> -}
> -
> int
> socheckuid(struct socket *so, uid_t uid)
> {
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAFn2buB8MdQ32BY9_ArQbAMp=UYFgm=mpQ0Ft2HjHvaRDHiRUQ>