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_at_gmail.com> wrote: > On Fri, Oct 14, 2016 at 09:21:52AM +0000, Hartmut.Brandt_at_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) > _at__at_ -159,15 +159,9 _at__at_ > 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, > _at__at_ -3075,10 +3069,7 _at__at_ > > 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: > _at__at_ -3282,29 +3273,34 _at__at_ > 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 > _at__at_ -3346,16 +3342,6 _at__at_ > 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) > { >Received on Fri Oct 14 2016 - 14:24:00 UTC
This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:41:08 UTC