Re: [MINIOS PATCH v3 2/5] reset file type in close() in one place only

[Samuel Thibault <samuel.thibault@xxxxxxxxxxxx>]

Juergen Gross, le dim. 16 janv. 2022 07:45:24 +0100, a ecrit:
> Today the file type in struct file is set to FTYPE_NONE for each
> file type individually. Do that at the end of close() handling for
> all types.
> 
> While at it wipe the complete struct file, too, in order to avoid
> old data creeping into a new allocated struct file.
> 
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>

Reviewed-by: Samuel Thibault <samuel.thibault@xxxxxxxxxxxx>

> ---
> V2:
> - new patch
> V3:
> - add error handling (Andrew Cooper)
> - use BUILD_BUG_ON() (Andrew Cooper)
> ---
>  lib/sys.c | 56 ++++++++++++++++++++++++++++---------------------------
>  lib/xs.c  |  1 -
>  2 files changed, 29 insertions(+), 28 deletions(-)
> 
> diff --git a/lib/sys.c b/lib/sys.c
> index 9a03875c..7df77cc6 100644
> --- a/lib/sys.c
> +++ b/lib/sys.c
> @@ -265,7 +265,7 @@ int read(int fd, void *buf, size_t nbytes)
>              return ret;
>          }
>  #ifdef HAVE_LWIP
> -     case FTYPE_SOCKET:
> +        case FTYPE_SOCKET:
>           return lwip_read(files[fd].fd, buf, nbytes);
>  #endif
>  #ifdef CONFIG_NETFRONT
> @@ -424,84 +424,86 @@ int fsync(int fd) {
>  
>  int close(int fd)
>  {
> +    int res = 0;
> +
> +    if ( fd < 0 || fd >= ARRAY_SIZE(files) )
> +        goto error;
> +
>      printk("close(%d)\n", fd);
>      switch (files[fd].type) {
>          default:
> -         files[fd].type = FTYPE_NONE;
> -         return 0;
> +            break;
>  #ifdef CONFIG_XENBUS
>       case FTYPE_XENBUS:
>              xs_daemon_close((void*)(intptr_t) fd);
> -            return 0;
> +            break;
>  #endif
>  #ifdef HAVE_LWIP
> -     case FTYPE_SOCKET: {
> -         int res = lwip_close(files[fd].fd);
> -         files[fd].type = FTYPE_NONE;
> -         return res;
> -     }
> +     case FTYPE_SOCKET:
> +            res = lwip_close(files[fd].fd);
> +            break;
>  #endif
>  #ifdef CONFIG_LIBXENCTRL
>       case FTYPE_XC:
>           minios_interface_close_fd(fd);
> -         return 0;
> +            break;
>  #endif
>  #ifdef CONFIG_LIBXENEVTCHN
>       case FTYPE_EVTCHN:
>           minios_evtchn_close_fd(fd);
> -            return 0;
> +            break;
>  #endif
>  #ifdef CONFIG_LIBXENGNTTAB
>       case FTYPE_GNTMAP:
>           minios_gnttab_close_fd(fd);
> -         return 0;
> +            break;
>  #endif
>  #ifdef CONFIG_NETFRONT
>       case FTYPE_TAP:
>           shutdown_netfront(files[fd].dev);
> -         files[fd].type = FTYPE_NONE;
> -         return 0;
> +            break;
>  #endif
>  #ifdef CONFIG_BLKFRONT
>       case FTYPE_BLK:
>              shutdown_blkfront(files[fd].dev);
> -         files[fd].type = FTYPE_NONE;
> -         return 0;
> +            break;
>  #endif
>  #ifdef CONFIG_TPMFRONT
>       case FTYPE_TPMFRONT:
>              shutdown_tpmfront(files[fd].dev);
> -         files[fd].type = FTYPE_NONE;
> -         return 0;
> +            break;
>  #endif
>  #ifdef CONFIG_TPM_TIS
>       case FTYPE_TPM_TIS:
>              shutdown_tpm_tis(files[fd].dev);
> -         files[fd].type = FTYPE_NONE;
> -         return 0;
> +            break;
>  #endif
>  #ifdef CONFIG_KBDFRONT
>       case FTYPE_KBD:
>              shutdown_kbdfront(files[fd].dev);
> -            files[fd].type = FTYPE_NONE;
> -            return 0;
> +            break;
>  #endif
>  #ifdef CONFIG_FBFRONT
>       case FTYPE_FB:
>              shutdown_fbfront(files[fd].dev);
> -            files[fd].type = FTYPE_NONE;
> -            return 0;
> +            break;
>  #endif
>  #ifdef CONFIG_CONSFRONT
>          case FTYPE_SAVEFILE:
>          case FTYPE_CONSOLE:
>              fini_consfront(files[fd].dev);
> -            files[fd].type = FTYPE_NONE;
> -            return 0;
> +            break;
>  #endif
>       case FTYPE_NONE:
> -         break;
> +            goto error;
>      }
> +
> +    memset(files + fd, 0, sizeof(struct file));
> +    BUILD_BUG_ON(FTYPE_NONE != 0);
> +
> +    return res;
> +
> + error:
>      printk("close(%d): Bad descriptor\n", fd);
>      errno = EBADF;
>      return -1;
> diff --git a/lib/xs.c b/lib/xs.c
> index 0459f52f..4af0f960 100644
> --- a/lib/xs.c
> +++ b/lib/xs.c
> @@ -35,7 +35,6 @@ void xs_daemon_close(struct xs_handle *h)
>          next = event->next;
>          free(event);
>      }
> -    files[fd].type = FTYPE_NONE;
>  }
>  
>  int xs_fileno(struct xs_handle *h)
> -- 
> 2.26.2
> 

-- 
Samuel
/*
 * [...] Note that 120 sec is defined in the protocol as the maximum
 * possible RTT.  I guess we'll have to use something other than TCP
 * to talk to the University of Mars.
 * PAWS allows us longer timeouts and large windows, so once implemented
 * ftp to mars will work nicely.
 */
(from /usr/src/linux/net/inet/tcp.c, concerning RTT [retransmission timeout])