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 <kostik...@gmail.com> wrote: > On Fri, Oct 14, 2016 at 09:21:52AM +0000, hartmut.bra...@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) > { > _______________________________________________ freebsd-current@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"