Re: [PATCH v2 16/18] mini-os: reset file type in close() in one place only

[Andrew Cooper <amc96@xxxxxxxx>]

On 11/01/2022 14:58, Juergen Gross wrote:
> diff --git a/lib/sys.c b/lib/sys.c
> index 0e6fe5d..323a7cd 100644
> --- a/lib/sys.c
> +++ b/lib/sys.c
> @@ -424,87 +424,82 @@ int fsync(int fd) {
>  
>  int close(int fd)
>  {
> +    int res = 0;
> +
>      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);

Hard tabs.

> +            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;
> +            printk("close(%d): Bad descriptor\n", fd);
> +            errno = EBADF;
> +            return -1;
>      }
> -    printk("close(%d): Bad descriptor\n", fd);
> -    errno = EBADF;
> -    return -1;
> +
> +    memset(files + fd, 0, sizeof(struct file));
> +    files[fd].type = FTYPE_NONE;

BUILD_BUG_ON(FTYPE_NONE != 0);

Life's too short to deal with a theoretical (and short sighted) future
where someone might want to change FTYPE_NONE away from being 0.

~Andrew