Re: Earlier FREF() for sys_ioctl()

[Alexander Bluhm]

On Tue, Apr 03, 2018 at 04:48:09PM +0200, Martin Pieuchot wrote:
> Similar to other diffs, this one move a FREF() right after
> fd_getfile_mode(), ok?

OK bluhm@

> Index: kern/sys_generic.c
> ===
> RCS file: /cvs/src/sys/kern/sys_generic.c,v
> retrieving revision 1.116
> diff -u -p -r1.116 sys_generic.c
> --- kern/sys_generic.c2 Jan 2018 06:38:45 -   1.116
> +++ kern/sys_generic.c3 Apr 2018 13:32:32 -
> @@ -393,29 +393,30 @@ sys_ioctl(struct proc *p, void *v, regis
>   struct file *fp;
>   struct filedesc *fdp;
>   u_long com = SCARG(uap, com);
> - int error;
> + int error = 0;
>   u_int size;
> - caddr_t data, memp;
> + caddr_t data, memp = NULL;
>   int tmp;
>  #define STK_PARAMS   128
>   long long stkbuf[STK_PARAMS / sizeof(long long)];
>  
>   fdp = p->p_fd;
> - fp = fd_getfile_mode(fdp, SCARG(uap, fd), FREAD|FWRITE);
> -
> - if (fp == NULL)
> + if ((fp = fd_getfile_mode(fdp, SCARG(uap, fd), FREAD|FWRITE)) == NULL)
>   return (EBADF);
> + FREF(fp);
>  
>   if (fp->f_type == DTYPE_SOCKET) {
>   struct socket *so = fp->f_data;
>  
> - if (so->so_state & SS_DNS)
> - return (EINVAL);
> + if (so->so_state & SS_DNS) {
> + error = EINVAL;
> + goto out;
> + }
>   }
>  
>   error = pledge_ioctl(p, com, fp);
>   if (error)
> - return (error);
> + goto out;
>  
>   switch (com) {
>   case FIONCLEX:
> @@ -426,7 +427,7 @@ sys_ioctl(struct proc *p, void *v, regis
>   else
>   fdp->fd_ofileflags[SCARG(uap, fd)] |= UF_EXCLOSE;
>   fdpunlock(fdp);
> - return (0);
> + goto out;
>   }
>  
>   /*
> @@ -434,10 +435,10 @@ sys_ioctl(struct proc *p, void *v, regis
>* copied to/from the user's address space.
>*/
>   size = IOCPARM_LEN(com);
> - if (size > IOCPARM_MAX)
> - return (ENOTTY);
> - FREF(fp);
> - memp = NULL;
> + if (size > IOCPARM_MAX) {
> + error = ENOTTY;
> + goto out;
> + }
>   if (size > sizeof (stkbuf)) {
>   memp = malloc(size, M_IOCTLOPS, M_WAITOK);
>   data = memp;
> @@ -525,8 +526,7 @@ sys_ioctl(struct proc *p, void *v, regis
>   error = copyout(data, SCARG(uap, data), size);
>  out:
>   FRELE(fp, p);
> - if (memp)
> - free(memp, M_IOCTLOPS, size);
> + free(memp, M_IOCTLOPS, size);
>   return (error);
>  }
>  

Re: Earlier FREF() for sys_ioctl()

[Martin Pieuchot]

On 03/04/18(Tue) 16:59, Mark Kettenis wrote:
> > Date: Tue, 3 Apr 2018 16:48:09 +0200
> > From: Martin Pieuchot 
> > 
> > Similar to other diffs, this one move a FREF() right after
> > fd_getfile_mode(), ok?
> > 
> > Index: kern/sys_generic.c
> > ===
> > RCS file: /cvs/src/sys/kern/sys_generic.c,v
> > retrieving revision 1.116
> > diff -u -p -r1.116 sys_generic.c
> > --- kern/sys_generic.c  2 Jan 2018 06:38:45 -   1.116
> > +++ kern/sys_generic.c  3 Apr 2018 13:32:32 -
> > @@ -393,29 +393,30 @@ sys_ioctl(struct proc *p, void *v, regis
> > struct file *fp;
> > struct filedesc *fdp;
> > u_long com = SCARG(uap, com);
> > -   int error;
> > +   int error = 0;
> > u_int size;
> > -   caddr_t data, memp;
> > +   caddr_t data, memp = NULL;
> > int tmp;
> >  #define STK_PARAMS 128
> > long long stkbuf[STK_PARAMS / sizeof(long long)];
> >  
> > fdp = p->p_fd;
> > -   fp = fd_getfile_mode(fdp, SCARG(uap, fd), FREAD|FWRITE);
> > -
> > -   if (fp == NULL)
> > +   if ((fp = fd_getfile_mode(fdp, SCARG(uap, fd), FREAD|FWRITE)) == NULL)
> > return (EBADF);
> 
> I find that assignments within if statements make code harder to read.

I agree but that's coherent with the rest of the file.  I don't mind
changing it though.

Anything else?

Re: Earlier FREF() for sys_ioctl()

[Mark Kettenis]

> Date: Tue, 3 Apr 2018 16:48:09 +0200
> From: Martin Pieuchot 
> 
> Similar to other diffs, this one move a FREF() right after
> fd_getfile_mode(), ok?
> 
> Index: kern/sys_generic.c
> ===
> RCS file: /cvs/src/sys/kern/sys_generic.c,v
> retrieving revision 1.116
> diff -u -p -r1.116 sys_generic.c
> --- kern/sys_generic.c2 Jan 2018 06:38:45 -   1.116
> +++ kern/sys_generic.c3 Apr 2018 13:32:32 -
> @@ -393,29 +393,30 @@ sys_ioctl(struct proc *p, void *v, regis
>   struct file *fp;
>   struct filedesc *fdp;
>   u_long com = SCARG(uap, com);
> - int error;
> + int error = 0;
>   u_int size;
> - caddr_t data, memp;
> + caddr_t data, memp = NULL;
>   int tmp;
>  #define STK_PARAMS   128
>   long long stkbuf[STK_PARAMS / sizeof(long long)];
>  
>   fdp = p->p_fd;
> - fp = fd_getfile_mode(fdp, SCARG(uap, fd), FREAD|FWRITE);
> -
> - if (fp == NULL)
> + if ((fp = fd_getfile_mode(fdp, SCARG(uap, fd), FREAD|FWRITE)) == NULL)
>   return (EBADF);

I find that assignments within if statements make code harder to read.