Re: Merge uipc_bind() with unp_bind()
On Mon, Nov 14, 2022 at 09:28:34AM +, Klemens Nanni wrote:
> On Mon, Nov 14, 2022 at 12:11:46PM +0300, Vitaliy Makkoveev wrote:
> > uipc_bind() only calls unp_bind(). Also it is the only caller of
> > unp_bind().
>
> For *_bind() alone this looks like zapping a useless indirection, but
> the rest of uipc_*() seems to consistently use unp_*() still, so I'm not
> convinced this one-off merge is an improvement.
>
You talk about unp_{,dis}connect(). Such unp_*() contains the code
called from multiple places, not only from corresponding uipc_*(). The
uipc_bind() and unp_bind() are the different case.
In the uipc_usrreq() times it made sense to have unp_bind() for prevent
code mess within big switch(), but since uipc_usrreq() was splitted to
multiple handlers there is no reason to keep both uipc_bind() and
unp_bind().
> >
> > Index: sys/kern/uipc_usrreq.c
> > ===
> > RCS file: /cvs/src/sys/kern/uipc_usrreq.c,v
> > retrieving revision 1.192
> > diff -u -p -r1.192 uipc_usrreq.c
> > --- sys/kern/uipc_usrreq.c 13 Nov 2022 16:01:32 - 1.192
> > +++ sys/kern/uipc_usrreq.c 14 Nov 2022 09:07:43 -
> > @@ -322,8 +322,93 @@ int
> > uipc_bind(struct socket *so, struct mbuf *nam, struct proc *p)
> > {
> > struct unpcb *unp = sotounpcb(so);
> > + struct sockaddr_un *soun;
> > + struct mbuf *nam2;
> > + struct vnode *vp;
> > + struct vattr vattr;
> > + int error;
> > + struct nameidata nd;
> > + size_t pathlen;
> >
> > - return unp_bind(unp, nam, p);
> > + if (unp->unp_flags & (UNP_BINDING | UNP_CONNECTING))
> > + return (EINVAL);
> > + if (unp->unp_vnode != NULL)
> > + return (EINVAL);
> > + if ((error = unp_nam2sun(nam, , )))
> > + return (error);
> > +
> > + unp->unp_flags |= UNP_BINDING;
> > +
> > + /*
> > +* Enforce `i_lock' -> `unplock' because fifo subsystem
> > +* requires it. The socket can't be closed concurrently
> > +* because the file descriptor reference is still held.
> > +*/
> > +
> > + sounlock(unp->unp_socket);
> > +
> > + nam2 = m_getclr(M_WAITOK, MT_SONAME);
> > + nam2->m_len = sizeof(struct sockaddr_un);
> > + memcpy(mtod(nam2, struct sockaddr_un *), soun,
> > + offsetof(struct sockaddr_un, sun_path) + pathlen);
> > + /* No need to NUL terminate: m_getclr() returns zero'd mbufs. */
> > +
> > + soun = mtod(nam2, struct sockaddr_un *);
> > +
> > + /* Fixup sun_len to keep it in sync with m_len. */
> > + soun->sun_len = nam2->m_len;
> > +
> > + NDINIT(, CREATE, NOFOLLOW | LOCKPARENT, UIO_SYSSPACE,
> > + soun->sun_path, p);
> > + nd.ni_pledge = PLEDGE_UNIX;
> > + nd.ni_unveil = UNVEIL_CREATE;
> > +
> > + KERNEL_LOCK();
> > +/* SHOULD BE ABLE TO ADOPT EXISTING AND wakeup() ALA FIFO's */
> > + error = namei();
> > + if (error != 0) {
> > + m_freem(nam2);
> > + solock(unp->unp_socket);
> > + goto out;
> > + }
> > + vp = nd.ni_vp;
> > + if (vp != NULL) {
> > + VOP_ABORTOP(nd.ni_dvp, _cnd);
> > + if (nd.ni_dvp == vp)
> > + vrele(nd.ni_dvp);
> > + else
> > + vput(nd.ni_dvp);
> > + vrele(vp);
> > + m_freem(nam2);
> > + error = EADDRINUSE;
> > + solock(unp->unp_socket);
> > + goto out;
> > + }
> > + VATTR_NULL();
> > + vattr.va_type = VSOCK;
> > + vattr.va_mode = ACCESSPERMS &~ p->p_fd->fd_cmask;
> > + error = VOP_CREATE(nd.ni_dvp, _vp, _cnd, );
> > + vput(nd.ni_dvp);
> > + if (error) {
> > + m_freem(nam2);
> > + solock(unp->unp_socket);
> > + goto out;
> > + }
> > + solock(unp->unp_socket);
> > + unp->unp_addr = nam2;
> > + vp = nd.ni_vp;
> > + vp->v_socket = unp->unp_socket;
> > + unp->unp_vnode = vp;
> > + unp->unp_connid.uid = p->p_ucred->cr_uid;
> > + unp->unp_connid.gid = p->p_ucred->cr_gid;
> > + unp->unp_connid.pid = p->p_p->ps_pid;
> > + unp->unp_flags |= UNP_FEIDSBIND;
> > + VOP_UNLOCK(vp);
> > +out:
> > + KERNEL_UNLOCK();
> > + unp->unp_flags &= ~UNP_BINDING;
> > +
> > + return (error);
> > }
> >
> > int
> > @@ -724,98 +809,6 @@ unp_detach(struct unpcb *unp)
> > }
> >
> > int
> > -unp_bind(struct unpcb *unp, struct mbuf *nam, struct proc *p)
> > -{
> > - struct sockaddr_un *soun;
> > - struct mbuf *nam2;
> > - struct vnode *vp;
> > - struct vattr vattr;
> > - int error;
> > - struct nameidata nd;
> > - size_t pathlen;
> > -
> > - if (unp->unp_flags & (UNP_BINDING | UNP_CONNECTING))
> > - return (EINVAL);
> > - if (unp->unp_vnode != NULL)
> > - return (EINVAL);
> > - if ((error = unp_nam2sun(nam, , )))
> > - return (error);
> > -
> > - unp->unp_flags |= UNP_BINDING;
> > -
> > - /*
> > -* Enforce `i_lock' -> `unplock' because fifo subsystem
> > -* requires it. The socket can't be closed concurrently
> > -*
Re: Merge uipc_bind() with unp_bind()
On Mon, Nov 14, 2022 at 12:11:46PM +0300, Vitaliy Makkoveev wrote:
> uipc_bind() only calls unp_bind(). Also it is the only caller of
> unp_bind().
For *_bind() alone this looks like zapping a useless indirection, but
the rest of uipc_*() seems to consistently use unp_*() still, so I'm not
convinced this one-off merge is an improvement.
>
> Index: sys/kern/uipc_usrreq.c
> ===
> RCS file: /cvs/src/sys/kern/uipc_usrreq.c,v
> retrieving revision 1.192
> diff -u -p -r1.192 uipc_usrreq.c
> --- sys/kern/uipc_usrreq.c13 Nov 2022 16:01:32 - 1.192
> +++ sys/kern/uipc_usrreq.c14 Nov 2022 09:07:43 -
> @@ -322,8 +322,93 @@ int
> uipc_bind(struct socket *so, struct mbuf *nam, struct proc *p)
> {
> struct unpcb *unp = sotounpcb(so);
> + struct sockaddr_un *soun;
> + struct mbuf *nam2;
> + struct vnode *vp;
> + struct vattr vattr;
> + int error;
> + struct nameidata nd;
> + size_t pathlen;
>
> - return unp_bind(unp, nam, p);
> + if (unp->unp_flags & (UNP_BINDING | UNP_CONNECTING))
> + return (EINVAL);
> + if (unp->unp_vnode != NULL)
> + return (EINVAL);
> + if ((error = unp_nam2sun(nam, , )))
> + return (error);
> +
> + unp->unp_flags |= UNP_BINDING;
> +
> + /*
> + * Enforce `i_lock' -> `unplock' because fifo subsystem
> + * requires it. The socket can't be closed concurrently
> + * because the file descriptor reference is still held.
> + */
> +
> + sounlock(unp->unp_socket);
> +
> + nam2 = m_getclr(M_WAITOK, MT_SONAME);
> + nam2->m_len = sizeof(struct sockaddr_un);
> + memcpy(mtod(nam2, struct sockaddr_un *), soun,
> + offsetof(struct sockaddr_un, sun_path) + pathlen);
> + /* No need to NUL terminate: m_getclr() returns zero'd mbufs. */
> +
> + soun = mtod(nam2, struct sockaddr_un *);
> +
> + /* Fixup sun_len to keep it in sync with m_len. */
> + soun->sun_len = nam2->m_len;
> +
> + NDINIT(, CREATE, NOFOLLOW | LOCKPARENT, UIO_SYSSPACE,
> + soun->sun_path, p);
> + nd.ni_pledge = PLEDGE_UNIX;
> + nd.ni_unveil = UNVEIL_CREATE;
> +
> + KERNEL_LOCK();
> +/* SHOULD BE ABLE TO ADOPT EXISTING AND wakeup() ALA FIFO's */
> + error = namei();
> + if (error != 0) {
> + m_freem(nam2);
> + solock(unp->unp_socket);
> + goto out;
> + }
> + vp = nd.ni_vp;
> + if (vp != NULL) {
> + VOP_ABORTOP(nd.ni_dvp, _cnd);
> + if (nd.ni_dvp == vp)
> + vrele(nd.ni_dvp);
> + else
> + vput(nd.ni_dvp);
> + vrele(vp);
> + m_freem(nam2);
> + error = EADDRINUSE;
> + solock(unp->unp_socket);
> + goto out;
> + }
> + VATTR_NULL();
> + vattr.va_type = VSOCK;
> + vattr.va_mode = ACCESSPERMS &~ p->p_fd->fd_cmask;
> + error = VOP_CREATE(nd.ni_dvp, _vp, _cnd, );
> + vput(nd.ni_dvp);
> + if (error) {
> + m_freem(nam2);
> + solock(unp->unp_socket);
> + goto out;
> + }
> + solock(unp->unp_socket);
> + unp->unp_addr = nam2;
> + vp = nd.ni_vp;
> + vp->v_socket = unp->unp_socket;
> + unp->unp_vnode = vp;
> + unp->unp_connid.uid = p->p_ucred->cr_uid;
> + unp->unp_connid.gid = p->p_ucred->cr_gid;
> + unp->unp_connid.pid = p->p_p->ps_pid;
> + unp->unp_flags |= UNP_FEIDSBIND;
> + VOP_UNLOCK(vp);
> +out:
> + KERNEL_UNLOCK();
> + unp->unp_flags &= ~UNP_BINDING;
> +
> + return (error);
> }
>
> int
> @@ -724,98 +809,6 @@ unp_detach(struct unpcb *unp)
> }
>
> int
> -unp_bind(struct unpcb *unp, struct mbuf *nam, struct proc *p)
> -{
> - struct sockaddr_un *soun;
> - struct mbuf *nam2;
> - struct vnode *vp;
> - struct vattr vattr;
> - int error;
> - struct nameidata nd;
> - size_t pathlen;
> -
> - if (unp->unp_flags & (UNP_BINDING | UNP_CONNECTING))
> - return (EINVAL);
> - if (unp->unp_vnode != NULL)
> - return (EINVAL);
> - if ((error = unp_nam2sun(nam, , )))
> - return (error);
> -
> - unp->unp_flags |= UNP_BINDING;
> -
> - /*
> - * Enforce `i_lock' -> `unplock' because fifo subsystem
> - * requires it. The socket can't be closed concurrently
> - * because the file descriptor reference is still held.
> - */
> -
> - sounlock(unp->unp_socket);
> -
> - nam2 = m_getclr(M_WAITOK, MT_SONAME);
> - nam2->m_len = sizeof(struct sockaddr_un);
> - memcpy(mtod(nam2, struct sockaddr_un *), soun,
> - offsetof(struct sockaddr_un, sun_path) + pathlen);
> - /* No need to NUL terminate: m_getclr() returns zero'd mbufs. */
> -
> - soun = mtod(nam2, struct sockaddr_un *);
> -
> - /* Fixup sun_len to keep it in sync with m_len. */
> - soun->sun_len = nam2->m_len;
> -
> -
Merge uipc_bind() with unp_bind()
uipc_bind() only calls unp_bind(). Also it is the only caller of
unp_bind().
Index: sys/kern/uipc_usrreq.c
===
RCS file: /cvs/src/sys/kern/uipc_usrreq.c,v
retrieving revision 1.192
diff -u -p -r1.192 uipc_usrreq.c
--- sys/kern/uipc_usrreq.c 13 Nov 2022 16:01:32 - 1.192
+++ sys/kern/uipc_usrreq.c 14 Nov 2022 09:07:43 -
@@ -322,8 +322,93 @@ int
uipc_bind(struct socket *so, struct mbuf *nam, struct proc *p)
{
struct unpcb *unp = sotounpcb(so);
+ struct sockaddr_un *soun;
+ struct mbuf *nam2;
+ struct vnode *vp;
+ struct vattr vattr;
+ int error;
+ struct nameidata nd;
+ size_t pathlen;
- return unp_bind(unp, nam, p);
+ if (unp->unp_flags & (UNP_BINDING | UNP_CONNECTING))
+ return (EINVAL);
+ if (unp->unp_vnode != NULL)
+ return (EINVAL);
+ if ((error = unp_nam2sun(nam, , )))
+ return (error);
+
+ unp->unp_flags |= UNP_BINDING;
+
+ /*
+* Enforce `i_lock' -> `unplock' because fifo subsystem
+* requires it. The socket can't be closed concurrently
+* because the file descriptor reference is still held.
+*/
+
+ sounlock(unp->unp_socket);
+
+ nam2 = m_getclr(M_WAITOK, MT_SONAME);
+ nam2->m_len = sizeof(struct sockaddr_un);
+ memcpy(mtod(nam2, struct sockaddr_un *), soun,
+ offsetof(struct sockaddr_un, sun_path) + pathlen);
+ /* No need to NUL terminate: m_getclr() returns zero'd mbufs. */
+
+ soun = mtod(nam2, struct sockaddr_un *);
+
+ /* Fixup sun_len to keep it in sync with m_len. */
+ soun->sun_len = nam2->m_len;
+
+ NDINIT(, CREATE, NOFOLLOW | LOCKPARENT, UIO_SYSSPACE,
+ soun->sun_path, p);
+ nd.ni_pledge = PLEDGE_UNIX;
+ nd.ni_unveil = UNVEIL_CREATE;
+
+ KERNEL_LOCK();
+/* SHOULD BE ABLE TO ADOPT EXISTING AND wakeup() ALA FIFO's */
+ error = namei();
+ if (error != 0) {
+ m_freem(nam2);
+ solock(unp->unp_socket);
+ goto out;
+ }
+ vp = nd.ni_vp;
+ if (vp != NULL) {
+ VOP_ABORTOP(nd.ni_dvp, _cnd);
+ if (nd.ni_dvp == vp)
+ vrele(nd.ni_dvp);
+ else
+ vput(nd.ni_dvp);
+ vrele(vp);
+ m_freem(nam2);
+ error = EADDRINUSE;
+ solock(unp->unp_socket);
+ goto out;
+ }
+ VATTR_NULL();
+ vattr.va_type = VSOCK;
+ vattr.va_mode = ACCESSPERMS &~ p->p_fd->fd_cmask;
+ error = VOP_CREATE(nd.ni_dvp, _vp, _cnd, );
+ vput(nd.ni_dvp);
+ if (error) {
+ m_freem(nam2);
+ solock(unp->unp_socket);
+ goto out;
+ }
+ solock(unp->unp_socket);
+ unp->unp_addr = nam2;
+ vp = nd.ni_vp;
+ vp->v_socket = unp->unp_socket;
+ unp->unp_vnode = vp;
+ unp->unp_connid.uid = p->p_ucred->cr_uid;
+ unp->unp_connid.gid = p->p_ucred->cr_gid;
+ unp->unp_connid.pid = p->p_p->ps_pid;
+ unp->unp_flags |= UNP_FEIDSBIND;
+ VOP_UNLOCK(vp);
+out:
+ KERNEL_UNLOCK();
+ unp->unp_flags &= ~UNP_BINDING;
+
+ return (error);
}
int
@@ -724,98 +809,6 @@ unp_detach(struct unpcb *unp)
}
int
-unp_bind(struct unpcb *unp, struct mbuf *nam, struct proc *p)
-{
- struct sockaddr_un *soun;
- struct mbuf *nam2;
- struct vnode *vp;
- struct vattr vattr;
- int error;
- struct nameidata nd;
- size_t pathlen;
-
- if (unp->unp_flags & (UNP_BINDING | UNP_CONNECTING))
- return (EINVAL);
- if (unp->unp_vnode != NULL)
- return (EINVAL);
- if ((error = unp_nam2sun(nam, , )))
- return (error);
-
- unp->unp_flags |= UNP_BINDING;
-
- /*
-* Enforce `i_lock' -> `unplock' because fifo subsystem
-* requires it. The socket can't be closed concurrently
-* because the file descriptor reference is still held.
-*/
-
- sounlock(unp->unp_socket);
-
- nam2 = m_getclr(M_WAITOK, MT_SONAME);
- nam2->m_len = sizeof(struct sockaddr_un);
- memcpy(mtod(nam2, struct sockaddr_un *), soun,
- offsetof(struct sockaddr_un, sun_path) + pathlen);
- /* No need to NUL terminate: m_getclr() returns zero'd mbufs. */
-
- soun = mtod(nam2, struct sockaddr_un *);
-
- /* Fixup sun_len to keep it in sync with m_len. */
- soun->sun_len = nam2->m_len;
-
- NDINIT(, CREATE, NOFOLLOW | LOCKPARENT, UIO_SYSSPACE,
- soun->sun_path, p);
- nd.ni_pledge = PLEDGE_UNIX;
- nd.ni_unveil = UNVEIL_CREATE;
-
- KERNEL_LOCK();
-/* SHOULD BE ABLE TO ADOPT EXISTING AND wakeup() ALA FIFO's */
- error = namei();
- if (error != 0) {
- m_freem(nam2);
-
