asio and kqueue (2nd trye) (was: RE: (boost::)asio and kqueue problem)

[Scott Mitchell]

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)
> @@ -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)
>  {
>