On Mon, Jan 30, 2023 at 05:03:30PM +0300, Vitaliy Makkoveev wrote:
> It makes sense to push solock() down to sosetopt() too. For a half cases
> (*pr_ctloutput)() is just null op, so there is no reason to call it.
> Also, a lot of things could be done without solock() held.
You do a bunch of things together
- push solock()
- move code around
- return error instead of error =
- set sb to &so->so_snd or &so->so_rcv
Some of the refactorings make sense, others change the code just
to be different in my eyes.
Can we discuss theses things in separate diffs?
bluhm
> Index: sys/kern/uipc_socket.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/uipc_socket.c,v
> retrieving revision 1.299
> diff -u -p -r1.299 uipc_socket.c
> --- sys/kern/uipc_socket.c 27 Jan 2023 21:01:59 -0000 1.299
> +++ sys/kern/uipc_socket.c 30 Jan 2023 14:02:29 -0000
> @@ -1777,63 +1777,25 @@ sosetopt(struct socket *so, int level, i
> {
> int error = 0;
>
> - soassertlocked(so);
> -
> if (level != SOL_SOCKET) {
> if (so->so_proto->pr_ctloutput) {
> + solock(so);
> error = (*so->so_proto->pr_ctloutput)(PRCO_SETOPT, so,
> level, optname, m);
> + sounlock(so);
> return (error);
> - }
> - error = ENOPROTOOPT;
> + } else
> + return (ENOPROTOOPT);
> } else {
> switch (optname) {
> - case SO_BINDANY:
> - if ((error = suser(curproc)) != 0) /* XXX */
> - return (error);
> - break;
> - }
> -
> - switch (optname) {
> -
> - case SO_LINGER:
> - if (m == NULL || m->m_len != sizeof (struct linger) ||
> - mtod(m, struct linger *)->l_linger < 0 ||
> - mtod(m, struct linger *)->l_linger > SHRT_MAX)
> - return (EINVAL);
> - so->so_linger = mtod(m, struct linger *)->l_linger;
> - /* FALLTHROUGH */
> -
> - case SO_BINDANY:
> - case SO_DEBUG:
> - case SO_KEEPALIVE:
> - case SO_USELOOPBACK:
> - case SO_BROADCAST:
> - case SO_REUSEADDR:
> - case SO_REUSEPORT:
> - case SO_OOBINLINE:
> - case SO_TIMESTAMP:
> - case SO_ZEROIZE:
> - if (m == NULL || m->m_len < sizeof (int))
> - return (EINVAL);
> - if (*mtod(m, int *))
> - so->so_options |= optname;
> - else
> - so->so_options &= ~optname;
> - break;
> -
> - case SO_DONTROUTE:
> - if (m == NULL || m->m_len < sizeof (int))
> - return (EINVAL);
> - if (*mtod(m, int *))
> - error = EOPNOTSUPP;
> - break;
> -
> case SO_SNDBUF:
> case SO_RCVBUF:
> case SO_SNDLOWAT:
> case SO_RCVLOWAT:
> {
> + struct sockbuf *sb = (
> + optname == (SO_SNDBUF | SO_SNDLOWAT) ?
> + &so->so_snd : &so->so_rcv);
> u_long cnt;
>
> if (m == NULL || m->m_len < sizeof (int))
> @@ -1841,43 +1803,42 @@ sosetopt(struct socket *so, int level, i
> cnt = *mtod(m, int *);
> if ((long)cnt <= 0)
> cnt = 1;
> - switch (optname) {
>
> - case SO_SNDBUF:
> - if (so->so_snd.sb_state & SS_CANTSENDMORE)
> - return (EINVAL);
> - if (sbcheckreserve(cnt, so->so_snd.sb_wat) ||
> - sbreserve(so, &so->so_snd, cnt))
> - return (ENOBUFS);
> - so->so_snd.sb_wat = cnt;
> - break;
> + solock(so);
>
> + switch (optname) {
> + case SO_SNDBUF:
> case SO_RCVBUF:
> - if (so->so_rcv.sb_state & SS_CANTRCVMORE)
> - return (EINVAL);
> - if (sbcheckreserve(cnt, so->so_rcv.sb_wat) ||
> - sbreserve(so, &so->so_rcv, cnt))
> - return (ENOBUFS);
> - so->so_rcv.sb_wat = cnt;
> + if (sb->sb_state &
> + (SS_CANTSENDMORE | SS_CANTRCVMORE)) {
> + error = EINVAL;
> + break;
> + }
> + if (sbcheckreserve(cnt, sb->sb_wat) ||
> + sbreserve(so, sb, cnt)) {
> + error = ENOBUFS;
> + break;
> + }
> + sb->sb_wat = cnt;
> break;
>
> - case SO_SNDLOWAT:
> - so->so_snd.sb_lowat =
> - (cnt > so->so_snd.sb_hiwat) ?
> - so->so_snd.sb_hiwat : cnt;
> - break;
> case SO_RCVLOWAT:
> - so->so_rcv.sb_lowat =
> - (cnt > so->so_rcv.sb_hiwat) ?
> - so->so_rcv.sb_hiwat : cnt;
> + case SO_SNDLOWAT:
> + sb->sb_lowat = (cnt > sb->sb_hiwat) ?
> + sb->sb_hiwat : cnt;
> break;
> }
> - break;
> +
> + sounlock(so);
> +
> + return (error);
> }
>
> case SO_SNDTIMEO:
> case SO_RCVTIMEO:
> {
> + struct sockbuf *sb = (optname == SO_SNDTIMEO ?
> + &so->so_snd : &so->so_rcv);
> struct timeval tv;
> uint64_t nsecs;
>
> @@ -1891,16 +1852,12 @@ sosetopt(struct socket *so, int level, i
> return (EDOM);
> if (nsecs == 0)
> nsecs = INFSLP;
> - switch (optname) {
>
> - case SO_SNDTIMEO:
> - so->so_snd.sb_timeo_nsecs = nsecs;
> - break;
> - case SO_RCVTIMEO:
> - so->so_rcv.sb_timeo_nsecs = nsecs;
> - break;
> - }
> - break;
> + solock(so);
> + sb->sb_timeo_nsecs = nsecs;
> + sounlock(so);
> +
> + return (0);
> }
>
> case SO_RTABLE:
> @@ -1911,18 +1868,30 @@ sosetopt(struct socket *so, int level, i
> so->so_proto->pr_domain;
>
> level = dom->dom_protosw->pr_protocol;
> +
> + solock(so);
> error = (*so->so_proto->pr_ctloutput)
> (PRCO_SETOPT, so, level, optname, m);
> + sounlock(so);
> +
> return (error);
> }
> - error = ENOPROTOOPT;
> - break;
> + return (ENOPROTOOPT);
> +
> + case SO_DONTROUTE:
> + if (m == NULL || m->m_len < sizeof (int))
> + return (EINVAL);
> + if (*mtod(m, int *))
> + return (EOPNOTSUPP);
> + return (0);
>
> #ifdef SOCKET_SPLICE
> case SO_SPLICE:
> + solock(so);
> if (m == NULL) {
> error = sosplice(so, -1, 0, NULL);
> } else if (m->m_len < sizeof(int)) {
> + sounlock(so);
> return (EINVAL);
> } else if (m->m_len < sizeof(struct splice)) {
> error = sosplice(so, *mtod(m, int *), 0, NULL);
> @@ -1932,9 +1901,52 @@ sosetopt(struct socket *so, int level, i
> mtod(m, struct splice *)->sp_max,
> &mtod(m, struct splice *)->sp_idle);
> }
> - break;
> + sounlock(so);
> +
> + return (error);
> #endif /* SOCKET_SPLICE */
>
> + }
> +
> + switch (optname) {
> + case SO_BINDANY:
> + if ((error = suser(curproc)) != 0) /* XXX */
> + return (error);
> + break;
> + }
> +
> + solock(so);
> +
> + switch (optname) {
> + case SO_LINGER:
> + if (m == NULL || m->m_len != sizeof (struct linger) ||
> + mtod(m, struct linger *)->l_linger < 0 ||
> + mtod(m, struct linger *)->l_linger > SHRT_MAX) {
> + error = EINVAL;
> + break;
> + }
> + so->so_linger = mtod(m, struct linger *)->l_linger;
> + /* FALLTHROUGH */
> + case SO_BINDANY:
> + case SO_DEBUG:
> + case SO_KEEPALIVE:
> + case SO_USELOOPBACK:
> + case SO_BROADCAST:
> + case SO_REUSEADDR:
> + case SO_REUSEPORT:
> + case SO_OOBINLINE:
> + case SO_TIMESTAMP:
> + case SO_ZEROIZE:
> + if (m == NULL || m->m_len < sizeof (int)) {
> + error = EINVAL;
> + break;
> + }
> + if (*mtod(m, int *))
> + so->so_options |= optname;
> + else
> + so->so_options &= ~optname;
> + break;
> +
> default:
> error = ENOPROTOOPT;
> break;
> @@ -1943,6 +1955,8 @@ sosetopt(struct socket *so, int level, i
> (*so->so_proto->pr_ctloutput)(PRCO_SETOPT, so,
> level, optname, m);
> }
> +
> + sounlock(so);
> }
>
> return (error);
> Index: sys/kern/uipc_syscalls.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/uipc_syscalls.c,v
> retrieving revision 1.211
> diff -u -p -r1.211 uipc_syscalls.c
> --- sys/kern/uipc_syscalls.c 27 Jan 2023 21:01:59 -0000 1.211
> +++ sys/kern/uipc_syscalls.c 30 Jan 2023 14:02:29 -0000
> @@ -1232,9 +1232,7 @@ sys_setsockopt(struct proc *p, void *v,
> m->m_len = SCARG(uap, valsize);
> }
> so = fp->f_data;
> - solock(so);
> error = sosetopt(so, SCARG(uap, level), SCARG(uap, name), m);
> - sounlock(so);
> bad:
> m_freem(m);
> FRELE(fp, p);
