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

[Juergen Gross <jgross@xxxxxxxx>]

On 11.01.22 19:11, Andrew Cooper wrote:
> 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.
Good idea.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature