[PATCH v2 00/18] mini-os: remove struct file dependency on config

[PATCH v2 00/18] mini-os: remove struct file dependency on config

[Juergen Gross]

Today the layout of struct file is depending on the Mini-OS
configuration. This is especially bad as the layout is exported to
external users like the Xen libraries built for Mini-OS, and those
are being built only once for multiple stubdom configurations.

Today there is no direct problem resulting from this, as the main
difference between struct file layouts is a large union containing all
the device specific data for the different file types. The largest
union member is not configuration dependant, so the build is currently
not broken.

In order to avoid any future problems this patch series is eliminating
the configuration dependency by replacing most of the device specific
union members by a single pointer.

The two union members used by Xen libraries can't be replaced yet, as
those need to be switched to use the generic pointer first.

In order to hide the Mini-OS internal implementation of the files
array, patches 15-17 are introducing a common framework to access a
struct file via its file descriptor, and to allocate new file types
dynamically instead of having them all pre-defined. The file type
specific operations are supplied via a function vector in order to
remove the dependency of lib/sys.c on all the various file types.

Changes in V2:
- added 3 more patches

Juergen Gross (18):
  mini-os: split struct file definition from its usage
  mini-os: makes file.read bool and move it ahead of device specific
    part
  mini-os: make offset a common struct file member for all types
  mini-os: replace multiple fd elements in struct file by common one
  mini-os: introduce a common dev pointer in struct file
  mini-os: eliminate blkfront union member in struct file
  mini-os: eliminate consfront union member in struct file
  mini-os: eliminate fbfront union member in struct file
  mini-os: eliminate kbdfront union member in struct file
  mini-os: eliminate netfront union member in struct file
  mini-os: move tpm respgot member of struct file to device specific
    data
  mini-os: eliminate tpmfront union member in struct file
  mini-os: eliminate tpmtis union member in struct file
  mini-os: eliminate xenbus union member in struct file
  mini-os: introduce get_file_from_fd()
  mini-os: reset file type in close() in one place only
  mini-os: use function vectors instead of switch for file operations
  mini-os: remove file type FTYPE_XC

 Config.mk                     |   1 -
 arch/x86/testbuild/all-no     |   1 -
 arch/x86/testbuild/all-yes    |   1 -
 arch/x86/testbuild/newxen-yes |   1 -
 blkfront.c                    |  15 +-
 console/xencons_ring.c        |   2 +-
 fbfront.c                     |  16 +-
 include/lib.h                 | 123 ++++++------
 include/tpmfront.h            |   2 +
 lib/sys.c                     | 363 +++++++++++++++++++++++-----------
 lib/xs.c                      |  14 +-
 netfront.c                    |   6 +-
 tpm_tis.c                     |  23 +--
 tpmfront.c                    |  33 ++--
 14 files changed, 352 insertions(+), 249 deletions(-)

-- 
2.26.2

[PATCH v2 01/18] mini-os: split struct file definition from its usage

[Juergen Gross]

Make the struct file definition standalone and use it for the
declaration of the files array.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
---
 include/lib.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/lib.h b/include/lib.h
index 39d6a18..a638bc9 100644
--- a/include/lib.h
+++ b/include/lib.h
@@ -181,7 +181,7 @@ struct evtchn_port_info {
         int bound;
 };
 
-extern struct file {
+struct file {
     enum fd_type type;
     union {
 	struct {
@@ -236,7 +236,9 @@ extern struct file {
 #endif
     };
     int read;	/* maybe available for read */
-} files[];
+};
+
+extern struct file files[];
 
 int alloc_fd(enum fd_type type);
 void close_all_files(void);
-- 
2.26.2

[PATCH v2 02/18] mini-os: makes file.read bool and move it ahead of device specific part

[Juergen Gross]

The read member of struct file should be bool.

In order to have the device specific part at the end of the structure
move "read" ahead of that.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
---
 blkfront.c             |  4 ++--
 console/xencons_ring.c |  2 +-
 fbfront.c              | 12 ++++++------
 include/lib.h          |  3 ++-
 netfront.c             |  4 ++--
 tpm_tis.c              |  2 +-
 tpmfront.c             |  6 +++---
 7 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/blkfront.c b/blkfront.c
index 834a978..7c8eb74 100644
--- a/blkfront.c
+++ b/blkfront.c
@@ -62,7 +62,7 @@ void blkfront_handler(evtchn_port_t port, struct pt_regs *regs, void *data)
     int fd = dev->fd;
 
     if (fd != -1)
-        files[fd].read = 1;
+        files[fd].read = true;
 #endif
     wake_up(&blkfront_queue);
 }
@@ -484,7 +484,7 @@ int blkfront_aio_poll(struct blkfront_dev *dev)
 moretodo:
 #ifdef HAVE_LIBC
     if (dev->fd != -1) {
-        files[dev->fd].read = 0;
+        files[dev->fd].read = false;
         mb(); /* Make sure to let the handler set read to 1 before we start looking at the ring */
     }
 #endif
diff --git a/console/xencons_ring.c b/console/xencons_ring.c
index b6db74e..c348f3c 100644
--- a/console/xencons_ring.c
+++ b/console/xencons_ring.c
@@ -102,7 +102,7 @@ void console_handle_input(evtchn_port_t port, struct pt_regs *regs, void *data)
         int fd = dev ? dev->fd : -1;
 
         if (fd != -1)
-            files[fd].read = 1;
+            files[fd].read = true;
 
         wake_up(&console_queue);
 #else
diff --git a/fbfront.c b/fbfront.c
index d3b3848..6725da1 100644
--- a/fbfront.c
+++ b/fbfront.c
@@ -45,7 +45,7 @@ void kbdfront_handler(evtchn_port_t port, struct pt_regs *regs, void *data)
     int fd = dev->fd;
 
     if (fd != -1)
-        files[fd].read = 1;
+        files[fd].read = true;
 #endif
     wake_up(&kbdfront_queue);
 }
