[PATCH v2 00/12] mini-os: remove device specific struct file members

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH v2 00/12] mini-os: remove device specific struct file members
@ 2022-01-11 15:12 Juergen Gross
  2022-01-11 15:12 ` [PATCH v2 01/12] mini-os: remove event channel specific struct file definitions Juergen Gross
                   ` (11 more replies)
  0 siblings, 12 replies; 48+ messages in thread
From: Juergen Gross @ 2022-01-11 15:12 UTC (permalink / raw)
  To: minios-devel, xen-devel; +Cc: samuel.thibault, wl, Juergen Gross

This small series is a followup to the series sent recently:

https://lists.xen.org/archives/html/xen-devel/2022-01/threads.html#00347

It contains the last cleanups related to struct file and can only be
applied after the Xen libraries have stopped using the related union
members:

https://lists.xen.org/archives/html/xen-devel/2022-01/threads.html#00355

The three series applied have been tested to work with:

- xenstorepvh-stubdom
- pv-grub
- ioemu-stubdom

Changes in V2:
- add many patches for using alloc_file_type() and struct file_ops

Juergen Gross (12):
  mini-os: remove event channel specific struct file definitions
  mini-os: remove gnttab specific member from struct file
  mini-os: use alloc_file_type() and get_file_from_fd() in xs
  mini-os: use alloc_file_type() and get_file_from_fd() in tpm_tis
  mini-os: use alloc_file_type() and get_file_from_fd() in tpmfront
  mini-os: use alloc_file_type() and get_file_from_fd() in blkfront
  mini-os: use get_file_from_fd() in netfront
  mini-os: use alloc_file_type() and get_file_from_fd() in fbfront
  mini-os: use file_ops and get_file_from_fd() for console
  mini-os: add struct file_ops for file type socket
  mini-os: add struct file_ops for FTYPE_FILE
  mini-os: make files array private to sys.c

 Config.mk                     |   2 -
 arch/x86/testbuild/all-no     |   2 -
 arch/x86/testbuild/all-yes    |   2 -
 arch/x86/testbuild/newxen-yes |   2 -
 blkfront.c                    |  92 ++++++--
 console/xenbus.c              | 125 ++++++++++
 console/xencons_ring.c        |   6 +-
 fbfront.c                     | 125 ++++++++--
 gntmap.c                      |   2 +-
 include/blkfront.h            |   5 -
 include/console.h             |   5 +
 include/lib.h                 |  27 +--
 include/tpm_tis.h             |   6 -
 include/tpmfront.h            |   5 -
 lib/sys.c                     | 429 ++++++++--------------------------
 lib/xs.c                      |  64 +++--
 netfront.c                    |  64 ++++-
 tpm_tis.c                     | 120 ++++++----
 tpmfront.c                    | 100 +++++---
 xenbus/xenbus.c               |   1 +
 20 files changed, 664 insertions(+), 520 deletions(-)

-- 
2.26.2



^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH v2 01/12] mini-os: remove event channel specific struct file definitions
  2022-01-11 15:12 [PATCH v2 00/12] mini-os: remove device specific struct file members Juergen Gross
@ 2022-01-11 15:12 ` Juergen Gross
  2022-01-11 19:50   ` Samuel Thibault
  2022-01-11 15:12 ` [PATCH v2 02/12] mini-os: remove gnttab specific member from struct file Juergen Gross
                   ` (10 subsequent siblings)
  11 siblings, 1 reply; 48+ messages in thread
From: Juergen Gross @ 2022-01-11 15:12 UTC (permalink / raw)
  To: minios-devel, xen-devel; +Cc: samuel.thibault, wl, Juergen Gross

The event channel specific union member in struct file is no longer
needed, so remove it together with the associated structure
definitions.

The event channel file type and its associated handling can be removed,
too, as libxenevtchn is now supplying a struct file_ops via a call of
alloc_file_type().

This removes all contents of CONFIG_LIBXENEVTCHN guarded sections, so
this config option can be removed.

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

diff --git a/Config.mk b/Config.mk
index 1e08388..c244adc 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_LIBXENEVTCHN
 CONFIG-$(CONFIG_XC) += CONFIG_LIBXENGNTTAB
 
 CONFIG-$(lwip) += CONFIG_LWIP
diff --git a/arch/x86/testbuild/all-no b/arch/x86/testbuild/all-no
index d6fc260..202c317 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_LIBXENEVTCHN = n
 CONFIG_LIBXENGNTTAB = n
 CONFIG_LWIP = n
 CONFIG_BALLOON = n
diff --git a/arch/x86/testbuild/all-yes b/arch/x86/testbuild/all-yes
index 98bbfeb..eb495a4 100644
--- a/arch/x86/testbuild/all-yes
+++ b/arch/x86/testbuild/all-yes
@@ -16,6 +16,5 @@ CONFIG_XENBUS = y
 CONFIG_BALLOON = y
 CONFIG_USE_XEN_CONSOLE = y
 # The following are special: they need support from outside
-CONFIG_LIBXENEVTCHN = n
 CONFIG_LIBXENGNTTAB = n
 CONFIG_LWIP = n
diff --git a/arch/x86/testbuild/newxen-yes b/arch/x86/testbuild/newxen-yes
index 0603200..bf25ace 100644
--- a/arch/x86/testbuild/newxen-yes
+++ b/arch/x86/testbuild/newxen-yes
@@ -17,6 +17,5 @@ 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_LIBXENEVTCHN = n
 CONFIG_LIBXENGNTTAB = n
 CONFIG_LWIP = n
diff --git a/include/lib.h b/include/lib.h
index 4892320..df972ef 100644
--- a/include/lib.h
+++ b/include/lib.h
@@ -169,8 +169,7 @@ void sanity_check(void);
 #define FTYPE_TPM_TIS   11
 #define FTYPE_XENBUS    12
 #define FTYPE_GNTMAP    13
-#define FTYPE_EVTCHN    14
-#define FTYPE_N         15
+#define FTYPE_N         14
 #define FTYPE_SPARE     16
 
 typedef int file_read_func(int fd, void *buf, size_t nbytes);
