[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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@xxxxxxxx>

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: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.