@@ -207,7 +207,7 @@ int kbdfront_receive(struct kbdfront_dev *dev, union xenkbd_in_event *buf, int n
 
 #ifdef HAVE_LIBC
     if (dev->fd != -1) {
-        files[dev->fd].read = 0;
+        files[dev->fd].read = false;
         mb(); /* Make sure to let the handler set read to 1 before we start looking at the ring */
     }
 #endif
@@ -229,7 +229,7 @@ int kbdfront_receive(struct kbdfront_dev *dev, union xenkbd_in_event *buf, int n
 #ifdef HAVE_LIBC
     if (cons != prod && dev->fd != -1)
         /* still some events to read */
-        files[dev->fd].read = 1;
+        files[dev->fd].read = true;
 #endif
 
     return i;
@@ -349,7 +349,7 @@ void fbfront_handler(evtchn_port_t port, struct pt_regs *regs, void *data)
     int fd = dev->fd;
 
     if (fd != -1)
-        files[fd].read = 1;
+        files[fd].read = true;
 #endif
     wake_up(&fbfront_queue);
 }
@@ -376,7 +376,7 @@ int fbfront_receive(struct fbfront_dev *dev, union xenfb_in_event *buf, int n)
 
 #ifdef HAVE_LIBC
     if (dev->fd != -1) {
-        files[dev->fd].read = 0;
+        files[dev->fd].read = false;
         mb(); /* Make sure to let the handler set read to 1 before we start looking at the ring */
     }
 #endif
@@ -398,7 +398,7 @@ int fbfront_receive(struct fbfront_dev *dev, union xenfb_in_event *buf, int n)
 #ifdef HAVE_LIBC
     if (cons != prod && dev->fd != -1)
         /* still some events to read */
-        files[dev->fd].read = 1;
+        files[dev->fd].read = true;
 #endif
 
     return i;
diff --git a/include/lib.h b/include/lib.h
index a638bc9..df2de9e 100644
--- a/include/lib.h
+++ b/include/lib.h
@@ -49,6 +49,7 @@
 #define _LIB_H_
 
 #include <stdarg.h>
+#include <stdbool.h>
 #include <stddef.h>
 #include <xen/xen.h>
 #include <xen/event_channel.h>
@@ -183,6 +184,7 @@ struct evtchn_port_info {
 
 struct file {
     enum fd_type type;
+    bool read;	/* maybe available for read */
     union {
 	struct {
             /* lwIP fd */
@@ -235,7 +237,6 @@ struct file {
         } xenbus;
 #endif
     };
-    int read;	/* maybe available for read */
 };
 
 extern struct file files[];
diff --git a/netfront.c b/netfront.c
index dfe065b..a566e34 100644
--- a/netfront.c
+++ b/netfront.c
@@ -255,7 +255,7 @@ void netfront_select_handler(evtchn_port_t port, struct pt_regs *regs, void *dat
     local_irq_restore(flags);
 
     if (fd != -1)
-        files[fd].read = 1;
+        files[fd].read = true;
     wake_up(&netfront_queue);
 }
 #endif
@@ -783,7 +783,7 @@ ssize_t netfront_receive(struct netfront_dev *dev, unsigned char *data, size_t l
     network_rx(dev);
     if (!dev->rlen && fd != -1)
         /* No data for us, make select stop returning */
-        files[fd].read = 0;
+        files[fd].read = false;
     /* Before re-enabling the interrupts, in case a packet just arrived in the
      * meanwhile. */
     local_irq_restore(flags);
diff --git a/tpm_tis.c b/tpm_tis.c
index 475ac5d..4a51027 100644
--- a/tpm_tis.c
+++ b/tpm_tis.c
@@ -845,7 +845,7 @@ int tpm_tis_send(struct tpm_chip* tpm, uint8_t* buf, size_t len) {
    }
 #ifdef HAVE_LIBC
    if(tpm->fd >= 0) {
-      files[tpm->fd].read = 0;
+      files[tpm->fd].read = false;
       files[tpm->fd].tpm_tis.respgot = 0;
       files[tpm->fd].tpm_tis.offset = 0;
    }
diff --git a/tpmfront.c b/tpmfront.c
index 6049244..d825b49 100644
--- a/tpmfront.c
+++ b/tpmfront.c
@@ -66,7 +66,7 @@ void tpmfront_handler(evtchn_port_t port, struct pt_regs *regs, void *data) {
    dev->waiting = 0;
 #ifdef HAVE_LIBC
    if(dev->fd >= 0) {
-      files[dev->fd].read = 1;
+      files[dev->fd].read = true;
    }
 #endif
    wake_up(&dev->waitq);
@@ -438,7 +438,7 @@ int tpmfront_send(struct tpmfront_dev* dev, const uint8_t* msg, size_t length)
    dev->resplen = 0;
 #ifdef HAVE_LIBC
    if(dev->fd >= 0) {
-      files[dev->fd].read = 0;
+      files[dev->fd].read = false;
       files[dev->fd].tpmfront.respgot = 0;
       files[dev->fd].tpmfront.offset = 0;
    }
@@ -611,7 +611,7 @@ int tpmfront_posix_fstat(int fd, struct stat* buf)
 
    /* If we have a response waiting, then read it now from the backend
     * so we can get its length*/
-   if(dev->waiting || (files[dev->fd].read == 1 && !files[dev->fd].tpmfront.respgot)) {
+   if(dev->waiting || (files[dev->fd].read && !files[dev->fd].tpmfront.respgot)) {
       if ((rc = tpmfront_recv(dev, &dummybuf, &dummysz)) != 0) {
 	 errno = EIO;
 	 return -1;
-- 
2.26.2

[PATCH v2 03/18] mini-os: make offset a common struct file member for all types

[Juergen Gross]

Currently 4 file types have an offset member in their private struct
file part. Make offset a common struct member shared by all file types.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
---
 blkfront.c    |  5 ++---
 include/lib.h |  5 +----
 lib/sys.c     | 14 +++++---------
 tpm_tis.c     | 11 +++++------
 tpmfront.c    | 11 +++++------
 5 files changed, 18 insertions(+), 28 deletions(-)

diff --git a/blkfront.c b/blkfront.c
index 7c8eb74..8137106 100644
--- a/blkfront.c
+++ b/blkfront.c
@@ -563,14 +563,13 @@ int blkfront_open(struct blkfront_dev *dev)
     dev->fd = alloc_fd(FTYPE_BLK);
     printk("blk_open(%s) -> %d\n", dev->nodename, dev->fd);
     files[dev->fd].blk.dev = dev;
-    files[dev->fd].blk.offset = 0;
     return dev->fd;
 }
 
 int blkfront_posix_rwop(int fd, uint8_t* buf, size_t count, int write)
 {
    struct blkfront_dev* dev = files[fd].blk.dev;
-   off_t offset = files[fd].blk.offset;
+   off_t offset = files[fd].offset;
    struct blkfront_aiocb aiocb;
    unsigned long long disksize = dev->info.sectors * dev->info.sector_size;
    unsigned int blocksize = dev->info.sector_size;
@@ -712,7 +711,7 @@ int blkfront_posix_rwop(int fd, uint8_t* buf, size_t count, int write)
    }
 
    free(copybuf);
-   files[fd].blk.offset += rc;
+   files[fd].offset += rc;
    return rc;
 
 }
diff --git a/include/lib.h b/include/lib.h
index df2de9e..4d9b14b 100644
--- a/include/lib.h
+++ b/include/lib.h
@@ -185,6 +185,7 @@ struct evtchn_port_info {
 struct file {
     enum fd_type type;
     bool read;	/* maybe available for read */
+    off_t offset;
     union {
 	struct {
             /* lwIP fd */
@@ -193,7 +194,6 @@ struct file {
 	struct {
             /* FS import fd */
 	    int fd;
-	    off_t offset;
 	} file;
 	struct {
 	    struct evtchn_port_list ports;
@@ -204,7 +204,6 @@ struct file {
 	} tap;
 	struct {
 	    struct blkfront_dev *dev;
-            off_t offset;
 	} blk;
 	struct {
 	    struct kbdfront_dev *dev;
@@ -219,14 +218,12 @@ struct file {
 	struct {
 	   struct tpmfront_dev *dev;
 	   int respgot;
-	   off_t offset;
 	} tpmfront;
 #endif
 #ifdef CONFIG_TPM_TIS
 	struct {
 	   struct tpm_chip *dev;
 	   int respgot;
-	   off_t offset;
 	} tpm_tis;
 #endif
 #ifdef CONFIG_XENBUS
diff --git a/lib/sys.c b/lib/sys.c
index e8d5eb2..e1cea70 100644
--- a/lib/sys.c
+++ b/lib/sys.c
@@ -107,6 +107,7 @@ int alloc_fd(enum fd_type type)
     for (i=0; i<NOFILE; i++) {
 	if (files[i].type == FTYPE_NONE) {
 	    files[i].type = type;
+            files[i].offset = 0;
 	    pthread_mutex_unlock(&fd_lock);
 	    return i;
 	}
@@ -363,25 +364,20 @@ int write(int fd, const void *buf, size_t nbytes)
 
 off_t lseek(int fd, off_t offset, int whence)
 {
-    off_t* target = NULL;
     switch(files[fd].type) {
 #ifdef CONFIG_BLKFRONT
        case FTYPE_BLK:
-          target = &files[fd].blk.offset;
           break;
 #endif
 #ifdef CONFIG_TPMFRONT
        case FTYPE_TPMFRONT:
-          target = &files[fd].tpmfront.offset;
           break;
 #endif
 #ifdef CONFIG_TPM_TIS
        case FTYPE_TPM_TIS:
-          target = &files[fd].tpm_tis.offset;
           break;
 #endif
        case FTYPE_FILE:
-          target = &files[fd].file.offset;
           break;
        default:
           /* Not implemented for this filetype */
@@ -391,10 +387,10 @@ off_t lseek(int fd, off_t offset, int whence)
 
     switch (whence) {
        case SEEK_SET:
-          *target = offset;
+          files[fd].offset = offset;
           break;
        case SEEK_CUR:
-          *target += offset;
+          files[fd].offset += offset;
           break;
        case SEEK_END:
           {
@@ -403,14 +399,14 @@ off_t lseek(int fd, off_t offset, int whence)
              ret = fstat(fd, &st);
              if (ret)
                 return -1;
-             *target = st.st_size + offset;
+             files[fd].offset = st.st_size + offset;
              break;
           }
        default:
           errno = EINVAL;
           return -1;
     }
-    return *target;
+    return files[fd].offset;
 }
 
 int fsync(int fd) {
diff --git a/tpm_tis.c b/tpm_tis.c
index 4a51027..8a632b1 100644
--- a/tpm_tis.c
+++ b/tpm_tis.c
@@ -847,7 +847,7 @@ int tpm_tis_send(struct tpm_chip* tpm, uint8_t* buf, size_t len) {
    if(tpm->fd >= 0) {
       files[tpm->fd].read = false;
       files[tpm->fd].tpm_tis.respgot = 0;
-      files[tpm->fd].tpm_tis.offset = 0;
+      files[tpm->fd].offset = 0;
    }
 #endif
    return len;
@@ -1290,7 +1290,6 @@ int tpm_tis_open(struct tpm_chip* tpm)
    tpm->fd = alloc_fd(FTYPE_TPM_TIS);
    printk("tpm_tis_open() -> %d\n", tpm->fd);
    files[tpm->fd].tpm_tis.dev = tpm;
-   files[tpm->fd].tpm_tis.offset = 0;
    files[tpm->fd].tpm_tis.respgot = 0;
    return tpm->fd;
 }
@@ -1340,13 +1339,13 @@ int tpm_tis_posix_read(int fd, uint8_t* buf, size_t count)
 
 
    /* Handle EOF case */
-   if(files[fd].tpm_tis.offset >= tpm->data_len) {
+   if(files[fd].offset >= tpm->data_len) {
       rc = 0;
    } else {
-      rc = min(tpm->data_len - files[fd].tpm_tis.offset, count);
-      memcpy(buf, tpm->data_buffer + files[fd].tpm_tis.offset, rc);
+      rc = min(tpm->data_len - files[fd].offset, count);
+      memcpy(buf, tpm->data_buffer + files[fd].offset, rc);
    }
-   files[fd].tpm_tis.offset += rc;
+   files[fd].offset += rc;
    /* Reset the data pending flag */
    return rc;
 }
diff --git a/tpmfront.c b/tpmfront.c
index d825b49..8b2a910 100644
--- a/tpmfront.c
+++ b/tpmfront.c
@@ -440,7 +440,7 @@ int tpmfront_send(struct tpmfront_dev* dev, const uint8_t* msg, size_t length)
    if(dev->fd >= 0) {
       files[dev->fd].read = false;
       files[dev->fd].tpmfront.respgot = 0;
-      files[dev->fd].tpmfront.offset = 0;
+      files[dev->fd].offset = 0;
    }
 #endif
    wmb();
@@ -539,7 +539,6 @@ int tpmfront_open(struct tpmfront_dev* dev)
    dev->fd = alloc_fd(FTYPE_TPMFRONT);
    printk("tpmfront_open(%s) -> %d\n", dev->nodename, dev->fd);
    files[dev->fd].tpmfront.dev = dev;
-   files[dev->fd].tpmfront.offset = 0;
    files[dev->fd].tpmfront.respgot = 0;
    return dev->fd;
 }
@@ -589,14 +588,14 @@ int tpmfront_posix_read(int fd, uint8_t* buf, size_t count)
    }
 
    /* handle EOF case */
-   if(files[dev->fd].tpmfront.offset >= dev->resplen) {
+   if(files[dev->fd].offset >= dev->resplen) {
       return 0;
    }
 
    /* Compute the number of bytes and do the copy operation */
-   if((rc = min(count, dev->resplen - files[dev->fd].tpmfront.offset)) != 0) {
-      memcpy(buf, dev->respbuf + files[dev->fd].tpmfront.offset, rc);
-      files[dev->fd].tpmfront.offset += rc;
+   if((rc = min(count, dev->resplen - files[dev->fd].offset)) != 0) {
+      memcpy(buf, dev->respbuf + files[dev->fd].offset, rc);
+      files[dev->fd].offset += rc;
    }
 
    return rc;
-- 
2.26.2

[PATCH v2 04/18] mini-os: replace multiple fd elements in struct file by common one

[Juergen Gross]

The type specific union in struct files contains two instances of
"int fd". Replace them by a common one.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
---
 include/lib.h |  9 +--------
 lib/sys.c     | 28 ++++++++++++++--------------
 2 files changed, 15 insertions(+), 22 deletions(-)

diff --git a/include/lib.h b/include/lib.h
index 4d9b14b..dc56f52 100644
--- a/include/lib.h
+++ b/include/lib.h
@@ -187,14 +187,7 @@ struct file {
     bool read;	/* maybe available for read */
     off_t offset;
     union {
-	struct {
-            /* lwIP fd */
-	    int fd;
-	} socket;
-	struct {
-            /* FS import fd */
-	    int fd;
-	} file;
+        int fd; /* Any fd from an upper layer. */
 	struct {
 	    struct evtchn_port_list ports;
 	} evtchn;
diff --git a/lib/sys.c b/lib/sys.c
index e1cea70..1da7401 100644
--- a/lib/sys.c
+++ b/lib/sys.c
@@ -258,7 +258,7 @@ int read(int fd, void *buf, size_t nbytes)
         }
 #ifdef HAVE_LWIP
 	case FTYPE_SOCKET:
-	    return lwip_read(files[fd].socket.fd, buf, nbytes);
+	    return lwip_read(files[fd].fd, buf, nbytes);
 #endif
 #ifdef CONFIG_NETFRONT
 	case FTYPE_TAP: {
@@ -335,7 +335,7 @@ int write(int fd, const void *buf, size_t nbytes)
 	    return nbytes;
 #ifdef HAVE_LWIP
 	case FTYPE_SOCKET:
-	    return lwip_write(files[fd].socket.fd, (void*) buf, nbytes);
+	    return lwip_write(files[fd].fd, (void*) buf, nbytes);
 #endif
 #ifdef CONFIG_NETFRONT
 	case FTYPE_TAP:
@@ -428,7 +428,7 @@ int close(int fd)
 #endif
 #ifdef HAVE_LWIP
 	case FTYPE_SOCKET: {
-	    int res = lwip_close(files[fd].socket.fd);
+	    int res = lwip_close(files[fd].fd);
 	    files[fd].type = FTYPE_NONE;
 	    return res;
 	}
@@ -594,7 +594,7 @@ int fcntl(int fd, int cmd, ...)
 	    if (files[fd].type == FTYPE_SOCKET && !(arg & ~O_NONBLOCK)) {
 		/* Only flag supported: non-blocking mode */
 		uint32_t nblock = !!(arg & O_NONBLOCK);
-		return lwip_ioctl(files[fd].socket.fd, FIONBIO, &nblock);
+		return lwip_ioctl(files[fd].fd, FIONBIO, &nblock);
 	    }
 	    /* Fallthrough */
 #endif
@@ -732,15 +732,15 @@ static int select_poll(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exce
     for (i = 0; i < nfds; i++) {
 	if (files[i].type == FTYPE_SOCKET) {
 	    if (FD_ISSET(i, readfds)) {
-		FD_SET(files[i].socket.fd, &sock_readfds);
+		FD_SET(files[i].fd, &sock_readfds);
 		sock_nfds = i+1;
 	    }
 	    if (FD_ISSET(i, writefds)) {
-		FD_SET(files[i].socket.fd, &sock_writefds);
+		FD_SET(files[i].fd, &sock_writefds);
 		sock_nfds = i+1;
 	    }
 	    if (FD_ISSET(i, exceptfds)) {
-		FD_SET(files[i].socket.fd, &sock_exceptfds);
+		FD_SET(files[i].fd, &sock_exceptfds);
 		sock_nfds = i+1;
 	    }
 	}
@@ -803,19 +803,19 @@ static int select_poll(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exce
 	case FTYPE_SOCKET:
 	    if (FD_ISSET(i, readfds)) {
 	        /* Optimize no-network-packet case.  */
-		if (sock_n && FD_ISSET(files[i].socket.fd, &sock_readfds))
+		if (sock_n && FD_ISSET(files[i].fd, &sock_readfds))
 		    n++;
 		else
 		    FD_CLR(i, readfds);
 	    }
             if (FD_ISSET(i, writefds)) {
-		if (sock_n && FD_ISSET(files[i].socket.fd, &sock_writefds))
+		if (sock_n && FD_ISSET(files[i].fd, &sock_writefds))
 		    n++;
 		else
 		    FD_CLR(i, writefds);
             }
             if (FD_ISSET(i, exceptfds)) {
-		if (sock_n && FD_ISSET(files[i].socket.fd, &sock_exceptfds))
+		if (sock_n && FD_ISSET(files[i].fd, &sock_exceptfds))
 		    n++;
 		else
 		    FD_CLR(i, exceptfds);
@@ -1112,7 +1112,7 @@ int socket(int domain, int type, int protocol)
 	return -1;
     res = alloc_fd(FTYPE_SOCKET);
     printk("socket -> %d\n", res);
-    files[res].socket.fd = fd;
+    files[res].fd = fd;
     return res;
 }
 
@@ -1124,11 +1124,11 @@ int accept(int s, struct sockaddr *addr, socklen_t *addrlen)
 	errno = EBADF;
 	return -1;
     }
-    fd = lwip_accept(files[s].socket.fd, addr, addrlen);
+    fd = lwip_accept(files[s].fd, addr, addrlen);
     if (fd < 0)
 	return -1;
     res = alloc_fd(FTYPE_SOCKET);
-    files[res].socket.fd = fd;
+    files[res].fd = fd;
     printk("accepted on %d -> %d\n", s, res);
     return res;
 }
@@ -1141,7 +1141,7 @@ ret name proto \
 	errno = EBADF; \
 	return -1; \
     } \
-    s = files[s].socket.fd; \
+    s = files[s].fd; \
     return lwip_##name args; \
 }
 
-- 
2.26.2

[PATCH v2 05/18] mini-os: introduce a common dev pointer in struct file

[Juergen Gross]

There are many dev pointers in a union in struct file. Prepare to
switch to a single one by introducing a new common one.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
---
 include/lib.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/lib.h b/include/lib.h
index dc56f52..60aaf1c 100644
--- a/include/lib.h
+++ b/include/lib.h
@@ -188,6 +188,7 @@ struct file {
     off_t offset;
     union {
         int fd; /* Any fd from an upper layer. */
+        void *dev;
 	struct {
 	    struct evtchn_port_list ports;
 	} evtchn;
-- 
2.26.2

[PATCH v2 06/18] mini-os: eliminate blkfront union member in struct file

[Juergen Gross]

Replace the blkfront specific union member in struct file with the
common dev pointer.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
---
 blkfront.c    | 6 +++---
 include/lib.h | 3 ---
 lib/sys.c     | 2 +-
 3 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/blkfront.c b/blkfront.c
index 8137106..e3f42be 100644
--- a/blkfront.c
+++ b/blkfront.c
@@ -562,13 +562,13 @@ int blkfront_open(struct blkfront_dev *dev)
     }
     dev->fd = alloc_fd(FTYPE_BLK);
     printk("blk_open(%s) -> %d\n", dev->nodename, dev->fd);
-    files[dev->fd].blk.dev = dev;
+    files[dev->fd].dev = dev;
     return dev->fd;
 }
 
 int blkfront_posix_rwop(int fd, uint8_t* buf, size_t count, int write)
 {
-   struct blkfront_dev* dev = files[fd].blk.dev;
+   struct blkfront_dev* dev = files[fd].dev;
    off_t offset = files[fd].offset;
    struct blkfront_aiocb aiocb;
    unsigned long long disksize = dev->info.sectors * dev->info.sector_size;
@@ -718,7 +718,7 @@ int blkfront_posix_rwop(int fd, uint8_t* buf, size_t count, int write)
 
 int blkfront_posix_fstat(int fd, struct stat* buf)
 {
-   struct blkfront_dev* dev = files[fd].blk.dev;
+   struct blkfront_dev* dev = files[fd].dev;
 
    buf->st_mode = dev->info.mode;
    buf->st_uid = 0;
diff --git a/include/lib.h b/include/lib.h
index 60aaf1c..3a40634 100644
--- a/include/lib.h
+++ b/include/lib.h
@@ -196,9 +196,6 @@ struct file {
 	struct {
 	    struct netfront_dev *dev;
 	} tap;
-	struct {
-	    struct blkfront_dev *dev;
-	} blk;
 	struct {
 	    struct kbdfront_dev *dev;
 	} kbd;
diff --git a/lib/sys.c b/lib/sys.c
index 1da7401..f2fdbdf 100644
--- a/lib/sys.c
+++ b/lib/sys.c
@@ -456,7 +456,7 @@ int close(int fd)
 #endif
 #ifdef CONFIG_BLKFRONT
 	case FTYPE_BLK:
-            shutdown_blkfront(files[fd].blk.dev);
+            shutdown_blkfront(files[fd].dev);
 	    files[fd].type = FTYPE_NONE;
 	    return 0;
 #endif
-- 
2.26.2

[PATCH v2 07/18] mini-os: eliminate consfront union member in struct file

[Juergen Gross]

Replace the consfront specific union member in struct file with the
common dev pointer.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
---
 include/lib.h |  3 ---
 lib/sys.c     | 31 +++++++++++++++++--------------
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/include/lib.h b/include/lib.h
index 3a40634..0cedae6 100644
--- a/include/lib.h
+++ b/include/lib.h
@@ -202,9 +202,6 @@ struct file {
 	struct {
 	    struct fbfront_dev *dev;
 	} fb;
-	struct {
-	    struct consfront_dev *dev;
-	} cons;
 #ifdef CONFIG_TPMFRONT
 	struct {
 	   struct tpmfront_dev *dev;
diff --git a/lib/sys.c b/lib/sys.c
index f2fdbdf..62c2020 100644
--- a/lib/sys.c
+++ b/lib/sys.c
@@ -179,7 +179,7 @@ int posix_openpt(int flags)
 
     dev = init_consfront(NULL);
     dev->fd = alloc_fd(FTYPE_CONSOLE);
-    files[dev->fd].cons.dev = dev;
+    files[dev->fd].dev = dev;
 
     printk("fd(%d) = posix_openpt\n", dev->fd);
     return(dev->fd);
@@ -194,7 +194,7 @@ int open_savefile(const char *path, int save)
 
     dev = init_consfront(nodename);
     dev->fd = alloc_fd(FTYPE_SAVEFILE);
-    files[dev->fd].cons.dev = dev;
+    files[dev->fd].dev = dev;
 
     printk("fd(%d) = open_savefile\n", dev->fd);
     return(dev->fd);
@@ -248,7 +248,7 @@ int read(int fd, void *buf, size_t nbytes)
             DEFINE_WAIT(w);
             while(1) {
                 add_waiter(w, console_queue);
-                ret = xencons_ring_recv(files[fd].cons.dev, buf, nbytes);
+                ret = xencons_ring_recv(files[fd].dev, buf, nbytes);
                 if (ret)
                     break;
                 schedule();
@@ -324,14 +324,14 @@ int write(int fd, const void *buf, size_t nbytes)
         case FTYPE_SAVEFILE: {
                 int ret = 0, tot = nbytes;
                 while (nbytes > 0) {
-                    ret = xencons_ring_send(files[fd].cons.dev, (char *)buf, nbytes);
+                    ret = xencons_ring_send(files[fd].dev, (char *)buf, nbytes);
                     nbytes -= ret;
                     buf = (char *)buf + ret;
                 }
                 return tot - nbytes;
             }
 	case FTYPE_CONSOLE:
-	    console_print(files[fd].cons.dev, (char *)buf, nbytes);
+	    console_print(files[fd].dev, (char *)buf, nbytes);
 	    return nbytes;
 #ifdef HAVE_LWIP
 	case FTYPE_SOCKET:
@@ -487,7 +487,7 @@ int close(int fd)
 #ifdef CONFIG_CONSFRONT
         case FTYPE_SAVEFILE:
         case FTYPE_CONSOLE:
-            fini_consfront(files[fd].cons.dev);
+            fini_consfront(files[fd].dev);
             files[fd].type = FTYPE_NONE;
             return 0;
 #endif
@@ -764,7 +764,7 @@ static int select_poll(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exce
 	    /* Fallthrough.  */
 	case FTYPE_CONSOLE:
 	    if (FD_ISSET(i, readfds)) {
-                if (xencons_ring_avail(files[i].cons.dev))
+                if (xencons_ring_avail(files[i].dev))
 		    n++;
 		else
 		    FD_CLR(i, readfds);
@@ -1447,6 +1447,8 @@ const struct termios default_termios = {0,             /* iflag */
 
 int tcsetattr(int fildes, int action, const struct termios *tios)
 {
+    struct consfront_dev *dev;
+
     if (fildes < 0 || fildes >= NOFILE) {
         errno = EBADF;
         return -1;
@@ -1472,21 +1474,21 @@ int tcsetattr(int fildes, int action, const struct termios *tios)
             return -1;
     }
 
-    if (files[fildes].cons.dev == NULL) {
+    dev = files[fildes].dev;
+    if (dev == NULL) {
         errno = ENOSYS;
         return -1;
     }
 
-    if (tios->c_oflag & OPOST)
-        files[fildes].cons.dev->is_raw = false;
-    else
-        files[fildes].cons.dev->is_raw = true;
+    dev->is_raw = !(tios->c_oflag & OPOST);
 
     return 0;
 }
 
 int tcgetattr(int fildes, struct termios *tios)
 {
+    struct consfront_dev *dev;
+
     if (fildes < 0 || fildes >= NOFILE) {
         errno = EBADF;
         return -1;
@@ -1497,7 +1499,8 @@ int tcgetattr(int fildes, struct termios *tios)
         return -1;
     }
 
-    if (files[fildes].cons.dev == NULL) {
+    dev = files[fildes].dev;
+    if (dev == NULL) {
         errno = ENOSYS;
         return 0;
     }
@@ -1509,7 +1512,7 @@ int tcgetattr(int fildes, struct termios *tios)
 
     memcpy(tios, &default_termios, sizeof(struct termios));
 
-    if (files[fildes].cons.dev->is_raw)
+    if (dev->is_raw)
         tios->c_oflag &= ~OPOST;
 
     return 0;
-- 
2.26.2

[PATCH v2 08/18] mini-os: eliminate fbfront union member in struct file

[Juergen Gross]

Replace the fbfront specific union member in struct file with the
common dev pointer.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
---
 fbfront.c     | 2 +-
 include/lib.h | 3 ---
 lib/sys.c     | 4 ++--
 3 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/fbfront.c b/fbfront.c
index 6725da1..c8410af 100644
--- a/fbfront.c
+++ b/fbfront.c
@@ -703,7 +703,7 @@ int fbfront_open(struct fbfront_dev *dev)
 {
     dev->fd = alloc_fd(FTYPE_FB);
     printk("fb_open(%s) -> %d\n", dev->nodename, dev->fd);
-    files[dev->fd].fb.dev = dev;
+    files[dev->fd].dev = dev;
     return dev->fd;
 }
 #endif
diff --git a/include/lib.h b/include/lib.h
index 0cedae6..2a9a01c 100644
--- a/include/lib.h
+++ b/include/lib.h
@@ -199,9 +199,6 @@ struct file {
 	struct {
 	    struct kbdfront_dev *dev;
 	} kbd;
-	struct {
-	    struct fbfront_dev *dev;
-	} fb;
 #ifdef CONFIG_TPMFRONT
 	struct {
 	   struct tpmfront_dev *dev;
diff --git a/lib/sys.c b/lib/sys.c
index 62c2020..2d48657 100644
--- a/lib/sys.c
+++ b/lib/sys.c
@@ -287,7 +287,7 @@ int read(int fd, void *buf, size_t nbytes)
         case FTYPE_FB: {
             int ret, n;
             n = nbytes / sizeof(union xenfb_in_event);
-            ret = fbfront_receive(files[fd].fb.dev, buf, n);
+            ret = fbfront_receive(files[fd].dev, buf, n);
 	    if (ret <= 0) {
 		errno = EAGAIN;
 		return -1;
@@ -480,7 +480,7 @@ int close(int fd)
 #endif
 #ifdef CONFIG_FBFRONT
 	case FTYPE_FB:
-            shutdown_fbfront(files[fd].fb.dev);
+            shutdown_fbfront(files[fd].dev);
             files[fd].type = FTYPE_NONE;
             return 0;
 #endif
-- 
2.26.2

[PATCH v2 09/18] mini-os: eliminate kbdfront union member in struct file

[Juergen Gross]

Replace the kbdfront specific union member in struct file with the
common dev pointer.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
---
 fbfront.c     | 2 +-
 include/lib.h | 3 ---
 lib/sys.c     | 4 ++--
 3 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/fbfront.c b/fbfront.c
index c8410af..1e055fb 100644
--- a/fbfront.c
+++ b/fbfront.c
@@ -302,7 +302,7 @@ int kbdfront_open(struct kbdfront_dev *dev)
 {
     dev->fd = alloc_fd(FTYPE_KBD);
     printk("kbd_open(%s) -> %d\n", dev->nodename, dev->fd);
-    files[dev->fd].kbd.dev = dev;
+    files[dev->fd].dev = dev;
     return dev->fd;
 }
 #endif
diff --git a/include/lib.h b/include/lib.h
index 2a9a01c..5201ed7 100644
--- a/include/lib.h
+++ b/include/lib.h
@@ -196,9 +196,6 @@ struct file {
 	struct {
 	    struct netfront_dev *dev;
 	} tap;
-	struct {
-	    struct kbdfront_dev *dev;
-	} kbd;
 #ifdef CONFIG_TPMFRONT
 	struct {
 	   struct tpmfront_dev *dev;
diff --git a/lib/sys.c b/lib/sys.c
index 2d48657..8c7ea3c 100644
--- a/lib/sys.c
+++ b/lib/sys.c
@@ -275,7 +275,7 @@ int read(int fd, void *buf, size_t nbytes)
         case FTYPE_KBD: {
             int ret, n;
             n = nbytes / sizeof(union xenkbd_in_event);
-            ret = kbdfront_receive(files[fd].kbd.dev, buf, n);
+            ret = kbdfront_receive(files[fd].dev, buf, n);
 	    if (ret <= 0) {
 		errno = EAGAIN;
 		return -1;
@@ -474,7 +474,7 @@ int close(int fd)
 #endif
 #ifdef CONFIG_KBDFRONT
 	case FTYPE_KBD:
-            shutdown_kbdfront(files[fd].kbd.dev);
+            shutdown_kbdfront(files[fd].dev);
             files[fd].type = FTYPE_NONE;
             return 0;
 #endif
-- 
2.26.2

[PATCH v2 10/18] mini-os: eliminate netfront union member in struct file

[Juergen Gross]

Replace the netfront specific union member in struct file with the
common dev pointer.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
---
 include/lib.h | 3 ---
 lib/sys.c     | 6 +++---
 netfront.c    | 2 +-
 3 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/include/lib.h b/include/lib.h
index 5201ed7..f2a124e 100644
--- a/include/lib.h
+++ b/include/lib.h
@@ -193,9 +193,6 @@ struct file {
 	    struct evtchn_port_list ports;
 	} evtchn;
 	struct gntmap gntmap;
-	struct {
-	    struct netfront_dev *dev;
-	} tap;
 #ifdef CONFIG_TPMFRONT
 	struct {
 	   struct tpmfront_dev *dev;
diff --git a/lib/sys.c b/lib/sys.c
index 8c7ea3c..b35e433 100644
--- a/lib/sys.c
+++ b/lib/sys.c
@@ -263,7 +263,7 @@ int read(int fd, void *buf, size_t nbytes)
 #ifdef CONFIG_NETFRONT
 	case FTYPE_TAP: {
 	    ssize_t ret;
-	    ret = netfront_receive(files[fd].tap.dev, buf, nbytes);
+	    ret = netfront_receive(files[fd].dev, buf, nbytes);
 	    if (ret <= 0) {
 		errno = EAGAIN;
 		return -1;
@@ -339,7 +339,7 @@ int write(int fd, const void *buf, size_t nbytes)
 #endif
 #ifdef CONFIG_NETFRONT
 	case FTYPE_TAP:
-	    netfront_xmit(files[fd].tap.dev, (void*) buf, nbytes);
+	    netfront_xmit(files[fd].dev, (void*) buf, nbytes);
 	    return nbytes;
 #endif
 #ifdef CONFIG_BLKFRONT
@@ -450,7 +450,7 @@ int close(int fd)
 #endif
 #ifdef CONFIG_NETFRONT
 	case FTYPE_TAP:
-	    shutdown_netfront(files[fd].tap.dev);
+	    shutdown_netfront(files[fd].dev);
 	    files[fd].type = FTYPE_NONE;
 	    return 0;
 #endif
diff --git a/netfront.c b/netfront.c
index a566e34..7696451 100644
--- a/netfront.c
+++ b/netfront.c
@@ -576,7 +576,7 @@ int netfront_tap_open(char *nodename) {
     }
     dev->fd = alloc_fd(FTYPE_TAP);
     printk("tap_open(%s) -> %d\n", nodename, dev->fd);
-    files[dev->fd].tap.dev = dev;
+    files[dev->fd].dev = dev;
     return dev->fd;
 }
 #endif
-- 
2.26.2

[PATCH v2 11/18] mini-os: move tpm respgot member of struct file to device specific data

[Juergen Gross]

Tpmfront has a "respgot" flag in struct file, which can be moved to the
device specific data. While at it make it a bool.

The respgot flag of the tpm_tis member of struct file can be removed,
as it is never read.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
---
 include/lib.h      |  2 --
 include/tpmfront.h |  2 ++
 tpm_tis.c          |  2 --
 tpmfront.c         | 10 +++++-----
 4 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/include/lib.h b/include/lib.h
index f2a124e..d740065 100644
--- a/include/lib.h
+++ b/include/lib.h
@@ -196,13 +196,11 @@ struct file {
 #ifdef CONFIG_TPMFRONT
 	struct {
 	   struct tpmfront_dev *dev;
-	   int respgot;
 	} tpmfront;
 #endif
 #ifdef CONFIG_TPM_TIS
 	struct {
 	   struct tpm_chip *dev;
-	   int respgot;
 	} tpm_tis;
 #endif
 #ifdef CONFIG_XENBUS
diff --git a/include/tpmfront.h b/include/tpmfront.h
index c489fae..b7da50e 100644
--- a/include/tpmfront.h
+++ b/include/tpmfront.h
@@ -25,6 +25,7 @@
 #ifndef TPMFRONT_H
 #define TPMFRONT_H
 
+#include <stdbool.h>
 #include <mini-os/types.h>
 #include <mini-os/os.h>
 #include <mini-os/events.h>
@@ -53,6 +54,7 @@ struct tpmfront_dev {
 
 #ifdef HAVE_LIBC
    int fd;
+   bool respgot;
 #endif
 
 };
diff --git a/tpm_tis.c b/tpm_tis.c
index 8a632b1..4127118 100644
--- a/tpm_tis.c
+++ b/tpm_tis.c
@@ -846,7 +846,6 @@ int tpm_tis_send(struct tpm_chip* tpm, uint8_t* buf, size_t len) {
 #ifdef HAVE_LIBC
    if(tpm->fd >= 0) {
       files[tpm->fd].read = false;
-      files[tpm->fd].tpm_tis.respgot = 0;
       files[tpm->fd].offset = 0;
    }
 #endif
@@ -1290,7 +1289,6 @@ int tpm_tis_open(struct tpm_chip* tpm)
    tpm->fd = alloc_fd(FTYPE_TPM_TIS);
    printk("tpm_tis_open() -> %d\n", tpm->fd);
    files[tpm->fd].tpm_tis.dev = tpm;
-   files[tpm->fd].tpm_tis.respgot = 0;
    return tpm->fd;
 }
 
diff --git a/tpmfront.c b/tpmfront.c
index 8b2a910..be671c2 100644
--- a/tpmfront.c
+++ b/tpmfront.c
@@ -439,8 +439,8 @@ int tpmfront_send(struct tpmfront_dev* dev, const uint8_t* msg, size_t length)
 #ifdef HAVE_LIBC
    if(dev->fd >= 0) {
       files[dev->fd].read = false;
-      files[dev->fd].tpmfront.respgot = 0;
       files[dev->fd].offset = 0;
+      dev->respgot = false;
    }
 #endif
    wmb();
@@ -499,7 +499,7 @@ int i;
 #endif
 #ifdef HAVE_LIBC
    if(dev->fd >= 0) {
-      files[dev->fd].tpmfront.respgot = 1;
+      dev->respgot = true;
    }
 #endif
 quit:
@@ -539,7 +539,7 @@ int tpmfront_open(struct tpmfront_dev* dev)
    dev->fd = alloc_fd(FTYPE_TPMFRONT);
    printk("tpmfront_open(%s) -> %d\n", dev->nodename, dev->fd);
    files[dev->fd].tpmfront.dev = dev;
-   files[dev->fd].tpmfront.respgot = 0;
+   dev->respgot = false;
    return dev->fd;
 }
 
@@ -580,7 +580,7 @@ int tpmfront_posix_read(int fd, uint8_t* buf, size_t count)
    }
 
    /* get the response if we haven't already */
-   if(files[dev->fd].tpmfront.respgot == 0) {
+   if (!dev->respgot) {
       if ((rc = tpmfront_recv(dev, &dummybuf, &dummysz)) != 0) {
 	 errno = EIO;
 	 return -1;
@@ -610,7 +610,7 @@ int tpmfront_posix_fstat(int fd, struct stat* buf)
 
    /* If we have a response waiting, then read it now from the backend
     * so we can get its length*/
-   if(dev->waiting || (files[dev->fd].read && !files[dev->fd].tpmfront.respgot)) {
+   if(dev->waiting || (files[dev->fd].read && !dev->respgot)) {
       if ((rc = tpmfront_recv(dev, &dummybuf, &dummysz)) != 0) {
 	 errno = EIO;
 	 return -1;
-- 
2.26.2

[PATCH v2 12/18] mini-os: eliminate tpmfront union member in struct file

[Juergen Gross]

Replace the tpmfront specific union member in struct file with the
common dev pointer.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
---
 include/lib.h | 5 -----
 lib/sys.c     | 2 +-
 tpmfront.c    | 8 ++++----
 3 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/include/lib.h b/include/lib.h
index d740065..2ddc076 100644
--- a/include/lib.h
+++ b/include/lib.h
@@ -193,11 +193,6 @@ struct file {
 	    struct evtchn_port_list ports;
 	} evtchn;
 	struct gntmap gntmap;
-#ifdef CONFIG_TPMFRONT
-	struct {
-	   struct tpmfront_dev *dev;
-	} tpmfront;
-#endif
 #ifdef CONFIG_TPM_TIS
 	struct {
 	   struct tpm_chip *dev;
diff --git a/lib/sys.c b/lib/sys.c
index b35e433..b042bf5 100644
--- a/lib/sys.c
+++ b/lib/sys.c
@@ -462,7 +462,7 @@ int close(int fd)
 #endif
 #ifdef CONFIG_TPMFRONT
 	case FTYPE_TPMFRONT:
-            shutdown_tpmfront(files[fd].tpmfront.dev);
+            shutdown_tpmfront(files[fd].dev);
 	    files[fd].type = FTYPE_NONE;
 	    return 0;
 #endif
diff --git a/tpmfront.c b/tpmfront.c
index be671c2..0a2fefc 100644
--- a/tpmfront.c
+++ b/tpmfront.c
@@ -538,7 +538,7 @@ int tpmfront_open(struct tpmfront_dev* dev)
 
    dev->fd = alloc_fd(FTYPE_TPMFRONT);
    printk("tpmfront_open(%s) -> %d\n", dev->nodename, dev->fd);
-   files[dev->fd].tpmfront.dev = dev;
+   files[dev->fd].dev = dev;
    dev->respgot = false;
    return dev->fd;
 }
@@ -547,7 +547,7 @@ int tpmfront_posix_write(int fd, const uint8_t* buf, size_t count)
 {
    int rc;
    struct tpmfront_dev* dev;
-   dev = files[fd].tpmfront.dev;
+   dev = files[fd].dev;
 
    if(count == 0) {
       return 0;
@@ -573,7 +573,7 @@ int tpmfront_posix_read(int fd, uint8_t* buf, size_t count)
    size_t dummysz;
    struct tpmfront_dev* dev;
 
-   dev = files[fd].tpmfront.dev;
+   dev = files[fd].dev;
 
    if(count == 0) {
       return 0;
@@ -606,7 +606,7 @@ int tpmfront_posix_fstat(int fd, struct stat* buf)
    uint8_t* dummybuf;
    size_t dummysz;
    int rc;
-   struct tpmfront_dev* dev = files[fd].tpmfront.dev;
+   struct tpmfront_dev* dev = files[fd].dev;
 
    /* If we have a response waiting, then read it now from the backend
     * so we can get its length*/
-- 
2.26.2

[PATCH v2 13/18] mini-os: eliminate tpmtis union member in struct file

[Juergen Gross]

Replace the tpmtis specific union member in struct file with the
common dev pointer.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
---
 include/lib.h | 5 -----
 lib/sys.c     | 2 +-
 tpm_tis.c     | 8 ++++----
 3 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/include/lib.h b/include/lib.h
index 2ddc076..d6a29ba 100644
--- a/include/lib.h
+++ b/include/lib.h
@@ -193,11 +193,6 @@ struct file {
 	    struct evtchn_port_list ports;
 	} evtchn;
 	struct gntmap gntmap;
-#ifdef CONFIG_TPM_TIS
-	struct {
-	   struct tpm_chip *dev;
-	} tpm_tis;
-#endif
 #ifdef CONFIG_XENBUS
         struct {
             /* To each xenbus FD is associated a queue of watch events for this
diff --git a/lib/sys.c b/lib/sys.c
index b042bf5..96fc769 100644
--- a/lib/sys.c
+++ b/lib/sys.c
@@ -468,7 +468,7 @@ int close(int fd)
 #endif
 #ifdef CONFIG_TPM_TIS
 	case FTYPE_TPM_TIS:
-            shutdown_tpm_tis(files[fd].tpm_tis.dev);
+            shutdown_tpm_tis(files[fd].dev);
 	    files[fd].type = FTYPE_NONE;
 	    return 0;
 #endif
diff --git a/tpm_tis.c b/tpm_tis.c
index 4127118..477f555 100644
--- a/tpm_tis.c
+++ b/tpm_tis.c
@@ -1288,14 +1288,14 @@ int tpm_tis_open(struct tpm_chip* tpm)
 
    tpm->fd = alloc_fd(FTYPE_TPM_TIS);
    printk("tpm_tis_open() -> %d\n", tpm->fd);
-   files[tpm->fd].tpm_tis.dev = tpm;
+   files[tpm->fd].dev = tpm;
    return tpm->fd;
 }
 
 int tpm_tis_posix_write(int fd, const uint8_t* buf, size_t count)
 {
    struct tpm_chip* tpm;
-   tpm = files[fd].tpm_tis.dev;
+   tpm = files[fd].dev;
 
    if(tpm->locality < 0) {
       printk("tpm_tis_posix_write() failed! locality not set!\n");
@@ -1323,7 +1323,7 @@ int tpm_tis_posix_read(int fd, uint8_t* buf, size_t count)
 {
    int rc;
    struct tpm_chip* tpm;
-   tpm = files[fd].tpm_tis.dev;
+   tpm = files[fd].dev;
 
    if(count == 0) {
       return 0;
@@ -1350,7 +1350,7 @@ int tpm_tis_posix_read(int fd, uint8_t* buf, size_t count)
 int tpm_tis_posix_fstat(int fd, struct stat* buf)
 {
    struct tpm_chip* tpm;
-   tpm = files[fd].tpm_tis.dev;
+   tpm = files[fd].dev;
 
    buf->st_mode = O_RDWR;
    buf->st_uid = 0;
-- 
2.26.2

[PATCH v2 14/18] mini-os: eliminate xenbus union member in struct file

[Juergen Gross]

Replace the xenbus specific union member in struct file with the
common dev pointer.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
---
 include/lib.h |  7 -------
 lib/sys.c     |  2 +-
 lib/xs.c      | 13 +++++++------
 3 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/include/lib.h b/include/lib.h
index d6a29ba..91364ba 100644
--- a/include/lib.h
+++ b/include/lib.h
@@ -193,13 +193,6 @@ struct file {
 	    struct evtchn_port_list ports;
 	} evtchn;
 	struct gntmap gntmap;
-#ifdef CONFIG_XENBUS
-        struct {
-            /* To each xenbus FD is associated a queue of watch events for this
-             * FD.  */
-            xenbus_event_queue events;
-        } xenbus;
-#endif
     };
 };
 
diff --git a/lib/sys.c b/lib/sys.c
index 96fc769..6f2b026 100644
--- a/lib/sys.c
+++ b/lib/sys.c
@@ -776,7 +776,7 @@ static int select_poll(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exce
 #ifdef CONFIG_XENBUS
 	case FTYPE_XENBUS:
 	    if (FD_ISSET(i, readfds)) {
-                if (files[i].xenbus.events)
+                if (files[i].dev)
 		    n++;
 		else
 		    FD_CLR(i, readfds);
diff --git a/lib/xs.c b/lib/xs.c
index 324bd05..0459f52 100644
--- a/lib/xs.c
+++ b/lib/xs.c
@@ -21,8 +21,8 @@ static inline int _xs_fileno(struct xs_handle *h) {
 struct xs_handle *xs_daemon_open()
 {
     int fd = alloc_fd(FTYPE_XENBUS);
-    files[fd].xenbus.events = NULL;
-    printk("xs_daemon_open -> %d, %p\n", fd, &files[fd].xenbus.events);
+    files[fd].dev = NULL;
+    printk("xs_daemon_open -> %d, %p\n", fd, &files[fd].dev);
     return (void*)(intptr_t) fd;
 }
 
@@ -30,7 +30,7 @@ void xs_daemon_close(struct xs_handle *h)
 {
     int fd = _xs_fileno(h);
     struct xenbus_event *event, *next;
-    for (event = files[fd].xenbus.events; event; event = next)
+    for (event = files[fd].dev; event; event = next)
     {
         next = event->next;
         free(event);
@@ -172,15 +172,16 @@ bool xs_watch(struct xs_handle *h, const char *path, const char *token)
 {
     int fd = _xs_fileno(h);
     printk("xs_watch(%s, %s)\n", path, token);
-    return xs_bool(xenbus_watch_path_token(XBT_NULL, path, token, &files[fd].xenbus.events));
+    return xs_bool(xenbus_watch_path_token(XBT_NULL, path, token,
+                   (xenbus_event_queue *)&files[fd].dev));
 }
 
 char **xs_read_watch(struct xs_handle *h, unsigned int *num)
 {
     int fd = _xs_fileno(h);
     struct xenbus_event *event;
-    event = files[fd].xenbus.events;
-    files[fd].xenbus.events = event->next;
+    event = files[fd].dev;
+    files[fd].dev = event->next;
     printk("xs_read_watch() -> %s %s\n", event->path, event->token);
     *num = 2;
     return (char **) &event->path;
-- 
2.26.2

[PATCH v2 15/18] mini-os: introduce get_file_from_fd()

[Juergen Gross]

Exporting the files[] array especially for components outside the
mini-os source tree is limiting the ability to change any file handling
in mini-os.

Introduce a new function get_file_from_fd() to return the struct file
pointer (or NULL) for a given file descriptor.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
---
 include/lib.h | 1 +
 lib/sys.c     | 8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/include/lib.h b/include/lib.h
index 91364ba..7a0546b 100644
--- a/include/lib.h
+++ b/include/lib.h
@@ -198,6 +198,7 @@ struct file {
 
 extern struct file files[];
 
+struct file *get_file_from_fd(int fd);
 int alloc_fd(enum fd_type type);
 void close_all_files(void);
 extern struct thread *main_thread;
diff --git a/lib/sys.c b/lib/sys.c
index 6f2b026..0e6fe5d 100644
--- a/lib/sys.c
+++ b/lib/sys.c
@@ -98,6 +98,14 @@ struct file files[NOFILE] = {
     { .type = FTYPE_CONSOLE }, /* stderr */
 };
 
+struct file *get_file_from_fd(int fd)
+{
+    if ( fd < 0 || fd >= NOFILE )
+        return NULL;
+
+    return (files[fd].type == FTYPE_NONE) ? NULL : files + fd;
+}
+
 DECLARE_WAIT_QUEUE_HEAD(event_queue);
 
 int alloc_fd(enum fd_type type)
-- 
2.26.2

Re: [PATCH v2 15/18] mini-os: introduce get_file_from_fd()

[Andrew Cooper]

On 11/01/2022 14:58, Juergen Gross wrote:
> diff --git a/lib/sys.c b/lib/sys.c
> index 6f2b026..0e6fe5d 100644
> --- a/lib/sys.c
> +++ b/lib/sys.c
> @@ -98,6 +98,14 @@ struct file files[NOFILE] = {
>      { .type = FTYPE_CONSOLE }, /* stderr */
>  };
>  
> +struct file *get_file_from_fd(int fd)
> +{
> +    if ( fd < 0 || fd >= NOFILE )

fd >= ARRAY_SIZE(files)  ?

Generates slightly safer code in the event that `struct file
files[NOFILE]` gets refactored.

~Andrew

> +        return NULL;
> +
> +    return (files[fd].type == FTYPE_NONE) ? NULL : files + fd;
> +}
> +
>  DECLARE_WAIT_QUEUE_HEAD(event_queue);
>  
>  int alloc_fd(enum fd_type type)

Re: [PATCH v2 15/18] mini-os: introduce get_file_from_fd()

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 575 bytes --]

On 11.01.22 19:17, Andrew Cooper wrote:
> On 11/01/2022 14:58, Juergen Gross wrote:
>> diff --git a/lib/sys.c b/lib/sys.c
>> index 6f2b026..0e6fe5d 100644
>> --- a/lib/sys.c
>> +++ b/lib/sys.c
>> @@ -98,6 +98,14 @@ struct file files[NOFILE] = {
>>       { .type = FTYPE_CONSOLE }, /* stderr */
>>   };
>>   
>> +struct file *get_file_from_fd(int fd)
>> +{
>> +    if ( fd < 0 || fd >= NOFILE )
> 
> fd >= ARRAY_SIZE(files)  ?
> 
> Generates slightly safer code in the event that `struct file
> files[NOFILE]` gets refactored.

Fine with me.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

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

[Juergen Gross]

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@suse.com>
---
V2:
- new patch
---
 lib/sys.c | 53 ++++++++++++++++++++++++-----------------------------
 lib/xs.c  |  1 -
 2 files changed, 24 insertions(+), 30 deletions(-)

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);
+            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;
+    return res;
 }
 
 static void init_stat(struct stat *buf)
diff --git a/lib/xs.c b/lib/xs.c
index 0459f52..4af0f96 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

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

[Samuel Thibault]

Juergen Gross, le mar. 11 janv. 2022 15:58:15 +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@suse.com>

Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

Thanks!

> ---
> V2:
> - new patch
> ---
>  lib/sys.c | 53 ++++++++++++++++++++++++-----------------------------
>  lib/xs.c  |  1 -
>  2 files changed, 24 insertions(+), 30 deletions(-)
> 
> 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);
> +            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;
> +    return res;
>  }
>  
>  static void init_stat(struct stat *buf)
> diff --git a/lib/xs.c b/lib/xs.c
> index 0459f52..4af0f96 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
Pour un père, autant mourir que de faire plein de calculs et pas s'occuper
de son fils
 -+- y sur #ens-mim - sombres histoires de zombies -+-

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

[Andrew Cooper]

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

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

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 3485 bytes --]

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 #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

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

[Andrew Cooper]

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) {

I know this bug is pre-existing, but the libc close() really ought to
sanity check fd before blindly indexing files[] with it.

I'd tentatively suggest that you want one extra goto from here, into
wherever the EBADF logic ends up, and it's probably worth including in
this patch.

~Andrew

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

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 717 bytes --]

On 11.01.22 20:14, 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) {
> 
> I know this bug is pre-existing, but the libc close() really ought to
> sanity check fd before blindly indexing files[] with it.
> 
> I'd tentatively suggest that you want one extra goto from here, into
> wherever the EBADF logic ends up, and it's probably worth including in
> this patch.

Will do that.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[PATCH v2 17/18] mini-os: use function vectors instead of switch for file operations

[Juergen Gross]

Introduce file type specific function vectors for all the needed file
operations which are file type specific in order to prepare replacing
the large switch statements in each generic file function.

Add a function to allocate a new file type dynamically in order to
prepare removing direct dependencies to external components in the
future. For this reason switch the file type from an enum to an
unsigned int. Prepare removal of some statically defines file types
by putting them at the end of the defined list.

Change the debug output for the file type from a single character to
a string in order to support a future removal of the file_types[]
array.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- new patch
---
 include/lib.h |  65 +++++++++++-----
 lib/sys.c     | 208 +++++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 217 insertions(+), 56 deletions(-)

diff --git a/include/lib.h b/include/lib.h
index 7a0546b..b40e213 100644
--- a/include/lib.h
+++ b/include/lib.h
@@ -66,6 +66,7 @@
 
 #ifdef HAVE_LIBC
 #include <sys/queue.h>
+#include <sys/stat.h>
 #include <stdio.h>
 #include <string.h>
 #else
@@ -154,25 +155,51 @@ do {                                                           \
 void sanity_check(void);
 
 #ifdef HAVE_LIBC
-enum fd_type {
-    FTYPE_NONE = 0,
-    FTYPE_CONSOLE,
-    FTYPE_FILE,
-    FTYPE_XENBUS,
-    FTYPE_XC,
-    FTYPE_EVTCHN,
-    FTYPE_GNTMAP,
-    FTYPE_SOCKET,
-    FTYPE_TAP,
-    FTYPE_BLK,
-    FTYPE_KBD,
-    FTYPE_FB,
-    FTYPE_MEM,
-    FTYPE_SAVEFILE,
-    FTYPE_TPMFRONT,
-    FTYPE_TPM_TIS,
+#define FTYPE_NONE       0
+#define FTYPE_CONSOLE    1
+#define FTYPE_FILE       2
+#define FTYPE_SOCKET     3
+#define FTYPE_MEM        4
+#define FTYPE_SAVEFILE   5
+#define FTYPE_FB         6
+#define FTYPE_KBD        7
+#define FTYPE_TAP        8
+#define FTYPE_BLK        9
+#define FTYPE_TPMFRONT  10
+#define FTYPE_TPM_TIS   11
+#define FTYPE_XENBUS    12
+#define FTYPE_GNTMAP    13
+#define FTYPE_EVTCHN    14
+#define FTYPE_XC        15
+#define FTYPE_N         16
+#define FTYPE_SPARE     16
+
+typedef int file_read_func(int fd, void *buf, size_t nbytes);
+typedef int file_write_func(int fd, const void *buf, size_t nbytes);
+typedef off_t file_lseek_func(int fd, off_t offset, int whence);
+typedef int file_close_func(int fd);
+typedef int file_fstat_func(int fd, struct stat *buf);
+typedef int file_fcntl_func(int fd, int cmd, va_list args);
+typedef bool file_select_func(int fd);
+
+struct file_ops {
+    const char *name;
+    file_read_func *read;
+    file_write_func *write;
+    file_lseek_func *lseek;
+    file_close_func *close;
+    file_fstat_func *fstat;
+    file_fcntl_func *fcntl;
+    file_select_func *select_rd;
+    file_select_func *select_wr;
 };
 
+unsigned int alloc_file_type(struct file_ops *ops);
+
+off_t lseek_default(int fd, off_t offset, int whence);
+bool select_yes(int fd);
+bool select_read_flag(int fd);
+
 LIST_HEAD(evtchn_port_list, evtchn_port_info);
 
 struct evtchn_port_info {
@@ -183,7 +210,7 @@ struct evtchn_port_info {
 };
 
 struct file {
-    enum fd_type type;
+    unsigned int type;
     bool read;	/* maybe available for read */
     off_t offset;
     union {
@@ -199,7 +226,7 @@ struct file {
 extern struct file files[];
 
 struct file *get_file_from_fd(int fd);
-int alloc_fd(enum fd_type type);
+int alloc_fd(unsigned int type);
 void close_all_files(void);
 extern struct thread *main_thread;
 void sparse(unsigned long data, size_t size);
diff --git a/lib/sys.c b/lib/sys.c
index 323a7cd..c327247 100644
--- a/lib/sys.c
+++ b/lib/sys.c
@@ -98,6 +98,39 @@ struct file files[NOFILE] = {
     { .type = FTYPE_CONSOLE }, /* stderr */
 };
 
+static struct file_ops file_ops_none = {
+    .name = "none",
+};
+
+static struct file_ops *file_ops[FTYPE_N + FTYPE_SPARE] = {
+    [FTYPE_NONE] = &file_ops_none,
+};
+
+unsigned int alloc_file_type(struct file_ops *ops)
+{
+    unsigned int i;
+
+    pthread_mutex_lock(&fd_lock);
+
+    for ( i = FTYPE_N; i < ARRAY_SIZE(file_ops) && file_ops[i]; i++ );
+    BUG_ON(i == ARRAY_SIZE(file_ops));
+    file_ops[i] = ops;
+
+    pthread_mutex_unlock(&fd_lock);
+
+    printk("New file type \"%s\"(%u) allocated\n", ops->name, i);
+
+    return i;
+}
+
+static struct file_ops *get_file_ops(unsigned int type)
+{
+    if ( type >= ARRAY_SIZE(file_ops) || !file_ops[type] )
+        return &file_ops_none;
+
+    return file_ops[type];
+}
+
 struct file *get_file_from_fd(int fd)
 {
     if ( fd < 0 || fd >= NOFILE )
@@ -108,7 +141,7 @@ struct file *get_file_from_fd(int fd)
 
 DECLARE_WAIT_QUEUE_HEAD(event_queue);
 
-int alloc_fd(enum fd_type type)
+int alloc_fd(unsigned int type)
 {
     int i;
     pthread_mutex_lock(&fd_lock);
@@ -249,6 +282,11 @@ int isatty(int fd)
 
 int read(int fd, void *buf, size_t nbytes)
 {
+    struct file_ops *ops = get_file_ops(files[fd].type);
+
+    if ( ops->read )
+        return ops->read(fd, buf, nbytes);
+
     switch (files[fd].type) {
         case FTYPE_SAVEFILE:
 	case FTYPE_CONSOLE: {
@@ -328,6 +366,11 @@ int read(int fd, void *buf, size_t nbytes)
 
 int write(int fd, const void *buf, size_t nbytes)
 {
+    struct file_ops *ops = get_file_ops(files[fd].type);
+
+    if ( ops->write )
+        return ops->write(fd, buf, nbytes);
+
     switch (files[fd].type) {
         case FTYPE_SAVEFILE: {
                 int ret = 0, tot = nbytes;
@@ -370,8 +413,45 @@ int write(int fd, const void *buf, size_t nbytes)
     return -1;
 }
 
+off_t lseek_default(int fd, off_t offset, int whence)
+{
+    switch ( whence )
+    {
+    case SEEK_SET:
+        files[fd].offset = offset;
+        break;
+
+    case SEEK_CUR:
+        files[fd].offset += offset;
+        break;
+
+    case SEEK_END:
+    {
+        struct stat st;
+        int ret;
+
+        ret = fstat(fd, &st);
+        if ( ret )
+            return -1;
+        files[fd].offset = st.st_size + offset;
+        break;
+    }
+
+    default:
+        errno = EINVAL;
+        return -1;
+    }
+
+    return files[fd].offset;
+}
+
 off_t lseek(int fd, off_t offset, int whence)
 {
+    struct file_ops *ops = get_file_ops(files[fd].type);
+
+    if ( ops->lseek )
+        return ops->lseek(fd, offset, whence);
+
     switch(files[fd].type) {
 #ifdef CONFIG_BLKFRONT
        case FTYPE_BLK:
@@ -393,28 +473,7 @@ off_t lseek(int fd, off_t offset, int whence)
           return (off_t) -1;
     }
 
-    switch (whence) {
-       case SEEK_SET:
-          files[fd].offset = offset;
-          break;
-       case SEEK_CUR:
-          files[fd].offset += offset;
-          break;
-       case SEEK_END:
-          {
-             struct stat st;
-             int ret;
-             ret = fstat(fd, &st);
-             if (ret)
-                return -1;
-             files[fd].offset = st.st_size + offset;
-             break;
-          }
-       default:
-          errno = EINVAL;
-          return -1;
-    }
-    return files[fd].offset;
+    return lseek_default(fd, offset, whence);
 }
 
 int fsync(int fd) {
@@ -425,8 +484,15 @@ int fsync(int fd) {
 int close(int fd)
 {
     int res = 0;
+    struct file_ops *ops = get_file_ops(files[fd].type);
 
     printk("close(%d)\n", fd);
+    if ( ops->close )
+    {
+        res = ops->close(fd);
+        goto out;
+    }
+
     switch (files[fd].type) {
         default:
             break;
@@ -497,6 +563,7 @@ int close(int fd)
             return -1;
     }
 
+ out:
     memset(files + fd, 0, sizeof(struct file));
     files[fd].type = FTYPE_NONE;
     return res;
@@ -521,7 +588,13 @@ int stat(const char *path, struct stat *buf)
 
 int fstat(int fd, struct stat *buf)
 {
+    struct file_ops *ops = get_file_ops(files[fd].type);
+
     init_stat(buf);
+
+    if ( ops->fstat )
+        return ops->fstat(fd, buf);
+
     switch (files[fd].type) {
 	case FTYPE_SAVEFILE:
 	case FTYPE_CONSOLE:
@@ -587,6 +660,18 @@ int fcntl(int fd, int cmd, ...)
 {
     long arg;
     va_list ap;
+    int res;
+    struct file_ops *ops = get_file_ops(files[fd].type);
+
+    if ( ops->fcntl )
+    {
+        va_start(ap, cmd);
+        res = ops->fcntl(fd, cmd, ap);
+        va_end(ap);
+
+        return res;
+    }
+
     va_start(ap, cmd);
     arg = va_arg(ap, long);
     va_end(ap);
@@ -640,18 +725,29 @@ int closedir(DIR *dir)
 /* We assume that only the main thread calls select(). */
 
 #if defined(LIBC_DEBUG) || defined(LIBC_VERBOSE)
-static const char file_types[] = {
-    [FTYPE_NONE]	= 'N',
-    [FTYPE_CONSOLE]	= 'C',
-    [FTYPE_XENBUS]	= 'S',
-    [FTYPE_XC]		= 'X',
-    [FTYPE_EVTCHN]	= 'E',
-    [FTYPE_SOCKET]	= 's',
-    [FTYPE_TAP]		= 'T',
-    [FTYPE_BLK]		= 'B',
-    [FTYPE_KBD]		= 'K',
-    [FTYPE_FB]		= 'G',
+static const char *file_types[] = {
+    [FTYPE_NONE]    = "none",
+    [FTYPE_CONSOLE] = "console",
+    [FTYPE_XENBUS]  = "xenbus",
+    [FTYPE_XC]      = "ctrl",
+    [FTYPE_EVTCHN]  = "evtchn",
+    [FTYPE_SOCKET]  = "socket",
+    [FTYPE_TAP]     = "net",
+    [FTYPE_BLK]     = "blk",
+    [FTYPE_KBD]     = "kbd",
+    [FTYPE_FB]      = "fb",
 };
+
+static char *get_type_name(unsigned int type)
+{
+    if ( type < ARRAY_SIZE(file_ops) && file_ops[type] )
+        return file_ops[type]->name;
+
+    if ( type < ARRAY_SIZE(file_types) && file_types[type] )
+        return file_types[type];
+
+    return "none";
+}
 #endif
 #ifdef LIBC_DEBUG
 static void dump_set(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds, struct timeval *timeout)
@@ -663,7 +759,7 @@ static void dump_set(int nfds, fd_set *readfds, fd_set *writefds, fd_set *except
 	if (FD_ISSET(i, set)) { \
 	    if (comma) \
 		printk(", "); \
-	    printk("%d(%c)", i, file_types[files[i].type]); \
+	    printk("%d(%s)", i, get_type_name(files[i].type)); \
 	    comma = 1; \
 	} \
     } \
@@ -697,7 +793,7 @@ static void dump_pollfds(struct pollfd *pfd, int nfds, int timeout)
         fd = pfd[i].fd;
         if (comma)
             printk(", ");
-        printk("%d(%c)/%02x", fd, file_types[files[fd].type],
+        printk("%d(%s)/%02x", fd, get_type_name(files[fd].type),
             pfd[i].events);
             comma = 1;
     }
@@ -709,6 +805,18 @@ static void dump_pollfds(struct pollfd *pfd, int nfds, int timeout)
 #define dump_pollfds(pfds, nfds, timeout)
 #endif
 
+bool select_yes(int fd)
+{
+    return true;
+}
+
+bool select_read_flag(int fd)
+{
+    struct file *file = get_file_from_fd(fd);
+
+    return file->read;
+}
+
 /* Just poll without blocking */
 static int select_poll(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds)
 {
@@ -762,9 +870,35 @@ static int select_poll(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exce
     for (i = 0; i < nfds; i++) {
 	switch(files[i].type) {
 	default:
+        {
+            struct file_ops *ops = file_ops[files[i].type];
+
+            if ( ops )
+            {
+                if ( FD_ISSET(i, readfds) )
+                {
+                    if ( ops->select_rd && ops->select_rd(i) )
+                        n++;
+                    else
+                        FD_CLR(i, readfds);
+                }
+                if ( FD_ISSET(i, writefds) )
+                {
+                    if ( ops->select_wr && ops->select_wr(i) )
+                        n++;
+                    else
+                        FD_CLR(i, writefds);
+                }
+	        FD_CLR(i, exceptfds);
+
+                break;
+            }
+
 	    if (FD_ISSET(i, readfds) || FD_ISSET(i, writefds) || FD_ISSET(i, exceptfds))
 		printk("bogus fd %d in select\n", i);
 	    /* Fallthrough.  */
+        }
+
 	case FTYPE_CONSOLE:
 	    if (FD_ISSET(i, readfds)) {
                 if (xencons_ring_avail(files[i].dev))
@@ -842,7 +976,7 @@ static int select_poll(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exce
 	printk("%d(%d): ", nb, sock_n);
 	for (i = 0; i < nfds; i++) {
 	    if (nbread[i] || nbwrite[i] || nbexcept[i])
-		printk(" %d(%c):", i, file_types[files[i].type]);
+		printk(" %d(%c):", i, get_type_name(files[i].type));
 	    if (nbread[i])
 	    	printk(" %dR", nbread[i]);
 	    if (nbwrite[i])
-- 
2.26.2

Re: [PATCH v2 17/18] mini-os: use function vectors instead of switch for file operations

[Andrew Cooper]

On 11/01/2022 14:58, Juergen Gross wrote:
> Introduce file type specific function vectors for all the needed file
> operations which are file type specific in order to prepare replacing
> the large switch statements in each generic file function.
>
> Add a function to allocate a new file type dynamically in order to
> prepare removing direct dependencies to external components in the
> future. For this reason switch the file type from an enum to an
> unsigned int. Prepare removal of some statically defines file types
> by putting them at the end of the defined list.
>
> Change the debug output for the file type from a single character to
> a string in order to support a future removal of the file_types[]
> array.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>

A few misc observations.

> diff --git a/include/lib.h b/include/lib.h
> index 7a0546b..b40e213 100644
> --- a/include/lib.h
> +++ b/include/lib.h
> @@ -154,25 +155,51 @@ do {                                                           \
>  void sanity_check(void);
>  
>  #ifdef HAVE_LIBC
> -enum fd_type {
> -    FTYPE_NONE = 0,
> -    FTYPE_CONSOLE,
> -    FTYPE_FILE,
> -    FTYPE_XENBUS,
> -    FTYPE_XC,
> -    FTYPE_EVTCHN,
> -    FTYPE_GNTMAP,
> -    FTYPE_SOCKET,
> -    FTYPE_TAP,
> -    FTYPE_BLK,
> -    FTYPE_KBD,
> -    FTYPE_FB,
> -    FTYPE_MEM,
> -    FTYPE_SAVEFILE,
> -    FTYPE_TPMFRONT,
> -    FTYPE_TPM_TIS,
> +#define FTYPE_NONE       0
> +#define FTYPE_CONSOLE    1
> +#define FTYPE_FILE       2
> +#define FTYPE_SOCKET     3
> +#define FTYPE_MEM        4
> +#define FTYPE_SAVEFILE   5
> +#define FTYPE_FB         6
> +#define FTYPE_KBD        7
> +#define FTYPE_TAP        8
> +#define FTYPE_BLK        9
> +#define FTYPE_TPMFRONT  10
> +#define FTYPE_TPM_TIS   11
> +#define FTYPE_XENBUS    12
> +#define FTYPE_GNTMAP    13
> +#define FTYPE_EVTCHN    14
> +#define FTYPE_XC        15
> +#define FTYPE_N         16
> +#define FTYPE_SPARE     16
> +
> +typedef int file_read_func(int fd, void *buf, size_t nbytes);
> +typedef int file_write_func(int fd, const void *buf, size_t nbytes);
> +typedef off_t file_lseek_func(int fd, off_t offset, int whence);
> +typedef int file_close_func(int fd);
> +typedef int file_fstat_func(int fd, struct stat *buf);
> +typedef int file_fcntl_func(int fd, int cmd, va_list args);
> +typedef bool file_select_func(int fd);

Are these typedef's useful?  I don't see them used anywhere other than
the file_ops structure.

> +
> +struct file_ops {
> +    const char *name;
> +    file_read_func *read;
> +    file_write_func *write;
> +    file_lseek_func *lseek;
> +    file_close_func *close;
> +    file_fstat_func *fstat;
> +    file_fcntl_func *fcntl;
> +    file_select_func *select_rd;
> +    file_select_func *select_wr;
>  };
>  
> +unsigned int alloc_file_type(struct file_ops *ops);

const.

> +
> +off_t lseek_default(int fd, off_t offset, int whence);
> +bool select_yes(int fd);
> +bool select_read_flag(int fd);
> +
>  LIST_HEAD(evtchn_port_list, evtchn_port_info);
>  
>  struct evtchn_port_info {
> @@ -183,7 +210,7 @@ struct evtchn_port_info {
>  };
>  
>  struct file {
> -    enum fd_type type;
> +    unsigned int type;
>      bool read;	/* maybe available for read */
>      off_t offset;
>      union {
> @@ -199,7 +226,7 @@ struct file {
>  extern struct file files[];
>  
>  struct file *get_file_from_fd(int fd);
> -int alloc_fd(enum fd_type type);
> +int alloc_fd(unsigned int type);
>  void close_all_files(void);
>  extern struct thread *main_thread;
>  void sparse(unsigned long data, size_t size);
> diff --git a/lib/sys.c b/lib/sys.c
> index 323a7cd..c327247 100644
> --- a/lib/sys.c
> +++ b/lib/sys.c
> @@ -98,6 +98,39 @@ struct file files[NOFILE] = {
>      { .type = FTYPE_CONSOLE }, /* stderr */
>  };
>  
> +static struct file_ops file_ops_none = {
> +    .name = "none",
> +};
> +
> +static struct file_ops *file_ops[FTYPE_N + FTYPE_SPARE] = {
> +    [FTYPE_NONE] = &file_ops_none,
> +};

Both want to be const, because all file_ops ought to live in rodata.

> +
> +unsigned int alloc_file_type(struct file_ops *ops)
> +{
> +    unsigned int i;
> +
> +    pthread_mutex_lock(&fd_lock);
> +
> +    for ( i = FTYPE_N; i < ARRAY_SIZE(file_ops) && file_ops[i]; i++ );

No need for a for loop.  Given the mutex, you can safely do something
like this:

static unsigned int i = FTYPE_N;

BUG_ON(i == ARRAY_SIZE(file_ops));
file_ops[i++] = ops;

Alternatively, if you do want to keep the loop, we might want to confirm
that the same ops structure doesn't get registered twice.

> +    BUG_ON(i == ARRAY_SIZE(file_ops));
> +    file_ops[i] = ops;
> +
> +    pthread_mutex_unlock(&fd_lock);
> +
> +    printk("New file type \"%s\"(%u) allocated\n", ops->name, i);
> +
> +    return i;
> +}
> +
> +static struct file_ops *get_file_ops(unsigned int type)
> +{
> +    if ( type >= ARRAY_SIZE(file_ops) || !file_ops[type] )
> +        return &file_ops_none;
> +
> +    return file_ops[type];
> +}
> +
>  struct file *get_file_from_fd(int fd)
>  {
>      if ( fd < 0 || fd >= NOFILE )
> @@ -108,7 +141,7 @@ struct file *get_file_from_fd(int fd)
>  
>  DECLARE_WAIT_QUEUE_HEAD(event_queue);
>  
> -int alloc_fd(enum fd_type type)
> +int alloc_fd(unsigned int type)
>  {
>      int i;
>      pthread_mutex_lock(&fd_lock);
> @@ -249,6 +282,11 @@ int isatty(int fd)
>  
>  int read(int fd, void *buf, size_t nbytes)
>  {
> +    struct file_ops *ops = get_file_ops(files[fd].type);
> +
> +    if ( ops->read )
> +        return ops->read(fd, buf, nbytes);
> +
>      switch (files[fd].type) {
>          case FTYPE_SAVEFILE:
>  	case FTYPE_CONSOLE: {
> @@ -328,6 +366,11 @@ int read(int fd, void *buf, size_t nbytes)
>  
>  int write(int fd, const void *buf, size_t nbytes)
>  {
> +    struct file_ops *ops = get_file_ops(files[fd].type);
> +
> +    if ( ops->write )
> +        return ops->write(fd, buf, nbytes);
> +
>      switch (files[fd].type) {
>          case FTYPE_SAVEFILE: {
>                  int ret = 0, tot = nbytes;
> @@ -370,8 +413,45 @@ int write(int fd, const void *buf, size_t nbytes)
>      return -1;
>  }
>  
> +off_t lseek_default(int fd, off_t offset, int whence)
> +{
> +    switch ( whence )
> +    {
> +    case SEEK_SET:
> +        files[fd].offset = offset;
> +        break;
> +
> +    case SEEK_CUR:
> +        files[fd].offset += offset;
> +        break;
> +
> +    case SEEK_END:
> +    {
> +        struct stat st;
> +        int ret;
> +
> +        ret = fstat(fd, &st);
> +        if ( ret )
> +            return -1;
> +        files[fd].offset = st.st_size + offset;
> +        break;
> +    }
> +
> +    default:
> +        errno = EINVAL;
> +        return -1;
> +    }
> +
> +    return files[fd].offset;
> +}
> +
>  off_t lseek(int fd, off_t offset, int whence)
>  {
> +    struct file_ops *ops = get_file_ops(files[fd].type);
> +
> +    if ( ops->lseek )
> +        return ops->lseek(fd, offset, whence);
> +
>      switch(files[fd].type) {
>  #ifdef CONFIG_BLKFRONT
>         case FTYPE_BLK:
> @@ -393,28 +473,7 @@ off_t lseek(int fd, off_t offset, int whence)
>            return (off_t) -1;
>      }
>  
> -    switch (whence) {
> -       case SEEK_SET:
> -          files[fd].offset = offset;
> -          break;
> -       case SEEK_CUR:
> -          files[fd].offset += offset;
> -          break;
> -       case SEEK_END:
> -          {
> -             struct stat st;
> -             int ret;
> -             ret = fstat(fd, &st);
> -             if (ret)
> -                return -1;
> -             files[fd].offset = st.st_size + offset;
> -             break;
> -          }
> -       default:
> -          errno = EINVAL;
> -          return -1;
> -    }
> -    return files[fd].offset;
> +    return lseek_default(fd, offset, whence);
>  }
>  
>  int fsync(int fd) {
> @@ -425,8 +484,15 @@ int fsync(int fd) {
>  int close(int fd)
>  {
>      int res = 0;
> +    struct file_ops *ops = get_file_ops(files[fd].type);
>  
>      printk("close(%d)\n", fd);
> +    if ( ops->close )
> +    {
> +        res = ops->close(fd);
> +        goto out;
> +    }
> +
>      switch (files[fd].type) {
>          default:
>              break;
> @@ -497,6 +563,7 @@ int close(int fd)
>              return -1;
>      }
>  
> + out:
>      memset(files + fd, 0, sizeof(struct file));
>      files[fd].type = FTYPE_NONE;
>      return res;
> @@ -521,7 +588,13 @@ int stat(const char *path, struct stat *buf)
>  
>  int fstat(int fd, struct stat *buf)
>  {
> +    struct file_ops *ops = get_file_ops(files[fd].type);
> +
>      init_stat(buf);
> +
> +    if ( ops->fstat )
> +        return ops->fstat(fd, buf);
> +
>      switch (files[fd].type) {
>  	case FTYPE_SAVEFILE:
>  	case FTYPE_CONSOLE:
> @@ -587,6 +660,18 @@ int fcntl(int fd, int cmd, ...)
>  {
>      long arg;
>      va_list ap;
> +    int res;
> +    struct file_ops *ops = get_file_ops(files[fd].type);
> +
> +    if ( ops->fcntl )
> +    {
> +        va_start(ap, cmd);
> +        res = ops->fcntl(fd, cmd, ap);
> +        va_end(ap);
> +
> +        return res;
> +    }
> +
>      va_start(ap, cmd);
>      arg = va_arg(ap, long);
>      va_end(ap);
> @@ -640,18 +725,29 @@ int closedir(DIR *dir)
>  /* We assume that only the main thread calls select(). */
>  
>  #if defined(LIBC_DEBUG) || defined(LIBC_VERBOSE)
> -static const char file_types[] = {
> -    [FTYPE_NONE]	= 'N',
> -    [FTYPE_CONSOLE]	= 'C',
> -    [FTYPE_XENBUS]	= 'S',
> -    [FTYPE_XC]		= 'X',
> -    [FTYPE_EVTCHN]	= 'E',
> -    [FTYPE_SOCKET]	= 's',
> -    [FTYPE_TAP]		= 'T',
> -    [FTYPE_BLK]		= 'B',
> -    [FTYPE_KBD]		= 'K',
> -    [FTYPE_FB]		= 'G',
> +static const char *file_types[] = {

static const char *const file_types[]

> +    [FTYPE_NONE]    = "none",
> +    [FTYPE_CONSOLE] = "console",
> +    [FTYPE_XENBUS]  = "xenbus",
> +    [FTYPE_XC]      = "ctrl",
> +    [FTYPE_EVTCHN]  = "evtchn",
> +    [FTYPE_SOCKET]  = "socket",
> +    [FTYPE_TAP]     = "net",
> +    [FTYPE_BLK]     = "blk",
> +    [FTYPE_KBD]     = "kbd",
> +    [FTYPE_FB]      = "fb",
>  };
> +
> +static char *get_type_name(unsigned int type)

const.

> +{
> +    if ( type < ARRAY_SIZE(file_ops) && file_ops[type] )
> +        return file_ops[type]->name;
> +
> +    if ( type < ARRAY_SIZE(file_types) && file_types[type] )
> +        return file_types[type];
> +
> +    return "none";
> +}
>  #endif
>  #ifdef LIBC_DEBUG
>  static void dump_set(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds, struct timeval *timeout)
> @@ -663,7 +759,7 @@ static void dump_set(int nfds, fd_set *readfds, fd_set *writefds, fd_set *except
>  	if (FD_ISSET(i, set)) { \
>  	    if (comma) \
>  		printk(", "); \
> -	    printk("%d(%c)", i, file_types[files[i].type]); \
> +	    printk("%d(%s)", i, get_type_name(files[i].type)); \
>  	    comma = 1; \
>  	} \
>      } \
> @@ -697,7 +793,7 @@ static void dump_pollfds(struct pollfd *pfd, int nfds, int timeout)
>          fd = pfd[i].fd;
>          if (comma)
>              printk(", ");
> -        printk("%d(%c)/%02x", fd, file_types[files[fd].type],
> +        printk("%d(%s)/%02x", fd, get_type_name(files[fd].type),
>              pfd[i].events);
>              comma = 1;
>      }
> @@ -709,6 +805,18 @@ static void dump_pollfds(struct pollfd *pfd, int nfds, int timeout)
>  #define dump_pollfds(pfds, nfds, timeout)
>  #endif
>  
> +bool select_yes(int fd)
> +{
> +    return true;
> +}
> +
> +bool select_read_flag(int fd)
> +{
> +    struct file *file = get_file_from_fd(fd);
> +
> +    return file->read;
> +}

I don't see these getting used, even in a fallback case.

> +
>  /* Just poll without blocking */
>  static int select_poll(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds)
>  {
> @@ -762,9 +870,35 @@ static int select_poll(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exce
>      for (i = 0; i < nfds; i++) {
>  	switch(files[i].type) {
>  	default:
> +        {
> +            struct file_ops *ops = file_ops[files[i].type];
> +
> +            if ( ops )
> +            {
> +                if ( FD_ISSET(i, readfds) )
> +                {
> +                    if ( ops->select_rd && ops->select_rd(i) )
> +                        n++;
> +                    else
> +                        FD_CLR(i, readfds);
> +                }
> +                if ( FD_ISSET(i, writefds) )
> +                {
> +                    if ( ops->select_wr && ops->select_wr(i) )
> +                        n++;
> +                    else
> +                        FD_CLR(i, writefds);
> +                }
> +	        FD_CLR(i, exceptfds);

Hard tab.

> +
> +                break;
> +            }
> +
>  	    if (FD_ISSET(i, readfds) || FD_ISSET(i, writefds) || FD_ISSET(i, exceptfds))
>  		printk("bogus fd %d in select\n", i);
>  	    /* Fallthrough.  */
> +        }

Is this fallthrough really appropriate, given the break in the ops case?

~Andrew

> +
>  	case FTYPE_CONSOLE:
>  	    if (FD_ISSET(i, readfds)) {
>                  if (xencons_ring_avail(files[i].dev))
> @@ -842,7 +976,7 @@ static int select_poll(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exce
>  	printk("%d(%d): ", nb, sock_n);
>  	for (i = 0; i < nfds; i++) {
>  	    if (nbread[i] || nbwrite[i] || nbexcept[i])
> -		printk(" %d(%c):", i, file_types[files[i].type]);
> +		printk(" %d(%c):", i, get_type_name(files[i].type));
>  	    if (nbread[i])
>  	    	printk(" %dR", nbread[i]);
>  	    if (nbwrite[i])

Re: [PATCH v2 17/18] mini-os: use function vectors instead of switch for file operations

[Samuel Thibault]

Andrew Cooper, le mar. 11 janv. 2022 18:08:27 +0000, a ecrit:
> On 11/01/2022 14:58, Juergen Gross wrote:
> > +typedef int file_read_func(int fd, void *buf, size_t nbytes);
> > +typedef int file_write_func(int fd, const void *buf, size_t nbytes);
> > +typedef off_t file_lseek_func(int fd, off_t offset, int whence);
> > +typedef int file_close_func(int fd);
> > +typedef int file_fstat_func(int fd, struct stat *buf);
> > +typedef int file_fcntl_func(int fd, int cmd, va_list args);
> > +typedef bool file_select_func(int fd);
> 
> Are these typedef's useful?  I don't see them used anywhere other than
> the file_ops structure.
> 
> > +
> > +struct file_ops {
> > +    const char *name;
> > +    file_read_func *read;
> > +    file_write_func *write;
> > +    file_lseek_func *lseek;
> > +    file_close_func *close;
> > +    file_fstat_func *fstat;
> > +    file_fcntl_func *fcntl;
> > +    file_select_func *select_rd;
> > +    file_select_func *select_wr;
> >  };

I agree, I'd rather see the protoypes inside the struct itself.

Samuel

Re: [PATCH v2 17/18] mini-os: use function vectors instead of switch for file operations

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 14419 bytes --]

On 11.01.22 19:08, Andrew Cooper wrote:
> On 11/01/2022 14:58, Juergen Gross wrote:
>> Introduce file type specific function vectors for all the needed file
>> operations which are file type specific in order to prepare replacing
>> the large switch statements in each generic file function.
>>
>> Add a function to allocate a new file type dynamically in order to
>> prepare removing direct dependencies to external components in the
>> future. For this reason switch the file type from an enum to an
>> unsigned int. Prepare removal of some statically defines file types
>> by putting them at the end of the defined list.
>>
>> Change the debug output for the file type from a single character to
>> a string in order to support a future removal of the file_types[]
>> array.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> A few misc observations.
> 
>> diff --git a/include/lib.h b/include/lib.h
>> index 7a0546b..b40e213 100644
>> --- a/include/lib.h
>> +++ b/include/lib.h
>> @@ -154,25 +155,51 @@ do {                                                           \
>>   void sanity_check(void);
>>   
>>   #ifdef HAVE_LIBC
>> -enum fd_type {
>> -    FTYPE_NONE = 0,
>> -    FTYPE_CONSOLE,
>> -    FTYPE_FILE,
>> -    FTYPE_XENBUS,
>> -    FTYPE_XC,
>> -    FTYPE_EVTCHN,
>> -    FTYPE_GNTMAP,
>> -    FTYPE_SOCKET,
>> -    FTYPE_TAP,
>> -    FTYPE_BLK,
>> -    FTYPE_KBD,
>> -    FTYPE_FB,
>> -    FTYPE_MEM,
>> -    FTYPE_SAVEFILE,
>> -    FTYPE_TPMFRONT,
>> -    FTYPE_TPM_TIS,
>> +#define FTYPE_NONE       0
>> +#define FTYPE_CONSOLE    1
>> +#define FTYPE_FILE       2
>> +#define FTYPE_SOCKET     3
>> +#define FTYPE_MEM        4
>> +#define FTYPE_SAVEFILE   5
>> +#define FTYPE_FB         6
>> +#define FTYPE_KBD        7
>> +#define FTYPE_TAP        8
>> +#define FTYPE_BLK        9
>> +#define FTYPE_TPMFRONT  10
>> +#define FTYPE_TPM_TIS   11
>> +#define FTYPE_XENBUS    12
>> +#define FTYPE_GNTMAP    13
>> +#define FTYPE_EVTCHN    14
>> +#define FTYPE_XC        15
>> +#define FTYPE_N         16
>> +#define FTYPE_SPARE     16
>> +
>> +typedef int file_read_func(int fd, void *buf, size_t nbytes);
>> +typedef int file_write_func(int fd, const void *buf, size_t nbytes);
>> +typedef off_t file_lseek_func(int fd, off_t offset, int whence);
>> +typedef int file_close_func(int fd);
>> +typedef int file_fstat_func(int fd, struct stat *buf);
>> +typedef int file_fcntl_func(int fd, int cmd, va_list args);
>> +typedef bool file_select_func(int fd);
> 
> Are these typedef's useful?  I don't see them used anywhere other than
> the file_ops structure.

I can drop them.

> 
>> +
>> +struct file_ops {
>> +    const char *name;
>> +    file_read_func *read;
>> +    file_write_func *write;
>> +    file_lseek_func *lseek;
>> +    file_close_func *close;
>> +    file_fstat_func *fstat;
>> +    file_fcntl_func *fcntl;
>> +    file_select_func *select_rd;
>> +    file_select_func *select_wr;
>>   };
>>   
>> +unsigned int alloc_file_type(struct file_ops *ops);
> 
> const.

Yes.

> 
>> +
>> +off_t lseek_default(int fd, off_t offset, int whence);
>> +bool select_yes(int fd);
>> +bool select_read_flag(int fd);
>> +
>>   LIST_HEAD(evtchn_port_list, evtchn_port_info);
>>   
>>   struct evtchn_port_info {
>> @@ -183,7 +210,7 @@ struct evtchn_port_info {
>>   };
>>   
>>   struct file {
>> -    enum fd_type type;
>> +    unsigned int type;
>>       bool read;	/* maybe available for read */
>>       off_t offset;
>>       union {
>> @@ -199,7 +226,7 @@ struct file {
>>   extern struct file files[];
>>   
>>   struct file *get_file_from_fd(int fd);
>> -int alloc_fd(enum fd_type type);
>> +int alloc_fd(unsigned int type);
>>   void close_all_files(void);
>>   extern struct thread *main_thread;
>>   void sparse(unsigned long data, size_t size);
>> diff --git a/lib/sys.c b/lib/sys.c
>> index 323a7cd..c327247 100644
>> --- a/lib/sys.c
>> +++ b/lib/sys.c
>> @@ -98,6 +98,39 @@ struct file files[NOFILE] = {
>>       { .type = FTYPE_CONSOLE }, /* stderr */
>>   };
>>   
>> +static struct file_ops file_ops_none = {
>> +    .name = "none",
>> +};
>> +
>> +static struct file_ops *file_ops[FTYPE_N + FTYPE_SPARE] = {
>> +    [FTYPE_NONE] = &file_ops_none,
>> +};
> 
> Both want to be const, because all file_ops ought to live in rodata.

file_ops[] can't be const, it will just point to const data.

> 
>> +
>> +unsigned int alloc_file_type(struct file_ops *ops)
>> +{
>> +    unsigned int i;
>> +
>> +    pthread_mutex_lock(&fd_lock);
>> +
>> +    for ( i = FTYPE_N; i < ARRAY_SIZE(file_ops) && file_ops[i]; i++ );
> 
> No need for a for loop.  Given the mutex, you can safely do something
> like this:
> 
> static unsigned int i = FTYPE_N;
> 
> BUG_ON(i == ARRAY_SIZE(file_ops));
> file_ops[i++] = ops;

Will change it that way.

> 
> Alternatively, if you do want to keep the loop, we might want to confirm
> that the same ops structure doesn't get registered twice.
> 
>> +    BUG_ON(i == ARRAY_SIZE(file_ops));
>> +    file_ops[i] = ops;
>> +
>> +    pthread_mutex_unlock(&fd_lock);
>> +
>> +    printk("New file type \"%s\"(%u) allocated\n", ops->name, i);
>> +
>> +    return i;
>> +}
>> +
>> +static struct file_ops *get_file_ops(unsigned int type)
>> +{
>> +    if ( type >= ARRAY_SIZE(file_ops) || !file_ops[type] )
>> +        return &file_ops_none;
>> +
>> +    return file_ops[type];
>> +}
>> +
>>   struct file *get_file_from_fd(int fd)
>>   {
>>       if ( fd < 0 || fd >= NOFILE )
>> @@ -108,7 +141,7 @@ struct file *get_file_from_fd(int fd)
>>   
>>   DECLARE_WAIT_QUEUE_HEAD(event_queue);
>>   
>> -int alloc_fd(enum fd_type type)
>> +int alloc_fd(unsigned int type)
>>   {
>>       int i;
>>       pthread_mutex_lock(&fd_lock);
>> @@ -249,6 +282,11 @@ int isatty(int fd)
>>   
>>   int read(int fd, void *buf, size_t nbytes)
>>   {
>> +    struct file_ops *ops = get_file_ops(files[fd].type);
>> +
>> +    if ( ops->read )
>> +        return ops->read(fd, buf, nbytes);
>> +
>>       switch (files[fd].type) {
>>           case FTYPE_SAVEFILE:
>>   	case FTYPE_CONSOLE: {
>> @@ -328,6 +366,11 @@ int read(int fd, void *buf, size_t nbytes)
>>   
>>   int write(int fd, const void *buf, size_t nbytes)
>>   {
>> +    struct file_ops *ops = get_file_ops(files[fd].type);
>> +
>> +    if ( ops->write )
>> +        return ops->write(fd, buf, nbytes);
>> +
>>       switch (files[fd].type) {
>>           case FTYPE_SAVEFILE: {
>>                   int ret = 0, tot = nbytes;
>> @@ -370,8 +413,45 @@ int write(int fd, const void *buf, size_t nbytes)
>>       return -1;
>>   }
>>   
>> +off_t lseek_default(int fd, off_t offset, int whence)
>> +{
>> +    switch ( whence )
>> +    {
>> +    case SEEK_SET:
>> +        files[fd].offset = offset;
>> +        break;
>> +
>> +    case SEEK_CUR:
>> +        files[fd].offset += offset;
>> +        break;
>> +
>> +    case SEEK_END:
>> +    {
>> +        struct stat st;
>> +        int ret;
>> +
>> +        ret = fstat(fd, &st);
>> +        if ( ret )
>> +            return -1;
>> +        files[fd].offset = st.st_size + offset;
>> +        break;
>> +    }
>> +
>> +    default:
>> +        errno = EINVAL;
>> +        return -1;
>> +    }
>> +
>> +    return files[fd].offset;
>> +}
>> +
>>   off_t lseek(int fd, off_t offset, int whence)
>>   {
>> +    struct file_ops *ops = get_file_ops(files[fd].type);
>> +
>> +    if ( ops->lseek )
>> +        return ops->lseek(fd, offset, whence);
>> +
>>       switch(files[fd].type) {
>>   #ifdef CONFIG_BLKFRONT
>>          case FTYPE_BLK:
>> @@ -393,28 +473,7 @@ off_t lseek(int fd, off_t offset, int whence)
>>             return (off_t) -1;
>>       }
>>   
>> -    switch (whence) {
>> -       case SEEK_SET:
>> -          files[fd].offset = offset;
>> -          break;
>> -       case SEEK_CUR:
>> -          files[fd].offset += offset;
>> -          break;
>> -       case SEEK_END:
>> -          {
>> -             struct stat st;
>> -             int ret;
>> -             ret = fstat(fd, &st);
>> -             if (ret)
>> -                return -1;
>> -             files[fd].offset = st.st_size + offset;
>> -             break;
>> -          }
>> -       default:
>> -          errno = EINVAL;
>> -          return -1;
>> -    }
>> -    return files[fd].offset;
>> +    return lseek_default(fd, offset, whence);
>>   }
>>   
>>   int fsync(int fd) {
>> @@ -425,8 +484,15 @@ int fsync(int fd) {
>>   int close(int fd)
>>   {
>>       int res = 0;
>> +    struct file_ops *ops = get_file_ops(files[fd].type);
>>   
>>       printk("close(%d)\n", fd);
>> +    if ( ops->close )
>> +    {
>> +        res = ops->close(fd);
>> +        goto out;
>> +    }
>> +
>>       switch (files[fd].type) {
>>           default:
>>               break;
>> @@ -497,6 +563,7 @@ int close(int fd)
>>               return -1;
>>       }
>>   
>> + out:
>>       memset(files + fd, 0, sizeof(struct file));
>>       files[fd].type = FTYPE_NONE;
>>       return res;
>> @@ -521,7 +588,13 @@ int stat(const char *path, struct stat *buf)
>>   
>>   int fstat(int fd, struct stat *buf)
>>   {
>> +    struct file_ops *ops = get_file_ops(files[fd].type);
>> +
>>       init_stat(buf);
>> +
>> +    if ( ops->fstat )
>> +        return ops->fstat(fd, buf);
>> +
>>       switch (files[fd].type) {
>>   	case FTYPE_SAVEFILE:
>>   	case FTYPE_CONSOLE:
>> @@ -587,6 +660,18 @@ int fcntl(int fd, int cmd, ...)
>>   {
>>       long arg;
>>       va_list ap;
>> +    int res;
>> +    struct file_ops *ops = get_file_ops(files[fd].type);
>> +
>> +    if ( ops->fcntl )
>> +    {
>> +        va_start(ap, cmd);
>> +        res = ops->fcntl(fd, cmd, ap);
>> +        va_end(ap);
>> +
>> +        return res;
>> +    }
>> +
>>       va_start(ap, cmd);
>>       arg = va_arg(ap, long);
>>       va_end(ap);
>> @@ -640,18 +725,29 @@ int closedir(DIR *dir)
>>   /* We assume that only the main thread calls select(). */
>>   
>>   #if defined(LIBC_DEBUG) || defined(LIBC_VERBOSE)
>> -static const char file_types[] = {
>> -    [FTYPE_NONE]	= 'N',
>> -    [FTYPE_CONSOLE]	= 'C',
>> -    [FTYPE_XENBUS]	= 'S',
>> -    [FTYPE_XC]		= 'X',
>> -    [FTYPE_EVTCHN]	= 'E',
>> -    [FTYPE_SOCKET]	= 's',
>> -    [FTYPE_TAP]		= 'T',
>> -    [FTYPE_BLK]		= 'B',
>> -    [FTYPE_KBD]		= 'K',
>> -    [FTYPE_FB]		= 'G',
>> +static const char *file_types[] = {
> 
> static const char *const file_types[]

Okay.

> 
>> +    [FTYPE_NONE]    = "none",
>> +    [FTYPE_CONSOLE] = "console",
>> +    [FTYPE_XENBUS]  = "xenbus",
>> +    [FTYPE_XC]      = "ctrl",
>> +    [FTYPE_EVTCHN]  = "evtchn",
>> +    [FTYPE_SOCKET]  = "socket",
>> +    [FTYPE_TAP]     = "net",
>> +    [FTYPE_BLK]     = "blk",
>> +    [FTYPE_KBD]     = "kbd",
>> +    [FTYPE_FB]      = "fb",
>>   };
>> +
>> +static char *get_type_name(unsigned int type)
> 
> const.

Okay.

> 
>> +{
>> +    if ( type < ARRAY_SIZE(file_ops) && file_ops[type] )
>> +        return file_ops[type]->name;
>> +
>> +    if ( type < ARRAY_SIZE(file_types) && file_types[type] )
>> +        return file_types[type];
>> +
>> +    return "none";
>> +}
>>   #endif
>>   #ifdef LIBC_DEBUG
>>   static void dump_set(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds, struct timeval *timeout)
>> @@ -663,7 +759,7 @@ static void dump_set(int nfds, fd_set *readfds, fd_set *writefds, fd_set *except
>>   	if (FD_ISSET(i, set)) { \
>>   	    if (comma) \
>>   		printk(", "); \
>> -	    printk("%d(%c)", i, file_types[files[i].type]); \
>> +	    printk("%d(%s)", i, get_type_name(files[i].type)); \
>>   	    comma = 1; \
>>   	} \
>>       } \
>> @@ -697,7 +793,7 @@ static void dump_pollfds(struct pollfd *pfd, int nfds, int timeout)
>>           fd = pfd[i].fd;
>>           if (comma)
>>               printk(", ");
>> -        printk("%d(%c)/%02x", fd, file_types[files[fd].type],
>> +        printk("%d(%s)/%02x", fd, get_type_name(files[fd].type),
>>               pfd[i].events);
>>               comma = 1;
>>       }
>> @@ -709,6 +805,18 @@ static void dump_pollfds(struct pollfd *pfd, int nfds, int timeout)
>>   #define dump_pollfds(pfds, nfds, timeout)
>>   #endif
>>   
>> +bool select_yes(int fd)
>> +{
>> +    return true;
>> +}
>> +
>> +bool select_read_flag(int fd)
>> +{
>> +    struct file *file = get_file_from_fd(fd);
>> +
>> +    return file->read;
>> +}
> 
> I don't see these getting used, even in a fallback case.

They will be used in later patches.

> 
>> +
>>   /* Just poll without blocking */
>>   static int select_poll(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds)
>>   {
>> @@ -762,9 +870,35 @@ static int select_poll(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exce
>>       for (i = 0; i < nfds; i++) {
>>   	switch(files[i].type) {
>>   	default:
>> +        {
>> +            struct file_ops *ops = file_ops[files[i].type];
>> +
>> +            if ( ops )
>> +            {
>> +                if ( FD_ISSET(i, readfds) )
>> +                {
>> +                    if ( ops->select_rd && ops->select_rd(i) )
>> +                        n++;
>> +                    else
>> +                        FD_CLR(i, readfds);
>> +                }
>> +                if ( FD_ISSET(i, writefds) )
>> +                {
>> +                    if ( ops->select_wr && ops->select_wr(i) )
>> +                        n++;
>> +                    else
>> +                        FD_CLR(i, writefds);
>> +                }
>> +	        FD_CLR(i, exceptfds);
> 
> Hard tab.

Thanks for noting.

> 
>> +
>> +                break;
>> +            }
>> +
>>   	    if (FD_ISSET(i, readfds) || FD_ISSET(i, writefds) || FD_ISSET(i, exceptfds))
>>   		printk("bogus fd %d in select\n", i);
>>   	    /* Fallthrough.  */
>> +        }
> 
> Is this fallthrough really appropriate, given the break in the ops case?

I think this was meant for the FTYPE_SAVEFILE case. For all other file
types this wouldn't make any sense.

Note that in the other Mini-OS series I'm setting the savefile select
callbacks appropriately.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2 17/18] mini-os: use function vectors instead of switch for file operations

[Andrew Cooper]

On 12/01/2022 07:34, Juergen Gross wrote:
> On 11.01.22 19:08, Andrew Cooper wrote:
>> On 11/01/2022 14:58, Juergen Gross wrote:
>>> +
>>> +off_t lseek_default(int fd, off_t offset, int whence);
>>> +bool select_yes(int fd);
>>> +bool select_read_flag(int fd);
>>> +
>>>   LIST_HEAD(evtchn_port_list, evtchn_port_info);
>>>     struct evtchn_port_info {
>>> @@ -183,7 +210,7 @@ struct evtchn_port_info {
>>>   };
>>>     struct file {
>>> -    enum fd_type type;
>>> +    unsigned int type;
>>>       bool read;    /* maybe available for read */
>>>       off_t offset;
>>>       union {
>>> @@ -199,7 +226,7 @@ struct file {
>>>   extern struct file files[];
>>>     struct file *get_file_from_fd(int fd);
>>> -int alloc_fd(enum fd_type type);
>>> +int alloc_fd(unsigned int type);
>>>   void close_all_files(void);
>>>   extern struct thread *main_thread;
>>>   void sparse(unsigned long data, size_t size);
>>> diff --git a/lib/sys.c b/lib/sys.c
>>> index 323a7cd..c327247 100644
>>> --- a/lib/sys.c
>>> +++ b/lib/sys.c
>>> @@ -98,6 +98,39 @@ struct file files[NOFILE] = {
>>>       { .type = FTYPE_CONSOLE }, /* stderr */
>>>   };
>>>   +static struct file_ops file_ops_none = {
>>> +    .name = "none",
>>> +};
>>> +
>>> +static struct file_ops *file_ops[FTYPE_N + FTYPE_SPARE] = {
>>> +    [FTYPE_NONE] = &file_ops_none,
>>> +};
>>
>> Both want to be const, because all file_ops ought to live in rodata.
>
> file_ops[] can't be const, it will just point to const data.

Oh, of course.

>>> @@ -709,6 +805,18 @@ static void dump_pollfds(struct pollfd *pfd,
>>> int nfds, int timeout)
>>>   #define dump_pollfds(pfds, nfds, timeout)
>>>   #endif
>>>   +bool select_yes(int fd)
>>> +{
>>> +    return true;
>>> +}
>>> +
>>> +bool select_read_flag(int fd)
>>> +{
>>> +    struct file *file = get_file_from_fd(fd);
>>> +
>>> +    return file->read;
>>> +}
>>
>> I don't see these getting used, even in a fallback case.
>
> They will be used in later patches.

Yeah - I found them in later patches.  It's probably worth a note in the
commit message saying "provide some functions useful for file_ops in
future patches", particularly as it's actually a later series which
takes them (due to the ordering constraints).

~Andrew

Re: [PATCH v2 17/18] mini-os: use function vectors instead of switch for file operations

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 2531 bytes --]

On 12.01.22 09:19, Andrew Cooper wrote:
> On 12/01/2022 07:34, Juergen Gross wrote:
>> On 11.01.22 19:08, Andrew Cooper wrote:
>>> On 11/01/2022 14:58, Juergen Gross wrote:
>>>> +
>>>> +off_t lseek_default(int fd, off_t offset, int whence);
>>>> +bool select_yes(int fd);
>>>> +bool select_read_flag(int fd);
>>>> +
>>>>    LIST_HEAD(evtchn_port_list, evtchn_port_info);
>>>>      struct evtchn_port_info {
>>>> @@ -183,7 +210,7 @@ struct evtchn_port_info {
>>>>    };
>>>>      struct file {
>>>> -    enum fd_type type;
>>>> +    unsigned int type;
>>>>        bool read;    /* maybe available for read */
>>>>        off_t offset;
>>>>        union {
>>>> @@ -199,7 +226,7 @@ struct file {
>>>>    extern struct file files[];
>>>>      struct file *get_file_from_fd(int fd);
>>>> -int alloc_fd(enum fd_type type);
>>>> +int alloc_fd(unsigned int type);
>>>>    void close_all_files(void);
>>>>    extern struct thread *main_thread;
>>>>    void sparse(unsigned long data, size_t size);
>>>> diff --git a/lib/sys.c b/lib/sys.c
>>>> index 323a7cd..c327247 100644
>>>> --- a/lib/sys.c
>>>> +++ b/lib/sys.c
>>>> @@ -98,6 +98,39 @@ struct file files[NOFILE] = {
>>>>        { .type = FTYPE_CONSOLE }, /* stderr */
>>>>    };
>>>>    +static struct file_ops file_ops_none = {
>>>> +    .name = "none",
>>>> +};
>>>> +
>>>> +static struct file_ops *file_ops[FTYPE_N + FTYPE_SPARE] = {
>>>> +    [FTYPE_NONE] = &file_ops_none,
>>>> +};
>>>
>>> Both want to be const, because all file_ops ought to live in rodata.
>>
>> file_ops[] can't be const, it will just point to const data.
> 
> Oh, of course.
> 
>>>> @@ -709,6 +805,18 @@ static void dump_pollfds(struct pollfd *pfd,
>>>> int nfds, int timeout)
>>>>    #define dump_pollfds(pfds, nfds, timeout)
>>>>    #endif
>>>>    +bool select_yes(int fd)
>>>> +{
>>>> +    return true;
>>>> +}
>>>> +
>>>> +bool select_read_flag(int fd)
>>>> +{
>>>> +    struct file *file = get_file_from_fd(fd);
>>>> +
>>>> +    return file->read;
>>>> +}
>>>
>>> I don't see these getting used, even in a fallback case.
>>
>> They will be used in later patches.
> 
> Yeah - I found them in later patches.  It's probably worth a note in the
> commit message saying "provide some functions useful for file_ops in
> future patches", particularly as it's actually a later series which
> takes them (due to the ordering constraints).

Okay, will add that note.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2 17/18] mini-os: use function vectors instead of switch for file operations

[Samuel Thibault]

Juergen Gross, le mar. 11 janv. 2022 15:58:16 +0100, a ecrit:
> +    for ( i = FTYPE_N; i < ARRAY_SIZE(file_ops) && file_ops[i]; i++ );

For such loops, I'd rather see the ';' on its own line, to make it clear
it's an empty loop.

Samuel

Re: [PATCH v2 17/18] mini-os: use function vectors instead of switch for file operations

[Samuel Thibault]

Juergen Gross, le mar. 11 janv. 2022 15:58:16 +0100, a ecrit:
> @@ -370,8 +413,45 @@ int write(int fd, const void *buf, size_t nbytes)
>      return -1;
>  }
>  
> +off_t lseek_default(int fd, off_t offset, int whence)
> +{
> +    switch ( whence )
> +    {

Is there a reason for making this one a separate function, unlike others
for which you kept the code in the main function?

Apart from that, this looks good to me.

> +    case SEEK_SET:
> +        files[fd].offset = offset;
> +        break;
> +
> +    case SEEK_CUR:
> +        files[fd].offset += offset;
> +        break;
> +
> +    case SEEK_END:
> +    {
> +        struct stat st;
> +        int ret;
> +
> +        ret = fstat(fd, &st);
> +        if ( ret )
> +            return -1;
> +        files[fd].offset = st.st_size + offset;
> +        break;
> +    }
> +
> +    default:
> +        errno = EINVAL;
> +        return -1;
> +    }
> +
> +    return files[fd].offset;
> +}
> +
>  off_t lseek(int fd, off_t offset, int whence)
>  {
> +    struct file_ops *ops = get_file_ops(files[fd].type);
> +
> +    if ( ops->lseek )
> +        return ops->lseek(fd, offset, whence);
> +
>      switch(files[fd].type) {
>  #ifdef CONFIG_BLKFRONT
>         case FTYPE_BLK:
> @@ -393,28 +473,7 @@ off_t lseek(int fd, off_t offset, int whence)
>            return (off_t) -1;
>      }
>  
> -    switch (whence) {
> -       case SEEK_SET:
> -          files[fd].offset = offset;
> -          break;
> -       case SEEK_CUR:
> -          files[fd].offset += offset;
> -          break;
> -       case SEEK_END:
> -          {
> -             struct stat st;
> -             int ret;
> -             ret = fstat(fd, &st);
> -             if (ret)
> -                return -1;
> -             files[fd].offset = st.st_size + offset;
> -             break;
> -          }
> -       default:
> -          errno = EINVAL;
> -          return -1;
> -    }
> -    return files[fd].offset;
> +    return lseek_default(fd, offset, whence);
>  }
>  
>  int fsync(int fd) {

Re: [PATCH v2 17/18] mini-os: use function vectors instead of switch for file operations

[Samuel Thibault]

Samuel Thibault, le mar. 11 janv. 2022 21:05:47 +0100, a ecrit:
> Juergen Gross, le mar. 11 janv. 2022 15:58:16 +0100, a ecrit:
> > @@ -370,8 +413,45 @@ int write(int fd, const void *buf, size_t nbytes)
> >      return -1;
> >  }
> >  
> > +off_t lseek_default(int fd, off_t offset, int whence)
> > +{
> > +    switch ( whence )
> > +    {
> 
> Is there a reason for making this one a separate function, unlike others
> for which you kept the code in the main function?

Ah, I guess that it's because in the end, it's the only function that
has a default implementation left?  And thus the rule is that unless the
method is set, the default implementation is ENOSYS?  I agree with that,
then.

Samuel

Re: [PATCH v2 17/18] mini-os: use function vectors instead of switch for file operations

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 527 bytes --]

On 11.01.22 21:05, Samuel Thibault wrote:
> Juergen Gross, le mar. 11 janv. 2022 15:58:16 +0100, a ecrit:
>> @@ -370,8 +413,45 @@ int write(int fd, const void *buf, size_t nbytes)
>>       return -1;
>>   }
>>   
>> +off_t lseek_default(int fd, off_t offset, int whence)
>> +{
>> +    switch ( whence )
>> +    {
> 
> Is there a reason for making this one a separate function, unlike others
> for which you kept the code in the main function?

Yes, it will be used as .lseek callback in some cases.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2 17/18] mini-os: use function vectors instead of switch for file operations

[Samuel Thibault]

On second thought:

Juergen Gross, le mar. 11 janv. 2022 15:58:16 +0100, a ecrit:
> +typedef int file_read_func(int fd, void *buf, size_t nbytes);
> +typedef int file_write_func(int fd, const void *buf, size_t nbytes);
> +typedef off_t file_lseek_func(int fd, off_t offset, int whence);
> +typedef int file_close_func(int fd);
> +typedef int file_fstat_func(int fd, struct stat *buf);
> +typedef int file_fcntl_func(int fd, int cmd, va_list args);
> +typedef bool file_select_func(int fd);

Is this really useful to pass to the functions the int fd, rather than
directly passing the file*?

Samuel

[PATCH v2 18/18] mini-os: remove file type FTYPE_XC

[Juergen Gross]

The only reason for the file type FTYPE_XC has been gone long time
ago: it was needed for xc_map_foreign_bulk(), which has been switched
to use libxenforeignmemory and doesn't need this special file any
more.

So remove everything related to FTYPE_XC.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- new patch
---
 Config.mk                     |  1 -
 arch/x86/testbuild/all-no     |  1 -
 arch/x86/testbuild/all-yes    |  1 -
 arch/x86/testbuild/newxen-yes |  1 -
 include/lib.h                 |  3 +--
 lib/sys.c                     | 13 -------------
 6 files changed, 1 insertion(+), 19 deletions(-)

diff --git a/Config.mk b/Config.mk
index 5e66089..1e08388 100644
--- a/Config.mk
+++ b/Config.mk
@@ -197,7 +197,6 @@ CONFIG-n += CONFIG_PARAVIRT
 endif
 # Support legacy CONFIG_XC value
 CONFIG_XC ?= $(libc)
-CONFIG-$(CONFIG_XC) += CONFIG_LIBXENCTRL
 CONFIG-$(CONFIG_XC) += CONFIG_LIBXENEVTCHN
 CONFIG-$(CONFIG_XC) += CONFIG_LIBXENGNTTAB
 
diff --git a/arch/x86/testbuild/all-no b/arch/x86/testbuild/all-no
index 7972ecd..d6fc260 100644
--- a/arch/x86/testbuild/all-no
+++ b/arch/x86/testbuild/all-no
@@ -13,7 +13,6 @@ CONFIG_FBFRONT = n
 CONFIG_KBDFRONT = n
 CONFIG_CONSFRONT = n
 CONFIG_XENBUS = n
-CONFIG_LIBXENCTRL = n
 CONFIG_LIBXENEVTCHN = n
 CONFIG_LIBXENGNTTAB = n
 CONFIG_LWIP = n
diff --git a/arch/x86/testbuild/all-yes b/arch/x86/testbuild/all-yes
index bc8eea5..98bbfeb 100644
--- a/arch/x86/testbuild/all-yes
+++ b/arch/x86/testbuild/all-yes
@@ -16,7 +16,6 @@ CONFIG_XENBUS = y
 CONFIG_BALLOON = y
 CONFIG_USE_XEN_CONSOLE = y
 # The following are special: they need support from outside
-CONFIG_LIBXENCTRL = n
 CONFIG_LIBXENEVTCHN = n
 CONFIG_LIBXENGNTTAB = n
 CONFIG_LWIP = n
diff --git a/arch/x86/testbuild/newxen-yes b/arch/x86/testbuild/newxen-yes
index f72123b..0603200 100644
--- a/arch/x86/testbuild/newxen-yes
+++ b/arch/x86/testbuild/newxen-yes
@@ -17,7 +17,6 @@ CONFIG_BALLOON = y
 CONFIG_USE_XEN_CONSOLE = y
 XEN_INTERFACE_VERSION=__XEN_LATEST_INTERFACE_VERSION__
 # The following are special: they need support from outside
-CONFIG_LIBXENCTRL = n
 CONFIG_LIBXENEVTCHN = n
 CONFIG_LIBXENGNTTAB = n
 CONFIG_LWIP = n
diff --git a/include/lib.h b/include/lib.h
index b40e213..4892320 100644
--- a/include/lib.h
+++ b/include/lib.h
@@ -170,8 +170,7 @@ void sanity_check(void);
 #define FTYPE_XENBUS    12
 #define FTYPE_GNTMAP    13
 #define FTYPE_EVTCHN    14
-#define FTYPE_XC        15
-#define FTYPE_N         16
+#define FTYPE_N         15
 #define FTYPE_SPARE     16
 
 typedef int file_read_func(int fd, void *buf, size_t nbytes);
diff --git a/lib/sys.c b/lib/sys.c
index c327247..52876e0 100644
--- a/lib/sys.c
+++ b/lib/sys.c
@@ -87,7 +87,6 @@
     }
 
 #define NOFILE 32
-extern void minios_interface_close_fd(int fd);
 extern void minios_evtchn_close_fd(int fd);
 extern void minios_gnttab_close_fd(int fd);
 
@@ -506,11 +505,6 @@ int close(int fd)
 	    res = lwip_close(files[fd].fd);
             break;
 #endif
-#ifdef CONFIG_LIBXENCTRL
-	case FTYPE_XC:
-	    minios_interface_close_fd(fd);
-            break;
-#endif
 #ifdef CONFIG_LIBXENEVTCHN
 	case FTYPE_EVTCHN:
 	    minios_evtchn_close_fd(fd);
@@ -729,7 +723,6 @@ static const char *file_types[] = {
     [FTYPE_NONE]    = "none",
     [FTYPE_CONSOLE] = "console",
     [FTYPE_XENBUS]  = "xenbus",
-    [FTYPE_XC]      = "ctrl",
     [FTYPE_EVTCHN]  = "evtchn",
     [FTYPE_SOCKET]  = "socket",
     [FTYPE_TAP]     = "net",
@@ -1510,12 +1503,6 @@ void *mmap(void *start, size_t length, int prot, int flags, int fd, off_t offset
 
     if (fd == -1)
         return map_zero(n, 1);
-#ifdef CONFIG_LIBXENCTRL
-    else if (files[fd].type == FTYPE_XC) {
-        unsigned long zero = 0;
-        return map_frames_ex(&zero, n, 0, 0, 1, DOMID_SELF, NULL, 0);
-    }
-#endif
     else if (files[fd].type == FTYPE_MEM) {
         unsigned long first_mfn = offset >> PAGE_SHIFT;
         return map_frames_ex(&first_mfn, n, 0, 1, 1, DOMID_IO, NULL, _PAGE_PRESENT|_PAGE_RW);
-- 
2.26.2

Re: [PATCH v2 18/18] mini-os: remove file type FTYPE_XC

[Andrew Cooper]

On 11/01/2022 14:58, Juergen Gross wrote:
> The only reason for the file type FTYPE_XC has been gone long time
> ago: it was needed for xc_map_foreign_bulk(), which has been switched
> to use libxenforeignmemory and doesn't need this special file any
> more.
>
> So remove everything related to FTYPE_XC.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>

You'd have less churn in the series if this came ahead of patch 17. 
There are at least 3 hunks below that you've already converted once, and
one of those hunks already needs a hard tabs fixup.

~Andrew

Re: [PATCH v2 18/18] mini-os: remove file type FTYPE_XC

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 644 bytes --]

On 11.01.22 19:19, Andrew Cooper wrote:
> On 11/01/2022 14:58, Juergen Gross wrote:
>> The only reason for the file type FTYPE_XC has been gone long time
>> ago: it was needed for xc_map_foreign_bulk(), which has been switched
>> to use libxenforeignmemory and doesn't need this special file any
>> more.
>>
>> So remove everything related to FTYPE_XC.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> You'd have less churn in the series if this came ahead of patch 17.
> There are at least 3 hunks below that you've already converted once, and
> one of those hunks already needs a hard tabs fixup.

Okay.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2 00/18] mini-os: remove struct file dependency on config

[Andrew Cooper]

On 11/01/2022 14:57, Juergen Gross wrote:
> Today the layout of struct file is depending on the Mini-OS
> configuration. This is especially bad as the layout is exported to
> external users like the Xen libraries built for Mini-OS, and those
> are being built only once for multiple stubdom configurations.
>
> Today there is no direct problem resulting from this, as the main
> difference between struct file layouts is a large union containing all
> the device specific data for the different file types. The largest
> union member is not configuration dependant, so the build is currently
> not broken.
>
> In order to avoid any future problems this patch series is eliminating
> the configuration dependency by replacing most of the device specific
> union members by a single pointer.
>
> The two union members used by Xen libraries can't be replaced yet, as
> those need to be switched to use the generic pointer first.
>
> In order to hide the Mini-OS internal implementation of the files
> array, patches 15-17 are introducing a common framework to access a
> struct file via its file descriptor, and to allocate new file types
> dynamically instead of having them all pre-defined. The file type
> specific operations are supplied via a function vector in order to
> remove the dependency of lib/sys.c on all the various file types.

Patches 1 thru 15 seem fine and ready to go.  Would it help to get those
committed now?

~Andrew

Re: [PATCH v2 00/18] mini-os: remove struct file dependency on config

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1628 bytes --]

On 11.01.22 20:42, Andrew Cooper wrote:
> On 11/01/2022 14:57, Juergen Gross wrote:
>> Today the layout of struct file is depending on the Mini-OS
>> configuration. This is especially bad as the layout is exported to
>> external users like the Xen libraries built for Mini-OS, and those
>> are being built only once for multiple stubdom configurations.
>>
>> Today there is no direct problem resulting from this, as the main
>> difference between struct file layouts is a large union containing all
>> the device specific data for the different file types. The largest
>> union member is not configuration dependant, so the build is currently
>> not broken.
>>
>> In order to avoid any future problems this patch series is eliminating
>> the configuration dependency by replacing most of the device specific
>> union members by a single pointer.
>>
>> The two union members used by Xen libraries can't be replaced yet, as
>> those need to be switched to use the generic pointer first.
>>
>> In order to hide the Mini-OS internal implementation of the files
>> array, patches 15-17 are introducing a common framework to access a
>> struct file via its file descriptor, and to allocate new file types
>> dynamically instead of having them all pre-defined. The file type
>> specific operations are supplied via a function vector in order to
>> remove the dependency of lib/sys.c on all the various file types.
> 
> Patches 1 thru 15 seem fine and ready to go.  Would it help to get those
> committed now?

With the one remark you had for patch 15 I think patches 1-14 could go
in now.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]