@@ -199,15 +198,6 @@ 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 {
-        LIST_ENTRY(evtchn_port_info) list;
-        evtchn_port_t port;
-        unsigned long pending;
-        int bound;
-};
-
 struct file {
     unsigned int type;
     bool read;	/* maybe available for read */
@@ -215,9 +205,6 @@ struct file {
     union {
         int fd; /* Any fd from an upper layer. */
         void *dev;
-	struct {
-	    struct evtchn_port_list ports;
-	} evtchn;
 	struct gntmap gntmap;
     };
 };
diff --git a/lib/sys.c b/lib/sys.c
index 52876e0..8fa1fee 100644
--- a/lib/sys.c
+++ b/lib/sys.c
@@ -505,11 +505,6 @@ int close(int fd)
 	    res = lwip_close(files[fd].fd);
             break;
 #endif
-#ifdef CONFIG_LIBXENEVTCHN
-	case FTYPE_EVTCHN:
-	    minios_evtchn_close_fd(fd);
-            break;
-#endif
 #ifdef CONFIG_LIBXENGNTTAB
 	case FTYPE_GNTMAP:
 	    minios_gnttab_close_fd(fd);
@@ -723,7 +718,6 @@ static const char *file_types[] = {
     [FTYPE_NONE]    = "none",
     [FTYPE_CONSOLE] = "console",
     [FTYPE_XENBUS]  = "xenbus",
-    [FTYPE_EVTCHN]  = "evtchn",
     [FTYPE_SOCKET]  = "socket",
     [FTYPE_TAP]     = "net",
     [FTYPE_BLK]     = "blk",
@@ -915,7 +909,6 @@ static int select_poll(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exce
 	    FD_CLR(i, exceptfds);
 	    break;
 #endif
-	case FTYPE_EVTCHN:
 	case FTYPE_TAP:
 	case FTYPE_BLK:
 	case FTYPE_KBD:
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH v2 02/12] mini-os: remove gnttab specific member from struct file
  2022-01-11 15:12 [PATCH v2 00/12] mini-os: remove device specific struct file members Juergen Gross
  2022-01-11 15:12 ` [PATCH v2 01/12] mini-os: remove event channel specific struct file definitions Juergen Gross
@ 2022-01-11 15:12 ` Juergen Gross
  2022-01-11 19:55   ` Samuel Thibault
  2022-01-11 20:12   ` Andrew Cooper
  2022-01-11 15:12 ` [PATCH v2 03/12] mini-os: use alloc_file_type() and get_file_from_fd() in xs Juergen Gross
                   ` (9 subsequent siblings)
  11 siblings, 2 replies; 48+ messages in thread
From: Juergen Gross @ 2022-01-11 15:12 UTC (permalink / raw)
  To: minios-devel, xen-devel; +Cc: samuel.thibault, wl, Juergen Gross

The event channel specific union member in struct file is no longer
needed, so remove it.

The gnttab file type and its associated handling can be removed, too,
as libxengnttab is now supplying a struct file_ops via a call of
alloc_file_type().

This removes all contents of CONFIG_LIBXENGNTTAB guarded sections, so
this config option can be removed.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 Config.mk                     | 1 -
 arch/x86/testbuild/all-no     | 1 -
 arch/x86/testbuild/all-yes    | 1 -
 arch/x86/testbuild/newxen-yes | 1 -
 gntmap.c                      | 2 +-
 include/lib.h                 | 4 +---
 lib/sys.c                     | 5 -----
 7 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/Config.mk b/Config.mk
index c244adc..eb84515 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_LIBXENGNTTAB
 
 CONFIG-$(lwip) += CONFIG_LWIP
 
diff --git a/arch/x86/testbuild/all-no b/arch/x86/testbuild/all-no
index 202c317..c429354 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_LIBXENGNTTAB = n
 CONFIG_LWIP = n
 CONFIG_BALLOON = n
 CONFIG_USE_XEN_CONSOLE = n
diff --git a/arch/x86/testbuild/all-yes b/arch/x86/testbuild/all-yes
index eb495a4..6c6096b 100644
--- a/arch/x86/testbuild/all-yes
+++ b/arch/x86/testbuild/all-yes
@@ -16,5 +16,4 @@ CONFIG_XENBUS = y
 CONFIG_BALLOON = y
 CONFIG_USE_XEN_CONSOLE = y
 # The following are special: they need support from outside
-CONFIG_LIBXENGNTTAB = n
 CONFIG_LWIP = n
diff --git a/arch/x86/testbuild/newxen-yes b/arch/x86/testbuild/newxen-yes
index bf25ace..88094fc 100644
--- a/arch/x86/testbuild/newxen-yes
+++ b/arch/x86/testbuild/newxen-yes
@@ -17,5 +17,4 @@ 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_LIBXENGNTTAB = n
 CONFIG_LWIP = n
diff --git a/gntmap.c b/gntmap.c
index 6fa1dac..3422ab2 100644
--- a/gntmap.c
+++ b/gntmap.c
@@ -3,7 +3,7 @@
  *
  * Diego Ongaro <diego.ongaro@citrix.com>, July 2008
  *
- * Files of type FTYPE_GNTMAP contain a gntmap, which is an array of
+ * Files of libxengnttab contain a gntmap, which is an array of
  * (host address, grant handle) pairs. Grant handles come from a hypervisor map
  * operation and are needed for the corresponding unmap.
  *
diff --git a/include/lib.h b/include/lib.h
index df972ef..283abb8 100644
--- a/include/lib.h
+++ b/include/lib.h
@@ -168,8 +168,7 @@ void sanity_check(void);
 #define FTYPE_TPMFRONT  10
 #define FTYPE_TPM_TIS   11
 #define FTYPE_XENBUS    12
-#define FTYPE_GNTMAP    13
-#define FTYPE_N         14
+#define FTYPE_N         13
 #define FTYPE_SPARE     16
 
 typedef int file_read_func(int fd, void *buf, size_t nbytes);
@@ -205,7 +204,6 @@ struct file {
     union {
         int fd; /* Any fd from an upper layer. */
         void *dev;
-	struct gntmap gntmap;
     };
 };
 
diff --git a/lib/sys.c b/lib/sys.c
index 8fa1fee..9540410 100644
--- a/lib/sys.c
+++ b/lib/sys.c
@@ -505,11 +505,6 @@ int close(int fd)
 	    res = lwip_close(files[fd].fd);
             break;
 #endif
-#ifdef CONFIG_LIBXENGNTTAB
-	case FTYPE_GNTMAP:
-	    minios_gnttab_close_fd(fd);
-            break;
-#endif
 #ifdef CONFIG_NETFRONT
 	case FTYPE_TAP:
 	    shutdown_netfront(files[fd].dev);
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH v2 03/12] mini-os: use alloc_file_type() and get_file_from_fd() in xs
  2022-01-11 15:12 [PATCH v2 00/12] mini-os: remove device specific struct file members Juergen Gross
  2022-01-11 15:12 ` [PATCH v2 01/12] mini-os: remove event channel specific struct file definitions Juergen Gross
  2022-01-11 15:12 ` [PATCH v2 02/12] mini-os: remove gnttab specific member from struct file Juergen Gross
@ 2022-01-11 15:12 ` Juergen Gross
  2022-01-11 20:06   ` Samuel Thibault
  2022-01-11 20:21   ` Andrew Cooper
  2022-01-11 15:12 ` [PATCH v2 04/12] mini-os: use alloc_file_type() and get_file_from_fd() in tpm_tis Juergen Gross
                   ` (8 subsequent siblings)
  11 siblings, 2 replies; 48+ messages in thread
From: Juergen Gross @ 2022-01-11 15:12 UTC (permalink / raw)
  To: minios-devel, xen-devel; +Cc: samuel.thibault, wl, Juergen Gross

Allocate the needed file type via alloc_file_type().

Instead of directly accessing the files[] array use get_file_from_fd().

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 include/lib.h   |  3 +--
 lib/sys.c       | 18 --------------
 lib/xs.c        | 64 +++++++++++++++++++++++++++++++++++++------------
 xenbus/xenbus.c |  1 +
 4 files changed, 51 insertions(+), 35 deletions(-)

diff --git a/include/lib.h b/include/lib.h
index 283abb8..05c7de5 100644
--- a/include/lib.h
+++ b/include/lib.h
@@ -167,8 +167,7 @@ void sanity_check(void);
 #define FTYPE_BLK        9
 #define FTYPE_TPMFRONT  10
 #define FTYPE_TPM_TIS   11
-#define FTYPE_XENBUS    12
-#define FTYPE_N         13
+#define FTYPE_N         12
 #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 9540410..d213ae5 100644
--- a/lib/sys.c
+++ b/lib/sys.c
@@ -495,11 +495,6 @@ int close(int fd)
     switch (files[fd].type) {
         default:
             break;
-#ifdef CONFIG_XENBUS
-	case FTYPE_XENBUS:
-            xs_daemon_close((void*)(intptr_t) fd);
-            break;
-#endif
 #ifdef HAVE_LWIP
 	case FTYPE_SOCKET:
 	    res = lwip_close(files[fd].fd);
@@ -712,7 +707,6 @@ int closedir(DIR *dir)
 static const char *file_types[] = {
     [FTYPE_NONE]    = "none",
     [FTYPE_CONSOLE] = "console",
-    [FTYPE_XENBUS]  = "xenbus",
     [FTYPE_SOCKET]  = "socket",
     [FTYPE_TAP]     = "net",
     [FTYPE_BLK]     = "blk",
@@ -892,18 +886,6 @@ static int select_poll(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exce
                 n++;
 	    FD_CLR(i, exceptfds);
 	    break;
-#ifdef CONFIG_XENBUS
-	case FTYPE_XENBUS:
-	    if (FD_ISSET(i, readfds)) {
-                if (files[i].dev)
-		    n++;
-		else
-		    FD_CLR(i, readfds);
-	    }
-	    FD_CLR(i, writefds);
-	    FD_CLR(i, exceptfds);
-	    break;
-#endif
 	case FTYPE_TAP:
 	case FTYPE_BLK:
 	case FTYPE_KBD:
diff --git a/lib/xs.c b/lib/xs.c
index 4af0f96..ac830d2 100644
--- a/lib/xs.c
+++ b/lib/xs.c
@@ -18,23 +18,55 @@ static inline int _xs_fileno(struct xs_handle *h) {
     return (intptr_t) h;
 }
 
+static int xs_close_fd(int fd)
+{
+    struct xenbus_event *event, *next;
+    struct file *file = get_file_from_fd(fd);
+
+    for (event = file->dev; event; event = next)
+    {
+        next = event->next;
+        free(event);
+    }
+
+    return 0;
+}
+
+static bool xs_can_read(int fd)
+{
+    struct file *file = get_file_from_fd(fd);
+
+    return file && file->dev;
+}
+
+static struct file_ops xenbus_ops = {
+    .name = "xenbus",
+    .close = xs_close_fd,
+    .select_rd = xs_can_read,
+};
+
 struct xs_handle *xs_daemon_open()
 {
-    int fd = alloc_fd(FTYPE_XENBUS);
-    files[fd].dev = NULL;
-    printk("xs_daemon_open -> %d, %p\n", fd, &files[fd].dev);
+    int fd;
+    struct file *file;
+    static unsigned int ftype_xenbus;
+
+    if ( !ftype_xenbus )
+        ftype_xenbus = alloc_file_type(&xenbus_ops);
+
+    fd = alloc_fd(ftype_xenbus);
+    file = get_file_from_fd(fd);
+    if ( !file )
+        return NULL;
+
+    file->dev = NULL;
+    printk("xs_daemon_open -> %d, %p\n", fd, &file->dev);
     return (void*)(intptr_t) fd;
 }
 
 void xs_daemon_close(struct xs_handle *h)
 {
-    int fd = _xs_fileno(h);
-    struct xenbus_event *event, *next;
-    for (event = files[fd].dev; event; event = next)
-    {
-        next = event->next;
-        free(event);
-    }
+    close(_xs_fileno(h));
 }
 
 int xs_fileno(struct xs_handle *h)
@@ -169,18 +201,20 @@ char **xs_directory(struct xs_handle *h, xs_transaction_t t,
 
 bool xs_watch(struct xs_handle *h, const char *path, const char *token)
 {
-    int fd = _xs_fileno(h);
+    struct file *file = get_file_from_fd(_xs_fileno(h));
+
     printk("xs_watch(%s, %s)\n", path, token);
     return xs_bool(xenbus_watch_path_token(XBT_NULL, path, token,
-                   (xenbus_event_queue *)&files[fd].dev));
+                   (xenbus_event_queue *)&file->dev));
 }
 
 char **xs_read_watch(struct xs_handle *h, unsigned int *num)
 {
-    int fd = _xs_fileno(h);
     struct xenbus_event *event;
-    event = files[fd].dev;
-    files[fd].dev = event->next;
+    struct file *file = get_file_from_fd(_xs_fileno(h));
+
+    event = file->dev;
+    file->dev = event->next;
     printk("xs_read_watch() -> %s %s\n", event->path, event->token);
     *num = 2;
     return (char **) &event->path;
diff --git a/xenbus/xenbus.c b/xenbus/xenbus.c
index b687678..785389f 100644
--- a/xenbus/xenbus.c
+++ b/xenbus/xenbus.c
@@ -393,6 +393,7 @@ static int allocate_xenbus_id(void)
 void init_xenbus(void)
 {
     int err;
+
     DEBUG("init_xenbus called.\n");
     create_thread("xenstore", xenbus_thread_func, NULL);
     DEBUG("buf at %p.\n", xenstore_buf);
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH v2 04/12] mini-os: use alloc_file_type() and get_file_from_fd() in tpm_tis
  2022-01-11 15:12 [PATCH v2 00/12] mini-os: remove device specific struct file members Juergen Gross
                   ` (2 preceding siblings ...)
  2022-01-11 15:12 ` [PATCH v2 03/12] mini-os: use alloc_file_type() and get_file_from_fd() in xs Juergen Gross
@ 2022-01-11 15:12 ` Juergen Gross
  2022-01-11 20:13   ` Samuel Thibault
  2022-01-11 20:29   ` Andrew Cooper
  2022-01-11 15:12 ` [PATCH v2 05/12] mini-os: use alloc_file_type() and get_file_from_fd() in tpmfront Juergen Gross
                   ` (7 subsequent siblings)
  11 siblings, 2 replies; 48+ messages in thread
From: Juergen Gross @ 2022-01-11 15:12 UTC (permalink / raw)
  To: minios-devel, xen-devel; +Cc: samuel.thibault, wl, Juergen Gross

Allocate a file type dynamically via alloc_file_type().

Instead of directly accessing the files[] array use get_file_from_fd().

Make some now local functions static and modify their prototypes to
match the file_ops requirements.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 include/lib.h     |   3 +-
 include/tpm_tis.h |   6 ---
 lib/sys.c         |  23 ---------
 tpm_tis.c         | 120 ++++++++++++++++++++++++++++++----------------
 4 files changed, 80 insertions(+), 72 deletions(-)

diff --git a/include/lib.h b/include/lib.h
index 05c7de5..d94d142 100644
--- a/include/lib.h
+++ b/include/lib.h
@@ -166,8 +166,7 @@ void sanity_check(void);
 #define FTYPE_TAP        8
 #define FTYPE_BLK        9
 #define FTYPE_TPMFRONT  10
-#define FTYPE_TPM_TIS   11
-#define FTYPE_N         12
+#define FTYPE_N         11
 #define FTYPE_SPARE     16
 
 typedef int file_read_func(int fd, void *buf, size_t nbytes);
diff --git a/include/tpm_tis.h b/include/tpm_tis.h
index 86e83f1..2af974d 100644
--- a/include/tpm_tis.h
+++ b/include/tpm_tis.h
@@ -37,14 +37,11 @@ struct tpm_chip;
 
 struct tpm_chip* init_tpm_tis(unsigned long baseaddr, int localities, unsigned int irq);
 struct tpm_chip* init_tpm2_tis(unsigned long baseaddr, int localities, unsigned int irq);
-void shutdown_tpm_tis(struct tpm_chip* tpm);
 
 int tpm_tis_request_locality(struct tpm_chip* tpm, int locality);
 int tpm_tis_cmd(struct tpm_chip* tpm, uint8_t* req, size_t reqlen, uint8_t** resp, size_t* resplen);
 
 #ifdef HAVE_LIBC
-#include <sys/stat.h>
-#include <fcntl.h>
 /* POSIX IO functions:
  * use tpm_tis_open() to get a file descriptor to the tpm device
  * use write() on the fd to send a command to the backend. You must
@@ -53,9 +50,6 @@ int tpm_tis_cmd(struct tpm_chip* tpm, uint8_t* req, size_t reqlen, uint8_t** res
  * fstat() to get the size of the response and lseek() to seek on it.
  */
 int tpm_tis_open(struct tpm_chip* tpm);
-int tpm_tis_posix_read(int fd, uint8_t* buf, size_t count);
-int tpm_tis_posix_write(int fd, const uint8_t* buf, size_t count);
-int tpm_tis_posix_fstat(int fd, struct stat* buf);
 #endif
 
 #endif
diff --git a/lib/sys.c b/lib/sys.c
index d213ae5..4060b42 100644
--- a/lib/sys.c
+++ b/lib/sys.c
@@ -29,7 +29,6 @@
 #include <blkfront.h>
 #include <fbfront.h>
 #include <tpmfront.h>
-#include <tpm_tis.h>
 #include <xenbus.h>
 #include <xenstore.h>
 #include <poll.h>
@@ -349,11 +348,6 @@ int read(int fd, void *buf, size_t nbytes)
         case FTYPE_TPMFRONT: {
 	    return tpmfront_posix_read(fd, buf, nbytes);
         }
-#endif
-#ifdef CONFIG_TPM_TIS
-        case FTYPE_TPM_TIS: {
-	    return tpm_tis_posix_read(fd, buf, nbytes);
-        }
 #endif
 	default:
 	    break;
@@ -399,10 +393,6 @@ int write(int fd, const void *buf, size_t nbytes)
 #ifdef CONFIG_TPMFRONT
 	case FTYPE_TPMFRONT:
 	    return tpmfront_posix_write(fd, buf, nbytes);
-#endif
-#ifdef CONFIG_TPM_TIS
-	case FTYPE_TPM_TIS:
-	    return tpm_tis_posix_write(fd, buf, nbytes);
 #endif
 	default:
 	    break;
@@ -459,10 +449,6 @@ off_t lseek(int fd, off_t offset, int whence)
 #ifdef CONFIG_TPMFRONT
        case FTYPE_TPMFRONT:
           break;
-#endif
-#ifdef CONFIG_TPM_TIS
-       case FTYPE_TPM_TIS:
-          break;
 #endif
        case FTYPE_FILE:
           break;
@@ -515,11 +501,6 @@ int close(int fd)
             shutdown_tpmfront(files[fd].dev);
             break;
 #endif
-#ifdef CONFIG_TPM_TIS
-	case FTYPE_TPM_TIS:
-            shutdown_tpm_tis(files[fd].dev);
-            break;
-#endif
 #ifdef CONFIG_KBDFRONT
 	case FTYPE_KBD:
             shutdown_kbdfront(files[fd].dev);
@@ -599,10 +580,6 @@ int fstat(int fd, struct stat *buf)
 #ifdef CONFIG_TPMFRONT
 	case FTYPE_TPMFRONT:
 	   return tpmfront_posix_fstat(fd, buf);
-#endif
-#ifdef CONFIG_TPM_TIS
-	case FTYPE_TPM_TIS:
-	   return tpm_tis_posix_fstat(fd, buf);
 #endif
 	default:
 	    break;
diff --git a/tpm_tis.c b/tpm_tis.c
index 477f555..abea7a1 100644
--- a/tpm_tis.c
+++ b/tpm_tis.c
@@ -792,6 +792,9 @@ int tpm_tis_send(struct tpm_chip* tpm, uint8_t* buf, size_t len) {
    int status, burstcnt = 0;
    int count = 0;
    uint32_t ordinal;
+#ifdef HAVE_LIBC
+   struct file *file = get_file_from_fd(tpm->fd);
+#endif
 
    if(tpm_tis_request_locality(tpm, tpm->locality) < 0) {
       return -EBUSY;
@@ -844,9 +847,10 @@ 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].offset = 0;
+   if ( file )
+   {
+      file->read = false;
+      file->offset = 0;
    }
 #endif
    return len;
@@ -1093,6 +1097,23 @@ ssize_t tpm_getcap(struct tpm_chip *chip, uint32_t subcap_id, cap_t *cap,
         return rc;
 }
 
+static void shutdown_tpm_tis(struct tpm_chip* tpm){
+   int i;
+
+   printk("Shutting down tpm_tis device\n");
+
+   iowrite32(TPM_INT_ENABLE(tpm, tpm->locality), ~TPM_GLOBAL_INT_ENABLE);
+
+   /*Unmap all of the mmio pages */
+   for(i = 0; i < 5; ++i) {
+      if(tpm->pages[i] != NULL) {
+	 iounmap(tpm->pages[i], PAGE_SIZE);
+	 tpm->pages[i] = NULL;
+      }
+   }
+   free(tpm);
+   return;
+}
 
 struct tpm_chip* init_tpm_tis(unsigned long baseaddr, int localities, unsigned int irq)
 {
@@ -1242,25 +1263,6 @@ abort_egress:
    return NULL;
 }
 
-void shutdown_tpm_tis(struct tpm_chip* tpm){
-   int i;
-
-   printk("Shutting down tpm_tis device\n");
-
-   iowrite32(TPM_INT_ENABLE(tpm, tpm->locality), ~TPM_GLOBAL_INT_ENABLE);
-
-   /*Unmap all of the mmio pages */
-   for(i = 0; i < 5; ++i) {
-      if(tpm->pages[i] != NULL) {
-	 iounmap(tpm->pages[i], PAGE_SIZE);
-	 tpm->pages[i] = NULL;
-      }
-   }
-   free(tpm);
-   return;
-}
-
-
 int tpm_tis_cmd(struct tpm_chip* tpm, uint8_t* req, size_t reqlen, uint8_t** resp, size_t* resplen)
 {
    if(tpm->locality < 0) {
@@ -1279,23 +1281,24 @@ int tpm_tis_cmd(struct tpm_chip* tpm, uint8_t* req, size_t reqlen, uint8_t** res
 }
 
 #ifdef HAVE_LIBC
-int tpm_tis_open(struct tpm_chip* tpm)
+#include <sys/stat.h>
+#include <fcntl.h>
+
+static int tpm_tis_close(int fd)
 {
-   /* Silently prevent multiple opens */
-   if(tpm->fd != -1) {
-      return tpm->fd;
-   }
+    struct file *file = get_file_from_fd(fd);
 
-   tpm->fd = alloc_fd(FTYPE_TPM_TIS);
-   printk("tpm_tis_open() -> %d\n", tpm->fd);
-   files[tpm->fd].dev = tpm;
-   return tpm->fd;
+    shutdown_tpm_tis(file->dev);
+
+    return 0;
 }
 
-int tpm_tis_posix_write(int fd, const uint8_t* buf, size_t count)
+static int tpm_tis_posix_write(int fd, const void *buf, size_t count)
 {
    struct tpm_chip* tpm;
-   tpm = files[fd].dev;
+   struct file *file = get_file_from_fd(fd);
+
+   tpm = file->dev;
 
    if(tpm->locality < 0) {
       printk("tpm_tis_posix_write() failed! locality not set!\n");
@@ -1319,11 +1322,13 @@ int tpm_tis_posix_write(int fd, const uint8_t* buf, size_t count)
    return count;
 }
 
-int tpm_tis_posix_read(int fd, uint8_t* buf, size_t count)
+static int tpm_tis_posix_read(int fd, void *buf, size_t count)
 {
    int rc;
    struct tpm_chip* tpm;
-   tpm = files[fd].dev;
+   struct file *file = get_file_from_fd(fd);
+
+   tpm = file->dev;
 
    if(count == 0) {
       return 0;
@@ -1337,20 +1342,24 @@ int tpm_tis_posix_read(int fd, uint8_t* buf, size_t count)
 
 
    /* Handle EOF case */
-   if(files[fd].offset >= tpm->data_len) {
+   if ( file->offset >= tpm->data_len )
+   {
       rc = 0;
    } else {
-      rc = min(tpm->data_len - files[fd].offset, count);
-      memcpy(buf, tpm->data_buffer + files[fd].offset, rc);
+      rc = min(tpm->data_len - file->offset, count);
+      memcpy(buf, tpm->data_buffer + file->offset, rc);
    }
-   files[fd].offset += rc;
+   file->offset += rc;
    /* Reset the data pending flag */
    return rc;
 }
-int tpm_tis_posix_fstat(int fd, struct stat* buf)
+
+static int tpm_tis_posix_fstat(int fd, struct stat *buf)
 {
    struct tpm_chip* tpm;
-   tpm = files[fd].dev;
+   struct file *file = get_file_from_fd(fd);
+
+   tpm = file->dev;
 
    buf->st_mode = O_RDWR;
    buf->st_uid = 0;
@@ -1360,6 +1369,35 @@ int tpm_tis_posix_fstat(int fd, struct stat* buf)
    return 0;
 }
 
+static struct file_ops tpm_tis_ops = {
+    .name = "tpm_tis",
+    .read = tpm_tis_posix_read,
+    .write = tpm_tis_posix_write,
+    .lseek = lseek_default,
+    .close = tpm_tis_close,
+    .fstat = tpm_tis_posix_fstat,
+};
+
+int tpm_tis_open(struct tpm_chip* tpm)
+{
+   struct file *file;
+   static unsigned int ftype_tis;
+
+   /* Silently prevent multiple opens */
+   if(tpm->fd != -1) {
+      return tpm->fd;
+   }
+
+   if ( !ftype_tis )
+       ftype_tis = alloc_file_type(&tpm_tis_ops);
+
+   tpm->fd = alloc_fd(ftype_tis);
+   printk("tpm_tis_open() -> %d\n", tpm->fd);
+   file = get_file_from_fd(tpm->fd);
+   file->dev = tpm;
+   return tpm->fd;
+}
+
 /* TPM 2.0 */
 
 /*TPM2.0 Selftest*/
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH v2 05/12] mini-os: use alloc_file_type() and get_file_from_fd() in tpmfront
  2022-01-11 15:12 [PATCH v2 00/12] mini-os: remove device specific struct file members Juergen Gross
                   ` (3 preceding siblings ...)
  2022-01-11 15:12 ` [PATCH v2 04/12] mini-os: use alloc_file_type() and get_file_from_fd() in tpm_tis Juergen Gross
@ 2022-01-11 15:12 ` Juergen Gross
  2022-01-11 20:15   ` Samuel Thibault
  2022-01-11 15:12 ` [PATCH v2 06/12] mini-os: use alloc_file_type() and get_file_from_fd() in blkfront Juergen Gross
                   ` (6 subsequent siblings)
  11 siblings, 1 reply; 48+ messages in thread
From: Juergen Gross @ 2022-01-11 15:12 UTC (permalink / raw)
  To: minios-devel, xen-devel; +Cc: samuel.thibault, wl, Juergen Gross

Allocate a file type dynamically via alloc_file_type().

Instead of directly accessing the files[] array use get_file_from_fd().

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 include/lib.h      |   3 +-
 include/tpmfront.h |   5 ---
 lib/sys.c          |  23 -----------
 tpmfront.c         | 100 ++++++++++++++++++++++++++++++++-------------
 4 files changed, 72 insertions(+), 59 deletions(-)

diff --git a/include/lib.h b/include/lib.h
index d94d142..f6478de 100644
--- a/include/lib.h
+++ b/include/lib.h
@@ -165,8 +165,7 @@ void sanity_check(void);
 #define FTYPE_KBD        7
 #define FTYPE_TAP        8
 #define FTYPE_BLK        9
-#define FTYPE_TPMFRONT  10
-#define FTYPE_N         11
+#define FTYPE_N         10
 #define FTYPE_SPARE     16
 
 typedef int file_read_func(int fd, void *buf, size_t nbytes);
diff --git a/include/tpmfront.h b/include/tpmfront.h
index b7da50e..a527371 100644
--- a/include/tpmfront.h
+++ b/include/tpmfront.h
@@ -81,7 +81,6 @@ int tpmfront_cmd(struct tpmfront_dev* dev, uint8_t* req, size_t reqlen, uint8_t*
 int tpmfront_set_locality(struct tpmfront_dev* dev, int locality);
 
 #ifdef HAVE_LIBC
-#include <sys/stat.h>
 /* POSIX IO functions:
  * use tpmfront_open() to get a file descriptor to the tpm device
  * use write() on the fd to send a command to the backend. You must
@@ -90,10 +89,6 @@ int tpmfront_set_locality(struct tpmfront_dev* dev, int locality);
  * fstat() to get the size of the response and lseek() to seek on it.
  */
 int tpmfront_open(struct tpmfront_dev* dev);
-int tpmfront_posix_read(int fd, uint8_t* buf, size_t count);
-int tpmfront_posix_write(int fd, const uint8_t* buf, size_t count);
-int tpmfront_posix_fstat(int fd, struct stat* buf);
 #endif
 
-
 #endif
diff --git a/lib/sys.c b/lib/sys.c
index 4060b42..ff6be52 100644
--- a/lib/sys.c
+++ b/lib/sys.c
@@ -28,7 +28,6 @@
 #include <netfront.h>
 #include <blkfront.h>
 #include <fbfront.h>
-#include <tpmfront.h>
 #include <xenbus.h>
 #include <xenstore.h>
 #include <poll.h>
@@ -343,11 +342,6 @@ int read(int fd, void *buf, size_t nbytes)
         case FTYPE_BLK: {
 	    return blkfront_posix_read(fd, buf, nbytes);
         }
-#endif
-#ifdef CONFIG_TPMFRONT
-        case FTYPE_TPMFRONT: {
-	    return tpmfront_posix_read(fd, buf, nbytes);
-        }
 #endif
 	default:
 	    break;
@@ -389,10 +383,6 @@ int write(int fd, const void *buf, size_t nbytes)
 #ifdef CONFIG_BLKFRONT
 	case FTYPE_BLK:
 	    return blkfront_posix_write(fd, buf, nbytes);
-#endif
-#ifdef CONFIG_TPMFRONT
-	case FTYPE_TPMFRONT:
-	    return tpmfront_posix_write(fd, buf, nbytes);
 #endif
 	default:
 	    break;
@@ -445,10 +435,6 @@ off_t lseek(int fd, off_t offset, int whence)
 #ifdef CONFIG_BLKFRONT
        case FTYPE_BLK:
           break;
-#endif
-#ifdef CONFIG_TPMFRONT
-       case FTYPE_TPMFRONT:
-          break;
 #endif
        case FTYPE_FILE:
           break;
@@ -496,11 +482,6 @@ int close(int fd)
             shutdown_blkfront(files[fd].dev);
             break;
 #endif
-#ifdef CONFIG_TPMFRONT
-	case FTYPE_TPMFRONT:
-            shutdown_tpmfront(files[fd].dev);
-            break;
-#endif
 #ifdef CONFIG_KBDFRONT
 	case FTYPE_KBD:
             shutdown_kbdfront(files[fd].dev);
@@ -576,10 +557,6 @@ int fstat(int fd, struct stat *buf)
 #ifdef CONFIG_BLKFRONT
 	case FTYPE_BLK:
 	   return blkfront_posix_fstat(fd, buf);
-#endif
-#ifdef CONFIG_TPMFRONT
-	case FTYPE_TPMFRONT:
-	   return tpmfront_posix_fstat(fd, buf);
 #endif
 	default:
 	    break;
diff --git a/tpmfront.c b/tpmfront.c
index 0a2fefc..a19a052 100644
--- a/tpmfront.c
+++ b/tpmfront.c
@@ -49,6 +49,10 @@
 void tpmfront_handler(evtchn_port_t port, struct pt_regs *regs, void *data) {
    struct tpmfront_dev* dev = (struct tpmfront_dev*) data;
    tpmif_shared_page_t *shr = dev->page;
+#ifdef HAVE_LIBC
+    struct file *file = get_file_from_fd(dev->fd);
+#endif
+
    /*If we get a response when we didnt make a request, just ignore it */
    if(!dev->waiting) {
       return;
@@ -65,8 +69,9 @@ 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 = true;
+   if ( file )
+   {
+      file->read = true;
    }
 #endif
    wake_up(&dev->waitq);
@@ -405,6 +410,10 @@ int tpmfront_send(struct tpmfront_dev* dev, const uint8_t* msg, size_t length)
 #ifdef TPMFRONT_PRINT_DEBUG
    int i;
 #endif
+#ifdef HAVE_LIBC
+    struct file *file = dev ? get_file_from_fd(dev->fd) : NULL;
+#endif
+
    /* Error Checking */
    if(dev == NULL || dev->state != XenbusStateConnected) {
       TPMFRONT_ERR("Tried to send message through disconnected frontend\n");
@@ -437,9 +446,10 @@ int tpmfront_send(struct tpmfront_dev* dev, const uint8_t* msg, size_t length)
    dev->waiting = 1;
    dev->resplen = 0;
 #ifdef HAVE_LIBC
-   if(dev->fd >= 0) {
-      files[dev->fd].read = false;
-      files[dev->fd].offset = 0;
+   if ( file )
+   {
+      file->read = false;
+      file->offset = 0;
       dev->respgot = false;
    }
 #endif
@@ -529,25 +539,14 @@ int tpmfront_set_locality(struct tpmfront_dev* dev, int locality)
 
 #ifdef HAVE_LIBC
 #include <errno.h>
-int tpmfront_open(struct tpmfront_dev* dev)
-{
-   /* Silently prevent multiple opens */
-   if(dev->fd != -1) {
-      return dev->fd;
-   }
 
-   dev->fd = alloc_fd(FTYPE_TPMFRONT);
-   printk("tpmfront_open(%s) -> %d\n", dev->nodename, dev->fd);
-   files[dev->fd].dev = dev;
-   dev->respgot = false;
-   return dev->fd;
-}
-
-int tpmfront_posix_write(int fd, const uint8_t* buf, size_t count)
+static int tpmfront_posix_write(int fd, const void *buf, size_t count)
 {
    int rc;
    struct tpmfront_dev* dev;
-   dev = files[fd].dev;
+   struct file *file = get_file_from_fd(fd);
+
+   dev = file->dev;
 
    if(count == 0) {
       return 0;
@@ -566,14 +565,15 @@ int tpmfront_posix_write(int fd, const uint8_t* buf, size_t count)
    return count;
 }
 
-int tpmfront_posix_read(int fd, uint8_t* buf, size_t count)
+static int tpmfront_posix_read(int fd, void *buf, size_t count)
 {
    int rc;
    uint8_t* dummybuf;
    size_t dummysz;
    struct tpmfront_dev* dev;
+   struct file *file = get_file_from_fd(fd);
 
-   dev = files[fd].dev;
+   dev = file->dev;
 
    if(count == 0) {
       return 0;
@@ -588,29 +588,33 @@ int tpmfront_posix_read(int fd, uint8_t* buf, size_t count)
    }
 
    /* handle EOF case */
-   if(files[dev->fd].offset >= dev->resplen) {
+   if ( file->offset >= dev->resplen )
+   {
       return 0;
    }
 
    /* Compute the number of bytes and do the copy operation */
-   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;
+   if ( (rc = min(count, dev->resplen - file->offset)) != 0 )
+   {
+      memcpy(buf, dev->respbuf + file->offset, rc);
+      file->offset += rc;
    }
 
    return rc;
 }
 
-int tpmfront_posix_fstat(int fd, struct stat* buf)
+static int tpmfront_posix_fstat(int fd, struct stat *buf)
 {
    uint8_t* dummybuf;
    size_t dummysz;
    int rc;
-   struct tpmfront_dev* dev = files[fd].dev;
+   struct file *file = get_file_from_fd(fd);
+   struct tpmfront_dev* dev = file->dev;
 
    /* 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 && !dev->respgot)) {
+   if ( dev->waiting || (file->read && !dev->respgot) )
+   {
       if ((rc = tpmfront_recv(dev, &dummybuf, &dummysz)) != 0) {
 	 errno = EIO;
 	 return -1;
@@ -626,5 +630,43 @@ int tpmfront_posix_fstat(int fd, struct stat* buf)
    return 0;
 }
 
+static int tpmfront_close_fd(int fd)
+{
+    struct file *file = get_file_from_fd(fd);
+
+    shutdown_tpmfront(file->dev);
+
+    return 0;
+}
+
+static struct file_ops tpmfront_ops = {
+    .name = "tpmfront",
+    .read = tpmfront_posix_read,
+    .write = tpmfront_posix_write,
+    .lseek = lseek_default,
+    .close = tpmfront_close_fd,
+    .fstat = tpmfront_posix_fstat,
+};
+
+int tpmfront_open(struct tpmfront_dev* dev)
+{
+   struct file *file;
+   static unsigned int ftype_tpmfront;
+
+   /* Silently prevent multiple opens */
+   if(dev->fd != -1) {
+      return dev->fd;
+   }
+
+   if ( !ftype_tpmfront )
+       ftype_tpmfront = alloc_file_type(&tpmfront_ops);
+
+   dev->fd = alloc_fd(ftype_tpmfront);
+   printk("tpmfront_open(%s) -> %d\n", dev->nodename, dev->fd);
+   file = get_file_from_fd(dev->fd);
+   file->dev = dev;
+   dev->respgot = false;
+   return dev->fd;
+}
 
 #endif
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH v2 06/12] mini-os: use alloc_file_type() and get_file_from_fd() in blkfront
  2022-01-11 15:12 [PATCH v2 00/12] mini-os: remove device specific struct file members Juergen Gross
                   ` (4 preceding siblings ...)
  2022-01-11 15:12 ` [PATCH v2 05/12] mini-os: use alloc_file_type() and get_file_from_fd() in tpmfront Juergen Gross
@ 2022-01-11 15:12 ` Juergen Gross
  2022-01-11 20:20   ` Samuel Thibault
  2022-01-11 15:12 ` [PATCH v2 07/12] mini-os: use get_file_from_fd() in netfront Juergen Gross
                   ` (5 subsequent siblings)
  11 siblings, 1 reply; 48+ messages in thread
From: Juergen Gross @ 2022-01-11 15:12 UTC (permalink / raw)
  To: minios-devel, xen-devel; +Cc: samuel.thibault, wl, Juergen Gross

Allocate the file type dynamically via alloc_file_type().

Instead of directly accessing the files[] array use get_file_from_fd().

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 blkfront.c         | 92 ++++++++++++++++++++++++++++++++++------------
 include/blkfront.h |  5 ---
 include/lib.h      |  3 +-
 lib/sys.c          | 24 ------------
 4 files changed, 69 insertions(+), 55 deletions(-)

diff --git a/blkfront.c b/blkfront.c
index e3f42be..b7ea1d7 100644
--- a/blkfront.c
+++ b/blkfront.c
@@ -59,10 +59,10 @@ void blkfront_handler(evtchn_port_t port, struct pt_regs *regs, void *data)
 {
 #ifdef HAVE_LIBC
     struct blkfront_dev *dev = data;
-    int fd = dev->fd;
+    struct file *file = get_file_from_fd(dev->fd);
 
-    if (fd != -1)
-        files[fd].read = true;
+    if ( file )
+        file->read = true;
 #endif
     wake_up(&blkfront_queue);
 }
@@ -483,9 +483,13 @@ int blkfront_aio_poll(struct blkfront_dev *dev)
 
 moretodo:
 #ifdef HAVE_LIBC
-    if (dev->fd != -1) {
-        files[dev->fd].read = false;
-        mb(); /* Make sure to let the handler set read to 1 before we start looking at the ring */
+    {
+        struct file *file = get_file_from_fd(dev->fd);
+
+        if ( file ) {
+            file->read = false;
+            mb(); /* Make sure to let the handler set read to 1 before we start looking at the ring */
+        }
     }
 #endif
 
@@ -554,22 +558,11 @@ moretodo:
 }
 
 #ifdef HAVE_LIBC
-int blkfront_open(struct blkfront_dev *dev)
-{
-    /* Silently prevent multiple opens */
-    if(dev->fd != -1) {
-       return dev->fd;
-    }
-    dev->fd = alloc_fd(FTYPE_BLK);
-    printk("blk_open(%s) -> %d\n", dev->nodename, dev->fd);
-    files[dev->fd].dev = dev;
-    return dev->fd;
-}
-
-int blkfront_posix_rwop(int fd, uint8_t* buf, size_t count, int write)
+static int blkfront_posix_rwop(int fd, uint8_t* buf, size_t count, int write)
 {
-   struct blkfront_dev* dev = files[fd].dev;
-   off_t offset = files[fd].offset;
+   struct file *file = get_file_from_fd(fd);
+   struct blkfront_dev* dev = file->dev;
+   off_t offset = file->offset;
    struct blkfront_aiocb aiocb;
    unsigned long long disksize = dev->info.sectors * dev->info.sector_size;
    unsigned int blocksize = dev->info.sector_size;
@@ -711,14 +704,25 @@ int blkfront_posix_rwop(int fd, uint8_t* buf, size_t count, int write)
    }
 
    free(copybuf);
-   files[fd].offset += rc;
+   file->offset += rc;
    return rc;
 
 }
 
-int blkfront_posix_fstat(int fd, struct stat* buf)
+static int blkfront_posix_read(int fd, void *buf, size_t nbytes)
+{
+    return blkfront_posix_rwop(fd, buf, nbytes, 0);
+}
+
+static int blkfront_posix_write(int fd, const void *buf, size_t nbytes)
 {
-   struct blkfront_dev* dev = files[fd].dev;
+    return blkfront_posix_rwop(fd, (void *)buf, nbytes, 1);
+}
+
+static int blkfront_posix_fstat(int fd, struct stat *buf)
+{
+   struct file *file = get_file_from_fd(fd);
+   struct blkfront_dev* dev = file->dev;
 
    buf->st_mode = dev->info.mode;
    buf->st_uid = 0;
@@ -728,4 +732,44 @@ int blkfront_posix_fstat(int fd, struct stat* buf)
 
    return 0;
 }
+
+static int blkfront_close_fd(int fd)
+{
+    struct file *file = get_file_from_fd(fd);
+
+    shutdown_blkfront(file->dev);
+
+    return 0;
+}
+
+static struct file_ops blk_ops = {
+    .name = "blk",
+    .read = blkfront_posix_read,
+    .write = blkfront_posix_write,
+    .lseek = lseek_default,
+    .close = blkfront_close_fd,
+    .fstat = blkfront_posix_fstat,
+    .select_rd = select_read_flag,
+};
+
+int blkfront_open(struct blkfront_dev *dev)
+{
+    struct file *file;
+    static unsigned int ftype_blk;
+
+    /* Silently prevent multiple opens */
+    if(dev->fd != -1) {
+       return dev->fd;
+    }
+
+    if ( !ftype_blk )
+        ftype_blk = alloc_file_type(&blk_ops);
+
+    dev->fd = alloc_fd(ftype_blk);
+    printk("blk_open(%s) -> %d\n", dev->nodename, dev->fd);
+    file = get_file_from_fd(dev->fd);
+    file->dev = dev;
+
+    return dev->fd;
+}
 #endif
diff --git a/include/blkfront.h b/include/blkfront.h
index 3528af9..7f84a0a 100644
--- a/include/blkfront.h
+++ b/include/blkfront.h
@@ -28,17 +28,12 @@ struct blkfront_info
 };
 struct blkfront_dev *init_blkfront(char *nodename, struct blkfront_info *info);
 #ifdef HAVE_LIBC
-#include <sys/stat.h>
 /* POSIX IO functions:
  * use blkfront_open() to get a file descriptor to the block device
  * Don't use the other blkfront posix functions here directly, instead use
  * read(), write(), lseek() and fstat() on the file descriptor
  */
 int blkfront_open(struct blkfront_dev *dev);
-int blkfront_posix_rwop(int fd, uint8_t* buf, size_t count, int write);
-#define blkfront_posix_write(fd, buf, count) blkfront_posix_rwop(fd, (uint8_t*)buf, count, 1)
-#define blkfront_posix_read(fd, buf, count) blkfront_posix_rwop(fd, (uint8_t*)buf, count, 0)
-int blkfront_posix_fstat(int fd, struct stat* buf);
 #endif
 void blkfront_aio(struct blkfront_aiocb *aiocbp, int write);
 #define blkfront_aio_read(aiocbp) blkfront_aio(aiocbp, 0)
diff --git a/include/lib.h b/include/lib.h
index f6478de..05f5083 100644
--- a/include/lib.h
+++ b/include/lib.h
@@ -164,8 +164,7 @@ void sanity_check(void);
 #define FTYPE_FB         6
 #define FTYPE_KBD        7
 #define FTYPE_TAP        8
-#define FTYPE_BLK        9
-#define FTYPE_N         10
+#define FTYPE_N          9
 #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 ff6be52..f84fedd 100644
--- a/lib/sys.c
+++ b/lib/sys.c
@@ -337,11 +337,6 @@ int read(int fd, void *buf, size_t nbytes)
 	    }
 	    return ret * sizeof(union xenfb_in_event);
         }
-#endif
-#ifdef CONFIG_BLKFRONT
-        case FTYPE_BLK: {
-	    return blkfront_posix_read(fd, buf, nbytes);
-        }
 #endif
 	default:
 	    break;
@@ -379,10 +374,6 @@ int write(int fd, const void *buf, size_t nbytes)
 	case FTYPE_TAP:
 	    netfront_xmit(files[fd].dev, (void*) buf, nbytes);
 	    return nbytes;
-#endif
-#ifdef CONFIG_BLKFRONT
-	case FTYPE_BLK:
-	    return blkfront_posix_write(fd, buf, nbytes);
 #endif
 	default:
 	    break;
@@ -432,10 +423,6 @@ off_t lseek(int fd, off_t offset, int whence)
         return ops->lseek(fd, offset, whence);
 
     switch(files[fd].type) {
-#ifdef CONFIG_BLKFRONT
-       case FTYPE_BLK:
-          break;
-#endif
        case FTYPE_FILE:
           break;
        default:
@@ -477,11 +464,6 @@ int close(int fd)
 	    shutdown_netfront(files[fd].dev);
             break;
 #endif
-#ifdef CONFIG_BLKFRONT
-	case FTYPE_BLK:
-            shutdown_blkfront(files[fd].dev);
-            break;
-#endif
 #ifdef CONFIG_KBDFRONT
 	case FTYPE_KBD:
             shutdown_kbdfront(files[fd].dev);
@@ -554,10 +536,6 @@ int fstat(int fd, struct stat *buf)
 	    buf->st_ctime = time(NULL);
 	    return 0;
 	}
-#ifdef CONFIG_BLKFRONT
-	case FTYPE_BLK:
-	   return blkfront_posix_fstat(fd, buf);
-#endif
 	default:
 	    break;
     }
@@ -663,7 +641,6 @@ static const char *file_types[] = {
     [FTYPE_CONSOLE] = "console",
     [FTYPE_SOCKET]  = "socket",
     [FTYPE_TAP]     = "net",
-    [FTYPE_BLK]     = "blk",
     [FTYPE_KBD]     = "kbd",
     [FTYPE_FB]      = "fb",
 };
@@ -841,7 +818,6 @@ static int select_poll(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exce
 	    FD_CLR(i, exceptfds);
 	    break;
 	case FTYPE_TAP:
-	case FTYPE_BLK:
 	case FTYPE_KBD:
 	case FTYPE_FB:
 	    if (FD_ISSET(i, readfds)) {
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH v2 07/12] mini-os: use get_file_from_fd() in netfront
  2022-01-11 15:12 [PATCH v2 00/12] mini-os: remove device specific struct file members Juergen Gross
                   ` (5 preceding siblings ...)
  2022-01-11 15:12 ` [PATCH v2 06/12] mini-os: use alloc_file_type() and get_file_from_fd() in blkfront Juergen Gross
@ 2022-01-11 15:12 ` Juergen Gross
  2022-01-11 20:22   ` Samuel Thibault
  2022-01-11 15:12 ` [PATCH v2 08/12] mini-os: use alloc_file_type() and get_file_from_fd() in fbfront Juergen Gross
                   ` (4 subsequent siblings)
  11 siblings, 1 reply; 48+ messages in thread
From: Juergen Gross @ 2022-01-11 15:12 UTC (permalink / raw)
  To: minios-devel, xen-devel; +Cc: samuel.thibault, wl, Juergen Gross

Instead of directly accessing the files[] array use get_file_from_fd().

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 include/lib.h |  3 +--
 lib/sys.c     | 23 ------------------
 netfront.c    | 64 ++++++++++++++++++++++++++++++++++++++++++++-------
 3 files changed, 57 insertions(+), 33 deletions(-)

diff --git a/include/lib.h b/include/lib.h
index 05f5083..aa8036e 100644
--- a/include/lib.h
+++ b/include/lib.h
@@ -163,8 +163,7 @@ void sanity_check(void);
 #define FTYPE_SAVEFILE   5
 #define FTYPE_FB         6
 #define FTYPE_KBD        7
-#define FTYPE_TAP        8
-#define FTYPE_N          9
+#define FTYPE_N          8
 #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 f84fedd..7f2e11f 100644
--- a/lib/sys.c
+++ b/lib/sys.c
@@ -303,17 +303,6 @@ int read(int fd, void *buf, size_t nbytes)
 	case FTYPE_SOCKET:
 	    return lwip_read(files[fd].fd, buf, nbytes);
 #endif
-#ifdef CONFIG_NETFRONT
-	case FTYPE_TAP: {
-	    ssize_t ret;
-	    ret = netfront_receive(files[fd].dev, buf, nbytes);
-	    if (ret <= 0) {
-		errno = EAGAIN;
-		return -1;
-	    }
-	    return ret;
-	}
-#endif
 #ifdef CONFIG_KBDFRONT
         case FTYPE_KBD: {
             int ret, n;
@@ -369,11 +358,6 @@ int write(int fd, const void *buf, size_t nbytes)
 #ifdef HAVE_LWIP
 	case FTYPE_SOCKET:
 	    return lwip_write(files[fd].fd, (void*) buf, nbytes);
-#endif
-#ifdef CONFIG_NETFRONT
-	case FTYPE_TAP:
-	    netfront_xmit(files[fd].dev, (void*) buf, nbytes);
-	    return nbytes;
 #endif
 	default:
 	    break;
@@ -459,11 +443,6 @@ int close(int fd)
 	    res = lwip_close(files[fd].fd);
             break;
 #endif
-#ifdef CONFIG_NETFRONT
-	case FTYPE_TAP:
-	    shutdown_netfront(files[fd].dev);
-            break;
-#endif
 #ifdef CONFIG_KBDFRONT
 	case FTYPE_KBD:
             shutdown_kbdfront(files[fd].dev);
@@ -640,7 +619,6 @@ static const char *file_types[] = {
     [FTYPE_NONE]    = "none",
     [FTYPE_CONSOLE] = "console",
     [FTYPE_SOCKET]  = "socket",
-    [FTYPE_TAP]     = "net",
     [FTYPE_KBD]     = "kbd",
     [FTYPE_FB]      = "fb",
 };
@@ -817,7 +795,6 @@ static int select_poll(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exce
                 n++;
 	    FD_CLR(i, exceptfds);
 	    break;
-	case FTYPE_TAP:
 	case FTYPE_KBD:
 	case FTYPE_FB:
 	    if (FD_ISSET(i, readfds)) {
diff --git a/netfront.c b/netfront.c
index 7696451..f86c6a2 100644
--- a/netfront.c
+++ b/netfront.c
@@ -248,14 +248,14 @@ void netfront_select_handler(evtchn_port_t port, struct pt_regs *regs, void *dat
 {
     int flags;
     struct netfront_dev *dev = data;
-    int fd = dev->fd;
+    struct file *file = get_file_from_fd(dev->fd);
 
     local_irq_save(flags);
     network_tx_buf_gc(dev);
     local_irq_restore(flags);
 
-    if (fd != -1)
-        files[fd].read = true;
+    if ( file )
+        file->read = true;
     wake_up(&netfront_queue);
 }
 #endif
@@ -565,8 +565,54 @@ error:
 }
 
 #ifdef HAVE_LIBC
+static int netfront_read(int fd, void *buf, size_t nbytes)
+{
+    ssize_t ret;
+    struct file *file = get_file_from_fd(fd);
+
+    ret = netfront_receive(file->dev, buf, nbytes);
+    if ( ret <= 0 )
+    {
+        errno = EAGAIN;
+        return -1;
+    }
+
+    return ret;
+}
+
+static int netfront_write(int fd, const void *buf, size_t nbytes)
+{
+    struct file *file = get_file_from_fd(fd);
+
+    netfront_xmit(file->dev, (void *)buf, nbytes);
+
+    return nbytes;
+}
+
+static int netfront_close_fd(int fd)
+{
+    struct file *file = get_file_from_fd(fd);
+
+    shutdown_netfront(file->dev);
+
+    return 0;
+}
+
+static struct file_ops netfront_ops = {
+    .name = "net",
+    .read = netfront_read,
+    .write = netfront_write,
+    .close = netfront_close_fd,
+    .select_rd = select_read_flag,
+};
+
 int netfront_tap_open(char *nodename) {
     struct netfront_dev *dev;
+    struct file *file;
+    static unsigned int ftype_netfront;
+
+    if ( !ftype_netfront )
+        ftype_netfront = alloc_file_type(&netfront_ops);
 
     dev = init_netfront(nodename, NETIF_SELECT_RX, NULL, NULL);
     if (!dev) {
@@ -574,9 +620,10 @@ int netfront_tap_open(char *nodename) {
 	errno = EIO;
 	return -1;
     }
-    dev->fd = alloc_fd(FTYPE_TAP);
+    dev->fd = alloc_fd(ftype_netfront);
     printk("tap_open(%s) -> %d\n", nodename, dev->fd);
-    files[dev->fd].dev = dev;
+    file = get_file_from_fd(dev->fd);
+    file->dev = dev;
     return dev->fd;
 }
 #endif
@@ -772,7 +819,8 @@ void netfront_xmit(struct netfront_dev *dev, unsigned char* data,int len)
 ssize_t netfront_receive(struct netfront_dev *dev, unsigned char *data, size_t len)
 {
     unsigned long flags;
-    int fd = dev->fd;
+    struct file *file = get_file_from_fd(dev->fd);
+
     ASSERT(current == main_thread);
 
     dev->rlen = 0;
@@ -781,9 +829,9 @@ ssize_t netfront_receive(struct netfront_dev *dev, unsigned char *data, size_t l
 
     local_irq_save(flags);
     network_rx(dev);
-    if (!dev->rlen && fd != -1)
+    if ( !dev->rlen && file )
         /* No data for us, make select stop returning */
-        files[fd].read = false;
+        file->read = false;
     /* Before re-enabling the interrupts, in case a packet just arrived in the
      * meanwhile. */
     local_irq_restore(flags);
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH v2 08/12] mini-os: use alloc_file_type() and get_file_from_fd() in fbfront
  2022-01-11 15:12 [PATCH v2 00/12] mini-os: remove device specific struct file members Juergen Gross
                   ` (6 preceding siblings ...)
  2022-01-11 15:12 ` [PATCH v2 07/12] mini-os: use get_file_from_fd() in netfront Juergen Gross
@ 2022-01-11 15:12 ` Juergen Gross
  2022-01-11 20:26   ` Samuel Thibault
  2022-01-11 15:12 ` [PATCH v2 09/12] mini-os: use file_ops and get_file_from_fd() for console Juergen Gross
                   ` (3 subsequent siblings)
  11 siblings, 1 reply; 48+ messages in thread
From: Juergen Gross @ 2022-01-11 15:12 UTC (permalink / raw)
  To: minios-devel, xen-devel; +Cc: samuel.thibault, wl, Juergen Gross

Allocate file types dynamically via alloc_file_type().

Instead of directly accessing the files[] array use get_file_from_fd().

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 fbfront.c     | 125 ++++++++++++++++++++++++++++++++++++++++++--------
 include/lib.h |   4 +-
 lib/sys.c     |  47 -------------------
 3 files changed, 106 insertions(+), 70 deletions(-)

diff --git a/fbfront.c b/fbfront.c
index 1e055fb..9c96963 100644
--- a/fbfront.c
+++ b/fbfront.c
@@ -14,6 +14,9 @@
 #include <mini-os/xmalloc.h>
 #include <mini-os/fbfront.h>
 #include <mini-os/lib.h>
+#ifdef HAVE_LIBC
+#include <errno.h>
+#endif
 
 DECLARE_WAIT_QUEUE_HEAD(kbdfront_queue);
 
@@ -42,10 +45,10 @@ void kbdfront_handler(evtchn_port_t port, struct pt_regs *regs, void *data)
 {
 #ifdef HAVE_LIBC
     struct kbdfront_dev *dev = data;
-    int fd = dev->fd;
+    struct file *file = get_file_from_fd(dev->fd);
 
-    if (fd != -1)
-        files[fd].read = true;
+    if ( file )
+        file->read = true;
 #endif
     wake_up(&kbdfront_queue);
 }
@@ -204,10 +207,12 @@ int kbdfront_receive(struct kbdfront_dev *dev, union xenkbd_in_event *buf, int n
     struct xenkbd_page *page = dev->page;
     uint32_t prod, cons;
     int i;
-
 #ifdef HAVE_LIBC
-    if (dev->fd != -1) {
-        files[dev->fd].read = false;
+    struct file *file = get_file_from_fd(dev->fd);
+
+    if ( file )
+    {
+        file->read = false;
         mb(); /* Make sure to let the handler set read to 1 before we start looking at the ring */
     }
 #endif
@@ -227,9 +232,9 @@ int kbdfront_receive(struct kbdfront_dev *dev, union xenkbd_in_event *buf, int n
     notify_remote_via_evtchn(dev->evtchn);
 
 #ifdef HAVE_LIBC
-    if (cons != prod && dev->fd != -1)
+    if ( cons != prod && file )
         /* still some events to read */
-        files[dev->fd].read = true;
+        file->read = true;
 #endif
 
     return i;
@@ -298,11 +303,50 @@ close_kbdfront:
 }
 
 #ifdef HAVE_LIBC
+static int kbd_read(int fd, void *buf, size_t nbytes)
+{
+    struct file *file = get_file_from_fd(fd);
+    int ret, n;
+
+    n = nbytes / sizeof(union xenkbd_in_event);
+    ret = kbdfront_receive(file->dev, buf, n);
+    if ( ret <= 0 )
+    {
+        errno = EAGAIN;
+        return -1;
+    }
+
+    return ret * sizeof(union xenkbd_in_event);
+}
+
+static int kbd_close_fd(int fd)
+{
+    struct file *file = get_file_from_fd(fd);
+
+    shutdown_kbdfront(file->dev);
+
+    return 0;
+}
+
+static struct file_ops kbd_ops = {
+    .name = "kbd",
+    .read = kbd_read,
+    .close = kbd_close_fd,
+    .select_rd = select_read_flag,
+};
+
 int kbdfront_open(struct kbdfront_dev *dev)
 {
-    dev->fd = alloc_fd(FTYPE_KBD);
+    struct file *file;
+    static unsigned int ftype_kbd;
+
+    if ( !ftype_kbd )
+        ftype_kbd = alloc_file_type(&kbd_ops);
+
+    dev->fd = alloc_fd(ftype_kbd);
     printk("kbd_open(%s) -> %d\n", dev->nodename, dev->fd);
-    files[dev->fd].dev = dev;
+    file = get_file_from_fd(dev->fd);
+    file->dev = dev;
     return dev->fd;
 }
 #endif
@@ -346,10 +390,10 @@ void fbfront_handler(evtchn_port_t port, struct pt_regs *regs, void *data)
 {
 #ifdef HAVE_LIBC
     struct fbfront_dev *dev = data;
-    int fd = dev->fd;
+    struct file *file = get_file_from_fd(dev->fd);
 
-    if (fd != -1)
-        files[fd].read = true;
+    if ( file )
+        file->read = true;
 #endif
     wake_up(&fbfront_queue);
 }
@@ -373,10 +417,12 @@ int fbfront_receive(struct fbfront_dev *dev, union xenfb_in_event *buf, int n)
     struct xenfb_page *page = dev->page;
     uint32_t prod, cons;
     int i;
-
 #ifdef HAVE_LIBC
-    if (dev->fd != -1) {
-        files[dev->fd].read = false;
+    struct file *file = get_file_from_fd(dev->fd);
+
+    if ( file )
+    {
+        file->read = false;
         mb(); /* Make sure to let the handler set read to 1 before we start looking at the ring */
     }
 #endif
@@ -396,9 +442,9 @@ int fbfront_receive(struct fbfront_dev *dev, union xenfb_in_event *buf, int n)
     notify_remote_via_evtchn(dev->evtchn);
 
 #ifdef HAVE_LIBC
-    if (cons != prod && dev->fd != -1)
+    if ( cons != prod && file )
         /* still some events to read */
-        files[dev->fd].read = true;
+        file->read = true;
 #endif
 
     return i;
@@ -699,11 +745,50 @@ close_fbfront:
 }
 
 #ifdef HAVE_LIBC
+static int fbfront_read(int fd, void *buf, size_t nbytes)
+{
+    struct file *file = get_file_from_fd(fd);
+    int ret, n;
+
+    n = nbytes / sizeof(union xenfb_in_event);
+    ret = fbfront_receive(file->dev, buf, n);
+    if ( ret <= 0 )
+    {
+        errno = EAGAIN;
+        return -1;
+    }
+
+    return ret * sizeof(union xenfb_in_event);
+}
+
+static int fbfront_close_fd(int fd)
+{
+    struct file *file = get_file_from_fd(fd);
+
+    shutdown_fbfront(file->dev);
+
+    return 0;
+}
+
+static struct file_ops fb_ops = {
+    .name = ".fb",
+    .read = fbfront_read,
+    .close = fbfront_close_fd,
+    .select_rd = select_read_flag,
+};
+
 int fbfront_open(struct fbfront_dev *dev)
 {
-    dev->fd = alloc_fd(FTYPE_FB);
+    struct file *file;
+    static unsigned int ftype_fb;
+
+    if ( !ftype_fb )
+        ftype_fb = alloc_file_type(&fb_ops);
+
+    dev->fd = alloc_fd(ftype_fb);
     printk("fb_open(%s) -> %d\n", dev->nodename, dev->fd);
-    files[dev->fd].dev = dev;
+    file = get_file_from_fd(dev->fd);
+    file->dev = dev;
     return dev->fd;
 }
 #endif
diff --git a/include/lib.h b/include/lib.h
index aa8036e..653a16e 100644
--- a/include/lib.h
+++ b/include/lib.h
@@ -161,9 +161,7 @@ void sanity_check(void);
 #define FTYPE_SOCKET     3
 #define FTYPE_MEM        4
 #define FTYPE_SAVEFILE   5
-#define FTYPE_FB         6
-#define FTYPE_KBD        7
-#define FTYPE_N          8
+#define FTYPE_N          6
 #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 7f2e11f..4fb844f 100644
--- a/lib/sys.c
+++ b/lib/sys.c
@@ -302,30 +302,6 @@ int read(int fd, void *buf, size_t nbytes)
 #ifdef HAVE_LWIP
 	case FTYPE_SOCKET:
 	    return lwip_read(files[fd].fd, buf, nbytes);
-#endif
-#ifdef CONFIG_KBDFRONT
-        case FTYPE_KBD: {
-            int ret, n;
-            n = nbytes / sizeof(union xenkbd_in_event);
-            ret = kbdfront_receive(files[fd].dev, buf, n);
-	    if (ret <= 0) {
-		errno = EAGAIN;
-		return -1;
-	    }
-	    return ret * sizeof(union xenkbd_in_event);
-        }
-#endif
-#ifdef CONFIG_FBFRONT
-        case FTYPE_FB: {
-            int ret, n;
-            n = nbytes / sizeof(union xenfb_in_event);
-            ret = fbfront_receive(files[fd].dev, buf, n);
-	    if (ret <= 0) {
-		errno = EAGAIN;
-		return -1;
-	    }
-	    return ret * sizeof(union xenfb_in_event);
-        }
 #endif
 	default:
 	    break;
@@ -443,16 +419,6 @@ int close(int fd)
 	    res = lwip_close(files[fd].fd);
             break;
 #endif
-#ifdef CONFIG_KBDFRONT
-	case FTYPE_KBD:
-            shutdown_kbdfront(files[fd].dev);
-            break;
-#endif
-#ifdef CONFIG_FBFRONT
-	case FTYPE_FB:
-            shutdown_fbfront(files[fd].dev);
-            break;
-#endif
 #ifdef CONFIG_CONSFRONT
         case FTYPE_SAVEFILE:
         case FTYPE_CONSOLE:
@@ -619,8 +585,6 @@ static const char *file_types[] = {
     [FTYPE_NONE]    = "none",
     [FTYPE_CONSOLE] = "console",
     [FTYPE_SOCKET]  = "socket",
-    [FTYPE_KBD]     = "kbd",
-    [FTYPE_FB]      = "fb",
 };
 
 static char *get_type_name(unsigned int type)
@@ -795,17 +759,6 @@ static int select_poll(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exce
                 n++;
 	    FD_CLR(i, exceptfds);
 	    break;
-	case FTYPE_KBD:
-	case FTYPE_FB:
-	    if (FD_ISSET(i, readfds)) {
-		if (files[i].read)
-		    n++;
-		else
-		    FD_CLR(i, readfds);
-	    }
-	    FD_CLR(i, writefds);
-	    FD_CLR(i, exceptfds);
-	    break;
 #ifdef HAVE_LWIP
 	case FTYPE_SOCKET:
 	    if (FD_ISSET(i, readfds)) {
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH v2 09/12] mini-os: use file_ops and get_file_from_fd() for console
  2022-01-11 15:12 [PATCH v2 00/12] mini-os: remove device specific struct file members Juergen Gross
                   ` (7 preceding siblings ...)
  2022-01-11 15:12 ` [PATCH v2 08/12] mini-os: use alloc_file_type() and get_file_from_fd() in fbfront Juergen Gross
@ 2022-01-11 15:12 ` Juergen Gross
  2022-01-11 20:35   ` Samuel Thibault
                     ` (2 more replies)
  2022-01-11 15:12 ` [PATCH v2 10/12] mini-os: add struct file_ops for file type socket Juergen Gross
                   ` (2 subsequent siblings)
  11 siblings, 3 replies; 48+ messages in thread
From: Juergen Gross @ 2022-01-11 15:12 UTC (permalink / raw)
  To: minios-devel, xen-devel; +Cc: samuel.thibault, wl, Juergen Gross

Add struct file_ops for the console related file types (FTYPE_CONSOLE
and savefile). FTYPE_CONSOLE remains statically defined, as it is used
to statically init stdin, stdout and stderr.

Instead of directly accessing the files[] array use get_file_from_fd().

With CONSOLE now handled via file_ops the bogus file descriptor case in
select_poll() now needs to be handled more explicit instead of dropping
into console handling, assuming that this case was basically meant to
cover SAVEFILE.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 console/xenbus.c       | 125 +++++++++++++++++++++++++++++++++++++++++
 console/xencons_ring.c |   6 +-
 include/console.h      |   5 ++
 include/lib.h          |   3 +-
 lib/sys.c              |  87 +++++++---------------------
 5 files changed, 155 insertions(+), 71 deletions(-)

diff --git a/console/xenbus.c b/console/xenbus.c
index 05fc31c..b856c84 100644
--- a/console/xenbus.c
+++ b/console/xenbus.c
@@ -192,3 +192,128 @@ void fini_consfront(struct consfront_dev *dev)
 {
     if (dev) free_consfront(dev);
 }
+
+#ifdef HAVE_LIBC
+static int consfront_read(int fd, void *buf, size_t nbytes)
+{
+    int ret;
+    struct file *file = get_file_from_fd(fd);
+    DEFINE_WAIT(w);
+
+    while ( 1 )
+    {
+        add_waiter(w, console_queue);
+        ret = xencons_ring_recv(file->dev, buf, nbytes);
+        if ( ret )
+            break;
+        schedule();
+    }
+
+    remove_waiter(w, console_queue);
+
+    return ret;
+}
+
+static int savefile_write(int fd, const void *buf, size_t nbytes)
+{
+    int ret = 0, tot = nbytes;
+    struct file *file = get_file_from_fd(fd);
+
+    while ( nbytes > 0 )
+    {
+        ret = xencons_ring_send(file->dev, (char *)buf, nbytes);
+        nbytes -= ret;
+        buf = (char *)buf + ret;
+    }
+
+    return tot - nbytes;
+}
+
+static int console_write(int fd, const void *buf, size_t nbytes)
+{
+    struct file *file = get_file_from_fd(fd);
+
+    console_print(file->dev, (char *)buf, nbytes);
+
+    return nbytes;
+}
+
+static int consfront_close_fd(int fd)
+{
+    struct file *file = get_file_from_fd(fd);
+
+    fini_consfront(file->dev);
+
+    return 0;
+}
+
+static int consfront_fstat(int fd, struct stat *buf)
+{
+    struct file *file = get_file_from_fd(fd);
+
+    buf->st_mode = S_IRUSR | S_IWUSR;
+    buf->st_mode |= (file->type == FTYPE_CONSOLE) ? S_IFCHR : S_IFREG;
+    buf->st_atime = buf->st_mtime = buf->st_ctime = time(NULL);
+
+    return 0;
+}
+
+static bool consfront_select_rd(int fd)
+{
+    struct file *file = get_file_from_fd(fd);
+
+    return xencons_ring_avail(file->dev);
+}
+
+static struct file_ops savefile_ops = {
+    .name = "savefile",
+    .read = consfront_read,
+    .write = savefile_write,
+    .close = consfront_close_fd,
+    .fstat = consfront_fstat,
+    .select_rd = consfront_select_rd,
+    .select_wr = select_yes,
+};
+
+struct file_ops console_ops = {
+    .name = "console",
+    .read = consfront_read,
+    .write = console_write,
+    .close = consfront_close_fd,
+    .fstat = consfront_fstat,
+    .select_rd = consfront_select_rd,
+    .select_wr = select_yes,
+};
+
+int open_consfront(char *nodename)
+{
+    struct consfront_dev *dev;
+    static unsigned int ftype_savefile;
+    unsigned int ftype;
+    struct file *file;
+
+    dev = init_consfront(nodename);
+    if ( !dev )
+        return -1;
+
+    if ( nodename )
+    {
+        if ( !ftype_savefile )
+            ftype_savefile = alloc_file_type(&savefile_ops);
+        ftype = ftype_savefile;
+    }
+    else
+        ftype = FTYPE_CONSOLE;
+
+    dev->fd = alloc_fd(ftype);
+    file = get_file_from_fd(dev->fd);
+    if ( !file )
+    {
+        fini_consfront(dev);
+        return -1;
+    }
+    file->dev = dev;
+
+    return dev->fd;
+}
+#endif
diff --git a/console/xencons_ring.c b/console/xencons_ring.c
index c348f3c..efedf46 100644
--- a/console/xencons_ring.c
+++ b/console/xencons_ring.c
@@ -99,10 +99,10 @@ void console_handle_input(evtchn_port_t port, struct pt_regs *regs, void *data)
 {
 	struct consfront_dev *dev = (struct consfront_dev *) data;
 #ifdef HAVE_LIBC
-        int fd = dev ? dev->fd : -1;
+        struct file *file = dev ? get_file_from_fd(dev->fd) : NULL;
 
-        if (fd != -1)
-            files[fd].read = true;
+        if ( file )
+            file->read = true;
 
         wake_up(&console_queue);
 #else
diff --git a/include/console.h b/include/console.h
index 0d7bf07..8c615d0 100644
--- a/include/console.h
+++ b/include/console.h
@@ -39,6 +39,7 @@
 #include <mini-os/os.h>
 #include <mini-os/traps.h>
 #include <mini-os/types.h>
+#include <mini-os/lib.h>
 #include <xen/grant_table.h>
 #include <xenbus.h>
 #include <xen/io/console.h>
@@ -93,5 +94,9 @@ int xencons_ring_send_no_notify(struct consfront_dev *dev, const char *data, uns
 int xencons_ring_avail(struct consfront_dev *dev);
 int xencons_ring_recv(struct consfront_dev *dev, char *data, unsigned len);
 void free_consfront(struct consfront_dev *dev);
+#ifdef HAVE_LIBC
+extern struct file_ops console_ops;
+int open_consfront(char *nodename);
+#endif
 
 #endif /* _LIB_CONSOLE_H_ */
diff --git a/include/lib.h b/include/lib.h
index 653a16e..c171fe8 100644
--- a/include/lib.h
+++ b/include/lib.h
@@ -160,8 +160,7 @@ void sanity_check(void);
 #define FTYPE_FILE       2
 #define FTYPE_SOCKET     3
 #define FTYPE_MEM        4
-#define FTYPE_SAVEFILE   5
-#define FTYPE_N          6
+#define FTYPE_N          5
 #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 4fb844f..3a8aa68 100644
--- a/lib/sys.c
+++ b/lib/sys.c
@@ -101,6 +101,9 @@ static struct file_ops file_ops_none = {
 
 static struct file_ops *file_ops[FTYPE_N + FTYPE_SPARE] = {
     [FTYPE_NONE] = &file_ops_none,
+#ifdef CONFIG_CONSFRONT
+    [FTYPE_CONSOLE] = &console_ops,
+#endif
 };
 
 unsigned int alloc_file_type(struct file_ops *ops)
@@ -211,31 +214,26 @@ int mkdir(const char *pathname, mode_t mode)
 #ifdef CONFIG_CONSFRONT
 int posix_openpt(int flags)
 {
-    struct consfront_dev *dev;
+    int fd;
 
     /* Ignore flags */
+    fd = open_consfront(NULL);
+    printk("fd(%d) = posix_openpt\n", fd);
 
-    dev = init_consfront(NULL);
-    dev->fd = alloc_fd(FTYPE_CONSOLE);
-    files[dev->fd].dev = dev;
-
-    printk("fd(%d) = posix_openpt\n", dev->fd);
-    return(dev->fd);
+    return fd;
 }
 
 int open_savefile(const char *path, int save)
 {
-    struct consfront_dev *dev;
+    int fd;
     char nodename[64];
 
     snprintf(nodename, sizeof(nodename), "device/console/%d", save ? SAVE_CONSOLE : RESTORE_CONSOLE);
 
-    dev = init_consfront(nodename);
-    dev->fd = alloc_fd(FTYPE_SAVEFILE);
-    files[dev->fd].dev = dev;
+    fd = open_consfront(nodename);
+    printk("fd(%d) = open_savefile\n", fd);
 
-    printk("fd(%d) = open_savefile\n", dev->fd);
-    return(dev->fd);
+    return fd;
 }
 #else
 int posix_openpt(int flags)
@@ -285,20 +283,6 @@ int read(int fd, void *buf, size_t nbytes)
         return ops->read(fd, buf, nbytes);
 
     switch (files[fd].type) {
-        case FTYPE_SAVEFILE:
-	case FTYPE_CONSOLE: {
-	    int ret;
-            DEFINE_WAIT(w);
-            while(1) {
-                add_waiter(w, console_queue);
-                ret = xencons_ring_recv(files[fd].dev, buf, nbytes);
-                if (ret)
-                    break;
-                schedule();
-            }
-            remove_waiter(w, console_queue);
-            return ret;
-        }
 #ifdef HAVE_LWIP
 	case FTYPE_SOCKET:
 	    return lwip_read(files[fd].fd, buf, nbytes);
@@ -319,18 +303,6 @@ int write(int fd, const void *buf, size_t nbytes)
         return ops->write(fd, buf, nbytes);
 
     switch (files[fd].type) {
-        case FTYPE_SAVEFILE: {
-                int ret = 0, tot = nbytes;
-                while (nbytes > 0) {
-                    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].dev, (char *)buf, nbytes);
-	    return nbytes;
 #ifdef HAVE_LWIP
 	case FTYPE_SOCKET:
 	    return lwip_write(files[fd].fd, (void*) buf, nbytes);
@@ -418,12 +390,6 @@ int close(int fd)
 	case FTYPE_SOCKET:
 	    res = lwip_close(files[fd].fd);
             break;
-#endif
-#ifdef CONFIG_CONSFRONT
-        case FTYPE_SAVEFILE:
-        case FTYPE_CONSOLE:
-            fini_consfront(files[fd].dev);
-            break;
 #endif
 	case FTYPE_NONE:
             printk("close(%d): Bad descriptor\n", fd);
@@ -464,15 +430,8 @@ int fstat(int fd, struct stat *buf)
         return ops->fstat(fd, buf);
 
     switch (files[fd].type) {
-	case FTYPE_SAVEFILE:
-	case FTYPE_CONSOLE:
 	case FTYPE_SOCKET: {
-            if (files[fd].type == FTYPE_CONSOLE)
-                buf->st_mode = S_IFCHR|S_IRUSR|S_IWUSR;
-            else if (files[fd].type == FTYPE_SOCKET)
-                buf->st_mode = S_IFSOCK|S_IRUSR|S_IWUSR;
-            else if (files[fd].type == FTYPE_SAVEFILE)
-                buf->st_mode = S_IFREG|S_IRUSR|S_IWUSR;
+            buf->st_mode = S_IFSOCK|S_IRUSR|S_IWUSR;
 	    buf->st_uid = 0;
 	    buf->st_gid = 0;
 	    buf->st_size = 0;
@@ -583,7 +542,6 @@ int closedir(DIR *dir)
 #if defined(LIBC_DEBUG) || defined(LIBC_VERBOSE)
 static const char *file_types[] = {
     [FTYPE_NONE]    = "none",
-    [FTYPE_CONSOLE] = "console",
     [FTYPE_SOCKET]  = "socket",
 };
 
@@ -744,21 +702,18 @@ static int select_poll(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exce
             }
 
 	    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))
-		    n++;
-		else
-		    FD_CLR(i, readfds);
+                if ( FD_ISSET(i, readfds) )
+                    FD_CLR(i, readfds);
+                if ( FD_ISSET(i, writefds) )
+                    FD_CLR(i, writefds);
+                if ( FD_ISSET(i, exceptfds) )
+                    FD_CLR(i, exceptfds);
             }
-	    if (FD_ISSET(i, writefds))
-                n++;
-	    FD_CLR(i, exceptfds);
 	    break;
+        }
+
 #ifdef HAVE_LWIP
 	case FTYPE_SOCKET:
 	    if (FD_ISSET(i, readfds)) {
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH v2 10/12] mini-os: add struct file_ops for file type socket
  2022-01-11 15:12 [PATCH v2 00/12] mini-os: remove device specific struct file members Juergen Gross
                   ` (8 preceding siblings ...)
  2022-01-11 15:12 ` [PATCH v2 09/12] mini-os: use file_ops and get_file_from_fd() for console Juergen Gross
@ 2022-01-11 15:12 ` Juergen Gross
  2022-01-11 20:38   ` Samuel Thibault
                     ` (3 more replies)
  2022-01-11 15:12 ` [PATCH v2 11/12] mini-os: add struct file_ops for FTYPE_FILE Juergen Gross
  2022-01-11 15:12 ` [PATCH v2 12/12] mini-os: make files array private to sys.c Juergen Gross
  11 siblings, 4 replies; 48+ messages in thread
From: Juergen Gross @ 2022-01-11 15:12 UTC (permalink / raw)
  To: minios-devel, xen-devel; +Cc: samuel.thibault, wl, Juergen Gross

Even with some special handling needed in select_poll(), add a struct
file_ops for FTYPE_SOCKET. Due to the need of the special handling it
isn't possible to use a dynamically allocated file type.

Most functions calling the file_ops methods can be simplified a lot now
that no file type specific handling is left. Same applies to the file
type name printing in debug/verbose mode.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 lib/sys.c | 153 +++++++++++++++++++++++++-----------------------------
 1 file changed, 70 insertions(+), 83 deletions(-)

diff --git a/lib/sys.c b/lib/sys.c
index 3a8aa68..12deaed 100644
--- a/lib/sys.c
+++ b/lib/sys.c
@@ -99,11 +99,70 @@ static struct file_ops file_ops_none = {
     .name = "none",
 };
 
+#ifdef HAVE_LWIP
+static int socket_read(int fd, void *buf, size_t nbytes)
+{
+    return lwip_read(fd, buf, nbytes);
+}
+
+static int socket_write(int fd, const void *buf, size_t nbytes)
+{
+    return lwip_write(fd, (void *)buf, nbytes);
+}
+
+static int close_socket_fd(int fd)
+{
+    struct file *file = get_file_from_fd(fd);
+
+    return lwip_close(file->fd);
+}
+
+static int socket_fstat(int fd, struct stat *buf)
+{
+    buf->st_mode = S_IFSOCK | S_IRUSR | S_IWUSR;
+    buf->st_atime = buf->st_mtime = buf->st_ctime = time(NULL);
+
+    return 0;
+}
+
+static int socket_fcntl(int fd, int cmd, va_list args)
+{
+    long arg;
+    struct file *file = get_file_from_fd(fd);
+
+    arg = va_arg(args, long);
+
+    if ( cmd == F_SETFL && !(arg & ~O_NONBLOCK) )
+    {
+        /* Only flag supported: non-blocking mode */
+        uint32_t nblock = !!(arg & O_NONBLOCK);
+
+        return lwip_ioctl(file->fd, FIONBIO, &nblock);
+    }
+
+    printk("fcntl(%d, %d, %lx/%lo)\n", fd, cmd, arg, arg);
+    errno = ENOSYS;
+    return -1;
+}
+
+static struct file_ops socket_ops = {
+    .name = "socket",
+    .read = socket_read,
+    .write = socket_write,
+    .close = close_socket_fd,
+    .fstat = socket_fstat,
+    .fcntl = socket_fcntl,
+};
+#endif
+
 static struct file_ops *file_ops[FTYPE_N + FTYPE_SPARE] = {
     [FTYPE_NONE] = &file_ops_none,
 #ifdef CONFIG_CONSFRONT
     [FTYPE_CONSOLE] = &console_ops,
 #endif
+#ifdef HAVE_LWIP
+    [FTYPE_SOCKET] = &socket_ops,
+#endif
 };
 
 unsigned int alloc_file_type(struct file_ops *ops)
@@ -282,14 +341,6 @@ int read(int fd, void *buf, size_t nbytes)
     if ( ops->read )
         return ops->read(fd, buf, nbytes);
 
-    switch (files[fd].type) {
-#ifdef HAVE_LWIP
-	case FTYPE_SOCKET:
-	    return lwip_read(files[fd].fd, buf, nbytes);
-#endif
-	default:
-	    break;
-    }
     printk("read(%d): Bad descriptor\n", fd);
     errno = EBADF;
     return -1;
@@ -302,14 +353,6 @@ int write(int fd, const void *buf, size_t nbytes)
     if ( ops->write )
         return ops->write(fd, buf, nbytes);
 
-    switch (files[fd].type) {
-#ifdef HAVE_LWIP
-	case FTYPE_SOCKET:
-	    return lwip_write(files[fd].fd, (void*) buf, nbytes);
-#endif
-	default:
-	    break;
-    }
     printk("write(%d): Bad descriptor\n", fd);
     errno = EBADF;
     return -1;
@@ -378,26 +421,14 @@ int close(int fd)
 
     printk("close(%d)\n", fd);
     if ( ops->close )
-    {
         res = ops->close(fd);
-        goto out;
-    }
-
-    switch (files[fd].type) {
-        default:
-            break;
-#ifdef HAVE_LWIP
-	case FTYPE_SOCKET:
-	    res = lwip_close(files[fd].fd);
-            break;
-#endif
-	case FTYPE_NONE:
-            printk("close(%d): Bad descriptor\n", fd);
-            errno = EBADF;
-            return -1;
+    else if ( files[fd].type == FTYPE_NONE )
+    {
+        printk("close(%d): Bad descriptor\n", fd);
+        errno = EBADF;
+        return -1;
     }
 
- out:
     memset(files + fd, 0, sizeof(struct file));
     files[fd].type = FTYPE_NONE;
     return res;
@@ -429,21 +460,6 @@ int fstat(int fd, struct stat *buf)
     if ( ops->fstat )
         return ops->fstat(fd, buf);
 
-    switch (files[fd].type) {
-	case FTYPE_SOCKET: {
-            buf->st_mode = S_IFSOCK|S_IRUSR|S_IWUSR;
-	    buf->st_uid = 0;
-	    buf->st_gid = 0;
-	    buf->st_size = 0;
-	    buf->st_atime = 
-	    buf->st_mtime = 
-	    buf->st_ctime = time(NULL);
-	    return 0;
-	}
-	default:
-	    break;
-    }
-
     printk("statf(%d): Bad descriptor\n", fd);
     errno = EBADF;
     return -1;
@@ -491,21 +507,9 @@ int fcntl(int fd, int cmd, ...)
     arg = va_arg(ap, long);
     va_end(ap);
 
-    switch (cmd) {
-#ifdef HAVE_LWIP
-	case F_SETFL:
-	    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].fd, FIONBIO, &nblock);
-	    }
-	    /* Fallthrough */
-#endif
-	default:
-	    printk("fcntl(%d, %d, %lx/%lo)\n", fd, cmd, arg, arg);
-	    errno = ENOSYS;
-	    return -1;
-    }
+    printk("fcntl(%d, %d, %lx/%lo)\n", fd, cmd, arg, arg);
+    errno = ENOSYS;
+    return -1;
 }
 
 DIR *opendir(const char *name)
@@ -539,23 +543,6 @@ 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]    = "none",
-    [FTYPE_SOCKET]  = "socket",
-};
-
-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)
 {
@@ -566,7 +553,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(%s)", i, get_type_name(files[i].type)); \
+	    printk("%d(%s)", i, get_file_ops(files[i].type)->name); \
 	    comma = 1; \
 	} \
     } \
@@ -600,7 +587,7 @@ static void dump_pollfds(struct pollfd *pfd, int nfds, int timeout)
         fd = pfd[i].fd;
         if (comma)
             printk(", ");
-        printk("%d(%s)/%02x", fd, get_type_name(files[fd].type),
+        printk("%d(%s)/%02x", fd, get_file_ops(files[fd].type)->name,
             pfd[i].events);
             comma = 1;
     }
@@ -754,7 +741,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, get_type_name(files[i].type));
+		printk(" %d(%c):", i, get_file_ops(files[i].type)->name);
 	    if (nbread[i])
 	    	printk(" %dR", nbread[i]);
 	    if (nbwrite[i])
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH v2 11/12] mini-os: add struct file_ops for FTYPE_FILE
  2022-01-11 15:12 [PATCH v2 00/12] mini-os: remove device specific struct file members Juergen Gross
                   ` (9 preceding siblings ...)
  2022-01-11 15:12 ` [PATCH v2 10/12] mini-os: add struct file_ops for file type socket Juergen Gross
@ 2022-01-11 15:12 ` Juergen Gross
  2022-01-11 20:42   ` Samuel Thibault
  2022-01-11 15:12 ` [PATCH v2 12/12] mini-os: make files array private to sys.c Juergen Gross
  11 siblings, 1 reply; 48+ messages in thread
From: Juergen Gross @ 2022-01-11 15:12 UTC (permalink / raw)
  To: minios-devel, xen-devel; +Cc: samuel.thibault, wl, Juergen Gross

FTYPE_FILE is the last relevant file type without a struct file_ops.
Add it.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 lib/sys.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/lib/sys.c b/lib/sys.c
index 12deaed..0f42e97 100644
--- a/lib/sys.c
+++ b/lib/sys.c
@@ -99,6 +99,11 @@ static struct file_ops file_ops_none = {
     .name = "none",
 };
 
+static struct file_ops file_file_ops = {
+    .name = "file",
+    .lseek = lseek_default,
+};
+
 #ifdef HAVE_LWIP
 static int socket_read(int fd, void *buf, size_t nbytes)
 {
@@ -160,6 +165,7 @@ static struct file_ops *file_ops[FTYPE_N + FTYPE_SPARE] = {
 #ifdef CONFIG_CONSFRONT
     [FTYPE_CONSOLE] = &console_ops,
 #endif
+    [FTYPE_FILE] = &file_file_ops,
 #ifdef HAVE_LWIP
     [FTYPE_SOCKET] = &socket_ops,
 #endif
@@ -397,16 +403,9 @@ off_t lseek(int fd, off_t offset, int whence)
     if ( ops->lseek )
         return ops->lseek(fd, offset, whence);
 
-    switch(files[fd].type) {
-       case FTYPE_FILE:
-          break;
-       default:
-          /* Not implemented for this filetype */
-          errno = ESPIPE;
-          return (off_t) -1;
-    }
-
-    return lseek_default(fd, offset, whence);
+    /* Not implemented for this filetype */
+    errno = ESPIPE;
+    return (off_t) -1;
 }
 
 int fsync(int fd) {
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH v2 12/12] mini-os: make files array private to sys.c
  2022-01-11 15:12 [PATCH v2 00/12] mini-os: remove device specific struct file members Juergen Gross
                   ` (10 preceding siblings ...)
  2022-01-11 15:12 ` [PATCH v2 11/12] mini-os: add struct file_ops for FTYPE_FILE Juergen Gross
@ 2022-01-11 15:12 ` Juergen Gross
  2022-01-11 20:42   ` Samuel Thibault
  11 siblings, 1 reply; 48+ messages in thread
From: Juergen Gross @ 2022-01-11 15:12 UTC (permalink / raw)
  To: minios-devel, xen-devel; +Cc: samuel.thibault, wl, Juergen Gross

There is no user of the files[] array outside of lib/sys.c left, so
it can be made static.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 include/lib.h | 2 --
 lib/sys.c     | 2 +-
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/include/lib.h b/include/lib.h
index c171fe8..80e804b 100644
--- a/include/lib.h
+++ b/include/lib.h
@@ -199,8 +199,6 @@ struct file {
     };
 };
 
-extern struct file files[];
-
 struct file *get_file_from_fd(int fd);
 int alloc_fd(unsigned int type);
 void close_all_files(void);
diff --git a/lib/sys.c b/lib/sys.c
index 0f42e97..c0cad87 100644
--- a/lib/sys.c
+++ b/lib/sys.c
@@ -89,7 +89,7 @@ extern void minios_evtchn_close_fd(int fd);
 extern void minios_gnttab_close_fd(int fd);
 
 pthread_mutex_t fd_lock = PTHREAD_MUTEX_INITIALIZER;
-struct file files[NOFILE] = {
+static struct file files[NOFILE] = {
     { .type = FTYPE_CONSOLE }, /* stdin */
     { .type = FTYPE_CONSOLE }, /* stdout */
     { .type = FTYPE_CONSOLE }, /* stderr */
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 01/12] mini-os: remove event channel specific struct file definitions
  2022-01-11 15:12 ` [PATCH v2 01/12] mini-os: remove event channel specific struct file definitions Juergen Gross
@ 2022-01-11 19:50   ` Samuel Thibault
  0 siblings, 0 replies; 48+ messages in thread
From: Samuel Thibault @ 2022-01-11 19:50 UTC (permalink / raw)
  To: Juergen Gross; +Cc: minios-devel, xen-devel, wl

Juergen Gross, le mar. 11 janv. 2022 16:12:04 +0100, a ecrit:
> The event channel specific union member in struct file is no longer
> needed, so remove it together with the associated structure
> definitions.
> 
> The event channel file type and its associated handling can be removed,
> too, as libxenevtchn is now supplying a struct file_ops via a call of
> alloc_file_type().
> 
> This removes all contents of CONFIG_LIBXENEVTCHN guarded sections, so
> this config option can be removed.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

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

> ---
>  Config.mk                     |  1 -
>  arch/x86/testbuild/all-no     |  1 -
>  arch/x86/testbuild/all-yes    |  1 -
>  arch/x86/testbuild/newxen-yes |  1 -
>  include/lib.h                 | 15 +--------------
>  lib/sys.c                     |  7 -------
>  6 files changed, 1 insertion(+), 25 deletions(-)
> 
> diff --git a/Config.mk b/Config.mk
> index 1e08388..c244adc 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_LIBXENEVTCHN
>  CONFIG-$(CONFIG_XC) += CONFIG_LIBXENGNTTAB
>  
>  CONFIG-$(lwip) += CONFIG_LWIP
> diff --git a/arch/x86/testbuild/all-no b/arch/x86/testbuild/all-no
> index d6fc260..202c317 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_LIBXENEVTCHN = n
>  CONFIG_LIBXENGNTTAB = n
>  CONFIG_LWIP = n
>  CONFIG_BALLOON = n
> diff --git a/arch/x86/testbuild/all-yes b/arch/x86/testbuild/all-yes
> index 98bbfeb..eb495a4 100644
> --- a/arch/x86/testbuild/all-yes
> +++ b/arch/x86/testbuild/all-yes
> @@ -16,6 +16,5 @@ CONFIG_XENBUS = y
>  CONFIG_BALLOON = y
>  CONFIG_USE_XEN_CONSOLE = y
>  # The following are special: they need support from outside
> -CONFIG_LIBXENEVTCHN = n
>  CONFIG_LIBXENGNTTAB = n
>  CONFIG_LWIP = n
> diff --git a/arch/x86/testbuild/newxen-yes b/arch/x86/testbuild/newxen-yes
> index 0603200..bf25ace 100644
> --- a/arch/x86/testbuild/newxen-yes
> +++ b/arch/x86/testbuild/newxen-yes
> @@ -17,6 +17,5 @@ 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_LIBXENEVTCHN = n
>  CONFIG_LIBXENGNTTAB = n
>  CONFIG_LWIP = n
> diff --git a/include/lib.h b/include/lib.h
> index 4892320..df972ef 100644
> --- a/include/lib.h
> +++ b/include/lib.h
> @@ -169,8 +169,7 @@ void sanity_check(void);
>  #define FTYPE_TPM_TIS   11
>  #define FTYPE_XENBUS    12
>  #define FTYPE_GNTMAP    13
> -#define FTYPE_EVTCHN    14
> -#define FTYPE_N         15
> +#define FTYPE_N         14
>  #define FTYPE_SPARE     16
>  
>  typedef int file_read_func(int fd, void *buf, size_t nbytes);
> @@ -199,15 +198,6 @@ 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 {
> -        LIST_ENTRY(evtchn_port_info) list;
> -        evtchn_port_t port;
> -        unsigned long pending;
> -        int bound;
> -};
> -
>  struct file {
>      unsigned int type;
>      bool read;	/* maybe available for read */
> @@ -215,9 +205,6 @@ struct file {
>      union {
>          int fd; /* Any fd from an upper layer. */
>          void *dev;
> -	struct {
> -	    struct evtchn_port_list ports;
> -	} evtchn;
>  	struct gntmap gntmap;
>      };
>  };
> diff --git a/lib/sys.c b/lib/sys.c
> index 52876e0..8fa1fee 100644
> --- a/lib/sys.c
> +++ b/lib/sys.c
> @@ -505,11 +505,6 @@ int close(int fd)
>  	    res = lwip_close(files[fd].fd);
>              break;
>  #endif
> -#ifdef CONFIG_LIBXENEVTCHN
> -	case FTYPE_EVTCHN:
> -	    minios_evtchn_close_fd(fd);
> -            break;
> -#endif
>  #ifdef CONFIG_LIBXENGNTTAB
>  	case FTYPE_GNTMAP:
>  	    minios_gnttab_close_fd(fd);
> @@ -723,7 +718,6 @@ static const char *file_types[] = {
>      [FTYPE_NONE]    = "none",
>      [FTYPE_CONSOLE] = "console",
>      [FTYPE_XENBUS]  = "xenbus",
> -    [FTYPE_EVTCHN]  = "evtchn",
>      [FTYPE_SOCKET]  = "socket",
>      [FTYPE_TAP]     = "net",
>      [FTYPE_BLK]     = "blk",
> @@ -915,7 +909,6 @@ static int select_poll(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exce
>  	    FD_CLR(i, exceptfds);
>  	    break;
>  #endif
> -	case FTYPE_EVTCHN:
>  	case FTYPE_TAP:
>  	case FTYPE_BLK:
>  	case FTYPE_KBD:
> -- 
> 2.26.2
> 

-- 
Samuel
<h> t: bah c'est tendre le pattern pour se faire matcher, hein


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 02/12] mini-os: remove gnttab specific member from struct file
  2022-01-11 15:12 ` [PATCH v2 02/12] mini-os: remove gnttab specific member from struct file Juergen Gross
@ 2022-01-11 19:55   ` Samuel Thibault
  2022-01-11 20:12   ` Andrew Cooper
  1 sibling, 0 replies; 48+ messages in thread
From: Samuel Thibault @ 2022-01-11 19:55 UTC (permalink / raw)
  To: Juergen Gross; +Cc: minios-devel, xen-devel, wl

Juergen Gross, le mar. 11 janv. 2022 16:12:05 +0100, a ecrit:
> The event channel specific union member in struct file is no longer
> needed, so remove it.
> 
> The gnttab file type and its associated handling can be removed, too,
> as libxengnttab is now supplying a struct file_ops via a call of
> alloc_file_type().
> 
> This removes all contents of CONFIG_LIBXENGNTTAB guarded sections, so
> this config option can be removed.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

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

> ---
>  Config.mk                     | 1 -
>  arch/x86/testbuild/all-no     | 1 -
>  arch/x86/testbuild/all-yes    | 1 -
>  arch/x86/testbuild/newxen-yes | 1 -
>  gntmap.c                      | 2 +-
>  include/lib.h                 | 4 +---
>  lib/sys.c                     | 5 -----
>  7 files changed, 2 insertions(+), 13 deletions(-)
> 
> diff --git a/Config.mk b/Config.mk
> index c244adc..eb84515 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_LIBXENGNTTAB
>  
>  CONFIG-$(lwip) += CONFIG_LWIP
>  
> diff --git a/arch/x86/testbuild/all-no b/arch/x86/testbuild/all-no
> index 202c317..c429354 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_LIBXENGNTTAB = n
>  CONFIG_LWIP = n
>  CONFIG_BALLOON = n
>  CONFIG_USE_XEN_CONSOLE = n
> diff --git a/arch/x86/testbuild/all-yes b/arch/x86/testbuild/all-yes
> index eb495a4..6c6096b 100644
> --- a/arch/x86/testbuild/all-yes
> +++ b/arch/x86/testbuild/all-yes
> @@ -16,5 +16,4 @@ CONFIG_XENBUS = y
>  CONFIG_BALLOON = y
>  CONFIG_USE_XEN_CONSOLE = y
>  # The following are special: they need support from outside
> -CONFIG_LIBXENGNTTAB = n
>  CONFIG_LWIP = n
> diff --git a/arch/x86/testbuild/newxen-yes b/arch/x86/testbuild/newxen-yes
> index bf25ace..88094fc 100644
> --- a/arch/x86/testbuild/newxen-yes
> +++ b/arch/x86/testbuild/newxen-yes
> @@ -17,5 +17,4 @@ 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_LIBXENGNTTAB = n
>  CONFIG_LWIP = n
> diff --git a/gntmap.c b/gntmap.c
> index 6fa1dac..3422ab2 100644
> --- a/gntmap.c
> +++ b/gntmap.c
> @@ -3,7 +3,7 @@
>   *
>   * Diego Ongaro <diego.ongaro@citrix.com>, July 2008
>   *
> - * Files of type FTYPE_GNTMAP contain a gntmap, which is an array of
> + * Files of libxengnttab contain a gntmap, which is an array of
>   * (host address, grant handle) pairs. Grant handles come from a hypervisor map
>   * operation and are needed for the corresponding unmap.
>   *
> diff --git a/include/lib.h b/include/lib.h
> index df972ef..283abb8 100644
> --- a/include/lib.h
> +++ b/include/lib.h
> @@ -168,8 +168,7 @@ void sanity_check(void);
>  #define FTYPE_TPMFRONT  10
>  #define FTYPE_TPM_TIS   11
>  #define FTYPE_XENBUS    12
> -#define FTYPE_GNTMAP    13
> -#define FTYPE_N         14
> +#define FTYPE_N         13
>  #define FTYPE_SPARE     16
>  
>  typedef int file_read_func(int fd, void *buf, size_t nbytes);
> @@ -205,7 +204,6 @@ struct file {
>      union {
>          int fd; /* Any fd from an upper layer. */
>          void *dev;
> -	struct gntmap gntmap;
>      };
>  };
>  
> diff --git a/lib/sys.c b/lib/sys.c
> index 8fa1fee..9540410 100644
> --- a/lib/sys.c
> +++ b/lib/sys.c
> @@ -505,11 +505,6 @@ int close(int fd)
>  	    res = lwip_close(files[fd].fd);
>              break;
>  #endif
> -#ifdef CONFIG_LIBXENGNTTAB
> -	case FTYPE_GNTMAP:
> -	    minios_gnttab_close_fd(fd);
> -            break;
> -#endif
>  #ifdef CONFIG_NETFRONT
>  	case FTYPE_TAP:
>  	    shutdown_netfront(files[fd].dev);
> -- 
> 2.26.2
> 

-- 
Samuel
Tu as lu les docs. Tu es devenu un informaticien. Que tu le veuilles
ou non. Lire la doc, c'est le Premier et Unique Commandement de
l'informaticien.
-+- TP in: Guide du Linuxien pervers - "L'évangile selon St Thomas"


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 03/12] mini-os: use alloc_file_type() and get_file_from_fd() in xs
  2022-01-11 15:12 ` [PATCH v2 03/12] mini-os: use alloc_file_type() and get_file_from_fd() in xs Juergen Gross
@ 2022-01-11 20:06   ` Samuel Thibault
  2022-01-11 20:11     ` Samuel Thibault
  2022-01-11 20:21   ` Andrew Cooper
  1 sibling, 1 reply; 48+ messages in thread
From: Samuel Thibault @ 2022-01-11 20:06 UTC (permalink / raw)
  To: Juergen Gross; +Cc: minios-devel, xen-devel, wl

Juergen Gross, le mar. 11 janv. 2022 16:12:06 +0100, a ecrit:
> Allocate the needed file type via alloc_file_type().
> 
> Instead of directly accessing the files[] array use get_file_from_fd().
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

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

> ---
>  include/lib.h   |  3 +--
>  lib/sys.c       | 18 --------------
>  lib/xs.c        | 64 +++++++++++++++++++++++++++++++++++++------------
>  xenbus/xenbus.c |  1 +
>  4 files changed, 51 insertions(+), 35 deletions(-)
> 
> diff --git a/include/lib.h b/include/lib.h
> index 283abb8..05c7de5 100644
> --- a/include/lib.h
> +++ b/include/lib.h
> @@ -167,8 +167,7 @@ void sanity_check(void);
>  #define FTYPE_BLK        9
>  #define FTYPE_TPMFRONT  10
>  #define FTYPE_TPM_TIS   11
> -#define FTYPE_XENBUS    12
> -#define FTYPE_N         13
> +#define FTYPE_N         12
>  #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 9540410..d213ae5 100644
> --- a/lib/sys.c
> +++ b/lib/sys.c
> @@ -495,11 +495,6 @@ int close(int fd)
>      switch (files[fd].type) {
>          default:
>              break;
> -#ifdef CONFIG_XENBUS
> -	case FTYPE_XENBUS:
> -            xs_daemon_close((void*)(intptr_t) fd);
> -            break;
> -#endif
>  #ifdef HAVE_LWIP
>  	case FTYPE_SOCKET:
>  	    res = lwip_close(files[fd].fd);
> @@ -712,7 +707,6 @@ int closedir(DIR *dir)
>  static const char *file_types[] = {
>      [FTYPE_NONE]    = "none",
>      [FTYPE_CONSOLE] = "console",
> -    [FTYPE_XENBUS]  = "xenbus",
>      [FTYPE_SOCKET]  = "socket",
>      [FTYPE_TAP]     = "net",
>      [FTYPE_BLK]     = "blk",
> @@ -892,18 +886,6 @@ static int select_poll(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exce
>                  n++;
>  	    FD_CLR(i, exceptfds);
>  	    break;
> -#ifdef CONFIG_XENBUS
> -	case FTYPE_XENBUS:
> -	    if (FD_ISSET(i, readfds)) {
> -                if (files[i].dev)
> -		    n++;
> -		else
> -		    FD_CLR(i, readfds);
> -	    }
> -	    FD_CLR(i, writefds);
> -	    FD_CLR(i, exceptfds);
> -	    break;
> -#endif
>  	case FTYPE_TAP:
>  	case FTYPE_BLK:
>  	case FTYPE_KBD:
> diff --git a/lib/xs.c b/lib/xs.c
> index 4af0f96..ac830d2 100644
> --- a/lib/xs.c
> +++ b/lib/xs.c
> @@ -18,23 +18,55 @@ static inline int _xs_fileno(struct xs_handle *h) {
>      return (intptr_t) h;
>  }
>  
> +static int xs_close_fd(int fd)
> +{
> +    struct xenbus_event *event, *next;
> +    struct file *file = get_file_from_fd(fd);
> +
> +    for (event = file->dev; event; event = next)
> +    {
> +        next = event->next;
> +        free(event);
> +    }
> +
> +    return 0;
> +}
> +
> +static bool xs_can_read(int fd)
> +{
> +    struct file *file = get_file_from_fd(fd);
> +
> +    return file && file->dev;
> +}
> +
> +static struct file_ops xenbus_ops = {
> +    .name = "xenbus",
> +    .close = xs_close_fd,
> +    .select_rd = xs_can_read,
> +};
> +
>  struct xs_handle *xs_daemon_open()
>  {
> -    int fd = alloc_fd(FTYPE_XENBUS);
> -    files[fd].dev = NULL;
> -    printk("xs_daemon_open -> %d, %p\n", fd, &files[fd].dev);
> +    int fd;
> +    struct file *file;
> +    static unsigned int ftype_xenbus;
> +
> +    if ( !ftype_xenbus )
> +        ftype_xenbus = alloc_file_type(&xenbus_ops);
> +
> +    fd = alloc_fd(ftype_xenbus);
> +    file = get_file_from_fd(fd);
> +    if ( !file )
> +        return NULL;
> +
> +    file->dev = NULL;
> +    printk("xs_daemon_open -> %d, %p\n", fd, &file->dev);
>      return (void*)(intptr_t) fd;
>  }
>  
>  void xs_daemon_close(struct xs_handle *h)
>  {
> -    int fd = _xs_fileno(h);
> -    struct xenbus_event *event, *next;
> -    for (event = files[fd].dev; event; event = next)
> -    {
> -        next = event->next;
> -        free(event);
> -    }
> +    close(_xs_fileno(h));
>  }
>  
>  int xs_fileno(struct xs_handle *h)
> @@ -169,18 +201,20 @@ char **xs_directory(struct xs_handle *h, xs_transaction_t t,
>  
>  bool xs_watch(struct xs_handle *h, const char *path, const char *token)
>  {
> -    int fd = _xs_fileno(h);
> +    struct file *file = get_file_from_fd(_xs_fileno(h));
> +
>      printk("xs_watch(%s, %s)\n", path, token);
>      return xs_bool(xenbus_watch_path_token(XBT_NULL, path, token,
> -                   (xenbus_event_queue *)&files[fd].dev));
> +                   (xenbus_event_queue *)&file->dev));
>  }
>  
>  char **xs_read_watch(struct xs_handle *h, unsigned int *num)
>  {
> -    int fd = _xs_fileno(h);
>      struct xenbus_event *event;
> -    event = files[fd].dev;
> -    files[fd].dev = event->next;
> +    struct file *file = get_file_from_fd(_xs_fileno(h));
> +
> +    event = file->dev;
> +    file->dev = event->next;
>      printk("xs_read_watch() -> %s %s\n", event->path, event->token);
>      *num = 2;
>      return (char **) &event->path;
> diff --git a/xenbus/xenbus.c b/xenbus/xenbus.c
> index b687678..785389f 100644
> --- a/xenbus/xenbus.c
> +++ b/xenbus/xenbus.c
> @@ -393,6 +393,7 @@ static int allocate_xenbus_id(void)
>  void init_xenbus(void)
>  {
>      int err;
> +
>      DEBUG("init_xenbus called.\n");
>      create_thread("xenstore", xenbus_thread_func, NULL);
>      DEBUG("buf at %p.\n", xenstore_buf);
> -- 
> 2.26.2
> 

-- 
Samuel
<y> muhahaha...
<y> ya un train qui part de Perrache à 14h57
<y> qui passe à Part-Dieu à 15h10
<y> si je le prends à Perrache, je suis en zone bleue
<y> si je le prends à Part-Dieu, je suis en zone blanche
<y> donc je vais le prendre à Perrache *mais* à Part-Dieu ;-)
 -+- #ens-mim - vive la SNCF -+-


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 03/12] mini-os: use alloc_file_type() and get_file_from_fd() in xs
  2022-01-11 20:06   ` Samuel Thibault
@ 2022-01-11 20:11     ` Samuel Thibault
  2022-01-11 20:14       ` Andrew Cooper
  0 siblings, 1 reply; 48+ messages in thread
From: Samuel Thibault @ 2022-01-11 20:11 UTC (permalink / raw)
  To: Juergen Gross, minios-devel, xen-devel, wl

Samuel Thibault, le mar. 11 janv. 2022 21:06:15 +0100, a ecrit:
> Juergen Gross, le mar. 11 janv. 2022 16:12:06 +0100, a ecrit:
> > Allocate the needed file type via alloc_file_type().
> > 
> > Instead of directly accessing the files[] array use get_file_from_fd().
> > 
> > Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> 
> > ---
> >  include/lib.h   |  3 +--
> >  lib/sys.c       | 18 --------------
> >  lib/xs.c        | 64 +++++++++++++++++++++++++++++++++++++------------
> >  xenbus/xenbus.c |  1 +
> >  4 files changed, 51 insertions(+), 35 deletions(-)
> > 
> > diff --git a/include/lib.h b/include/lib.h
> > index 283abb8..05c7de5 100644
> > --- a/include/lib.h
> > +++ b/include/lib.h
> > @@ -167,8 +167,7 @@ void sanity_check(void);
> >  #define FTYPE_BLK        9
> >  #define FTYPE_TPMFRONT  10
> >  #define FTYPE_TPM_TIS   11
> > -#define FTYPE_XENBUS    12
> > -#define FTYPE_N         13
> > +#define FTYPE_N         12
> >  #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 9540410..d213ae5 100644
> > --- a/lib/sys.c
> > +++ b/lib/sys.c
> > @@ -495,11 +495,6 @@ int close(int fd)
> >      switch (files[fd].type) {
> >          default:
> >              break;
> > -#ifdef CONFIG_XENBUS
> > -	case FTYPE_XENBUS:
> > -            xs_daemon_close((void*)(intptr_t) fd);
> > -            break;
> > -#endif
> >  #ifdef HAVE_LWIP
> >  	case FTYPE_SOCKET:
> >  	    res = lwip_close(files[fd].fd);
> > @@ -712,7 +707,6 @@ int closedir(DIR *dir)
> >  static const char *file_types[] = {
> >      [FTYPE_NONE]    = "none",
> >      [FTYPE_CONSOLE] = "console",
> > -    [FTYPE_XENBUS]  = "xenbus",
> >      [FTYPE_SOCKET]  = "socket",
> >      [FTYPE_TAP]     = "net",
> >      [FTYPE_BLK]     = "blk",
> > @@ -892,18 +886,6 @@ static int select_poll(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exce
> >                  n++;
> >  	    FD_CLR(i, exceptfds);
> >  	    break;
> > -#ifdef CONFIG_XENBUS
> > -	case FTYPE_XENBUS:
> > -	    if (FD_ISSET(i, readfds)) {
> > -                if (files[i].dev)
> > -		    n++;
> > -		else
> > -		    FD_CLR(i, readfds);
> > -	    }
> > -	    FD_CLR(i, writefds);
> > -	    FD_CLR(i, exceptfds);
> > -	    break;
> > -#endif
> >  	case FTYPE_TAP:
> >  	case FTYPE_BLK:
> >  	case FTYPE_KBD:
> > diff --git a/lib/xs.c b/lib/xs.c
> > index 4af0f96..ac830d2 100644
> > --- a/lib/xs.c
> > +++ b/lib/xs.c
> > @@ -18,23 +18,55 @@ static inline int _xs_fileno(struct xs_handle *h) {
> >      return (intptr_t) h;
> >  }
> >  
> > +static int xs_close_fd(int fd)
> > +{
> > +    struct xenbus_event *event, *next;
> > +    struct file *file = get_file_from_fd(fd);

This...

> > +    for (event = file->dev; event; event = next)
> > +    {
> > +        next = event->next;
> > +        free(event);
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static bool xs_can_read(int fd)
> > +{
> > +    struct file *file = get_file_from_fd(fd);

... and that would be simplified by directly getting the file* from the
parameters.

> > +    return file && file->dev;
> > +}
> > +
> > +static struct file_ops xenbus_ops = {
> > +    .name = "xenbus",
> > +    .close = xs_close_fd,
> > +    .select_rd = xs_can_read,
> > +};
> > +
> >  struct xs_handle *xs_daemon_open()
> >  {
> > -    int fd = alloc_fd(FTYPE_XENBUS);
> > -    files[fd].dev = NULL;
> > -    printk("xs_daemon_open -> %d, %p\n", fd, &files[fd].dev);
> > +    int fd;
> > +    struct file *file;
> > +    static unsigned int ftype_xenbus;
> > +
> > +    if ( !ftype_xenbus )
> > +        ftype_xenbus = alloc_file_type(&xenbus_ops);
> > +
> > +    fd = alloc_fd(ftype_xenbus);
> > +    file = get_file_from_fd(fd);
> > +    if ( !file )
> > +        return NULL;
> > +
> > +    file->dev = NULL;
> > +    printk("xs_daemon_open -> %d, %p\n", fd, &file->dev);
> >      return (void*)(intptr_t) fd;
> >  }
> >  
> >  void xs_daemon_close(struct xs_handle *h)
> >  {
> > -    int fd = _xs_fileno(h);
> > -    struct xenbus_event *event, *next;
> > -    for (event = files[fd].dev; event; event = next)
> > -    {
> > -        next = event->next;
> > -        free(event);
> > -    }
> > +    close(_xs_fileno(h));
> >  }
> >  
> >  int xs_fileno(struct xs_handle *h)
> > @@ -169,18 +201,20 @@ char **xs_directory(struct xs_handle *h, xs_transaction_t t,
> >  
> >  bool xs_watch(struct xs_handle *h, const char *path, const char *token)
> >  {
> > -    int fd = _xs_fileno(h);
> > +    struct file *file = get_file_from_fd(_xs_fileno(h));
> > +
> >      printk("xs_watch(%s, %s)\n", path, token);
> >      return xs_bool(xenbus_watch_path_token(XBT_NULL, path, token,
> > -                   (xenbus_event_queue *)&files[fd].dev));
> > +                   (xenbus_event_queue *)&file->dev));
> >  }
> >  
> >  char **xs_read_watch(struct xs_handle *h, unsigned int *num)
> >  {
> > -    int fd = _xs_fileno(h);
> >      struct xenbus_event *event;
> > -    event = files[fd].dev;
> > -    files[fd].dev = event->next;
> > +    struct file *file = get_file_from_fd(_xs_fileno(h));
> > +
> > +    event = file->dev;
> > +    file->dev = event->next;
> >      printk("xs_read_watch() -> %s %s\n", event->path, event->token);
> >      *num = 2;
> >      return (char **) &event->path;
> > diff --git a/xenbus/xenbus.c b/xenbus/xenbus.c
> > index b687678..785389f 100644
> > --- a/xenbus/xenbus.c
> > +++ b/xenbus/xenbus.c
> > @@ -393,6 +393,7 @@ static int allocate_xenbus_id(void)
> >  void init_xenbus(void)
> >  {
> >      int err;
> > +
> >      DEBUG("init_xenbus called.\n");
> >      create_thread("xenstore", xenbus_thread_func, NULL);
> >      DEBUG("buf at %p.\n", xenstore_buf);
> > -- 
> > 2.26.2
> > 
> 
> -- 
> Samuel
> <y> muhahaha...
> <y> ya un train qui part de Perrache à 14h57
> <y> qui passe à Part-Dieu à 15h10
> <y> si je le prends à Perrache, je suis en zone bleue
> <y> si je le prends à Part-Dieu, je suis en zone blanche
> <y> donc je vais le prendre à Perrache *mais* à Part-Dieu ;-)
>  -+- #ens-mim - vive la SNCF -+-

-- 
Samuel
<c> hiri, le cri ici, c des marrants
<c> j'ai un rep ".uglyhackdirectorywithoutacls" ds mon home
 -+- #ens-mim en stage -+-


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 02/12] mini-os: remove gnttab specific member from struct file
  2022-01-11 15:12 ` [PATCH v2 02/12] mini-os: remove gnttab specific member from struct file Juergen Gross
  2022-01-11 19:55   ` Samuel Thibault
@ 2022-01-11 20:12   ` Andrew Cooper
  2022-01-12  7:44     ` Juergen Gross
  1 sibling, 1 reply; 48+ messages in thread
From: Andrew Cooper @ 2022-01-11 20:12 UTC (permalink / raw)
  To: Juergen Gross, minios-devel, xen-devel; +Cc: samuel.thibault, wl

On 11/01/2022 15:12, Juergen Gross wrote:
> The event channel specific union member in struct file is no longer

Too much copy&paste from the previous patch?

The actual contents look fine.

~Andrew

> needed, so remove it.
>
> The gnttab file type and its associated handling can be removed, too,
> as libxengnttab is now supplying a struct file_ops via a call of
> alloc_file_type().
>
> This removes all contents of CONFIG_LIBXENGNTTAB guarded sections, so
> this config option can be removed.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>



^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 04/12] mini-os: use alloc_file_type() and get_file_from_fd() in tpm_tis
  2022-01-11 15:12 ` [PATCH v2 04/12] mini-os: use alloc_file_type() and get_file_from_fd() in tpm_tis Juergen Gross
@ 2022-01-11 20:13   ` Samuel Thibault
  2022-01-11 20:29   ` Andrew Cooper
  1 sibling, 0 replies; 48+ messages in thread
From: Samuel Thibault @ 2022-01-11 20:13 UTC (permalink / raw)
  To: Juergen Gross; +Cc: minios-devel, xen-devel, wl

Juergen Gross, le mar. 11 janv. 2022 16:12:07 +0100, a ecrit:
> Allocate a file type dynamically via alloc_file_type().
> 
> Instead of directly accessing the files[] array use get_file_from_fd().
> 
> Make some now local functions static and modify their prototypes to
> match the file_ops requirements.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

modulo the int fd / file * thing,

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

> ---
>  include/lib.h     |   3 +-
>  include/tpm_tis.h |   6 ---
>  lib/sys.c         |  23 ---------
>  tpm_tis.c         | 120 ++++++++++++++++++++++++++++++----------------
>  4 files changed, 80 insertions(+), 72 deletions(-)
> 
> diff --git a/include/lib.h b/include/lib.h
> index 05c7de5..d94d142 100644
> --- a/include/lib.h
> +++ b/include/lib.h
> @@ -166,8 +166,7 @@ void sanity_check(void);
>  #define FTYPE_TAP        8
>  #define FTYPE_BLK        9
>  #define FTYPE_TPMFRONT  10
> -#define FTYPE_TPM_TIS   11
> -#define FTYPE_N         12
> +#define FTYPE_N         11
>  #define FTYPE_SPARE     16
>  
>  typedef int file_read_func(int fd, void *buf, size_t nbytes);
> diff --git a/include/tpm_tis.h b/include/tpm_tis.h
> index 86e83f1..2af974d 100644
> --- a/include/tpm_tis.h
> +++ b/include/tpm_tis.h
> @@ -37,14 +37,11 @@ struct tpm_chip;
>  
>  struct tpm_chip* init_tpm_tis(unsigned long baseaddr, int localities, unsigned int irq);
>  struct tpm_chip* init_tpm2_tis(unsigned long baseaddr, int localities, unsigned int irq);
> -void shutdown_tpm_tis(struct tpm_chip* tpm);
>  
>  int tpm_tis_request_locality(struct tpm_chip* tpm, int locality);
>  int tpm_tis_cmd(struct tpm_chip* tpm, uint8_t* req, size_t reqlen, uint8_t** resp, size_t* resplen);
>  
>  #ifdef HAVE_LIBC
> -#include <sys/stat.h>
> -#include <fcntl.h>
>  /* POSIX IO functions:
>   * use tpm_tis_open() to get a file descriptor to the tpm device
>   * use write() on the fd to send a command to the backend. You must
> @@ -53,9 +50,6 @@ int tpm_tis_cmd(struct tpm_chip* tpm, uint8_t* req, size_t reqlen, uint8_t** res
>   * fstat() to get the size of the response and lseek() to seek on it.
>   */
>  int tpm_tis_open(struct tpm_chip* tpm);
> -int tpm_tis_posix_read(int fd, uint8_t* buf, size_t count);
> -int tpm_tis_posix_write(int fd, const uint8_t* buf, size_t count);
> -int tpm_tis_posix_fstat(int fd, struct stat* buf);
>  #endif
>  
>  #endif
> diff --git a/lib/sys.c b/lib/sys.c
> index d213ae5..4060b42 100644
> --- a/lib/sys.c
> +++ b/lib/sys.c
> @@ -29,7 +29,6 @@
>  #include <blkfront.h>
>  #include <fbfront.h>
>  #include <tpmfront.h>
> -#include <tpm_tis.h>
>  #include <xenbus.h>
>  #include <xenstore.h>
>  #include <poll.h>
> @@ -349,11 +348,6 @@ int read(int fd, void *buf, size_t nbytes)
>          case FTYPE_TPMFRONT: {
>  	    return tpmfront_posix_read(fd, buf, nbytes);
>          }
> -#endif
> -#ifdef CONFIG_TPM_TIS
> -        case FTYPE_TPM_TIS: {
> -	    return tpm_tis_posix_read(fd, buf, nbytes);
> -        }
>  #endif
>  	default:
>  	    break;
> @@ -399,10 +393,6 @@ int write(int fd, const void *buf, size_t nbytes)
>  #ifdef CONFIG_TPMFRONT
>  	case FTYPE_TPMFRONT:
>  	    return tpmfront_posix_write(fd, buf, nbytes);
> -#endif
> -#ifdef CONFIG_TPM_TIS
> -	case FTYPE_TPM_TIS:
> -	    return tpm_tis_posix_write(fd, buf, nbytes);
>  #endif
>  	default:
>  	    break;
> @@ -459,10 +449,6 @@ off_t lseek(int fd, off_t offset, int whence)
>  #ifdef CONFIG_TPMFRONT
>         case FTYPE_TPMFRONT:
>            break;
> -#endif
> -#ifdef CONFIG_TPM_TIS
> -       case FTYPE_TPM_TIS:
> -          break;
>  #endif
>         case FTYPE_FILE:
>            break;
> @@ -515,11 +501,6 @@ int close(int fd)
>              shutdown_tpmfront(files[fd].dev);
>              break;
>  #endif
> -#ifdef CONFIG_TPM_TIS
> -	case FTYPE_TPM_TIS:
> -            shutdown_tpm_tis(files[fd].dev);
> -            break;
> -#endif
>  #ifdef CONFIG_KBDFRONT
>  	case FTYPE_KBD:
>              shutdown_kbdfront(files[fd].dev);
> @@ -599,10 +580,6 @@ int fstat(int fd, struct stat *buf)
>  #ifdef CONFIG_TPMFRONT
>  	case FTYPE_TPMFRONT:
>  	   return tpmfront_posix_fstat(fd, buf);
> -#endif
> -#ifdef CONFIG_TPM_TIS
> -	case FTYPE_TPM_TIS:
> -	   return tpm_tis_posix_fstat(fd, buf);
>  #endif
>  	default:
>  	    break;
> diff --git a/tpm_tis.c b/tpm_tis.c
> index 477f555..abea7a1 100644
> --- a/tpm_tis.c
> +++ b/tpm_tis.c
> @@ -792,6 +792,9 @@ int tpm_tis_send(struct tpm_chip* tpm, uint8_t* buf, size_t len) {
>     int status, burstcnt = 0;
>     int count = 0;
>     uint32_t ordinal;
> +#ifdef HAVE_LIBC
> +   struct file *file = get_file_from_fd(tpm->fd);
> +#endif
>  
>     if(tpm_tis_request_locality(tpm, tpm->locality) < 0) {
>        return -EBUSY;
> @@ -844,9 +847,10 @@ 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].offset = 0;
> +   if ( file )
> +   {
> +      file->read = false;
> +      file->offset = 0;
>     }
>  #endif
>     return len;
> @@ -1093,6 +1097,23 @@ ssize_t tpm_getcap(struct tpm_chip *chip, uint32_t subcap_id, cap_t *cap,
>          return rc;
>  }
>  
> +static void shutdown_tpm_tis(struct tpm_chip* tpm){
> +   int i;
> +
> +   printk("Shutting down tpm_tis device\n");
> +
> +   iowrite32(TPM_INT_ENABLE(tpm, tpm->locality), ~TPM_GLOBAL_INT_ENABLE);
> +
> +   /*Unmap all of the mmio pages */
> +   for(i = 0; i < 5; ++i) {
> +      if(tpm->pages[i] != NULL) {
> +	 iounmap(tpm->pages[i], PAGE_SIZE);
> +	 tpm->pages[i] = NULL;
> +      }
> +   }
> +   free(tpm);
> +   return;
> +}
>  
>  struct tpm_chip* init_tpm_tis(unsigned long baseaddr, int localities, unsigned int irq)
>  {
> @@ -1242,25 +1263,6 @@ abort_egress:
>     return NULL;
>  }
>  
> -void shutdown_tpm_tis(struct tpm_chip* tpm){
> -   int i;
> -
> -   printk("Shutting down tpm_tis device\n");
> -
> -   iowrite32(TPM_INT_ENABLE(tpm, tpm->locality), ~TPM_GLOBAL_INT_ENABLE);
> -
> -   /*Unmap all of the mmio pages */
> -   for(i = 0; i < 5; ++i) {
> -      if(tpm->pages[i] != NULL) {
> -	 iounmap(tpm->pages[i], PAGE_SIZE);
> -	 tpm->pages[i] = NULL;
> -      }
> -   }
> -   free(tpm);
> -   return;
> -}
> -
> -
>  int tpm_tis_cmd(struct tpm_chip* tpm, uint8_t* req, size_t reqlen, uint8_t** resp, size_t* resplen)
>  {
>     if(tpm->locality < 0) {
> @@ -1279,23 +1281,24 @@ int tpm_tis_cmd(struct tpm_chip* tpm, uint8_t* req, size_t reqlen, uint8_t** res
>  }
>  
>  #ifdef HAVE_LIBC
> -int tpm_tis_open(struct tpm_chip* tpm)
> +#include <sys/stat.h>
> +#include <fcntl.h>
> +
> +static int tpm_tis_close(int fd)
>  {
> -   /* Silently prevent multiple opens */
> -   if(tpm->fd != -1) {
> -      return tpm->fd;
> -   }
> +    struct file *file = get_file_from_fd(fd);
>  
> -   tpm->fd = alloc_fd(FTYPE_TPM_TIS);
> -   printk("tpm_tis_open() -> %d\n", tpm->fd);
> -   files[tpm->fd].dev = tpm;
> -   return tpm->fd;
> +    shutdown_tpm_tis(file->dev);
> +
> +    return 0;
>  }
>  
> -int tpm_tis_posix_write(int fd, const uint8_t* buf, size_t count)
> +static int tpm_tis_posix_write(int fd, const void *buf, size_t count)
>  {
>     struct tpm_chip* tpm;
> -   tpm = files[fd].dev;
> +   struct file *file = get_file_from_fd(fd);
> +
> +   tpm = file->dev;
>  
>     if(tpm->locality < 0) {
>        printk("tpm_tis_posix_write() failed! locality not set!\n");
> @@ -1319,11 +1322,13 @@ int tpm_tis_posix_write(int fd, const uint8_t* buf, size_t count)
>     return count;
>  }
>  
> -int tpm_tis_posix_read(int fd, uint8_t* buf, size_t count)
> +static int tpm_tis_posix_read(int fd, void *buf, size_t count)
>  {
>     int rc;
>     struct tpm_chip* tpm;
> -   tpm = files[fd].dev;
> +   struct file *file = get_file_from_fd(fd);
> +
> +   tpm = file->dev;
>  
>     if(count == 0) {
>        return 0;
> @@ -1337,20 +1342,24 @@ int tpm_tis_posix_read(int fd, uint8_t* buf, size_t count)
>  
>  
>     /* Handle EOF case */
> -   if(files[fd].offset >= tpm->data_len) {
> +   if ( file->offset >= tpm->data_len )
> +   {
>        rc = 0;
>     } else {
> -      rc = min(tpm->data_len - files[fd].offset, count);
> -      memcpy(buf, tpm->data_buffer + files[fd].offset, rc);
> +      rc = min(tpm->data_len - file->offset, count);
> +      memcpy(buf, tpm->data_buffer + file->offset, rc);
>     }
> -   files[fd].offset += rc;
> +   file->offset += rc;
>     /* Reset the data pending flag */
>     return rc;
>  }
> -int tpm_tis_posix_fstat(int fd, struct stat* buf)
> +
> +static int tpm_tis_posix_fstat(int fd, struct stat *buf)
>  {
>     struct tpm_chip* tpm;
> -   tpm = files[fd].dev;
> +   struct file *file = get_file_from_fd(fd);
> +
> +   tpm = file->dev;
>  
>     buf->st_mode = O_RDWR;
>     buf->st_uid = 0;
> @@ -1360,6 +1369,35 @@ int tpm_tis_posix_fstat(int fd, struct stat* buf)
>     return 0;
>  }
>  
> +static struct file_ops tpm_tis_ops = {
> +    .name = "tpm_tis",
> +    .read = tpm_tis_posix_read,
> +    .write = tpm_tis_posix_write,
> +    .lseek = lseek_default,
> +    .close = tpm_tis_close,
> +    .fstat = tpm_tis_posix_fstat,
> +};
> +
> +int tpm_tis_open(struct tpm_chip* tpm)
> +{
> +   struct file *file;
> +   static unsigned int ftype_tis;
> +
> +   /* Silently prevent multiple opens */
> +   if(tpm->fd != -1) {
> +      return tpm->fd;
> +   }
> +
> +   if ( !ftype_tis )
> +       ftype_tis = alloc_file_type(&tpm_tis_ops);
> +
> +   tpm->fd = alloc_fd(ftype_tis);
> +   printk("tpm_tis_open() -> %d\n", tpm->fd);
> +   file = get_file_from_fd(tpm->fd);
> +   file->dev = tpm;
> +   return tpm->fd;
> +}
> +
>  /* TPM 2.0 */
>  
>  /*TPM2.0 Selftest*/
> -- 
> 2.26.2
> 

-- 
Samuel
Now, it we had this sort of thing:
  yield -a     for yield to all traffic
  yield -t     for yield to trucks
  yield -f     for yield to people walking (yield foot)
  yield -d t*  for yield on days starting with t
...you'd have a lot of dead people at intersections, and traffic jams you
wouldn't believe...
(Discussion in comp.os.linux.misc on the intuitiveness of commands.)


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 03/12] mini-os: use alloc_file_type() and get_file_from_fd() in xs
  2022-01-11 20:11     ` Samuel Thibault
@ 2022-01-11 20:14       ` Andrew Cooper
  0 siblings, 0 replies; 48+ messages in thread
From: Andrew Cooper @ 2022-01-11 20:14 UTC (permalink / raw)
  To: Samuel Thibault, Juergen Gross, minios-devel, xen-devel, wl

On 11/01/2022 20:11, Samuel Thibault wrote:
> Samuel Thibault, le mar. 11 janv. 2022 21:06:15 +0100, a ecrit:
>> Juergen Gross, le mar. 11 janv. 2022 16:12:06 +0100, a ecrit:
>>> diff --git a/lib/xs.c b/lib/xs.c
>>> index 4af0f96..ac830d2 100644
>>> --- a/lib/xs.c
>>> +++ b/lib/xs.c
>>> @@ -18,23 +18,55 @@ static inline int _xs_fileno(struct xs_handle *h) {
>>>      return (intptr_t) h;
>>>  }
>>>  
>>> +static int xs_close_fd(int fd)
>>> +{
>>> +    struct xenbus_event *event, *next;
>>> +    struct file *file = get_file_from_fd(fd);
> This...
>
>>> +    for (event = file->dev; event; event = next)
>>> +    {
>>> +        next = event->next;
>>> +        free(event);
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static bool xs_can_read(int fd)
>>> +{
>>> +    struct file *file = get_file_from_fd(fd);
> ... and that would be simplified by directly getting the file* from the
> parameters.

I actually make exactly the same suggestion for close() in
libxenevtchn.  The callback hooks don't need the same API as the libc
functions.

~Andrew


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 05/12] mini-os: use alloc_file_type() and get_file_from_fd() in tpmfront
  2022-01-11 15:12 ` [PATCH v2 05/12] mini-os: use alloc_file_type() and get_file_from_fd() in tpmfront Juergen Gross
@ 2022-01-11 20:15   ` Samuel Thibault
  0 siblings, 0 replies; 48+ messages in thread
From: Samuel Thibault @ 2022-01-11 20:15 UTC (permalink / raw)
  To: Juergen Gross; +Cc: minios-devel, xen-devel, wl

Juergen Gross, le mar. 11 janv. 2022 16:12:08 +0100, a ecrit:
> Allocate a file type dynamically via alloc_file_type().
> 
> Instead of directly accessing the files[] array use get_file_from_fd().
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>


modulo the int fd / file * thing,

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


> ---
>  include/lib.h      |   3 +-
>  include/tpmfront.h |   5 ---
>  lib/sys.c          |  23 -----------
>  tpmfront.c         | 100 ++++++++++++++++++++++++++++++++-------------
>  4 files changed, 72 insertions(+), 59 deletions(-)
> 
> diff --git a/include/lib.h b/include/lib.h
> index d94d142..f6478de 100644
> --- a/include/lib.h
> +++ b/include/lib.h
> @@ -165,8 +165,7 @@ void sanity_check(void);
>  #define FTYPE_KBD        7
>  #define FTYPE_TAP        8
>  #define FTYPE_BLK        9
> -#define FTYPE_TPMFRONT  10
> -#define FTYPE_N         11
> +#define FTYPE_N         10
>  #define FTYPE_SPARE     16
>  
>  typedef int file_read_func(int fd, void *buf, size_t nbytes);
> diff --git a/include/tpmfront.h b/include/tpmfront.h
> index b7da50e..a527371 100644
> --- a/include/tpmfront.h
> +++ b/include/tpmfront.h
> @@ -81,7 +81,6 @@ int tpmfront_cmd(struct tpmfront_dev* dev, uint8_t* req, size_t reqlen, uint8_t*
>  int tpmfront_set_locality(struct tpmfront_dev* dev, int locality);
>  
>  #ifdef HAVE_LIBC
> -#include <sys/stat.h>
>  /* POSIX IO functions:
>   * use tpmfront_open() to get a file descriptor to the tpm device
>   * use write() on the fd to send a command to the backend. You must
> @@ -90,10 +89,6 @@ int tpmfront_set_locality(struct tpmfront_dev* dev, int locality);
>   * fstat() to get the size of the response and lseek() to seek on it.
>   */
>  int tpmfront_open(struct tpmfront_dev* dev);
> -int tpmfront_posix_read(int fd, uint8_t* buf, size_t count);
> -int tpmfront_posix_write(int fd, const uint8_t* buf, size_t count);
> -int tpmfront_posix_fstat(int fd, struct stat* buf);
>  #endif
>  
> -
>  #endif
> diff --git a/lib/sys.c b/lib/sys.c
> index 4060b42..ff6be52 100644
> --- a/lib/sys.c
> +++ b/lib/sys.c
> @@ -28,7 +28,6 @@
>  #include <netfront.h>
>  #include <blkfront.h>
>  #include <fbfront.h>
> -#include <tpmfront.h>
>  #include <xenbus.h>
>  #include <xenstore.h>
>  #include <poll.h>
> @@ -343,11 +342,6 @@ int read(int fd, void *buf, size_t nbytes)
>          case FTYPE_BLK: {
>  	    return blkfront_posix_read(fd, buf, nbytes);
>          }
> -#endif
> -#ifdef CONFIG_TPMFRONT
> -        case FTYPE_TPMFRONT: {
> -	    return tpmfront_posix_read(fd, buf, nbytes);
> -        }
>  #endif
>  	default:
>  	    break;
> @@ -389,10 +383,6 @@ int write(int fd, const void *buf, size_t nbytes)
>  #ifdef CONFIG_BLKFRONT
>  	case FTYPE_BLK:
>  	    return blkfront_posix_write(fd, buf, nbytes);
> -#endif
> -#ifdef CONFIG_TPMFRONT
> -	case FTYPE_TPMFRONT:
> -	    return tpmfront_posix_write(fd, buf, nbytes);
>  #endif
>  	default:
>  	    break;
> @@ -445,10 +435,6 @@ off_t lseek(int fd, off_t offset, int whence)
>  #ifdef CONFIG_BLKFRONT
>         case FTYPE_BLK:
>            break;
> -#endif
> -#ifdef CONFIG_TPMFRONT
> -       case FTYPE_TPMFRONT:
> -          break;
>  #endif
>         case FTYPE_FILE:
>            break;
> @@ -496,11 +482,6 @@ int close(int fd)
>              shutdown_blkfront(files[fd].dev);
>              break;
>  #endif
> -#ifdef CONFIG_TPMFRONT
> -	case FTYPE_TPMFRONT:
> -            shutdown_tpmfront(files[fd].dev);
> -            break;
> -#endif
>  #ifdef CONFIG_KBDFRONT
>  	case FTYPE_KBD:
>              shutdown_kbdfront(files[fd].dev);
> @@ -576,10 +557,6 @@ int fstat(int fd, struct stat *buf)
>  #ifdef CONFIG_BLKFRONT
>  	case FTYPE_BLK:
>  	   return blkfront_posix_fstat(fd, buf);
> -#endif
> -#ifdef CONFIG_TPMFRONT
> -	case FTYPE_TPMFRONT:
> -	   return tpmfront_posix_fstat(fd, buf);
>  #endif
>  	default:
>  	    break;
> diff --git a/tpmfront.c b/tpmfront.c
> index 0a2fefc..a19a052 100644
> --- a/tpmfront.c
> +++ b/tpmfront.c
> @@ -49,6 +49,10 @@
>  void tpmfront_handler(evtchn_port_t port, struct pt_regs *regs, void *data) {
>     struct tpmfront_dev* dev = (struct tpmfront_dev*) data;
>     tpmif_shared_page_t *shr = dev->page;
> +#ifdef HAVE_LIBC
> +    struct file *file = get_file_from_fd(dev->fd);
> +#endif
> +
>     /*If we get a response when we didnt make a request, just ignore it */
>     if(!dev->waiting) {
>        return;
> @@ -65,8 +69,9 @@ 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 = true;
> +   if ( file )
> +   {
> +      file->read = true;
>     }
>  #endif
>     wake_up(&dev->waitq);
> @@ -405,6 +410,10 @@ int tpmfront_send(struct tpmfront_dev* dev, const uint8_t* msg, size_t length)
>  #ifdef TPMFRONT_PRINT_DEBUG
>     int i;
>  #endif
> +#ifdef HAVE_LIBC
> +    struct file *file = dev ? get_file_from_fd(dev->fd) : NULL;
> +#endif
> +
>     /* Error Checking */
>     if(dev == NULL || dev->state != XenbusStateConnected) {
>        TPMFRONT_ERR("Tried to send message through disconnected frontend\n");
> @@ -437,9 +446,10 @@ int tpmfront_send(struct tpmfront_dev* dev, const uint8_t* msg, size_t length)
>     dev->waiting = 1;
>     dev->resplen = 0;
>  #ifdef HAVE_LIBC
> -   if(dev->fd >= 0) {
> -      files[dev->fd].read = false;
> -      files[dev->fd].offset = 0;
> +   if ( file )
> +   {
> +      file->read = false;
> +      file->offset = 0;
>        dev->respgot = false;
>     }
>  #endif
> @@ -529,25 +539,14 @@ int tpmfront_set_locality(struct tpmfront_dev* dev, int locality)
>  
>  #ifdef HAVE_LIBC
>  #include <errno.h>
> -int tpmfront_open(struct tpmfront_dev* dev)
> -{
> -   /* Silently prevent multiple opens */
> -   if(dev->fd != -1) {
> -      return dev->fd;
> -   }
>  
> -   dev->fd = alloc_fd(FTYPE_TPMFRONT);
> -   printk("tpmfront_open(%s) -> %d\n", dev->nodename, dev->fd);
> -   files[dev->fd].dev = dev;
> -   dev->respgot = false;
> -   return dev->fd;
> -}
> -
> -int tpmfront_posix_write(int fd, const uint8_t* buf, size_t count)
> +static int tpmfront_posix_write(int fd, const void *buf, size_t count)
>  {
>     int rc;
>     struct tpmfront_dev* dev;
> -   dev = files[fd].dev;
> +   struct file *file = get_file_from_fd(fd);
> +
> +   dev = file->dev;
>  
>     if(count == 0) {
>        return 0;
> @@ -566,14 +565,15 @@ int tpmfront_posix_write(int fd, const uint8_t* buf, size_t count)
>     return count;
>  }
>  
> -int tpmfront_posix_read(int fd, uint8_t* buf, size_t count)
> +static int tpmfront_posix_read(int fd, void *buf, size_t count)
>  {
>     int rc;
>     uint8_t* dummybuf;
>     size_t dummysz;
>     struct tpmfront_dev* dev;
> +   struct file *file = get_file_from_fd(fd);
>  
> -   dev = files[fd].dev;
> +   dev = file->dev;
>  
>     if(count == 0) {
>        return 0;
> @@ -588,29 +588,33 @@ int tpmfront_posix_read(int fd, uint8_t* buf, size_t count)
>     }
>  
>     /* handle EOF case */
> -   if(files[dev->fd].offset >= dev->resplen) {
> +   if ( file->offset >= dev->resplen )
> +   {
>        return 0;
>     }
>  
>     /* Compute the number of bytes and do the copy operation */
> -   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;
> +   if ( (rc = min(count, dev->resplen - file->offset)) != 0 )
> +   {
> +      memcpy(buf, dev->respbuf + file->offset, rc);
> +      file->offset += rc;
>     }
>  
>     return rc;
>  }
>  
> -int tpmfront_posix_fstat(int fd, struct stat* buf)
> +static int tpmfront_posix_fstat(int fd, struct stat *buf)
>  {
>     uint8_t* dummybuf;
>     size_t dummysz;
>     int rc;
> -   struct tpmfront_dev* dev = files[fd].dev;
> +   struct file *file = get_file_from_fd(fd);
> +   struct tpmfront_dev* dev = file->dev;
>  
>     /* 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 && !dev->respgot)) {
> +   if ( dev->waiting || (file->read && !dev->respgot) )
> +   {
>        if ((rc = tpmfront_recv(dev, &dummybuf, &dummysz)) != 0) {
>  	 errno = EIO;
>  	 return -1;
> @@ -626,5 +630,43 @@ int tpmfront_posix_fstat(int fd, struct stat* buf)
>     return 0;
>  }
>  
> +static int tpmfront_close_fd(int fd)
> +{
> +    struct file *file = get_file_from_fd(fd);
> +
> +    shutdown_tpmfront(file->dev);
> +
> +    return 0;
> +}
> +
> +static struct file_ops tpmfront_ops = {
> +    .name = "tpmfront",
> +    .read = tpmfront_posix_read,
> +    .write = tpmfront_posix_write,
> +    .lseek = lseek_default,
> +    .close = tpmfront_close_fd,
> +    .fstat = tpmfront_posix_fstat,
> +};
> +
> +int tpmfront_open(struct tpmfront_dev* dev)
> +{
> +   struct file *file;
> +   static unsigned int ftype_tpmfront;
> +
> +   /* Silently prevent multiple opens */
> +   if(dev->fd != -1) {
> +      return dev->fd;
> +   }
> +
> +   if ( !ftype_tpmfront )
> +       ftype_tpmfront = alloc_file_type(&tpmfront_ops);
> +
> +   dev->fd = alloc_fd(ftype_tpmfront);
> +   printk("tpmfront_open(%s) -> %d\n", dev->nodename, dev->fd);
> +   file = get_file_from_fd(dev->fd);
> +   file->dev = dev;
> +   dev->respgot = false;
> +   return dev->fd;
> +}
>  
>  #endif
> -- 
> 2.26.2
> 

-- 
Samuel
"...very few phenomena can pull someone out of Deep Hack Mode, with two
noted exceptions: being struck by lightning, or worse, your *computer*
being struck by lightning."
(By Matt Welsh)


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 06/12] mini-os: use alloc_file_type() and get_file_from_fd() in blkfront
  2022-01-11 15:12 ` [PATCH v2 06/12] mini-os: use alloc_file_type() and get_file_from_fd() in blkfront Juergen Gross
@ 2022-01-11 20:20   ` Samuel Thibault
  0 siblings, 0 replies; 48+ messages in thread
From: Samuel Thibault @ 2022-01-11 20:20 UTC (permalink / raw)
  To: Juergen Gross; +Cc: minios-devel, xen-devel, wl

Juergen Gross, le mar. 11 janv. 2022 16:12:09 +0100, a ecrit:
> Allocate the file type dynamically via alloc_file_type().
> 
> Instead of directly accessing the files[] array use get_file_from_fd().
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

modulo the int fd / file * thing,

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

> ---
>  blkfront.c         | 92 ++++++++++++++++++++++++++++++++++------------
>  include/blkfront.h |  5 ---
>  include/lib.h      |  3 +-
>  lib/sys.c          | 24 ------------
>  4 files changed, 69 insertions(+), 55 deletions(-)
> 
> diff --git a/blkfront.c b/blkfront.c
> index e3f42be..b7ea1d7 100644
> --- a/blkfront.c
> +++ b/blkfront.c
> @@ -59,10 +59,10 @@ void blkfront_handler(evtchn_port_t port, struct pt_regs *regs, void *data)
>  {
>  #ifdef HAVE_LIBC
>      struct blkfront_dev *dev = data;
> -    int fd = dev->fd;
> +    struct file *file = get_file_from_fd(dev->fd);
>  
> -    if (fd != -1)
> -        files[fd].read = true;
> +    if ( file )
> +        file->read = true;
>  #endif
>      wake_up(&blkfront_queue);
>  }
> @@ -483,9 +483,13 @@ int blkfront_aio_poll(struct blkfront_dev *dev)
>  
>  moretodo:
>  #ifdef HAVE_LIBC
> -    if (dev->fd != -1) {
> -        files[dev->fd].read = false;
> -        mb(); /* Make sure to let the handler set read to 1 before we start looking at the ring */
> +    {
> +        struct file *file = get_file_from_fd(dev->fd);
> +
> +        if ( file ) {
> +            file->read = false;
> +            mb(); /* Make sure to let the handler set read to 1 before we start looking at the ring */
> +        }
>      }
>  #endif
>  
> @@ -554,22 +558,11 @@ moretodo:
>  }
>  
>  #ifdef HAVE_LIBC
> -int blkfront_open(struct blkfront_dev *dev)
> -{
> -    /* Silently prevent multiple opens */
> -    if(dev->fd != -1) {
> -       return dev->fd;
> -    }
> -    dev->fd = alloc_fd(FTYPE_BLK);
> -    printk("blk_open(%s) -> %d\n", dev->nodename, dev->fd);
> -    files[dev->fd].dev = dev;
> -    return dev->fd;
> -}
> -
> -int blkfront_posix_rwop(int fd, uint8_t* buf, size_t count, int write)
> +static int blkfront_posix_rwop(int fd, uint8_t* buf, size_t count, int write)
>  {
> -   struct blkfront_dev* dev = files[fd].dev;
> -   off_t offset = files[fd].offset;
> +   struct file *file = get_file_from_fd(fd);
> +   struct blkfront_dev* dev = file->dev;
> +   off_t offset = file->offset;
>     struct blkfront_aiocb aiocb;
>     unsigned long long disksize = dev->info.sectors * dev->info.sector_size;
>     unsigned int blocksize = dev->info.sector_size;
> @@ -711,14 +704,25 @@ int blkfront_posix_rwop(int fd, uint8_t* buf, size_t count, int write)
>     }
>  
>     free(copybuf);
> -   files[fd].offset += rc;
> +   file->offset += rc;
>     return rc;
>  
>  }
>  
> -int blkfront_posix_fstat(int fd, struct stat* buf)
> +static int blkfront_posix_read(int fd, void *buf, size_t nbytes)
> +{
> +    return blkfront_posix_rwop(fd, buf, nbytes, 0);
> +}
> +
> +static int blkfront_posix_write(int fd, const void *buf, size_t nbytes)
>  {
> -   struct blkfront_dev* dev = files[fd].dev;
> +    return blkfront_posix_rwop(fd, (void *)buf, nbytes, 1);
> +}
> +
> +static int blkfront_posix_fstat(int fd, struct stat *buf)
> +{
> +   struct file *file = get_file_from_fd(fd);
> +   struct blkfront_dev* dev = file->dev;
>  
>     buf->st_mode = dev->info.mode;
>     buf->st_uid = 0;
> @@ -728,4 +732,44 @@ int blkfront_posix_fstat(int fd, struct stat* buf)
>  
>     return 0;
>  }
> +
> +static int blkfront_close_fd(int fd)
> +{
> +    struct file *file = get_file_from_fd(fd);
> +
> +    shutdown_blkfront(file->dev);
> +
> +    return 0;
> +}
> +
> +static struct file_ops blk_ops = {
> +    .name = "blk",
> +    .read = blkfront_posix_read,
> +    .write = blkfront_posix_write,
> +    .lseek = lseek_default,
> +    .close = blkfront_close_fd,
> +    .fstat = blkfront_posix_fstat,
> +    .select_rd = select_read_flag,
> +};
> +
> +int blkfront_open(struct blkfront_dev *dev)
> +{
> +    struct file *file;
> +    static unsigned int ftype_blk;
> +
> +    /* Silently prevent multiple opens */
> +    if(dev->fd != -1) {
> +       return dev->fd;
> +    }
> +
> +    if ( !ftype_blk )
> +        ftype_blk = alloc_file_type(&blk_ops);
> +
> +    dev->fd = alloc_fd(ftype_blk);
> +    printk("blk_open(%s) -> %d\n", dev->nodename, dev->fd);
> +    file = get_file_from_fd(dev->fd);
> +    file->dev = dev;
> +
> +    return dev->fd;
> +}
>  #endif
> diff --git a/include/blkfront.h b/include/blkfront.h
> index 3528af9..7f84a0a 100644
> --- a/include/blkfront.h
> +++ b/include/blkfront.h
> @@ -28,17 +28,12 @@ struct blkfront_info
>  };
>  struct blkfront_dev *init_blkfront(char *nodename, struct blkfront_info *info);
>  #ifdef HAVE_LIBC
> -#include <sys/stat.h>
>  /* POSIX IO functions:
>   * use blkfront_open() to get a file descriptor to the block device
>   * Don't use the other blkfront posix functions here directly, instead use
>   * read(), write(), lseek() and fstat() on the file descriptor
>   */
>  int blkfront_open(struct blkfront_dev *dev);
> -int blkfront_posix_rwop(int fd, uint8_t* buf, size_t count, int write);
> -#define blkfront_posix_write(fd, buf, count) blkfront_posix_rwop(fd, (uint8_t*)buf, count, 1)
> -#define blkfront_posix_read(fd, buf, count) blkfront_posix_rwop(fd, (uint8_t*)buf, count, 0)
> -int blkfront_posix_fstat(int fd, struct stat* buf);
>  #endif
>  void blkfront_aio(struct blkfront_aiocb *aiocbp, int write);
>  #define blkfront_aio_read(aiocbp) blkfront_aio(aiocbp, 0)
> diff --git a/include/lib.h b/include/lib.h
> index f6478de..05f5083 100644
> --- a/include/lib.h
> +++ b/include/lib.h
> @@ -164,8 +164,7 @@ void sanity_check(void);
>  #define FTYPE_FB         6
>  #define FTYPE_KBD        7
>  #define FTYPE_TAP        8
> -#define FTYPE_BLK        9
> -#define FTYPE_N         10
> +#define FTYPE_N          9
>  #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 ff6be52..f84fedd 100644
> --- a/lib/sys.c
> +++ b/lib/sys.c
> @@ -337,11 +337,6 @@ int read(int fd, void *buf, size_t nbytes)
>  	    }
>  	    return ret * sizeof(union xenfb_in_event);
>          }
> -#endif
> -#ifdef CONFIG_BLKFRONT
> -        case FTYPE_BLK: {
> -	    return blkfront_posix_read(fd, buf, nbytes);
> -        }
>  #endif
>  	default:
>  	    break;
> @@ -379,10 +374,6 @@ int write(int fd, const void *buf, size_t nbytes)
>  	case FTYPE_TAP:
>  	    netfront_xmit(files[fd].dev, (void*) buf, nbytes);
>  	    return nbytes;
> -#endif
> -#ifdef CONFIG_BLKFRONT
> -	case FTYPE_BLK:
> -	    return blkfront_posix_write(fd, buf, nbytes);
>  #endif
>  	default:
>  	    break;
> @@ -432,10 +423,6 @@ off_t lseek(int fd, off_t offset, int whence)
>          return ops->lseek(fd, offset, whence);
>  
>      switch(files[fd].type) {
> -#ifdef CONFIG_BLKFRONT
> -       case FTYPE_BLK:
> -          break;
> -#endif
>         case FTYPE_FILE:
>            break;
>         default:
> @@ -477,11 +464,6 @@ int close(int fd)
>  	    shutdown_netfront(files[fd].dev);
>              break;
>  #endif
> -#ifdef CONFIG_BLKFRONT
> -	case FTYPE_BLK:
> -            shutdown_blkfront(files[fd].dev);
> -            break;
> -#endif
>  #ifdef CONFIG_KBDFRONT
>  	case FTYPE_KBD:
>              shutdown_kbdfront(files[fd].dev);
> @@ -554,10 +536,6 @@ int fstat(int fd, struct stat *buf)
>  	    buf->st_ctime = time(NULL);
>  	    return 0;
>  	}
> -#ifdef CONFIG_BLKFRONT
> -	case FTYPE_BLK:
> -	   return blkfront_posix_fstat(fd, buf);
> -#endif
>  	default:
>  	    break;
>      }
> @@ -663,7 +641,6 @@ static const char *file_types[] = {
>      [FTYPE_CONSOLE] = "console",
>      [FTYPE_SOCKET]  = "socket",
>      [FTYPE_TAP]     = "net",
> -    [FTYPE_BLK]     = "blk",
>      [FTYPE_KBD]     = "kbd",
>      [FTYPE_FB]      = "fb",
>  };
> @@ -841,7 +818,6 @@ static int select_poll(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exce
>  	    FD_CLR(i, exceptfds);
>  	    break;
>  	case FTYPE_TAP:
> -	case FTYPE_BLK:
>  	case FTYPE_KBD:
>  	case FTYPE_FB:
>  	    if (FD_ISSET(i, readfds)) {
> -- 
> 2.26.2
> 

-- 
Samuel
gawk; talk; nice; date; wine; grep; touch; unzip; strip;  \
touch; gasp; finger; gasp; lyx; gasp; latex; mount; fsck; \
more; yes; gasp; umount; make clean; make mrproper; sleep


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 03/12] mini-os: use alloc_file_type() and get_file_from_fd() in xs
  2022-01-11 15:12 ` [PATCH v2 03/12] mini-os: use alloc_file_type() and get_file_from_fd() in xs Juergen Gross
  2022-01-11 20:06   ` Samuel Thibault
@ 2022-01-11 20:21   ` Andrew Cooper
  2022-01-12  7:52     ` Juergen Gross
  1 sibling, 1 reply; 48+ messages in thread
From: Andrew Cooper @ 2022-01-11 20:21 UTC (permalink / raw)
  To: Juergen Gross, minios-devel, xen-devel; +Cc: samuel.thibault, wl

On 11/01/2022 15:12, Juergen Gross wrote:
>  void xs_daemon_close(struct xs_handle *h)
>  {
> -    int fd = _xs_fileno(h);
> -    struct xenbus_event *event, *next;
> -    for (event = files[fd].dev; event; event = next)
> -    {
> -        next = event->next;
> -        free(event);
> -    }
> +    close(_xs_fileno(h));
>  }

You've deleted the sole caller of xs_daemon_close() from the main
close() function.

That said, I'm very confused, because nothing in minios declares it. 
The declaration appears to come from xenstore.h, which is clearly
included unconditionally (when it ought not to be), but libxenstore also
defines the function too...

~Andrew


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 07/12] mini-os: use get_file_from_fd() in netfront
  2022-01-11 15:12 ` [PATCH v2 07/12] mini-os: use get_file_from_fd() in netfront Juergen Gross
@ 2022-01-11 20:22   ` Samuel Thibault
  0 siblings, 0 replies; 48+ messages in thread
From: Samuel Thibault @ 2022-01-11 20:22 UTC (permalink / raw)
  To: Juergen Gross; +Cc: minios-devel, xen-devel, wl

Juergen Gross, le mar. 11 janv. 2022 16:12:10 +0100, a ecrit:
> Instead of directly accessing the files[] array use get_file_from_fd().
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

modulo the int fd / file * thing,

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

> ---
>  include/lib.h |  3 +--
>  lib/sys.c     | 23 ------------------
>  netfront.c    | 64 ++++++++++++++++++++++++++++++++++++++++++++-------
>  3 files changed, 57 insertions(+), 33 deletions(-)
> 
> diff --git a/include/lib.h b/include/lib.h
> index 05f5083..aa8036e 100644
> --- a/include/lib.h
> +++ b/include/lib.h
> @@ -163,8 +163,7 @@ void sanity_check(void);
>  #define FTYPE_SAVEFILE   5
>  #define FTYPE_FB         6
>  #define FTYPE_KBD        7
> -#define FTYPE_TAP        8
> -#define FTYPE_N          9
> +#define FTYPE_N          8
>  #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 f84fedd..7f2e11f 100644
> --- a/lib/sys.c
> +++ b/lib/sys.c
> @@ -303,17 +303,6 @@ int read(int fd, void *buf, size_t nbytes)
>  	case FTYPE_SOCKET:
>  	    return lwip_read(files[fd].fd, buf, nbytes);
>  #endif
> -#ifdef CONFIG_NETFRONT
> -	case FTYPE_TAP: {
> -	    ssize_t ret;
> -	    ret = netfront_receive(files[fd].dev, buf, nbytes);
> -	    if (ret <= 0) {
> -		errno = EAGAIN;
> -		return -1;
> -	    }
> -	    return ret;
> -	}
> -#endif
>  #ifdef CONFIG_KBDFRONT
>          case FTYPE_KBD: {
>              int ret, n;
> @@ -369,11 +358,6 @@ int write(int fd, const void *buf, size_t nbytes)
>  #ifdef HAVE_LWIP
>  	case FTYPE_SOCKET:
>  	    return lwip_write(files[fd].fd, (void*) buf, nbytes);
> -#endif
> -#ifdef CONFIG_NETFRONT
> -	case FTYPE_TAP:
> -	    netfront_xmit(files[fd].dev, (void*) buf, nbytes);
> -	    return nbytes;
>  #endif
>  	default:
>  	    break;
> @@ -459,11 +443,6 @@ int close(int fd)
>  	    res = lwip_close(files[fd].fd);
>              break;
>  #endif
> -#ifdef CONFIG_NETFRONT
> -	case FTYPE_TAP:
> -	    shutdown_netfront(files[fd].dev);
> -            break;
> -#endif
>  #ifdef CONFIG_KBDFRONT
>  	case FTYPE_KBD:
>              shutdown_kbdfront(files[fd].dev);
> @@ -640,7 +619,6 @@ static const char *file_types[] = {
>      [FTYPE_NONE]    = "none",
>      [FTYPE_CONSOLE] = "console",
>      [FTYPE_SOCKET]  = "socket",
> -    [FTYPE_TAP]     = "net",
>      [FTYPE_KBD]     = "kbd",
>      [FTYPE_FB]      = "fb",
>  };
> @@ -817,7 +795,6 @@ static int select_poll(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exce
>                  n++;
>  	    FD_CLR(i, exceptfds);
>  	    break;
> -	case FTYPE_TAP:
>  	case FTYPE_KBD:
>  	case FTYPE_FB:
>  	    if (FD_ISSET(i, readfds)) {
> diff --git a/netfront.c b/netfront.c
> index 7696451..f86c6a2 100644
> --- a/netfront.c
> +++ b/netfront.c
> @@ -248,14 +248,14 @@ void netfront_select_handler(evtchn_port_t port, struct pt_regs *regs, void *dat
>  {
>      int flags;
>      struct netfront_dev *dev = data;
> -    int fd = dev->fd;
> +    struct file *file = get_file_from_fd(dev->fd);
>  
>      local_irq_save(flags);
>      network_tx_buf_gc(dev);
>      local_irq_restore(flags);
>  
> -    if (fd != -1)
> -        files[fd].read = true;
> +    if ( file )
> +        file->read = true;
>      wake_up(&netfront_queue);
>  }
>  #endif
> @@ -565,8 +565,54 @@ error:
>  }
>  
>  #ifdef HAVE_LIBC
> +static int netfront_read(int fd, void *buf, size_t nbytes)
> +{
> +    ssize_t ret;
> +    struct file *file = get_file_from_fd(fd);
> +
> +    ret = netfront_receive(file->dev, buf, nbytes);
> +    if ( ret <= 0 )
> +    {
> +        errno = EAGAIN;
> +        return -1;
> +    }
> +
> +    return ret;
> +}
> +
> +static int netfront_write(int fd, const void *buf, size_t nbytes)
> +{
> +    struct file *file = get_file_from_fd(fd);
> +
> +    netfront_xmit(file->dev, (void *)buf, nbytes);
> +
> +    return nbytes;
> +}
> +
> +static int netfront_close_fd(int fd)
> +{
> +    struct file *file = get_file_from_fd(fd);
> +
> +    shutdown_netfront(file->dev);
> +
> +    return 0;
> +}
> +
> +static struct file_ops netfront_ops = {
> +    .name = "net",
> +    .read = netfront_read,
> +    .write = netfront_write,
> +    .close = netfront_close_fd,
> +    .select_rd = select_read_flag,
> +};
> +
>  int netfront_tap_open(char *nodename) {
>      struct netfront_dev *dev;
> +    struct file *file;
> +    static unsigned int ftype_netfront;
> +
> +    if ( !ftype_netfront )
> +        ftype_netfront = alloc_file_type(&netfront_ops);
>  
>      dev = init_netfront(nodename, NETIF_SELECT_RX, NULL, NULL);
>      if (!dev) {
> @@ -574,9 +620,10 @@ int netfront_tap_open(char *nodename) {
>  	errno = EIO;
>  	return -1;
>      }
> -    dev->fd = alloc_fd(FTYPE_TAP);
> +    dev->fd = alloc_fd(ftype_netfront);
>      printk("tap_open(%s) -> %d\n", nodename, dev->fd);
> -    files[dev->fd].dev = dev;
> +    file = get_file_from_fd(dev->fd);
> +    file->dev = dev;
>      return dev->fd;
>  }
>  #endif
> @@ -772,7 +819,8 @@ void netfront_xmit(struct netfront_dev *dev, unsigned char* data,int len)
>  ssize_t netfront_receive(struct netfront_dev *dev, unsigned char *data, size_t len)
>  {
>      unsigned long flags;
> -    int fd = dev->fd;
> +    struct file *file = get_file_from_fd(dev->fd);
> +
>      ASSERT(current == main_thread);
>  
>      dev->rlen = 0;
> @@ -781,9 +829,9 @@ ssize_t netfront_receive(struct netfront_dev *dev, unsigned char *data, size_t l
>  
>      local_irq_save(flags);
>      network_rx(dev);
> -    if (!dev->rlen && fd != -1)
> +    if ( !dev->rlen && file )
>          /* No data for us, make select stop returning */
> -        files[fd].read = false;
> +        file->read = false;
>      /* Before re-enabling the interrupts, in case a packet just arrived in the
>       * meanwhile. */
>      local_irq_restore(flags);
> -- 
> 2.26.2
> 

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


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 08/12] mini-os: use alloc_file_type() and get_file_from_fd() in fbfront
  2022-01-11 15:12 ` [PATCH v2 08/12] mini-os: use alloc_file_type() and get_file_from_fd() in fbfront Juergen Gross
@ 2022-01-11 20:26   ` Samuel Thibault
  2022-01-12  7:52     ` Juergen Gross
  0 siblings, 1 reply; 48+ messages in thread
From: Samuel Thibault @ 2022-01-11 20:26 UTC (permalink / raw)
  To: Juergen Gross; +Cc: minios-devel, xen-devel, wl

Juergen Gross, le mar. 11 janv. 2022 16:12:11 +0100, a ecrit:
> +static struct file_ops fb_ops = {
> +    .name = ".fb",

Why a dot?


Apart from that, and modulo the int fd / file * thing,

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



^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 04/12] mini-os: use alloc_file_type() and get_file_from_fd() in tpm_tis
  2022-01-11 15:12 ` [PATCH v2 04/12] mini-os: use alloc_file_type() and get_file_from_fd() in tpm_tis Juergen Gross
  2022-01-11 20:13   ` Samuel Thibault
@ 2022-01-11 20:29   ` Andrew Cooper
  2022-01-11 20:58     ` Jason Andryuk
  2022-01-12  7:54     ` Juergen Gross
  1 sibling, 2 replies; 48+ messages in thread
From: Andrew Cooper @ 2022-01-11 20:29 UTC (permalink / raw)
  To: Juergen Gross, minios-devel, xen-devel
  Cc: samuel.thibault, wl, Jason Andryuk, Daniel Smith

On 11/01/2022 15:12, Juergen Gross wrote:
> diff --git a/tpm_tis.c b/tpm_tis.c
> index 477f555..abea7a1 100644
> --- a/tpm_tis.c
> +++ b/tpm_tis.c
> @@ -1093,6 +1097,23 @@ ssize_t tpm_getcap(struct tpm_chip *chip, uint32_t subcap_id, cap_t *cap,
>          return rc;
>  }
>  
> +static void shutdown_tpm_tis(struct tpm_chip* tpm){

Style, as you're moving it.  Also in the function.

> @@ -1360,6 +1369,35 @@ int tpm_tis_posix_fstat(int fd, struct stat* buf)
>     return 0;
>  }
>  
> +static struct file_ops tpm_tis_ops = {
> +    .name = "tpm_tis",
> +    .read = tpm_tis_posix_read,
> +    .write = tpm_tis_posix_write,
> +    .lseek = lseek_default,
> +    .close = tpm_tis_close,
> +    .fstat = tpm_tis_posix_fstat,
> +};
> +
> +int tpm_tis_open(struct tpm_chip* tpm)

Style.

> +{
> +   struct file *file;
> +   static unsigned int ftype_tis;
> +
> +   /* Silently prevent multiple opens */
> +   if(tpm->fd != -1) {
> +      return tpm->fd;
> +   }

Another WTF moment.  We silently swallow multiple open()s, but don't
refcout close()s ?

This cannot be correct, or sensible, behaviour.

Jason/Daniel - thoughts?

~Andrew


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 09/12] mini-os: use file_ops and get_file_from_fd() for console
  2022-01-11 15:12 ` [PATCH v2 09/12] mini-os: use file_ops and get_file_from_fd() for console Juergen Gross
@ 2022-01-11 20:35   ` Samuel Thibault
  2022-01-12  7:57     ` Juergen Gross
  2022-01-12 10:30   ` Samuel Thibault
  2022-01-12 11:23   ` Andrew Cooper
  2 siblings, 1 reply; 48+ messages in thread
From: Samuel Thibault @ 2022-01-11 20:35 UTC (permalink / raw)
  To: Juergen Gross; +Cc: minios-devel, xen-devel, wl

Juergen Gross, le mar. 11 janv. 2022 16:12:12 +0100, a ecrit:
> +static int consfront_fstat(int fd, struct stat *buf)
> +{
> +    struct file *file = get_file_from_fd(fd);
> +
> +    buf->st_mode = S_IRUSR | S_IWUSR;
> +    buf->st_mode |= (file->type == FTYPE_CONSOLE) ? S_IFCHR : S_IFREG;
> +    buf->st_atime = buf->st_mtime = buf->st_ctime = time(NULL);
> +
> +    return 0;
> +}

This seems to be missing filling st_uid, st_gid, etc.?

Samuel


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 10/12] mini-os: add struct file_ops for file type socket
  2022-01-11 15:12 ` [PATCH v2 10/12] mini-os: add struct file_ops for file type socket Juergen Gross
@ 2022-01-11 20:38   ` Samuel Thibault
  2022-01-12 10:30   ` Samuel Thibault
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 48+ messages in thread
From: Samuel Thibault @ 2022-01-11 20:38 UTC (permalink / raw)
  To: Juergen Gross; +Cc: minios-devel, xen-devel, wl

Juergen Gross, le mar. 11 janv. 2022 16:12:13 +0100, a ecrit:
> +static int socket_fstat(int fd, struct stat *buf)
> +{
> +    buf->st_mode = S_IFSOCK | S_IRUSR | S_IWUSR;
> +    buf->st_atime = buf->st_mtime = buf->st_ctime = time(NULL);
> +
> +    return 0;
> +}

Similarly, this seems to be missing setting st_uid, st_gid, st_size?

Samuel


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 11/12] mini-os: add struct file_ops for FTYPE_FILE
  2022-01-11 15:12 ` [PATCH v2 11/12] mini-os: add struct file_ops for FTYPE_FILE Juergen Gross
@ 2022-01-11 20:42   ` Samuel Thibault
  0 siblings, 0 replies; 48+ messages in thread
From: Samuel Thibault @ 2022-01-11 20:42 UTC (permalink / raw)
  To: Juergen Gross; +Cc: minios-devel, xen-devel, wl

Juergen Gross, le mar. 11 janv. 2022 16:12:14 +0100, a ecrit:
> FTYPE_FILE is the last relevant file type without a struct file_ops.
> Add it.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

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

> ---
>  lib/sys.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/sys.c b/lib/sys.c
> index 12deaed..0f42e97 100644
> --- a/lib/sys.c
> +++ b/lib/sys.c
> @@ -99,6 +99,11 @@ static struct file_ops file_ops_none = {
>      .name = "none",
>  };
>  
> +static struct file_ops file_file_ops = {
> +    .name = "file",
> +    .lseek = lseek_default,
> +};
> +
>  #ifdef HAVE_LWIP
>  static int socket_read(int fd, void *buf, size_t nbytes)
>  {
> @@ -160,6 +165,7 @@ static struct file_ops *file_ops[FTYPE_N + FTYPE_SPARE] = {
>  #ifdef CONFIG_CONSFRONT
>      [FTYPE_CONSOLE] = &console_ops,
>  #endif
> +    [FTYPE_FILE] = &file_file_ops,
>  #ifdef HAVE_LWIP
>      [FTYPE_SOCKET] = &socket_ops,
>  #endif
> @@ -397,16 +403,9 @@ off_t lseek(int fd, off_t offset, int whence)
>      if ( ops->lseek )
>          return ops->lseek(fd, offset, whence);
>  
> -    switch(files[fd].type) {
> -       case FTYPE_FILE:
> -          break;
> -       default:
> -          /* Not implemented for this filetype */
> -          errno = ESPIPE;
> -          return (off_t) -1;
> -    }
> -
> -    return lseek_default(fd, offset, whence);
> +    /* Not implemented for this filetype */
> +    errno = ESPIPE;
> +    return (off_t) -1;
>  }
>  
>  int fsync(int fd) {
> -- 
> 2.26.2
> 

-- 
Samuel
"...[Linux's] capacity to talk via any medium except smoke signals."
(By Dr. Greg Wettstein, Roger Maris Cancer Center)


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 12/12] mini-os: make files array private to sys.c
  2022-01-11 15:12 ` [PATCH v2 12/12] mini-os: make files array private to sys.c Juergen Gross
@ 2022-01-11 20:42   ` Samuel Thibault
  0 siblings, 0 replies; 48+ messages in thread
From: Samuel Thibault @ 2022-01-11 20:42 UTC (permalink / raw)
  To: Juergen Gross; +Cc: minios-devel, xen-devel, wl

Juergen Gross, le mar. 11 janv. 2022 16:12:15 +0100, a ecrit:
> There is no user of the files[] array outside of lib/sys.c left, so
> it can be made static.

Yay!

> Signed-off-by: Juergen Gross <jgross@suse.com>

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

> ---
>  include/lib.h | 2 --
>  lib/sys.c     | 2 +-
>  2 files changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/include/lib.h b/include/lib.h
> index c171fe8..80e804b 100644
> --- a/include/lib.h
> +++ b/include/lib.h
> @@ -199,8 +199,6 @@ struct file {
>      };
>  };
>  
> -extern struct file files[];
> -
>  struct file *get_file_from_fd(int fd);
>  int alloc_fd(unsigned int type);
>  void close_all_files(void);
> diff --git a/lib/sys.c b/lib/sys.c
> index 0f42e97..c0cad87 100644
> --- a/lib/sys.c
> +++ b/lib/sys.c
> @@ -89,7 +89,7 @@ extern void minios_evtchn_close_fd(int fd);
>  extern void minios_gnttab_close_fd(int fd);
>  
>  pthread_mutex_t fd_lock = PTHREAD_MUTEX_INITIALIZER;
> -struct file files[NOFILE] = {
> +static struct file files[NOFILE] = {
>      { .type = FTYPE_CONSOLE }, /* stdin */
>      { .type = FTYPE_CONSOLE }, /* stdout */
>      { .type = FTYPE_CONSOLE }, /* stderr */
> -- 
> 2.26.2
> 

-- 
Samuel
#include <culture.h>


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 04/12] mini-os: use alloc_file_type() and get_file_from_fd() in tpm_tis
  2022-01-11 20:29   ` Andrew Cooper
@ 2022-01-11 20:58     ` Jason Andryuk
  2022-01-12  7:54     ` Juergen Gross
  1 sibling, 0 replies; 48+ messages in thread
From: Jason Andryuk @ 2022-01-11 20:58 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Juergen Gross, minios-devel, xen-devel, Samuel Thibault, Wei Liu,
	Daniel Smith

On Tue, Jan 11, 2022 at 3:29 PM Andrew Cooper <amc96@srcf.net> wrote:
>
> On 11/01/2022 15:12, Juergen Gross wrote:
> > diff --git a/tpm_tis.c b/tpm_tis.c
> > index 477f555..abea7a1 100644
> > --- a/tpm_tis.c
> > +++ b/tpm_tis.c
> > @@ -1093,6 +1097,23 @@ ssize_t tpm_getcap(struct tpm_chip *chip, uint32_t subcap_id, cap_t *cap,
> >          return rc;
> >  }
> >
> > +static void shutdown_tpm_tis(struct tpm_chip* tpm){
>
> Style, as you're moving it.  Also in the function.
>
> > @@ -1360,6 +1369,35 @@ int tpm_tis_posix_fstat(int fd, struct stat* buf)
> >     return 0;
> >  }
> >
> > +static struct file_ops tpm_tis_ops = {
> > +    .name = "tpm_tis",
> > +    .read = tpm_tis_posix_read,
> > +    .write = tpm_tis_posix_write,
> > +    .lseek = lseek_default,
> > +    .close = tpm_tis_close,
> > +    .fstat = tpm_tis_posix_fstat,
> > +};
> > +
> > +int tpm_tis_open(struct tpm_chip* tpm)
>
> Style.
>
> > +{
> > +   struct file *file;
> > +   static unsigned int ftype_tis;
> > +
> > +   /* Silently prevent multiple opens */
> > +   if(tpm->fd != -1) {
> > +      return tpm->fd;
> > +   }
>
> Another WTF moment.  We silently swallow multiple open()s, but don't
> refcout close()s ?
>
> This cannot be correct, or sensible, behaviour.
>
> Jason/Daniel - thoughts?

Looks like vtpmmgr only opens a single global fd, so it has not been a
problem in practice.

You need some sort of synchronization to let multiple entities access
the TPM.  So limiting to only a single entity/single FD is a
reasonable restriction.

Regards,
Jason


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 02/12] mini-os: remove gnttab specific member from struct file
  2022-01-11 20:12   ` Andrew Cooper
@ 2022-01-12  7:44     ` Juergen Gross
  0 siblings, 0 replies; 48+ messages in thread
From: Juergen Gross @ 2022-01-12  7:44 UTC (permalink / raw)
  To: Andrew Cooper, minios-devel, xen-devel; +Cc: samuel.thibault, wl


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

On 11.01.22 21:12, Andrew Cooper wrote:
> On 11/01/2022 15:12, Juergen Gross wrote:
>> The event channel specific union member in struct file is no longer
> 
> Too much copy&paste from the previous patch?

Seems so, yes. :-)


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 --]

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 03/12] mini-os: use alloc_file_type() and get_file_from_fd() in xs
  2022-01-11 20:21   ` Andrew Cooper
@ 2022-01-12  7:52     ` Juergen Gross
  2022-01-12  8:12       ` Andrew Cooper
  0 siblings, 1 reply; 48+ messages in thread
From: Juergen Gross @ 2022-01-12  7:52 UTC (permalink / raw)
  To: Andrew Cooper, minios-devel, xen-devel; +Cc: samuel.thibault, wl


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

On 11.01.22 21:21, Andrew Cooper wrote:
> On 11/01/2022 15:12, Juergen Gross wrote:
>>   void xs_daemon_close(struct xs_handle *h)
>>   {
>> -    int fd = _xs_fileno(h);
>> -    struct xenbus_event *event, *next;
>> -    for (event = files[fd].dev; event; event = next)
>> -    {
>> -        next = event->next;
>> -        free(event);
>> -    }
>> +    close(_xs_fileno(h));
>>   }
> 
> You've deleted the sole caller of xs_daemon_close() from the main
> close() function.
> 
> That said, I'm very confused, because nothing in minios declares it.
> The declaration appears to come from xenstore.h, which is clearly
> included unconditionally (when it ought not to be), but libxenstore also
> defines the function too...
I already thought of restructuring this mess.

lib/xs.c is the Mini-OS variant of libxenstore, and as such it shares
quite some code with libxenstore.

So the correct thing to do would be to split libxenstore into a common
part and a posix/Mini-OS part, drop lib/xs.c, and use the library in
Mini-OS instead.

But this should be done in a separate series.


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 --]

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 08/12] mini-os: use alloc_file_type() and get_file_from_fd() in fbfront
  2022-01-11 20:26   ` Samuel Thibault
@ 2022-01-12  7:52     ` Juergen Gross
  0 siblings, 0 replies; 48+ messages in thread
From: Juergen Gross @ 2022-01-12  7:52 UTC (permalink / raw)
  To: Samuel Thibault, minios-devel, xen-devel, wl


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

On 11.01.22 21:26, Samuel Thibault wrote:
> Juergen Gross, le mar. 11 janv. 2022 16:12:11 +0100, a ecrit:
>> +static struct file_ops fb_ops = {
>> +    .name = ".fb",
> 
> Why a dot?

I had spare one lying around... ;-)

Thanks for noticing!


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 --]

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 04/12] mini-os: use alloc_file_type() and get_file_from_fd() in tpm_tis
  2022-01-11 20:29   ` Andrew Cooper
  2022-01-11 20:58     ` Jason Andryuk
@ 2022-01-12  7:54     ` Juergen Gross
  2022-01-12  8:14       ` Andrew Cooper
  1 sibling, 1 reply; 48+ messages in thread
From: Juergen Gross @ 2022-01-12  7:54 UTC (permalink / raw)
  To: Andrew Cooper, minios-devel, xen-devel
  Cc: samuel.thibault, wl, Jason Andryuk, Daniel Smith


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

On 11.01.22 21:29, Andrew Cooper wrote:
> On 11/01/2022 15:12, Juergen Gross wrote:
>> diff --git a/tpm_tis.c b/tpm_tis.c
>> index 477f555..abea7a1 100644
>> --- a/tpm_tis.c
>> +++ b/tpm_tis.c
>> @@ -1093,6 +1097,23 @@ ssize_t tpm_getcap(struct tpm_chip *chip, uint32_t subcap_id, cap_t *cap,
>>           return rc;
>>   }
>>   
>> +static void shutdown_tpm_tis(struct tpm_chip* tpm){
> 
> Style, as you're moving it.  Also in the function.
> 
>> @@ -1360,6 +1369,35 @@ int tpm_tis_posix_fstat(int fd, struct stat* buf)
>>      return 0;
>>   }
>>   
>> +static struct file_ops tpm_tis_ops = {
>> +    .name = "tpm_tis",
>> +    .read = tpm_tis_posix_read,
>> +    .write = tpm_tis_posix_write,
>> +    .lseek = lseek_default,
>> +    .close = tpm_tis_close,
>> +    .fstat = tpm_tis_posix_fstat,
>> +};
>> +
>> +int tpm_tis_open(struct tpm_chip* tpm)
> 
> Style.

Ah, yes I should have corrected this while moving the function.

> 
>> +{
>> +   struct file *file;
>> +   static unsigned int ftype_tis;
>> +
>> +   /* Silently prevent multiple opens */
>> +   if(tpm->fd != -1) {
>> +      return tpm->fd;
>> +   }
> 
> Another WTF moment.  We silently swallow multiple open()s, but don't
> refcout close()s ?
> 
> This cannot be correct, or sensible, behaviour.
> 
> Jason/Daniel - thoughts?

I just moved the function, but I can change this, of course.


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 --]

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 09/12] mini-os: use file_ops and get_file_from_fd() for console
  2022-01-11 20:35   ` Samuel Thibault
@ 2022-01-12  7:57     ` Juergen Gross
  2022-01-12 10:30       ` Samuel Thibault
  0 siblings, 1 reply; 48+ messages in thread
From: Juergen Gross @ 2022-01-12  7:57 UTC (permalink / raw)
  To: Samuel Thibault, minios-devel, xen-devel, wl


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

On 11.01.22 21:35, Samuel Thibault wrote:
> Juergen Gross, le mar. 11 janv. 2022 16:12:12 +0100, a ecrit:
>> +static int consfront_fstat(int fd, struct stat *buf)
>> +{
>> +    struct file *file = get_file_from_fd(fd);
>> +
>> +    buf->st_mode = S_IRUSR | S_IWUSR;
>> +    buf->st_mode |= (file->type == FTYPE_CONSOLE) ? S_IFCHR : S_IFREG;
>> +    buf->st_atime = buf->st_mtime = buf->st_ctime = time(NULL);
>> +
>> +    return 0;
>> +}
> 
> This seems to be missing filling st_uid, st_gid, etc.?

Not really. Those are set to zero via the call of init_stat()
in fstat().


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 --]

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 03/12] mini-os: use alloc_file_type() and get_file_from_fd() in xs
  2022-01-12  7:52     ` Juergen Gross
@ 2022-01-12  8:12       ` Andrew Cooper
  0 siblings, 0 replies; 48+ messages in thread
From: Andrew Cooper @ 2022-01-12  8:12 UTC (permalink / raw)
  To: Juergen Gross, minios-devel, xen-devel; +Cc: samuel.thibault, wl

On 12/01/2022 07:52, Juergen Gross wrote:
> On 11.01.22 21:21, Andrew Cooper wrote:
>> On 11/01/2022 15:12, Juergen Gross wrote:
>>>   void xs_daemon_close(struct xs_handle *h)
>>>   {
>>> -    int fd = _xs_fileno(h);
>>> -    struct xenbus_event *event, *next;
>>> -    for (event = files[fd].dev; event; event = next)
>>> -    {
>>> -        next = event->next;
>>> -        free(event);
>>> -    }
>>> +    close(_xs_fileno(h));
>>>   }
>>
>> You've deleted the sole caller of xs_daemon_close() from the main
>> close() function.
>>
>> That said, I'm very confused, because nothing in minios declares it.
>> The declaration appears to come from xenstore.h, which is clearly
>> included unconditionally (when it ought not to be), but libxenstore also
>> defines the function too...
> I already thought of restructuring this mess.
>
> lib/xs.c is the Mini-OS variant of libxenstore, and as such it shares
> quite some code with libxenstore.
>
> So the correct thing to do would be to split libxenstore into a common
> part and a posix/Mini-OS part, drop lib/xs.c, and use the library in
> Mini-OS instead.
>
> But this should be done in a separate series.

Sounds like a plan.

~Andrew


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 04/12] mini-os: use alloc_file_type() and get_file_from_fd() in tpm_tis
  2022-01-12  7:54     ` Juergen Gross
@ 2022-01-12  8:14       ` Andrew Cooper
  0 siblings, 0 replies; 48+ messages in thread
From: Andrew Cooper @ 2022-01-12  8:14 UTC (permalink / raw)
  To: Juergen Gross, minios-devel, xen-devel
  Cc: samuel.thibault, wl, Jason Andryuk, Daniel Smith

On 12/01/2022 07:54, Juergen Gross wrote:
> On 11.01.22 21:29, Andrew Cooper wrote:
>> On 11/01/2022 15:12, Juergen Gross wrote:
>>> diff --git a/tpm_tis.c b/tpm_tis.c
>>> index 477f555..abea7a1 100644
>>> --- a/tpm_tis.c
>>> +++ b/tpm_tis.c
>>> @@ -1093,6 +1097,23 @@ ssize_t tpm_getcap(struct tpm_chip *chip,
>>> uint32_t subcap_id, cap_t *cap,
>>>           return rc;
>>>   }
>>>   +static void shutdown_tpm_tis(struct tpm_chip* tpm){
>>
>> Style, as you're moving it.  Also in the function.
>>
>>> @@ -1360,6 +1369,35 @@ int tpm_tis_posix_fstat(int fd, struct stat*
>>> buf)
>>>      return 0;
>>>   }
>>>   +static struct file_ops tpm_tis_ops = {
>>> +    .name = "tpm_tis",
>>> +    .read = tpm_tis_posix_read,
>>> +    .write = tpm_tis_posix_write,
>>> +    .lseek = lseek_default,
>>> +    .close = tpm_tis_close,
>>> +    .fstat = tpm_tis_posix_fstat,
>>> +};
>>> +
>>> +int tpm_tis_open(struct tpm_chip* tpm)
>>
>> Style.
>
> Ah, yes I should have corrected this while moving the function.
>
>>
>>> +{
>>> +   struct file *file;
>>> +   static unsigned int ftype_tis;
>>> +
>>> +   /* Silently prevent multiple opens */
>>> +   if(tpm->fd != -1) {
>>> +      return tpm->fd;
>>> +   }
>>
>> Another WTF moment.  We silently swallow multiple open()s, but don't
>> refcout close()s ?
>>
>> This cannot be correct, or sensible, behaviour.
>>
>> Jason/Daniel - thoughts?
>
> I just moved the function, but I can change this, of course.

It's a pattern elsewhere too.  Much as it's horrible logic, it probably
doesn't want reworking in a patch like this, and the series as a whole
is big enough so I'm not going to suggest you tackle it too.

~Andrew


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 09/12] mini-os: use file_ops and get_file_from_fd() for console
  2022-01-11 15:12 ` [PATCH v2 09/12] mini-os: use file_ops and get_file_from_fd() for console Juergen Gross
  2022-01-11 20:35   ` Samuel Thibault
@ 2022-01-12 10:30   ` Samuel Thibault
  2022-01-12 11:23   ` Andrew Cooper
  2 siblings, 0 replies; 48+ messages in thread
From: Samuel Thibault @ 2022-01-12 10:30 UTC (permalink / raw)
  To: Juergen Gross; +Cc: minios-devel, xen-devel, wl

Juergen Gross, le mar. 11 janv. 2022 16:12:12 +0100, a ecrit:
> Add struct file_ops for the console related file types (FTYPE_CONSOLE
> and savefile). FTYPE_CONSOLE remains statically defined, as it is used
> to statically init stdin, stdout and stderr.
> 
> Instead of directly accessing the files[] array use get_file_from_fd().
> 
> With CONSOLE now handled via file_ops the bogus file descriptor case in
> select_poll() now needs to be handled more explicit instead of dropping
> into console handling, assuming that this case was basically meant to
> cover SAVEFILE.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

modulo the int fd / file * thing,

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

> ---
>  console/xenbus.c       | 125 +++++++++++++++++++++++++++++++++++++++++
>  console/xencons_ring.c |   6 +-
>  include/console.h      |   5 ++
>  include/lib.h          |   3 +-
>  lib/sys.c              |  87 +++++++---------------------
>  5 files changed, 155 insertions(+), 71 deletions(-)
> 
> diff --git a/console/xenbus.c b/console/xenbus.c
> index 05fc31c..b856c84 100644
> --- a/console/xenbus.c
> +++ b/console/xenbus.c
> @@ -192,3 +192,128 @@ void fini_consfront(struct consfront_dev *dev)
>  {
>      if (dev) free_consfront(dev);
>  }
> +
> +#ifdef HAVE_LIBC
> +static int consfront_read(int fd, void *buf, size_t nbytes)
> +{
> +    int ret;
> +    struct file *file = get_file_from_fd(fd);
> +    DEFINE_WAIT(w);
> +
> +    while ( 1 )
> +    {
> +        add_waiter(w, console_queue);
> +        ret = xencons_ring_recv(file->dev, buf, nbytes);
> +        if ( ret )
> +            break;
> +        schedule();
> +    }
> +
> +    remove_waiter(w, console_queue);
> +
> +    return ret;
> +}
> +
> +static int savefile_write(int fd, const void *buf, size_t nbytes)
> +{
> +    int ret = 0, tot = nbytes;
> +    struct file *file = get_file_from_fd(fd);
> +
> +    while ( nbytes > 0 )
> +    {
> +        ret = xencons_ring_send(file->dev, (char *)buf, nbytes);
> +        nbytes -= ret;
> +        buf = (char *)buf + ret;
> +    }
> +
> +    return tot - nbytes;
> +}
> +
> +static int console_write(int fd, const void *buf, size_t nbytes)
> +{
> +    struct file *file = get_file_from_fd(fd);
> +
> +    console_print(file->dev, (char *)buf, nbytes);
> +
> +    return nbytes;
> +}
> +
> +static int consfront_close_fd(int fd)
> +{
> +    struct file *file = get_file_from_fd(fd);
> +
> +    fini_consfront(file->dev);
> +
> +    return 0;
> +}
> +
> +static int consfront_fstat(int fd, struct stat *buf)
> +{
> +    struct file *file = get_file_from_fd(fd);
> +
> +    buf->st_mode = S_IRUSR | S_IWUSR;
> +    buf->st_mode |= (file->type == FTYPE_CONSOLE) ? S_IFCHR : S_IFREG;
> +    buf->st_atime = buf->st_mtime = buf->st_ctime = time(NULL);
> +
> +    return 0;
> +}
> +
> +static bool consfront_select_rd(int fd)
> +{
> +    struct file *file = get_file_from_fd(fd);
> +
> +    return xencons_ring_avail(file->dev);
> +}
> +
> +static struct file_ops savefile_ops = {
> +    .name = "savefile",
> +    .read = consfront_read,
> +    .write = savefile_write,
> +    .close = consfront_close_fd,
> +    .fstat = consfront_fstat,
> +    .select_rd = consfront_select_rd,
> +    .select_wr = select_yes,
> +};
> +
> +struct file_ops console_ops = {
> +    .name = "console",
> +    .read = consfront_read,
> +    .write = console_write,
> +    .close = consfront_close_fd,
> +    .fstat = consfront_fstat,
> +    .select_rd = consfront_select_rd,
> +    .select_wr = select_yes,
> +};
> +
> +int open_consfront(char *nodename)
> +{
> +    struct consfront_dev *dev;
> +    static unsigned int ftype_savefile;
> +    unsigned int ftype;
> +    struct file *file;
> +
> +    dev = init_consfront(nodename);
> +    if ( !dev )
> +        return -1;
> +
> +    if ( nodename )
> +    {
> +        if ( !ftype_savefile )
> +            ftype_savefile = alloc_file_type(&savefile_ops);
> +        ftype = ftype_savefile;
> +    }
> +    else
> +        ftype = FTYPE_CONSOLE;
> +
> +    dev->fd = alloc_fd(ftype);
> +    file = get_file_from_fd(dev->fd);
> +    if ( !file )
> +    {
> +        fini_consfront(dev);
> +        return -1;
> +    }
> +    file->dev = dev;
> +
> +    return dev->fd;
> +}
> +#endif
> diff --git a/console/xencons_ring.c b/console/xencons_ring.c
> index c348f3c..efedf46 100644
> --- a/console/xencons_ring.c
> +++ b/console/xencons_ring.c
> @@ -99,10 +99,10 @@ void console_handle_input(evtchn_port_t port, struct pt_regs *regs, void *data)
>  {
>  	struct consfront_dev *dev = (struct consfront_dev *) data;
>  #ifdef HAVE_LIBC
> -        int fd = dev ? dev->fd : -1;
> +        struct file *file = dev ? get_file_from_fd(dev->fd) : NULL;
>  
> -        if (fd != -1)
> -            files[fd].read = true;
> +        if ( file )
> +            file->read = true;
>  
>          wake_up(&console_queue);
>  #else
> diff --git a/include/console.h b/include/console.h
> index 0d7bf07..8c615d0 100644
> --- a/include/console.h
> +++ b/include/console.h
> @@ -39,6 +39,7 @@
>  #include <mini-os/os.h>
>  #include <mini-os/traps.h>
>  #include <mini-os/types.h>
> +#include <mini-os/lib.h>
>  #include <xen/grant_table.h>
>  #include <xenbus.h>
>  #include <xen/io/console.h>
> @@ -93,5 +94,9 @@ int xencons_ring_send_no_notify(struct consfront_dev *dev, const char *data, uns
>  int xencons_ring_avail(struct consfront_dev *dev);
>  int xencons_ring_recv(struct consfront_dev *dev, char *data, unsigned len);
>  void free_consfront(struct consfront_dev *dev);
> +#ifdef HAVE_LIBC
> +extern struct file_ops console_ops;
> +int open_consfront(char *nodename);
> +#endif
>  
>  #endif /* _LIB_CONSOLE_H_ */
> diff --git a/include/lib.h b/include/lib.h
> index 653a16e..c171fe8 100644
> --- a/include/lib.h
> +++ b/include/lib.h
> @@ -160,8 +160,7 @@ void sanity_check(void);
>  #define FTYPE_FILE       2
>  #define FTYPE_SOCKET     3
>  #define FTYPE_MEM        4
> -#define FTYPE_SAVEFILE   5
> -#define FTYPE_N          6
> +#define FTYPE_N          5
>  #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 4fb844f..3a8aa68 100644
> --- a/lib/sys.c
> +++ b/lib/sys.c
> @@ -101,6 +101,9 @@ static struct file_ops file_ops_none = {
>  
>  static struct file_ops *file_ops[FTYPE_N + FTYPE_SPARE] = {
>      [FTYPE_NONE] = &file_ops_none,
> +#ifdef CONFIG_CONSFRONT
> +    [FTYPE_CONSOLE] = &console_ops,
> +#endif
>  };
>  
>  unsigned int alloc_file_type(struct file_ops *ops)
> @@ -211,31 +214,26 @@ int mkdir(const char *pathname, mode_t mode)
>  #ifdef CONFIG_CONSFRONT
>  int posix_openpt(int flags)
>  {
> -    struct consfront_dev *dev;
> +    int fd;
>  
>      /* Ignore flags */
> +    fd = open_consfront(NULL);
> +    printk("fd(%d) = posix_openpt\n", fd);
>  
> -    dev = init_consfront(NULL);
> -    dev->fd = alloc_fd(FTYPE_CONSOLE);
> -    files[dev->fd].dev = dev;
> -
> -    printk("fd(%d) = posix_openpt\n", dev->fd);
> -    return(dev->fd);
> +    return fd;
>  }
>  
>  int open_savefile(const char *path, int save)
>  {
> -    struct consfront_dev *dev;
> +    int fd;
>      char nodename[64];
>  
>      snprintf(nodename, sizeof(nodename), "device/console/%d", save ? SAVE_CONSOLE : RESTORE_CONSOLE);
>  
> -    dev = init_consfront(nodename);
> -    dev->fd = alloc_fd(FTYPE_SAVEFILE);
> -    files[dev->fd].dev = dev;
> +    fd = open_consfront(nodename);
> +    printk("fd(%d) = open_savefile\n", fd);
>  
> -    printk("fd(%d) = open_savefile\n", dev->fd);
> -    return(dev->fd);
> +    return fd;
>  }
>  #else
>  int posix_openpt(int flags)
> @@ -285,20 +283,6 @@ int read(int fd, void *buf, size_t nbytes)
>          return ops->read(fd, buf, nbytes);
>  
>      switch (files[fd].type) {
> -        case FTYPE_SAVEFILE:
> -	case FTYPE_CONSOLE: {
> -	    int ret;
> -            DEFINE_WAIT(w);
> -            while(1) {
> -                add_waiter(w, console_queue);
> -                ret = xencons_ring_recv(files[fd].dev, buf, nbytes);
> -                if (ret)
> -                    break;
> -                schedule();
> -            }
> -            remove_waiter(w, console_queue);
> -            return ret;
> -        }
>  #ifdef HAVE_LWIP
>  	case FTYPE_SOCKET:
>  	    return lwip_read(files[fd].fd, buf, nbytes);
> @@ -319,18 +303,6 @@ int write(int fd, const void *buf, size_t nbytes)
>          return ops->write(fd, buf, nbytes);
>  
>      switch (files[fd].type) {
> -        case FTYPE_SAVEFILE: {
> -                int ret = 0, tot = nbytes;
> -                while (nbytes > 0) {
> -                    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].dev, (char *)buf, nbytes);
> -	    return nbytes;
>  #ifdef HAVE_LWIP
>  	case FTYPE_SOCKET:
>  	    return lwip_write(files[fd].fd, (void*) buf, nbytes);
> @@ -418,12 +390,6 @@ int close(int fd)
>  	case FTYPE_SOCKET:
>  	    res = lwip_close(files[fd].fd);
>              break;
> -#endif
> -#ifdef CONFIG_CONSFRONT
> -        case FTYPE_SAVEFILE:
> -        case FTYPE_CONSOLE:
> -            fini_consfront(files[fd].dev);
> -            break;
>  #endif
>  	case FTYPE_NONE:
>              printk("close(%d): Bad descriptor\n", fd);
> @@ -464,15 +430,8 @@ int fstat(int fd, struct stat *buf)
>          return ops->fstat(fd, buf);
>  
>      switch (files[fd].type) {
> -	case FTYPE_SAVEFILE:
> -	case FTYPE_CONSOLE:
>  	case FTYPE_SOCKET: {
> -            if (files[fd].type == FTYPE_CONSOLE)
> -                buf->st_mode = S_IFCHR|S_IRUSR|S_IWUSR;
> -            else if (files[fd].type == FTYPE_SOCKET)
> -                buf->st_mode = S_IFSOCK|S_IRUSR|S_IWUSR;
> -            else if (files[fd].type == FTYPE_SAVEFILE)
> -                buf->st_mode = S_IFREG|S_IRUSR|S_IWUSR;
> +            buf->st_mode = S_IFSOCK|S_IRUSR|S_IWUSR;
>  	    buf->st_uid = 0;
>  	    buf->st_gid = 0;
>  	    buf->st_size = 0;
> @@ -583,7 +542,6 @@ int closedir(DIR *dir)
>  #if defined(LIBC_DEBUG) || defined(LIBC_VERBOSE)
>  static const char *file_types[] = {
>      [FTYPE_NONE]    = "none",
> -    [FTYPE_CONSOLE] = "console",
>      [FTYPE_SOCKET]  = "socket",
>  };
>  
> @@ -744,21 +702,18 @@ static int select_poll(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exce
>              }
>  
>  	    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))
> -		    n++;
> -		else
> -		    FD_CLR(i, readfds);
> +                if ( FD_ISSET(i, readfds) )
> +                    FD_CLR(i, readfds);
> +                if ( FD_ISSET(i, writefds) )
> +                    FD_CLR(i, writefds);
> +                if ( FD_ISSET(i, exceptfds) )
> +                    FD_CLR(i, exceptfds);
>              }
> -	    if (FD_ISSET(i, writefds))
> -                n++;
> -	    FD_CLR(i, exceptfds);
>  	    break;
> +        }
> +
>  #ifdef HAVE_LWIP
>  	case FTYPE_SOCKET:
>  	    if (FD_ISSET(i, readfds)) {
> -- 
> 2.26.2
> 

-- 
Samuel
<y> t1 faich
<y> les programmes ils segfaultent jamais quand on veut
 -+- #ens-mim en plein débogage -+-


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 10/12] mini-os: add struct file_ops for file type socket
  2022-01-11 15:12 ` [PATCH v2 10/12] mini-os: add struct file_ops for file type socket Juergen Gross
  2022-01-11 20:38   ` Samuel Thibault
@ 2022-01-12 10:30   ` Samuel Thibault
  2022-01-12 11:25   ` Andrew Cooper
  2022-01-12 11:28   ` Andrew Cooper
  3 siblings, 0 replies; 48+ messages in thread
From: Samuel Thibault @ 2022-01-12 10:30 UTC (permalink / raw)
  To: Juergen Gross; +Cc: minios-devel, xen-devel, wl

Juergen Gross, le mar. 11 janv. 2022 16:12:13 +0100, a ecrit:
> Even with some special handling needed in select_poll(), add a struct
> file_ops for FTYPE_SOCKET. Due to the need of the special handling it
> isn't possible to use a dynamically allocated file type.
> 
> Most functions calling the file_ops methods can be simplified a lot now
> that no file type specific handling is left. Same applies to the file
> type name printing in debug/verbose mode.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

modulo the int fd / file * thing,

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

> ---
>  lib/sys.c | 153 +++++++++++++++++++++++++-----------------------------
>  1 file changed, 70 insertions(+), 83 deletions(-)
> 
> diff --git a/lib/sys.c b/lib/sys.c
> index 3a8aa68..12deaed 100644
> --- a/lib/sys.c
> +++ b/lib/sys.c
> @@ -99,11 +99,70 @@ static struct file_ops file_ops_none = {
>      .name = "none",
>  };
>  
> +#ifdef HAVE_LWIP
> +static int socket_read(int fd, void *buf, size_t nbytes)
> +{
> +    return lwip_read(fd, buf, nbytes);
> +}
> +
> +static int socket_write(int fd, const void *buf, size_t nbytes)
> +{
> +    return lwip_write(fd, (void *)buf, nbytes);
> +}
> +
> +static int close_socket_fd(int fd)
> +{
> +    struct file *file = get_file_from_fd(fd);
> +
> +    return lwip_close(file->fd);
> +}
> +
> +static int socket_fstat(int fd, struct stat *buf)
> +{
> +    buf->st_mode = S_IFSOCK | S_IRUSR | S_IWUSR;
> +    buf->st_atime = buf->st_mtime = buf->st_ctime = time(NULL);
> +
> +    return 0;
> +}
> +
> +static int socket_fcntl(int fd, int cmd, va_list args)
> +{
> +    long arg;
> +    struct file *file = get_file_from_fd(fd);
> +
> +    arg = va_arg(args, long);
> +
> +    if ( cmd == F_SETFL && !(arg & ~O_NONBLOCK) )
> +    {
> +        /* Only flag supported: non-blocking mode */
> +        uint32_t nblock = !!(arg & O_NONBLOCK);
> +
> +        return lwip_ioctl(file->fd, FIONBIO, &nblock);
> +    }
> +
> +    printk("fcntl(%d, %d, %lx/%lo)\n", fd, cmd, arg, arg);
> +    errno = ENOSYS;
> +    return -1;
> +}
> +
> +static struct file_ops socket_ops = {
> +    .name = "socket",
> +    .read = socket_read,
> +    .write = socket_write,
> +    .close = close_socket_fd,
> +    .fstat = socket_fstat,
> +    .fcntl = socket_fcntl,
> +};
> +#endif
> +
>  static struct file_ops *file_ops[FTYPE_N + FTYPE_SPARE] = {
>      [FTYPE_NONE] = &file_ops_none,
>  #ifdef CONFIG_CONSFRONT
>      [FTYPE_CONSOLE] = &console_ops,
>  #endif
> +#ifdef HAVE_LWIP
> +    [FTYPE_SOCKET] = &socket_ops,
> +#endif
>  };
>  
>  unsigned int alloc_file_type(struct file_ops *ops)
> @@ -282,14 +341,6 @@ int read(int fd, void *buf, size_t nbytes)
>      if ( ops->read )
>          return ops->read(fd, buf, nbytes);
>  
> -    switch (files[fd].type) {
> -#ifdef HAVE_LWIP
> -	case FTYPE_SOCKET:
> -	    return lwip_read(files[fd].fd, buf, nbytes);
> -#endif
> -	default:
> -	    break;
> -    }
>      printk("read(%d): Bad descriptor\n", fd);
>      errno = EBADF;
>      return -1;
> @@ -302,14 +353,6 @@ int write(int fd, const void *buf, size_t nbytes)
>      if ( ops->write )
>          return ops->write(fd, buf, nbytes);
>  
> -    switch (files[fd].type) {
> -#ifdef HAVE_LWIP
> -	case FTYPE_SOCKET:
> -	    return lwip_write(files[fd].fd, (void*) buf, nbytes);
> -#endif
> -	default:
> -	    break;
> -    }
>      printk("write(%d): Bad descriptor\n", fd);
>      errno = EBADF;
>      return -1;
> @@ -378,26 +421,14 @@ int close(int fd)
>  
>      printk("close(%d)\n", fd);
>      if ( ops->close )
> -    {
>          res = ops->close(fd);
> -        goto out;
> -    }
> -
> -    switch (files[fd].type) {
> -        default:
> -            break;
> -#ifdef HAVE_LWIP
> -	case FTYPE_SOCKET:
> -	    res = lwip_close(files[fd].fd);
> -            break;
> -#endif
> -	case FTYPE_NONE:
> -            printk("close(%d): Bad descriptor\n", fd);
> -            errno = EBADF;
> -            return -1;
> +    else if ( files[fd].type == FTYPE_NONE )
> +    {
> +        printk("close(%d): Bad descriptor\n", fd);
> +        errno = EBADF;
> +        return -1;
>      }
>  
> - out:
>      memset(files + fd, 0, sizeof(struct file));
>      files[fd].type = FTYPE_NONE;
>      return res;
> @@ -429,21 +460,6 @@ int fstat(int fd, struct stat *buf)
>      if ( ops->fstat )
>          return ops->fstat(fd, buf);
>  
> -    switch (files[fd].type) {
> -	case FTYPE_SOCKET: {
> -            buf->st_mode = S_IFSOCK|S_IRUSR|S_IWUSR;
> -	    buf->st_uid = 0;
> -	    buf->st_gid = 0;
> -	    buf->st_size = 0;
> -	    buf->st_atime = 
> -	    buf->st_mtime = 
> -	    buf->st_ctime = time(NULL);
> -	    return 0;
> -	}
> -	default:
> -	    break;
> -    }
> -
>      printk("statf(%d): Bad descriptor\n", fd);
>      errno = EBADF;
>      return -1;
> @@ -491,21 +507,9 @@ int fcntl(int fd, int cmd, ...)
>      arg = va_arg(ap, long);
>      va_end(ap);
>  
> -    switch (cmd) {
> -#ifdef HAVE_LWIP
> -	case F_SETFL:
> -	    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].fd, FIONBIO, &nblock);
> -	    }
> -	    /* Fallthrough */
> -#endif
> -	default:
> -	    printk("fcntl(%d, %d, %lx/%lo)\n", fd, cmd, arg, arg);
> -	    errno = ENOSYS;
> -	    return -1;
> -    }
> +    printk("fcntl(%d, %d, %lx/%lo)\n", fd, cmd, arg, arg);
> +    errno = ENOSYS;
> +    return -1;
>  }
>  
>  DIR *opendir(const char *name)
> @@ -539,23 +543,6 @@ 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]    = "none",
> -    [FTYPE_SOCKET]  = "socket",
> -};
> -
> -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)
>  {
> @@ -566,7 +553,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(%s)", i, get_type_name(files[i].type)); \
> +	    printk("%d(%s)", i, get_file_ops(files[i].type)->name); \
>  	    comma = 1; \
>  	} \
>      } \
> @@ -600,7 +587,7 @@ static void dump_pollfds(struct pollfd *pfd, int nfds, int timeout)
>          fd = pfd[i].fd;
>          if (comma)
>              printk(", ");
> -        printk("%d(%s)/%02x", fd, get_type_name(files[fd].type),
> +        printk("%d(%s)/%02x", fd, get_file_ops(files[fd].type)->name,
>              pfd[i].events);
>              comma = 1;
>      }
> @@ -754,7 +741,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, get_type_name(files[i].type));
> +		printk(" %d(%c):", i, get_file_ops(files[i].type)->name);
>  	    if (nbread[i])
>  	    	printk(" %dR", nbread[i]);
>  	    if (nbwrite[i])
> -- 
> 2.26.2
> 

-- 
Samuel
Running Windows on a Pentium is like having a brand new Porsche but only
be able to drive backwards with the handbrake on.
(Unknown source)


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 09/12] mini-os: use file_ops and get_file_from_fd() for console
  2022-01-12  7:57     ` Juergen Gross
@ 2022-01-12 10:30       ` Samuel Thibault
  0 siblings, 0 replies; 48+ messages in thread
From: Samuel Thibault @ 2022-01-12 10:30 UTC (permalink / raw)
  To: Juergen Gross; +Cc: minios-devel, xen-devel, wl

Juergen Gross, le mer. 12 janv. 2022 08:57:07 +0100, a ecrit:
> On 11.01.22 21:35, Samuel Thibault wrote:
> > Juergen Gross, le mar. 11 janv. 2022 16:12:12 +0100, a ecrit:
> > > +static int consfront_fstat(int fd, struct stat *buf)
> > > +{
> > > +    struct file *file = get_file_from_fd(fd);
> > > +
> > > +    buf->st_mode = S_IRUSR | S_IWUSR;
> > > +    buf->st_mode |= (file->type == FTYPE_CONSOLE) ? S_IFCHR : S_IFREG;
> > > +    buf->st_atime = buf->st_mtime = buf->st_ctime = time(NULL);
> > > +
> > > +    return 0;
> > > +}
> > 
> > This seems to be missing filling st_uid, st_gid, etc.?
> 
> Not really. Those are set to zero via the call of init_stat()
> in fstat().

Ah, I missed that call indeed.

Samuel


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 09/12] mini-os: use file_ops and get_file_from_fd() for console
  2022-01-11 15:12 ` [PATCH v2 09/12] mini-os: use file_ops and get_file_from_fd() for console Juergen Gross
  2022-01-11 20:35   ` Samuel Thibault
  2022-01-12 10:30   ` Samuel Thibault
@ 2022-01-12 11:23   ` Andrew Cooper
  2022-01-12 11:30     ` Juergen Gross
  2 siblings, 1 reply; 48+ messages in thread
From: Andrew Cooper @ 2022-01-12 11:23 UTC (permalink / raw)
  To: Juergen Gross, minios-devel, xen-devel; +Cc: samuel.thibault, wl

On 11/01/2022 15:12, Juergen Gross wrote:
> +static int savefile_write(int fd, const void *buf, size_t nbytes)
> +{
> +    int ret = 0, tot = nbytes;
> +    struct file *file = get_file_from_fd(fd);
> +
> +    while ( nbytes > 0 )
> +    {
> +        ret = xencons_ring_send(file->dev, (char *)buf, nbytes);
> +        nbytes -= ret;
> +        buf = (char *)buf + ret;
> +    }
> +
> +    return tot - nbytes;
> +}
> +
> +static int console_write(int fd, const void *buf, size_t nbytes)
> +{
> +    struct file *file = get_file_from_fd(fd);
> +
> +    console_print(file->dev, (char *)buf, nbytes);

I've just noticed this while committing the previous series, and I know
it is a preexisting bug, but the casts here are utterly unsafe, because
they're casting away constness.

console_print() is easy to fix, and just requires a prototype
adjustment.  That said, it also desperately also needs to fix 'int
length' to size_t to avoid problems with negative length VLAs on the stack.

xencons_ring_send() already takes const char *, so I'm pretty sure you
can just drop the casts here.  It too ought to not truncate size_t bytes
to "unsigned".

~Andrew

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 10/12] mini-os: add struct file_ops for file type socket
  2022-01-11 15:12 ` [PATCH v2 10/12] mini-os: add struct file_ops for file type socket Juergen Gross
  2022-01-11 20:38   ` Samuel Thibault
  2022-01-12 10:30   ` Samuel Thibault
@ 2022-01-12 11:25   ` Andrew Cooper
  2022-01-12 11:31     ` Juergen Gross
  2022-01-12 11:28   ` Andrew Cooper
  3 siblings, 1 reply; 48+ messages in thread
From: Andrew Cooper @ 2022-01-12 11:25 UTC (permalink / raw)
  To: Juergen Gross, minios-devel, xen-devel; +Cc: samuel.thibault, wl

On 11/01/2022 15:12, Juergen Gross wrote:
> +static int socket_write(int fd, const void *buf, size_t nbytes)
> +{
> +    return lwip_write(fd, (void *)buf, nbytes);

This void cast was bogus before, and can be dropped.  lwip_write()
already takes a const pointer.

~Andrew


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 10/12] mini-os: add struct file_ops for file type socket
  2022-01-11 15:12 ` [PATCH v2 10/12] mini-os: add struct file_ops for file type socket Juergen Gross
                     ` (2 preceding siblings ...)
  2022-01-12 11:25   ` Andrew Cooper
@ 2022-01-12 11:28   ` Andrew Cooper
  2022-01-12 11:32     ` Juergen Gross
  3 siblings, 1 reply; 48+ messages in thread
From: Andrew Cooper @ 2022-01-12 11:28 UTC (permalink / raw)
  To: Juergen Gross, minios-devel, xen-devel; +Cc: samuel.thibault, wl

On 11/01/2022 15:12, Juergen Gross wrote:
> diff --git a/lib/sys.c b/lib/sys.c
> index 3a8aa68..12deaed 100644
> --- a/lib/sys.c
> +++ b/lib/sys.c
> @@ -99,11 +99,70 @@ static struct file_ops file_ops_none = {
>      .name = "none",
>  };
>  
> +#ifdef HAVE_LWIP
> +static int socket_read(int fd, void *buf, size_t nbytes)
> +{
> +    return lwip_read(fd, buf, nbytes);
> +}
> +
> +static int socket_write(int fd, const void *buf, size_t nbytes)
> +{
> +    return lwip_write(fd, (void *)buf, nbytes);
> +}
> +
> +static int close_socket_fd(int fd)
> +{
> +    struct file *file = get_file_from_fd(fd);
> +
> +    return lwip_close(file->fd);
> +}

Actually, on further thoughts...

> +static struct file_ops socket_ops = {
> +    .name = "socket",
> +    .read = socket_read,
> +    .write = socket_write,
> +    .close = close_socket_fd,
> +    .fstat = socket_fstat,
> +    .fcntl = socket_fcntl,
> +};

read, write and close should dispatch directly to lwip_* and not bounce
through a no-op local function.

~Andrew


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 09/12] mini-os: use file_ops and get_file_from_fd() for console
  2022-01-12 11:23   ` Andrew Cooper
@ 2022-01-12 11:30     ` Juergen Gross
  0 siblings, 0 replies; 48+ messages in thread
From: Juergen Gross @ 2022-01-12 11:30 UTC (permalink / raw)
  To: Andrew Cooper, minios-devel, xen-devel; +Cc: samuel.thibault, wl


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

On 12.01.22 12:23, Andrew Cooper wrote:
> On 11/01/2022 15:12, Juergen Gross wrote:
>> +static int savefile_write(int fd, const void *buf, size_t nbytes)
>> +{
>> +    int ret = 0, tot = nbytes;
>> +    struct file *file = get_file_from_fd(fd);
>> +
>> +    while ( nbytes > 0 )
>> +    {
>> +        ret = xencons_ring_send(file->dev, (char *)buf, nbytes);
>> +        nbytes -= ret;
>> +        buf = (char *)buf + ret;
>> +    }
>> +
>> +    return tot - nbytes;
>> +}
>> +
>> +static int console_write(int fd, const void *buf, size_t nbytes)
>> +{
>> +    struct file *file = get_file_from_fd(fd);
>> +
>> +    console_print(file->dev, (char *)buf, nbytes);
> 
> I've just noticed this while committing the previous series, and I know
> it is a preexisting bug, but the casts here are utterly unsafe, because
> they're casting away constness.
> 
> console_print() is easy to fix, and just requires a prototype
> adjustment.  That said, it also desperately also needs to fix 'int
> length' to size_t to avoid problems with negative length VLAs on the stack.
> 
> xencons_ring_send() already takes const char *, so I'm pretty sure you
> can just drop the casts here.  It too ought to not truncate size_t bytes
> to "unsigned".

Okay, 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 --]

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 10/12] mini-os: add struct file_ops for file type socket
  2022-01-12 11:25   ` Andrew Cooper
@ 2022-01-12 11:31     ` Juergen Gross
  0 siblings, 0 replies; 48+ messages in thread
From: Juergen Gross @ 2022-01-12 11:31 UTC (permalink / raw)
  To: Andrew Cooper, minios-devel, xen-devel; +Cc: samuel.thibault, wl


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

On 12.01.22 12:25, Andrew Cooper wrote:
> On 11/01/2022 15:12, Juergen Gross wrote:
>> +static int socket_write(int fd, const void *buf, size_t nbytes)
>> +{
>> +    return lwip_write(fd, (void *)buf, nbytes);
> 
> This void cast was bogus before, and can be dropped.  lwip_write()
> already takes a const pointer.

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 --]

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 10/12] mini-os: add struct file_ops for file type socket
  2022-01-12 11:28   ` Andrew Cooper
@ 2022-01-12 11:32     ` Juergen Gross
  2022-01-12 11:33       ` Andrew Cooper
  0 siblings, 1 reply; 48+ messages in thread
From: Juergen Gross @ 2022-01-12 11:32 UTC (permalink / raw)
  To: Andrew Cooper, minios-devel, xen-devel; +Cc: samuel.thibault, wl


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

On 12.01.22 12:28, Andrew Cooper wrote:
> On 11/01/2022 15:12, Juergen Gross wrote:
>> diff --git a/lib/sys.c b/lib/sys.c
>> index 3a8aa68..12deaed 100644
>> --- a/lib/sys.c
>> +++ b/lib/sys.c
>> @@ -99,11 +99,70 @@ static struct file_ops file_ops_none = {
>>       .name = "none",
>>   };
>>   
>> +#ifdef HAVE_LWIP
>> +static int socket_read(int fd, void *buf, size_t nbytes)
>> +{
>> +    return lwip_read(fd, buf, nbytes);
>> +}
>> +
>> +static int socket_write(int fd, const void *buf, size_t nbytes)
>> +{
>> +    return lwip_write(fd, (void *)buf, nbytes);
>> +}
>> +
>> +static int close_socket_fd(int fd)
>> +{
>> +    struct file *file = get_file_from_fd(fd);
>> +
>> +    return lwip_close(file->fd);
>> +}
> 
> Actually, on further thoughts...
> 
>> +static struct file_ops socket_ops = {
>> +    .name = "socket",
>> +    .read = socket_read,
>> +    .write = socket_write,
>> +    .close = close_socket_fd,
>> +    .fstat = socket_fstat,
>> +    .fcntl = socket_fcntl,
>> +};
> 
> read, write and close should dispatch directly to lwip_* and not bounce
> through a no-op local function.

Not with changing the first parameter to struct file *.

BTW, this patch had a bug, as the calls need to use file->fd instead of
fd.


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 --]

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 10/12] mini-os: add struct file_ops for file type socket
  2022-01-12 11:32     ` Juergen Gross
@ 2022-01-12 11:33       ` Andrew Cooper
  0 siblings, 0 replies; 48+ messages in thread
From: Andrew Cooper @ 2022-01-12 11:33 UTC (permalink / raw)
  To: Juergen Gross, minios-devel, xen-devel; +Cc: samuel.thibault, wl

On 12/01/2022 11:32, Juergen Gross wrote:
> On 12.01.22 12:28, Andrew Cooper wrote:
>> On 11/01/2022 15:12, Juergen Gross wrote:
>>> diff --git a/lib/sys.c b/lib/sys.c
>>> index 3a8aa68..12deaed 100644
>>> --- a/lib/sys.c
>>> +++ b/lib/sys.c
>>> @@ -99,11 +99,70 @@ static struct file_ops file_ops_none = {
>>>       .name = "none",
>>>   };
>>>   +#ifdef HAVE_LWIP
>>> +static int socket_read(int fd, void *buf, size_t nbytes)
>>> +{
>>> +    return lwip_read(fd, buf, nbytes);
>>> +}
>>> +
>>> +static int socket_write(int fd, const void *buf, size_t nbytes)
>>> +{
>>> +    return lwip_write(fd, (void *)buf, nbytes);
>>> +}
>>> +
>>> +static int close_socket_fd(int fd)
>>> +{
>>> +    struct file *file = get_file_from_fd(fd);
>>> +
>>> +    return lwip_close(file->fd);
>>> +}
>>
>> Actually, on further thoughts...
>>
>>> +static struct file_ops socket_ops = {
>>> +    .name = "socket",
>>> +    .read = socket_read,
>>> +    .write = socket_write,
>>> +    .close = close_socket_fd,
>>> +    .fstat = socket_fstat,
>>> +    .fcntl = socket_fcntl,
>>> +};
>>
>> read, write and close should dispatch directly to lwip_* and not bounce
>> through a no-op local function.
>
> Not with changing the first parameter to struct file *.

Oh, good point.

>
> BTW, this patch had a bug, as the calls need to use file->fd instead of
> fd.

Yay for multiple fd spaces.

~Andrew


^ permalink raw reply	[flat|nested] 48+ messages in thread

end of thread, other threads:[~2022-01-12 11:33 UTC | newest]

Thread overview: 48+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-11 15:12 [PATCH v2 00/12] mini-os: remove device specific struct file members Juergen Gross
2022-01-11 15:12 ` [PATCH v2 01/12] mini-os: remove event channel specific struct file definitions Juergen Gross
2022-01-11 19:50   ` Samuel Thibault
2022-01-11 15:12 ` [PATCH v2 02/12] mini-os: remove gnttab specific member from struct file Juergen Gross
2022-01-11 19:55   ` Samuel Thibault
2022-01-11 20:12   ` Andrew Cooper
2022-01-12  7:44     ` Juergen Gross
2022-01-11 15:12 ` [PATCH v2 03/12] mini-os: use alloc_file_type() and get_file_from_fd() in xs Juergen Gross
2022-01-11 20:06   ` Samuel Thibault
2022-01-11 20:11     ` Samuel Thibault
2022-01-11 20:14       ` Andrew Cooper
2022-01-11 20:21   ` Andrew Cooper
2022-01-12  7:52     ` Juergen Gross
2022-01-12  8:12       ` Andrew Cooper
2022-01-11 15:12 ` [PATCH v2 04/12] mini-os: use alloc_file_type() and get_file_from_fd() in tpm_tis Juergen Gross
2022-01-11 20:13   ` Samuel Thibault
2022-01-11 20:29   ` Andrew Cooper
2022-01-11 20:58     ` Jason Andryuk
2022-01-12  7:54     ` Juergen Gross
2022-01-12  8:14       ` Andrew Cooper
2022-01-11 15:12 ` [PATCH v2 05/12] mini-os: use alloc_file_type() and get_file_from_fd() in tpmfront Juergen Gross
2022-01-11 20:15   ` Samuel Thibault
2022-01-11 15:12 ` [PATCH v2 06/12] mini-os: use alloc_file_type() and get_file_from_fd() in blkfront Juergen Gross
2022-01-11 20:20   ` Samuel Thibault
2022-01-11 15:12 ` [PATCH v2 07/12] mini-os: use get_file_from_fd() in netfront Juergen Gross
2022-01-11 20:22   ` Samuel Thibault
2022-01-11 15:12 ` [PATCH v2 08/12] mini-os: use alloc_file_type() and get_file_from_fd() in fbfront Juergen Gross
2022-01-11 20:26   ` Samuel Thibault
2022-01-12  7:52     ` Juergen Gross
2022-01-11 15:12 ` [PATCH v2 09/12] mini-os: use file_ops and get_file_from_fd() for console Juergen Gross
2022-01-11 20:35   ` Samuel Thibault
2022-01-12  7:57     ` Juergen Gross
2022-01-12 10:30       ` Samuel Thibault
2022-01-12 10:30   ` Samuel Thibault
2022-01-12 11:23   ` Andrew Cooper
2022-01-12 11:30     ` Juergen Gross
2022-01-11 15:12 ` [PATCH v2 10/12] mini-os: add struct file_ops for file type socket Juergen Gross
2022-01-11 20:38   ` Samuel Thibault
2022-01-12 10:30   ` Samuel Thibault
2022-01-12 11:25   ` Andrew Cooper
2022-01-12 11:31     ` Juergen Gross
2022-01-12 11:28   ` Andrew Cooper
2022-01-12 11:32     ` Juergen Gross
2022-01-12 11:33       ` Andrew Cooper
2022-01-11 15:12 ` [PATCH v2 11/12] mini-os: add struct file_ops for FTYPE_FILE Juergen Gross
2022-01-11 20:42   ` Samuel Thibault
2022-01-11 15:12 ` [PATCH v2 12/12] mini-os: make files array private to sys.c Juergen Gross
2022-01-11 20:42   ` Samuel Thibault

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.