* [PATCH 0/7] Mini-OS: ad minimal 9pfs support
@ 2023-02-03 9:18 Juergen Gross
2023-02-03 9:18 ` [PATCH 1/7] Mini-OS: xenbus: add support for reading node from directory Juergen Gross
` (7 more replies)
0 siblings, 8 replies; 28+ messages in thread
From: Juergen Gross @ 2023-02-03 9:18 UTC (permalink / raw)
To: minios-devel, xen-devel; +Cc: samuel.thibault, wl, Juergen Gross
This series is adding minimal support to use 9pfs in Mini-OS. It is
adding a PV 9pfs frontend and the ability to open, close, read and
write files.
The series has been tested with qemu as 9pfs backend in a PVH Mini-OS
guest (I've used a slightly modified Xenstore-stubdom for that purpose
in order to reuse the build runes).
This series is meant to setup the stage for adding file based logging
support to Xenstore-stubdom and later to add live update support (being
able to save the LU data stream in a dom0 file makes this a _lot_
easier).
In order to keep Mini-OS's license I have only used the protocol docs
available on the internet [1] and the verified those with the qemu 9pfs
backend implementation (especially for supporting the 9P2000.u variant,
as qemu doesn't support the basic 9P2000 protocol).
The needed fixed values of the protocol have been taken from [2].
[1]: http://ericvh.github.io/9p-rfc/rfc9p2000.html
[2]: https://github.com/0intro/libixp
Juergen Gross (7):
Mini-OS: xenbus: add support for reading node from directory
Mini-OS: add concept of mount points
Mini-OS: add support for runtime mounts
Mini-OS: add 9pfs frontend
Mini-OS: add 9pfs transport layer
Mini-OS: add open and close handling to the 9pfs frontend
Mini-OS: add read and write support to 9pfsfront
9pfront.c | 1201 +++++++++++++++++++++++++++++++++
Config.mk | 1 +
Makefile | 1 +
arch/x86/testbuild/all-no | 1 +
arch/x86/testbuild/all-yes | 1 +
arch/x86/testbuild/newxen-yes | 1 +
include/9pfront.h | 7 +
include/lib.h | 14 +
include/xenbus.h | 6 +
lib/sys.c | 128 +++-
xenbus.c | 39 +-
11 files changed, 1378 insertions(+), 22 deletions(-)
create mode 100644 9pfront.c
create mode 100644 include/9pfront.h
--
2.35.3
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/7] Mini-OS: xenbus: add support for reading node from directory
2023-02-03 9:18 [PATCH 0/7] Mini-OS: ad minimal 9pfs support Juergen Gross
@ 2023-02-03 9:18 ` Juergen Gross
2023-02-04 14:01 ` Samuel Thibault
2023-02-03 9:18 ` [PATCH 2/7] Mini-OS: add concept of mount points Juergen Gross
` (6 subsequent siblings)
7 siblings, 1 reply; 28+ messages in thread
From: Juergen Gross @ 2023-02-03 9:18 UTC (permalink / raw)
To: minios-devel, xen-devel; +Cc: samuel.thibault, wl, Juergen Gross
Especially PV device drivers often need to read multiple Xenstore
nodes from a common dirctory. Add support for reading a string or an
unsigned value by specifying the directory and the node.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
include/xenbus.h | 6 ++++++
xenbus.c | 39 ++++++++++++++++++++++++++++++++++++---
2 files changed, 42 insertions(+), 3 deletions(-)
diff --git a/include/xenbus.h b/include/xenbus.h
index 3871f358..c0fc0ac5 100644
--- a/include/xenbus.h
+++ b/include/xenbus.h
@@ -108,6 +108,12 @@ int xenbus_read_integer(const char *path);
* read and parsing were successful, 0 if not */
int xenbus_read_uuid(const char* path, unsigned char uuid[16]);
+/* Support functions for reading values from directory/node tuple. */
+char *xenbus_read_string(xenbus_transaction_t xbt, const char *dir,
+ const char *node, char **value);
+char *xenbus_read_unsigned(xenbus_transaction_t xbt, const char *dir,
+ const char *node, unsigned int *value);
+
/* Contraction of snprintf and xenbus_write(path/node). */
char* xenbus_printf(xenbus_transaction_t xbt,
const char* node, const char* path,
diff --git a/xenbus.c b/xenbus.c
index 81e9b65d..811cde25 100644
--- a/xenbus.c
+++ b/xenbus.c
@@ -936,16 +936,21 @@ int xenbus_read_uuid(const char *path, unsigned char uuid[16])
return 1;
}
+#define BUFFER_SIZE 256
+static void xenbus_build_path(const char *dir, const char *node, char *res)
+{
+ BUG_ON(strlen(dir) + strlen(node) + 1 >= BUFFER_SIZE);
+ sprintf(res,"%s/%s", dir, node);
+}
+
char *xenbus_printf(xenbus_transaction_t xbt, const char* node,
const char* path, const char* fmt, ...)
{
-#define BUFFER_SIZE 256
char fullpath[BUFFER_SIZE];
char val[BUFFER_SIZE];
va_list args;
- BUG_ON(strlen(node) + strlen(path) + 1 >= BUFFER_SIZE);
- sprintf(fullpath,"%s/%s", node, path);
+ xenbus_build_path(node, path, fullpath);
va_start(args, fmt);
vsprintf(val, fmt, args);
va_end(args);
@@ -964,6 +969,34 @@ domid_t xenbus_get_self_id(void)
return ret;
}
+char *xenbus_read_string(xenbus_transaction_t xbt, const char *dir,
+ const char *node, char **value)
+{
+ char path[BUFFER_SIZE];
+
+ xenbus_build_path(dir, node, path);
+
+ return xenbus_read(xbt, path, value);
+}
+
+char *xenbus_read_unsigned(xenbus_transaction_t xbt, const char *dir,
+ const char *node, unsigned int *value)
+{
+ char path[BUFFER_SIZE];
+ char *msg;
+ char *str;
+
+ xenbus_build_path(dir, node, path);
+ msg = xenbus_read(xbt, path, &str);
+ if ( msg )
+ return msg;
+
+ sscanf(str, "%u", value);
+ free(str);
+
+ return NULL;
+}
+
/*
* Local variables:
* mode: C
--
2.35.3
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 2/7] Mini-OS: add concept of mount points
2023-02-03 9:18 [PATCH 0/7] Mini-OS: ad minimal 9pfs support Juergen Gross
2023-02-03 9:18 ` [PATCH 1/7] Mini-OS: xenbus: add support for reading node from directory Juergen Gross
@ 2023-02-03 9:18 ` Juergen Gross
2023-02-05 12:40 ` Samuel Thibault
2023-02-05 12:45 ` Samuel Thibault
2023-02-03 9:18 ` [PATCH 3/7] Mini-OS: add support for runtime mounts Juergen Gross
` (5 subsequent siblings)
7 siblings, 2 replies; 28+ messages in thread
From: Juergen Gross @ 2023-02-03 9:18 UTC (permalink / raw)
To: minios-devel, xen-devel; +Cc: samuel.thibault, wl, Juergen Gross
Add the concept of mount points to Mini-OS. A mount point is a path
associated with a device pointer and an open() callback. A mount point
can be either a file (e.g. "/dev/mem") or a directory ("/var/log").
This allows to replace the special casing in the generic open()
handling with a generic mount point handling.
Prepare the open() callbacks to support creating new files by adding a
mode parameter.
Additionally add a close() prototype to include/lib.h, as it is missing
today.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
include/lib.h | 9 ++++++
lib/sys.c | 80 +++++++++++++++++++++++++++++++++++++++------------
2 files changed, 70 insertions(+), 19 deletions(-)
diff --git a/include/lib.h b/include/lib.h
index bec99646..36d94ec4 100644
--- a/include/lib.h
+++ b/include/lib.h
@@ -187,6 +187,13 @@ struct file_ops {
bool (*select_wr)(struct file *file);
};
+struct mount_point {
+ const char *path;
+ int (*open)(struct mount_point *mnt, const char *pathname, int flags,
+ mode_t mode);
+ void *dev;
+};
+
unsigned int alloc_file_type(const struct file_ops *ops);
off_t lseek_default(struct file *file, off_t offset, int whence);
@@ -198,6 +205,8 @@ int alloc_fd(unsigned int type);
void close_all_files(void);
extern struct thread *main_thread;
void sparse(unsigned long data, size_t size);
+
+int close(int fd);
#endif
#endif /* _LIB_H_ */
diff --git a/lib/sys.c b/lib/sys.c
index 8f8a3de2..1475c621 100644
--- a/lib/sys.c
+++ b/lib/sys.c
@@ -263,11 +263,6 @@ char *getcwd(char *buf, size_t size)
return buf;
}
-#define LOG_PATH "/var/log/"
-#define SAVE_PATH "/var/lib/xen"
-#define SAVE_CONSOLE 1
-#define RESTORE_CONSOLE 2
-
int mkdir(const char *pathname, mode_t mode)
{
errno = EIO;
@@ -286,18 +281,30 @@ int posix_openpt(int flags)
return fd;
}
+static int open_pt(struct mount_point *mnt, const char *pathname, int flags,
+ mode_t mode)
+{
+ return posix_openpt(flags);
+}
+
int open_savefile(const char *path, int save)
{
int fd;
char nodename[64];
- snprintf(nodename, sizeof(nodename), "device/console/%d", save ? SAVE_CONSOLE : RESTORE_CONSOLE);
+ snprintf(nodename, sizeof(nodename), "device/console/%d", save ? 1 : 2);
fd = open_consfront(nodename);
printk("fd(%d) = open_savefile\n", fd);
return fd;
}
+
+static int open_save(struct mount_point *mnt, const char *pathname, int flags,
+ mode_t mode)
+{
+ return open_savefile(pathname, flags & O_WRONLY);
+}
#else
int posix_openpt(int flags)
{
@@ -311,24 +318,59 @@ int open_savefile(const char *path, int save)
}
#endif
-int open(const char *pathname, int flags, ...)
+static int open_log(struct mount_point *mnt, const char *pathname, int flags,
+ mode_t mode)
{
int fd;
+
/* Ugly, but fine. */
- if (!strncmp(pathname,LOG_PATH,strlen(LOG_PATH))) {
- fd = alloc_fd(FTYPE_CONSOLE);
- printk("open(%s) -> %d\n", pathname, fd);
- return fd;
+ fd = alloc_fd(FTYPE_CONSOLE);
+ printk("open(%s) -> %d\n", pathname, fd);
+ return fd;
+}
+
+static int open_mem(struct mount_point *mnt, const char *pathname, int flags,
+ mode_t mode)
+{
+ int fd;
+
+ fd = alloc_fd(FTYPE_MEM);
+ printk("open(%s) -> %d\n", pathname, fd);
+ return fd;
+}
+
+static struct mount_point mount_points[] = {
+ { .path = "/var/log", .open = open_log, .dev = NULL },
+ { .path = "/dev/mem", .open = open_mem, .dev = NULL },
+#ifdef CONFIG_CONSFRONT
+ { .path = "/dev/ptmx", .open = open_pt, .dev = NULL },
+ { .path = "/var/lib/xen", .open = open_save, .dev = NULL },
+#endif
+};
+
+int open(const char *pathname, int flags, ...)
+{
+ unsigned int m, mlen;
+ struct mount_point *mnt;
+ mode_t mode = 0;
+ va_list ap;
+
+ if ( flags & O_CREAT )
+ {
+ va_start(ap, flags);
+ mode = va_arg(ap, mode_t);
+ va_end(ap);
}
- if (!strncmp(pathname, "/dev/mem", strlen("/dev/mem"))) {
- fd = alloc_fd(FTYPE_MEM);
- printk("open(/dev/mem) -> %d\n", fd);
- return fd;
+
+ for ( m = 0; m < ARRAY_SIZE(mount_points); m++ )
+ {
+ mnt = mount_points + m;
+ mlen = strlen(mnt->path);
+ if ( !strncmp(pathname, mnt->path, mlen) &&
+ (pathname[mlen] == '/' || pathname[mlen] == 0) )
+ return mnt->open(mnt, pathname, flags, mode);
}
- if (!strncmp(pathname, "/dev/ptmx", strlen("/dev/ptmx")))
- return posix_openpt(flags);
- if (!strncmp(pathname,SAVE_PATH,strlen(SAVE_PATH)))
- return open_savefile(pathname, flags & O_WRONLY);
+
errno = EIO;
return -1;
}
--
2.35.3
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 3/7] Mini-OS: add support for runtime mounts
2023-02-03 9:18 [PATCH 0/7] Mini-OS: ad minimal 9pfs support Juergen Gross
2023-02-03 9:18 ` [PATCH 1/7] Mini-OS: xenbus: add support for reading node from directory Juergen Gross
2023-02-03 9:18 ` [PATCH 2/7] Mini-OS: add concept of mount points Juergen Gross
@ 2023-02-03 9:18 ` Juergen Gross
2023-02-05 12:42 ` Samuel Thibault
2023-02-03 9:18 ` [PATCH 4/7] Mini-OS: add 9pfs frontend Juergen Gross
` (4 subsequent siblings)
7 siblings, 1 reply; 28+ messages in thread
From: Juergen Gross @ 2023-02-03 9:18 UTC (permalink / raw)
To: minios-devel, xen-devel; +Cc: samuel.thibault, wl, Juergen Gross
Add the support to mount a device at runtime. The number of dynamic
mounts is limited by a #define.
For devices supporting multiple files struct file is modified to hold
a pointer to file specific data in contrast to device specific data.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
include/lib.h | 5 +++++
lib/sys.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 54 insertions(+), 1 deletion(-)
diff --git a/include/lib.h b/include/lib.h
index 36d94ec4..fd8c36de 100644
--- a/include/lib.h
+++ b/include/lib.h
@@ -172,6 +172,7 @@ struct file {
union {
int fd; /* Any fd from an upper layer. */
void *dev;
+ void *filedata;
};
};
@@ -194,6 +195,10 @@ struct mount_point {
void *dev;
};
+int mount(const char *path, void *dev,
+ int (*open)(struct mount_point *, const char *, int, mode_t));
+void umount(const char *path);
+
unsigned int alloc_file_type(const struct file_ops *ops);
off_t lseek_default(struct file *file, off_t offset, int whence);
diff --git a/lib/sys.c b/lib/sys.c
index 1475c621..4171bfd6 100644
--- a/lib/sys.c
+++ b/lib/sys.c
@@ -339,7 +339,14 @@ static int open_mem(struct mount_point *mnt, const char *pathname, int flags,
return fd;
}
-static struct mount_point mount_points[] = {
+#ifdef CONFIG_CONSFRONT
+#define STATIC_MNTS 4
+#else
+#define STATIC_MNTS 2
+#endif
+#define DYNAMIC_MNTS 8
+
+static struct mount_point mount_points[STATIC_MNTS + DYNAMIC_MNTS] = {
{ .path = "/var/log", .open = open_log, .dev = NULL },
{ .path = "/dev/mem", .open = open_mem, .dev = NULL },
#ifdef CONFIG_CONSFRONT
@@ -365,6 +372,8 @@ int open(const char *pathname, int flags, ...)
for ( m = 0; m < ARRAY_SIZE(mount_points); m++ )
{
mnt = mount_points + m;
+ if ( !mnt->path )
+ continue;
mlen = strlen(mnt->path);
if ( !strncmp(pathname, mnt->path, mlen) &&
(pathname[mlen] == '/' || pathname[mlen] == 0) )
@@ -375,6 +384,45 @@ int open(const char *pathname, int flags, ...)
return -1;
}
+int mount(const char *path, void *dev,
+ int (*open)(struct mount_point *, const char *, int, mode_t))
+{
+ unsigned int m;
+ struct mount_point *mnt;
+
+ for ( m = 0; m < ARRAY_SIZE(mount_points); m++ )
+ {
+ mnt = mount_points + m;
+ if ( !mnt->path )
+ {
+ mnt->path = strdup(path);
+ mnt->open = open;
+ mnt->dev = dev;
+ return 0;
+ }
+ }
+
+ errno = ENOSPC;
+ return -1;
+}
+
+void umount(const char *path)
+{
+ unsigned int m;
+ struct mount_point *mnt;
+
+ for ( m = 0; m < ARRAY_SIZE(mount_points); m++ )
+ {
+ mnt = mount_points + m;
+ if ( mnt->path && !strcmp(mnt->path, path) )
+ {
+ free((char *)mnt->path);
+ mnt->path = NULL;
+ return;
+ }
+ }
+}
+
int isatty(int fd)
{
return files[fd].type == FTYPE_CONSOLE;
--
2.35.3
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 4/7] Mini-OS: add 9pfs frontend
2023-02-03 9:18 [PATCH 0/7] Mini-OS: ad minimal 9pfs support Juergen Gross
` (2 preceding siblings ...)
2023-02-03 9:18 ` [PATCH 3/7] Mini-OS: add support for runtime mounts Juergen Gross
@ 2023-02-03 9:18 ` Juergen Gross
2023-02-06 9:01 ` Samuel Thibault
2023-02-03 9:18 ` [PATCH 5/7] Mini-OS: add 9pfs transport layer Juergen Gross
` (3 subsequent siblings)
7 siblings, 1 reply; 28+ messages in thread
From: Juergen Gross @ 2023-02-03 9:18 UTC (permalink / raw)
To: minios-devel, xen-devel; +Cc: samuel.thibault, wl, Juergen Gross
Add a frontend for the 9pfs PV device. For now add only the code needed
to connect to the backend and the related disconnect functionality. The
9pfs protocol support will be added later.
Due to its nature (ability to access files) the whole code is guarded
by "#ifdef HAVE_LIBC".
Signed-off-by: Juergen Gross <jgross@suse.com>
---
9pfront.c | 280 ++++++++++++++++++++++++++++++++++
Config.mk | 1 +
Makefile | 1 +
arch/x86/testbuild/all-no | 1 +
arch/x86/testbuild/all-yes | 1 +
arch/x86/testbuild/newxen-yes | 1 +
include/9pfront.h | 7 +
7 files changed, 292 insertions(+)
create mode 100644 9pfront.c
create mode 100644 include/9pfront.h
diff --git a/9pfront.c b/9pfront.c
new file mode 100644
index 00000000..cf4b5cb3
--- /dev/null
+++ b/9pfront.c
@@ -0,0 +1,280 @@
+/*
+ * Minimal 9pfs PV frontend for Mini-OS.
+ * Copyright (c) 2023 Juergen Gross, SUSE Software Solution GmbH
+ */
+
+#include <mini-os/os.h>
+#include <mini-os/lib.h>
+#include <mini-os/events.h>
+#include <mini-os/gnttab.h>
+#include <mini-os/xenbus.h>
+#include <mini-os/xmalloc.h>
+#include <errno.h>
+#include <xen/io/9pfs.h>
+#include <mini-os/9pfront.h>
+
+#ifdef HAVE_LIBC
+struct dev_9pfs {
+ int id;
+ char nodename[20];
+ unsigned int dom;
+ char *backend;
+
+ char *tag;
+ const char *mnt;
+
+ struct xen_9pfs_data_intf *intf;
+ struct xen_9pfs_data data;
+ RING_IDX prod_pvt_out;
+ RING_IDX cons_pvt_in;
+
+ grant_ref_t ring_ref;
+ evtchn_port_t evtchn;
+ unsigned int ring_order;
+ xenbus_event_queue events;
+};
+
+#define DEFAULT_9PFS_RING_ORDER 4
+
+static unsigned int ftype_9pfs;
+
+static void intr_9pfs(evtchn_port_t port, struct pt_regs *regs, void *data)
+{
+}
+
+static int open_9pfs(struct mount_point *mnt, const char *pathname, int flags,
+ mode_t mode)
+{
+ errno = ENOSYS;
+
+ return -1;
+}
+
+static void free_9pfront(struct dev_9pfs *dev)
+{
+ unsigned int i;
+
+ if ( dev->data.in && dev->intf )
+ {
+ for ( i = 0; i < (1 << dev->ring_order); i++ )
+ gnttab_end_access(dev->intf->ref[i]);
+ free_pages(dev->data.in, dev->ring_order);
+ }
+ unbind_evtchn(dev->evtchn);
+ gnttab_end_access(dev->ring_ref);
+ free_page(dev->intf);
+ free(dev->backend);
+ free(dev->tag);
+ free(dev);
+}
+
+void *init_9pfront(unsigned int id, const char *mnt)
+{
+ struct dev_9pfs *dev;
+ char *msg;
+ char *reason = "";
+ xenbus_transaction_t xbt;
+ int retry = 1;
+ char bepath[64] = { 0 };
+ XenbusState state;
+ unsigned int i;
+ void *addr;
+ char *version;
+ char *v;
+
+ printk("9pfsfront add %u, for mount at %s\n", id, mnt);
+ dev = malloc(sizeof(*dev));
+ memset(dev, 0, sizeof(*dev));
+ snprintf(dev->nodename, sizeof(dev->nodename), "device/9pfs/%u", id);
+ dev->id = id;
+
+ msg = xenbus_read_unsigned(XBT_NIL, dev->nodename, "backend-id", &dev->dom);
+ if ( msg )
+ goto err;
+ msg = xenbus_read_string(XBT_NIL, dev->nodename, "backend", &dev->backend);
+ if ( msg )
+ goto err;
+ msg = xenbus_read_string(XBT_NIL, dev->nodename, "tag", &dev->tag);
+ if ( msg )
+ goto err;
+
+ snprintf(bepath, sizeof(bepath), "%s/state", dev->backend);
+ free(xenbus_watch_path_token(XBT_NIL, bepath, bepath, &dev->events));
+ state = xenbus_read_integer(bepath);
+ while ( msg == NULL && state < XenbusStateInitWait )
+ msg = xenbus_wait_for_state_change(bepath, &state, &dev->events);
+ if ( msg || state != XenbusStateInitWait )
+ {
+ reason = "illegal backend state";
+ goto err;
+ }
+
+ msg = xenbus_read_unsigned(XBT_NIL, dev->backend, "max-ring-page-order",
+ &dev->ring_order);
+ if ( msg )
+ goto err;
+ if ( dev->ring_order > DEFAULT_9PFS_RING_ORDER )
+ dev->ring_order = DEFAULT_9PFS_RING_ORDER;
+
+ msg = xenbus_read_string(XBT_NIL, dev->backend, "versions", &version);
+ if ( msg )
+ goto err;
+ for ( v = version; *v; v++ )
+ {
+ if ( strtoul(v, &v, 10) == 1 )
+ {
+ v = NULL;
+ break;
+ }
+ }
+ free(version);
+ if ( v )
+ {
+ reason = "version 1 not supported";
+ goto err;
+ }
+
+ dev->ring_ref = gnttab_alloc_and_grant((void **)&dev->intf);
+ memset(dev->intf, 0, PAGE_SIZE);
+ if ( evtchn_alloc_unbound(dev->dom, intr_9pfs, dev, &dev->evtchn) )
+ {
+ reason = "no event channel";
+ goto err;
+ }
+ dev->intf->ring_order = dev->ring_order;
+ dev->data.in = (void *)alloc_pages(dev->ring_order);
+ dev->data.out = dev->data.in + XEN_FLEX_RING_SIZE(dev->ring_order);
+ for ( i = 0; i < (1 << dev->ring_order); i++ )
+ {
+ addr = dev->data.in + i * PAGE_SIZE;
+ dev->intf->ref[i] = gnttab_grant_access(dev->dom, virt_to_mfn(addr), 0);
+ }
+
+ while ( retry )
+ {
+ msg = xenbus_transaction_start(&xbt);
+ if ( msg )
+ {
+ free(msg);
+ msg = NULL;
+ reason = "starting transaction";
+ goto err;
+ }
+
+ msg = xenbus_printf(xbt, dev->nodename, "version", "%u", 1);
+ if ( msg )
+ goto err_tr;
+ msg = xenbus_printf(xbt, dev->nodename, "num-rings", "%u", 1);
+ if ( msg )
+ goto err_tr;
+ msg = xenbus_printf(xbt, dev->nodename, "ring-ref0", "%u",
+ dev->ring_ref);
+ if ( msg )
+ goto err_tr;
+ msg = xenbus_printf(xbt, dev->nodename, "event-channel-0", "%u",
+ dev->evtchn);
+ if ( msg )
+ goto err_tr;
+ msg = xenbus_printf(xbt, dev->nodename, "state", "%u",
+ XenbusStateInitialised);
+ if ( msg )
+ goto err_tr;
+
+ free(xenbus_transaction_end(xbt, 0, &retry));
+ }
+
+ state = xenbus_read_integer(bepath);
+ while ( msg == NULL && state < XenbusStateConnected )
+ msg = xenbus_wait_for_state_change(bepath, &state, &dev->events);
+ if ( msg || state != XenbusStateConnected )
+ {
+ reason = "illegal backend state";
+ goto err;
+ }
+
+ msg = xenbus_printf(XBT_NIL, dev->nodename, "state", "%u",
+ XenbusStateConnected);
+ if ( msg )
+ goto err;
+
+ unmask_evtchn(dev->evtchn);
+
+ dev->mnt = mnt;
+ if ( mount(dev->mnt, dev, open_9pfs) )
+ {
+ reason = "mount failed";
+ goto err;
+ }
+
+ return dev;
+
+ err_tr:
+ free(xenbus_transaction_end(xbt, 1, &retry));
+
+ err:
+ if ( bepath[0] )
+ free(xenbus_unwatch_path_token(XBT_NIL, bepath, bepath));
+ if ( msg )
+ printk("9pfsfront add %u failed, error %s accessing Xenstore\n",
+ id, msg);
+ else
+ printk("9pfsfront add %u failed, %s\n", id, reason);
+ free_9pfront(dev);
+ free(msg);
+ return NULL;
+}
+
+void shutdown_9pfront(void *dev)
+{
+ struct dev_9pfs *dev9p = dev;
+ char bepath[64];
+ XenbusState state;
+ char *msg;
+ char *reason = "";
+
+ umount(dev9p->mnt);
+ snprintf(bepath, sizeof(bepath), "%s/state", dev9p->backend);
+
+ msg = xenbus_printf(XBT_NIL, dev9p->nodename, "state", "%u",
+ XenbusStateClosing);
+ if ( msg )
+ goto err;
+
+ state = xenbus_read_integer(bepath);
+ while ( msg == NULL && state < XenbusStateClosing)
+ msg = xenbus_wait_for_state_change(bepath, &state, &dev9p->events);
+ if ( msg || state != XenbusStateClosing )
+ {
+ reason = "illegal backend state";
+ goto err;
+ }
+
+ msg = xenbus_printf(XBT_NIL, dev9p->nodename, "state", "%u",
+ XenbusStateClosed);
+ if ( msg )
+ goto err;
+
+ free_9pfront(dev9p);
+
+ return;
+
+ err:
+ if ( msg )
+ printk("9pfsfront shutdown %u failed, error %s accessing Xenstore\n",
+ dev9p->id, msg);
+ else
+ printk("9pfsfront shutdown %u failed, %s\n", dev9p->id, reason);
+ free(msg);
+}
+
+static const struct file_ops ops_9pfs = {
+ .name = "9pfs",
+};
+
+__attribute__((constructor))
+static void initialize_9pfs(void)
+{
+ ftype_9pfs = alloc_file_type(&ops_9pfs);
+}
+
+#endif /* HAVE_LIBC */
diff --git a/Config.mk b/Config.mk
index 1a24b30e..677e93df 100644
--- a/Config.mk
+++ b/Config.mk
@@ -187,6 +187,7 @@ CONFIG-n += CONFIG_QEMU_XS_ARGS
CONFIG-n += CONFIG_TEST
CONFIG-n += CONFIG_PCIFRONT
CONFIG-n += CONFIG_TPMFRONT
+CONFIG-n += CONFIG_9PFRONT
CONFIG-n += CONFIG_TPM_TIS
CONFIG-n += CONFIG_TPMBACK
CONFIG-n += CONFIG_BALLOON
diff --git a/Makefile b/Makefile
index 747c7c01..7ee181a2 100644
--- a/Makefile
+++ b/Makefile
@@ -39,6 +39,7 @@ SUBDIRS := lib
src-$(CONFIG_BLKFRONT) += blkfront.c
src-$(CONFIG_CONSFRONT) += consfront.c
src-$(CONFIG_TPMFRONT) += tpmfront.c
+src-$(CONFIG_9PFRONT) += 9pfront.c
src-$(CONFIG_TPM_TIS) += tpm_tis.c
src-$(CONFIG_TPMBACK) += tpmback.c
src-y += console.c
diff --git a/arch/x86/testbuild/all-no b/arch/x86/testbuild/all-no
index f79a1012..5b3e99ed 100644
--- a/arch/x86/testbuild/all-no
+++ b/arch/x86/testbuild/all-no
@@ -12,6 +12,7 @@ CONFIG_NETFRONT = n
CONFIG_FBFRONT = n
CONFIG_KBDFRONT = n
CONFIG_CONSFRONT = n
+CONFIG_9PFRONT = n
CONFIG_XENBUS = n
CONFIG_LIBXS = n
CONFIG_LWIP = n
diff --git a/arch/x86/testbuild/all-yes b/arch/x86/testbuild/all-yes
index faa3af32..8ae489a4 100644
--- a/arch/x86/testbuild/all-yes
+++ b/arch/x86/testbuild/all-yes
@@ -12,6 +12,7 @@ CONFIG_NETFRONT = y
CONFIG_FBFRONT = y
CONFIG_KBDFRONT = y
CONFIG_CONSFRONT = y
+CONFIG_9PFRONT = y
CONFIG_XENBUS = y
CONFIG_LIBXS = y
CONFIG_BALLOON = y
diff --git a/arch/x86/testbuild/newxen-yes b/arch/x86/testbuild/newxen-yes
index dc83e670..ee27a328 100644
--- a/arch/x86/testbuild/newxen-yes
+++ b/arch/x86/testbuild/newxen-yes
@@ -12,6 +12,7 @@ CONFIG_NETFRONT = y
CONFIG_FBFRONT = y
CONFIG_KBDFRONT = y
CONFIG_CONSFRONT = y
+CONFIG_9PFRONT = y
CONFIG_XENBUS = y
CONFIG_LIBXS = y
CONFIG_BALLOON = y
diff --git a/include/9pfront.h b/include/9pfront.h
new file mode 100644
index 00000000..722ec564
--- /dev/null
+++ b/include/9pfront.h
@@ -0,0 +1,7 @@
+#ifndef __9PFRONT_H__
+#define __9PFRONT_H__
+
+void *init_9pfront(unsigned int id, const char *mnt);
+void shutdown_9pfront(void *dev);
+
+#endif /* __9PFRONT_H__ */
--
2.35.3
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 5/7] Mini-OS: add 9pfs transport layer
2023-02-03 9:18 [PATCH 0/7] Mini-OS: ad minimal 9pfs support Juergen Gross
` (3 preceding siblings ...)
2023-02-03 9:18 ` [PATCH 4/7] Mini-OS: add 9pfs frontend Juergen Gross
@ 2023-02-03 9:18 ` Juergen Gross
2023-02-06 9:40 ` Samuel Thibault
2023-02-03 9:18 ` [PATCH 6/7] Mini-OS: add open and close handling to the 9pfs frontend Juergen Gross
` (2 subsequent siblings)
7 siblings, 1 reply; 28+ messages in thread
From: Juergen Gross @ 2023-02-03 9:18 UTC (permalink / raw)
To: minios-devel, xen-devel; +Cc: samuel.thibault, wl, Juergen Gross
Add the transport layer of 9pfs. This is basically the infrastructure
to send requests to the backend and to receive the related answers via
the rings.
As a first example add the version and attach requests of the 9pfs
protocol when mounting a new 9pfs device. For the version use the
"9P2000.u" variant, as it is the smallest subset supported by the qemu
based backend.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
9pfront.c | 451 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 451 insertions(+)
diff --git a/9pfront.c b/9pfront.c
index cf4b5cb3..f6fac50a 100644
--- a/9pfront.c
+++ b/9pfront.c
@@ -7,6 +7,8 @@
#include <mini-os/lib.h>
#include <mini-os/events.h>
#include <mini-os/gnttab.h>
+#include <mini-os/semaphore.h>
+#include <mini-os/wait.h>
#include <mini-os/xenbus.h>
#include <mini-os/xmalloc.h>
#include <errno.h>
@@ -14,6 +16,9 @@
#include <mini-os/9pfront.h>
#ifdef HAVE_LIBC
+
+#define N_REQS 64
+
struct dev_9pfs {
int id;
char nodename[20];
@@ -22,6 +27,7 @@ struct dev_9pfs {
char *tag;
const char *mnt;
+ unsigned int msize_max;
struct xen_9pfs_data_intf *intf;
struct xen_9pfs_data data;
@@ -32,14 +38,443 @@ struct dev_9pfs {
evtchn_port_t evtchn;
unsigned int ring_order;
xenbus_event_queue events;
+
+ unsigned int free_reqs;
+ struct req {
+ unsigned int id;
+ unsigned int next_free; /* N_REQS == end of list. */
+ unsigned int cmd;
+ int result;
+ bool inflight;
+ unsigned char *data; /* Returned data. */
+ } req[N_REQS];
+
+ struct wait_queue_head waitq;
+ struct semaphore ring_out_sem;
+ struct semaphore ring_in_sem;
};
#define DEFAULT_9PFS_RING_ORDER 4
+#define P9_CMD_VERSION 100
+#define P9_CMD_ATTACH 104
+#define P9_CMD_ERROR 107
+
+struct p9_header {
+ uint32_t size;
+ uint8_t cmd;
+ uint16_t tag;
+} __attribute__((packed));
+
+#define P9_VERSION "9P2000.u"
+#define P9_ROOT_FID ~0
+
static unsigned int ftype_9pfs;
+static struct req *get_free_req(struct dev_9pfs *dev)
+{
+ struct req *req;
+
+ if ( dev->free_reqs == N_REQS )
+ return NULL;
+
+ req = dev->req + dev->free_reqs;
+ dev->free_reqs = req->next_free;
+
+ return req;
+}
+
+static void put_free_req(struct dev_9pfs *dev, struct req *req)
+{
+ req->next_free = dev->free_reqs;
+ req->inflight = false;
+ req->data = NULL;
+ dev->free_reqs = req->id;
+}
+
+static unsigned int ring_out_free(struct dev_9pfs *dev)
+{
+ RING_IDX ring_size = XEN_FLEX_RING_SIZE(dev->ring_order);
+ unsigned int queued;
+
+ queued = xen_9pfs_queued(dev->prod_pvt_out, dev->intf->out_cons, ring_size);
+ rmb();
+
+ return ring_size - queued;
+}
+
+static unsigned int ring_in_data(struct dev_9pfs *dev)
+{
+ RING_IDX ring_size = XEN_FLEX_RING_SIZE(dev->ring_order);
+ unsigned int queued;
+
+ queued = xen_9pfs_queued(dev->intf->in_prod, dev->cons_pvt_in, ring_size);
+ rmb();
+
+ return queued;
+}
+
+static void copy_to_ring(struct dev_9pfs *dev, void *data, unsigned int len)
+{
+ RING_IDX ring_size = XEN_FLEX_RING_SIZE(dev->ring_order);
+ RING_IDX prod = xen_9pfs_mask(dev->prod_pvt_out, ring_size);
+ RING_IDX cons = xen_9pfs_mask(dev->intf->out_cons, ring_size);
+
+ xen_9pfs_write_packet(dev->data.out, data, len, &prod, cons, ring_size);
+ dev->prod_pvt_out += len;
+}
+
+static void copy_from_ring(struct dev_9pfs *dev, void *data, unsigned int len)
+{
+ RING_IDX ring_size = XEN_FLEX_RING_SIZE(dev->ring_order);
+ RING_IDX prod = xen_9pfs_mask(dev->intf->in_prod, ring_size);
+ RING_IDX cons = xen_9pfs_mask(dev->cons_pvt_in, ring_size);
+
+ xen_9pfs_read_packet(data, dev->data.in, len, prod, &cons, ring_size);
+ dev->cons_pvt_in += len;
+}
+
+/*
+ * send_9p() and rcv_9p() are using a special format string for specifying
+ * the kind of data sent/expected. Each data item is represented by a single
+ * character:
+ * U: 4 byte unsigned integer (uint32_t)
+ * S: String (2 byte length + <length> characters)
+ * in the rcv_9p() case the data for string is allocated (length omitted,
+ * string terminated by a NUL character)
+ * Q: A 13 byte "qid", consisting of 1 byte file type, 4 byte file version
+ * and 8 bytes unique file id. Only valid for receiving.
+ */
+static void send_9p(struct dev_9pfs *dev, struct req *req, const char *fmt, ...)
+{
+ struct p9_header hdr;
+ va_list ap, aq;
+ const char *f;
+ uint32_t intval;
+ uint16_t len;
+ char *strval;
+
+ hdr.size = sizeof(hdr);
+ hdr.cmd = req->cmd;
+ hdr.tag = req->id;
+
+ va_start(ap, fmt);
+
+ va_copy(aq, ap);
+ for ( f = fmt; *f; f++ )
+ {
+ switch ( *f )
+ {
+ case 'U':
+ hdr.size += 4;
+ intval = va_arg(aq, unsigned int);
+ break;
+ case 'S':
+ hdr.size += 2;
+ strval = va_arg(aq, char *);
+ hdr.size += strlen(strval);
+ break;
+ default:
+ printk("send_9p: unknown format character %c\n", *f);
+ break;
+ }
+ }
+ va_end(aq);
+
+ /*
+ * Waiting for free space must be done in the critical section!
+ * Otherwise we might get overtaken by other short requests.
+ */
+ down(&dev->ring_out_sem);
+
+ wait_event(dev->waitq, ring_out_free(dev) >= hdr.size);
+
+ copy_to_ring(dev, &hdr, sizeof(hdr));
+ for ( f = fmt; *f; f++ )
+ {
+ switch ( *f )
+ {
+ case 'U':
+ intval = va_arg(ap, unsigned int);
+ copy_to_ring(dev, &intval, sizeof(intval));
+ break;
+ case 'S':
+ strval = va_arg(ap, char *);
+ len = strlen(strval);
+ copy_to_ring(dev, &len, sizeof(len));
+ copy_to_ring(dev, strval, len);
+ break;
+ }
+ }
+
+ wmb(); /* Data on ring must be seen before updating index. */
+ dev->intf->out_prod = dev->prod_pvt_out;
+ req->inflight = true;
+
+ up(&dev->ring_out_sem);
+
+ va_end(ap);
+
+ notify_remote_via_evtchn(dev->evtchn);
+}
+
+/*
+ * Using an opportunistic approach for receiving data: in case multiple
+ * requests are outstanding (which is very unlikely), we nevertheless need
+ * to consume all data available until we reach the desired request.
+ * For requests other than the one we are waiting for, we link the complete
+ * data to the request via an intermediate buffer. For our own request we can
+ * omit that buffer and directly fill the caller provided variables.
+ */
+static void copy_bufs(unsigned char **buf1, unsigned char **buf2,
+ unsigned int *len1, unsigned int *len2,
+ void *target, unsigned int len)
+{
+ if ( len <= *len1 )
+ {
+ memcpy(target, *buf1, len);
+ *buf1 += len;
+ *len1 -= len;
+ }
+ else
+ {
+ memcpy(target, *buf1, *len1);
+ target = (char *)target + *len1;
+ len -= *len1;
+ *buf1 = *buf2;
+ *len1 = *len2;
+ *buf2 = NULL;
+ *len2 = 0;
+ if ( len > *len1 )
+ len = *len1;
+ memcpy(target, *buf1, *len1);
+ }
+}
+
+static void rcv_9p_copy(struct dev_9pfs *dev, struct req *req,
+ struct p9_header *hdr, const char *fmt, va_list ap)
+{
+ struct p9_header *h = hdr ? hdr : (void *)req->data;
+ RING_IDX cons = dev->cons_pvt_in + h->size - sizeof(*h);
+ RING_IDX ring_size = XEN_FLEX_RING_SIZE(dev->ring_order);
+ unsigned char *buf1, *buf2;
+ unsigned int len1, len2;
+ const char *f;
+ char *str;
+ uint16_t len;
+ uint32_t err;
+ uint32_t *val;
+ char **strval;
+ uint8_t *qval;
+
+ if ( hdr )
+ {
+ buf1 = xen_9pfs_get_ring_ptr(dev->data.in, dev->cons_pvt_in, ring_size);
+ buf2 = xen_9pfs_get_ring_ptr(dev->data.in, 0, ring_size);
+ len1 = ring_size - xen_9pfs_mask(dev->cons_pvt_in, ring_size);
+ if ( len1 > h->size - sizeof(*h) )
+ len1 = h->size - sizeof(*h);
+ len2 = h->size - sizeof(*h) - len1;
+ }
+ else
+ {
+ buf1 = req->data + sizeof(*h);
+ buf2 = NULL;
+ len1 = h->size - sizeof(*h);
+ len2 = 0;
+ }
+
+ if ( h->cmd == P9_CMD_ERROR )
+ {
+ copy_bufs(&buf1, &buf2, &len1, &len2, &len, sizeof(len));
+ str = malloc(len + 1);
+ copy_bufs(&buf1, &buf2, &len1, &len2, str, len);
+ str[len] = 0;
+ printk("9pfs: request %u resulted in \"%s\"\n", req->cmd, str);
+ free(str);
+ err = EIO;
+ copy_bufs(&buf1, &buf2, &len1, &len2, &err, sizeof(err));
+ req->result = err;
+
+ if ( hdr )
+ dev->cons_pvt_in = cons;
+
+ return;
+ }
+
+ if ( h->cmd != req->cmd + 1 )
+ {
+ req->result = EDOM;
+ printk("9pfs: illegal response: wrong return type (%u instead of %u)\n",
+ h->cmd, req->cmd + 1);
+
+ if ( hdr )
+ dev->cons_pvt_in = cons;
+
+ return;
+ }
+
+ req->result = 0;
+
+ for ( f = fmt; *f; f++ )
+ {
+ switch ( *f )
+ {
+ case 'U':
+ val = va_arg(ap, uint32_t *);
+ copy_bufs(&buf1, &buf2, &len1, &len2, val, sizeof(*val));
+ break;
+ case 'S':
+ strval = va_arg(ap, char **);
+ copy_bufs(&buf1, &buf2, &len1, &len2, &len, sizeof(len));
+ *strval = malloc(len + 1);
+ copy_bufs(&buf1, &buf2, &len1, &len2, *strval, len);
+ (*strval)[len] = 0;
+ break;
+ case 'Q':
+ qval = va_arg(ap, uint8_t *);
+ copy_bufs(&buf1, &buf2, &len1, &len2, qval, 13);
+ break;
+ default:
+ printk("rcv_9p: unknown format character %c\n", *f);
+ break;
+ }
+ }
+
+ if ( hdr )
+ dev->cons_pvt_in = cons;
+}
+
+static bool rcv_9p_one(struct dev_9pfs *dev, struct req *req, const char *fmt,
+ va_list ap)
+{
+ struct p9_header hdr;
+ struct req *tmp;
+
+ if ( req->data )
+ {
+ rcv_9p_copy(dev, req, NULL, fmt, ap);
+ free(req->data);
+ req->data = NULL;
+
+ return true;
+ }
+
+ wait_event(dev->waitq, ring_in_data(dev) >= sizeof(hdr));
+
+ copy_from_ring(dev, &hdr, sizeof(hdr));
+
+ wait_event(dev->waitq, ring_in_data(dev) >= hdr.size - sizeof(hdr));
+
+ tmp = dev->req + hdr.tag;
+ if ( hdr.tag >= N_REQS || !tmp->inflight )
+ {
+ printk("9pfs: illegal response: %s\n",
+ hdr.tag >= N_REQS ? "tag out of bounds" : "request not pending");
+ dev->cons_pvt_in += hdr.size - sizeof(hdr);
+
+ return false;
+ }
+
+ tmp->inflight = false;
+
+ if ( tmp != req )
+ {
+ tmp->data = malloc(hdr.size);
+ memcpy(tmp->data, &hdr, sizeof(hdr));
+ copy_from_ring(dev, tmp->data + sizeof(hdr), hdr.size - sizeof(hdr));
+
+ return false;
+ }
+
+ rcv_9p_copy(dev, req, &hdr, fmt, ap);
+
+ return true;
+}
+
+static void rcv_9p(struct dev_9pfs *dev, struct req *req, const char *fmt, ...)
+{
+ va_list ap;
+
+ va_start(ap, fmt);
+
+ down(&dev->ring_in_sem);
+
+ while ( !rcv_9p_one(dev, req, fmt, ap) );
+
+ rmb(); /* Read all data before updating ring index. */
+ dev->intf->in_cons = dev->cons_pvt_in;
+
+ up(&dev->ring_in_sem);
+
+ va_end(ap);
+}
+
+static int p9_version(struct dev_9pfs *dev)
+{
+ unsigned int msize = XEN_FLEX_RING_SIZE(dev->ring_order) / 2;
+ struct req *req = get_free_req(dev);
+ char *verret;
+ int ret;
+
+ if ( !req )
+ return ENOENT;
+
+ req->cmd = P9_CMD_VERSION;
+ send_9p(dev, req, "US", msize, P9_VERSION);
+ rcv_9p(dev, req, "US", &dev->msize_max, &verret);
+ ret = req->result;
+
+ put_free_req(dev, req);
+
+ if ( ret )
+ return ret;
+
+ if ( strcmp(verret, P9_VERSION) )
+ ret = ENOMSG;
+ free(verret);
+
+ return ret;
+}
+
+static int p9_attach(struct dev_9pfs *dev)
+{
+ uint32_t fid = P9_ROOT_FID;
+ uint32_t afid = 0;
+ uint32_t uid = 0;
+ uint8_t qid[13];
+ struct req *req = get_free_req(dev);
+ int ret;
+
+ if ( !req )
+ return ENOENT;
+
+ req->cmd = P9_CMD_ATTACH;
+ send_9p(dev, req, "UUSSU", fid, afid, "root", "root", uid);
+ rcv_9p(dev, req, "Q", qid);
+ ret = req->result;
+
+ put_free_req(dev, req);
+
+ return ret;
+}
+
+static int connect_9pfs(struct dev_9pfs *dev)
+{
+ int ret;
+
+ ret = p9_version(dev);
+ if ( ret )
+ return ret;
+
+ return p9_attach(dev);
+}
+
static void intr_9pfs(evtchn_port_t port, struct pt_regs *regs, void *data)
{
+ struct dev_9pfs *dev = data;
+
+ wake_up(&dev->waitq);
}
static int open_9pfs(struct mount_point *mnt, const char *pathname, int flags,
@@ -87,6 +522,16 @@ void *init_9pfront(unsigned int id, const char *mnt)
memset(dev, 0, sizeof(*dev));
snprintf(dev->nodename, sizeof(dev->nodename), "device/9pfs/%u", id);
dev->id = id;
+ init_waitqueue_head(&dev->waitq);
+ init_SEMAPHORE(&dev->ring_out_sem, 1);
+ init_SEMAPHORE(&dev->ring_in_sem, 1);
+
+ for ( i = 0; i < N_REQS; i++ )
+ {
+ dev->req[i].id = i;
+ dev->req[i].next_free = i + 1;
+ }
+ dev->free_reqs = 0;
msg = xenbus_read_unsigned(XBT_NIL, dev->nodename, "backend-id", &dev->dom);
if ( msg )
@@ -199,6 +644,12 @@ void *init_9pfront(unsigned int id, const char *mnt)
unmask_evtchn(dev->evtchn);
+ if ( connect_9pfs(dev) )
+ {
+ reason = "9pfs connect failed";
+ goto err;
+ }
+
dev->mnt = mnt;
if ( mount(dev->mnt, dev, open_9pfs) )
{
--
2.35.3
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 6/7] Mini-OS: add open and close handling to the 9pfs frontend
2023-02-03 9:18 [PATCH 0/7] Mini-OS: ad minimal 9pfs support Juergen Gross
` (4 preceding siblings ...)
2023-02-03 9:18 ` [PATCH 5/7] Mini-OS: add 9pfs transport layer Juergen Gross
@ 2023-02-03 9:18 ` Juergen Gross
2023-02-03 12:50 ` Juergen Gross
2023-02-06 10:05 ` Samuel Thibault
2023-02-03 9:18 ` [PATCH 7/7] Mini-OS: add read and write support to 9pfsfront Juergen Gross
2023-02-06 10:14 ` [PATCH 0/7] Mini-OS: ad minimal 9pfs support Samuel Thibault
7 siblings, 2 replies; 28+ messages in thread
From: Juergen Gross @ 2023-02-03 9:18 UTC (permalink / raw)
To: minios-devel, xen-devel; +Cc: samuel.thibault, wl, Juergen Gross
Add the open() and close() support to the 9pfs frontend. This requires
to split the path name and to walk to the desired directory level.
The opened file needs to be queried via stat in order to obtain the
data needed for proper access (access rights, size, type of file).
Signed-off-by: Juergen Gross <jgross@suse.com>
---
9pfront.c | 285 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 283 insertions(+), 2 deletions(-)
diff --git a/9pfront.c b/9pfront.c
index f6fac50a..ebe48601 100644
--- a/9pfront.c
+++ b/9pfront.c
@@ -6,6 +6,7 @@
#include <mini-os/os.h>
#include <mini-os/lib.h>
#include <mini-os/events.h>
+#include <mini-os/fcntl.h>
#include <mini-os/gnttab.h>
#include <mini-os/semaphore.h>
#include <mini-os/wait.h>
@@ -52,13 +53,32 @@ struct dev_9pfs {
struct wait_queue_head waitq;
struct semaphore ring_out_sem;
struct semaphore ring_in_sem;
+
+ unsigned long long fid_mask; /* Bit mask for free fids. */
+};
+
+struct file_9pfs {
+ uint32_t fid;
+ struct dev_9pfs *dev;
+ bool append;
};
#define DEFAULT_9PFS_RING_ORDER 4
+/* P9 protocol commands (response is either cmd+1 or P9_CMD_ERROR). */
#define P9_CMD_VERSION 100
#define P9_CMD_ATTACH 104
#define P9_CMD_ERROR 107
+#define P9_CMD_WALK 110
+#define P9_CMD_OPEN 112
+#define P9_CMD_CREATE 114
+#define P9_CMD_CLUNK 120
+
+/* P9 protocol open flags. */
+#define P9_OREAD 0 /* read */
+#define P9_OWRITE 1 /* write */
+#define P9_ORDWR 2 /* read and write */
+#define P9_OTRUNC 16 /* or'ed in, truncate file first */
struct p9_header {
uint32_t size;
@@ -67,10 +87,27 @@ struct p9_header {
} __attribute__((packed));
#define P9_VERSION "9P2000.u"
-#define P9_ROOT_FID ~0
+#define P9_ROOT_FID 0
static unsigned int ftype_9pfs;
+static unsigned int get_fid(struct dev_9pfs *dev)
+{
+ unsigned int fid;
+
+ fid = ffs(dev->fid_mask);
+ if ( fid )
+ dev->fid_mask &= 1ULL << (fid - 1);
+
+ return fid;
+}
+
+static void put_fid(struct dev_9pfs *dev, unsigned int fid)
+{
+ if ( fid )
+ dev->fid_mask |= 1ULL << (fid - 1);
+}
+
static struct req *get_free_req(struct dev_9pfs *dev)
{
struct req *req;
@@ -138,6 +175,9 @@ static void copy_from_ring(struct dev_9pfs *dev, void *data, unsigned int len)
* send_9p() and rcv_9p() are using a special format string for specifying
* the kind of data sent/expected. Each data item is represented by a single
* character:
+ * b: 1 byte unsigned integer (uint8_t)
+ * Only valid for sending.
+ * u: 2 byte unsigned integer (uint16_t)
* U: 4 byte unsigned integer (uint32_t)
* S: String (2 byte length + <length> characters)
* in the rcv_9p() case the data for string is allocated (length omitted,
@@ -151,7 +191,9 @@ static void send_9p(struct dev_9pfs *dev, struct req *req, const char *fmt, ...)
va_list ap, aq;
const char *f;
uint32_t intval;
+ uint16_t shortval;
uint16_t len;
+ uint8_t byte;
char *strval;
hdr.size = sizeof(hdr);
@@ -165,6 +207,14 @@ static void send_9p(struct dev_9pfs *dev, struct req *req, const char *fmt, ...)
{
switch ( *f )
{
+ case 'b':
+ hdr.size += 1;
+ byte = va_arg(aq, unsigned int);
+ break;
+ case 'u':
+ hdr.size += 2;
+ shortval = va_arg(aq, unsigned int);
+ break;
case 'U':
hdr.size += 4;
intval = va_arg(aq, unsigned int);
@@ -194,6 +244,14 @@ static void send_9p(struct dev_9pfs *dev, struct req *req, const char *fmt, ...)
{
switch ( *f )
{
+ case 'b':
+ byte = va_arg(ap, unsigned int);
+ copy_to_ring(dev, &byte, sizeof(byte));
+ break;
+ case 'u':
+ shortval = va_arg(ap, unsigned int);
+ copy_to_ring(dev, &shortval, sizeof(shortval));
+ break;
case 'U':
intval = va_arg(ap, unsigned int);
copy_to_ring(dev, &intval, sizeof(intval));
@@ -263,6 +321,7 @@ static void rcv_9p_copy(struct dev_9pfs *dev, struct req *req,
char *str;
uint16_t len;
uint32_t err;
+ uint16_t *shortval;
uint32_t *val;
char **strval;
uint8_t *qval;
@@ -320,6 +379,10 @@ static void rcv_9p_copy(struct dev_9pfs *dev, struct req *req,
{
switch ( *f )
{
+ case 'u':
+ shortval = va_arg(ap, uint16_t *);
+ copy_bufs(&buf1, &buf2, &len1, &len2, shortval, sizeof(*shortval));
+ break;
case 'U':
val = va_arg(ap, uint32_t *);
copy_bufs(&buf1, &buf2, &len1, &len2, val, sizeof(*val));
@@ -459,6 +522,134 @@ static int p9_attach(struct dev_9pfs *dev)
return ret;
}
+static int p9_clunk(struct dev_9pfs *dev, uint32_t fid)
+{
+ struct req *req = get_free_req(dev);
+ int ret;
+
+ if ( !req )
+ return ENOENT;
+
+ req->cmd = P9_CMD_CLUNK;
+ send_9p(dev, req, "U", fid);
+ rcv_9p(dev, req, "");
+ ret = req->result;
+
+ put_free_req(dev, req);
+
+ return ret;
+}
+
+static int p9_walk(struct dev_9pfs *dev, uint32_t fid, uint32_t newfid,
+ char *name)
+{
+ struct req *req = get_free_req(dev);
+ int ret;
+ uint16_t nqid;
+ uint8_t qid[13];
+
+ if ( !req )
+ return ENOENT;
+
+ req->cmd = P9_CMD_WALK;
+ if ( name[0] )
+ send_9p(dev, req, "UUuS", fid, newfid, 1, name);
+ else
+ send_9p(dev, req, "UUu", fid, newfid, 0);
+ rcv_9p(dev, req, "uQ", &nqid, qid);
+
+ ret = req->result;
+
+ put_free_req(dev, req);
+
+ return ret;
+}
+
+static int p9_open(struct dev_9pfs *dev, uint32_t fid, uint8_t omode)
+{
+ struct req *req = get_free_req(dev);
+ int ret;
+ uint8_t qid[13];
+ uint32_t iounit;
+
+ if ( !req )
+ return ENOENT;
+
+ req->cmd = P9_CMD_OPEN;
+ send_9p(dev, req, "Ub", fid, omode);
+ rcv_9p(dev, req, "QU", qid, &iounit);
+
+ ret = req->result;
+
+ put_free_req(dev, req);
+
+ return ret;
+}
+
+static int p9_create(struct dev_9pfs *dev, uint32_t fid, char *path,
+ uint32_t mode, uint8_t omode)
+{
+ struct req *req = get_free_req(dev);
+ int ret;
+ uint8_t qid[13];
+ uint32_t iounit;
+
+ if ( !req )
+ return ENOENT;
+
+ req->cmd = P9_CMD_CREATE;
+ send_9p(dev, req, "USUbS", fid, path, mode, omode, "");
+ rcv_9p(dev, req, "QU", qid, &iounit);
+
+ ret = req->result;
+
+ put_free_req(dev, req);
+
+ return ret;
+}
+
+/*
+ * Walk from root <steps> levels with the levels listed in <*paths> as a
+ * sequence of names. Returns the number of steps not having been able to
+ * walk, with <*paths> pointing at the name of the failing walk step.
+ * <fid> will be associated with the last successful walk step. Note that
+ * the first step should always succeed, as it is an empty walk in order
+ * to start at the root (needed for creating new files in root).
+ */
+static unsigned int walk_9pfs(struct dev_9pfs *dev, uint32_t fid,
+ unsigned int steps, char **paths)
+{
+ uint32_t curr_fid = P9_ROOT_FID;
+ int ret;
+
+ while ( steps-- )
+ {
+ ret = p9_walk(dev, curr_fid, fid, *paths);
+ if ( ret )
+ return steps + 1;
+ curr_fid = fid;
+ *paths += strlen(*paths) + 1;
+ }
+
+ return 0;
+}
+
+static unsigned int split_path(const char *pathname, char **split_ptr)
+{
+ unsigned int parts = 1;
+ char *p;
+
+ *split_ptr = strdup(pathname);
+
+ for ( p = strchr(*split_ptr, '/'); p; p = strchr(p + 1, '/') )
+ {
+ *p = 0;
+ parts++;
+ }
+
+ return parts;
+}
+
static int connect_9pfs(struct dev_9pfs *dev)
{
int ret;
@@ -477,10 +668,98 @@ static void intr_9pfs(evtchn_port_t port, struct pt_regs *regs, void *data)
wake_up(&dev->waitq);
}
+static int close_9pfs(struct file *file)
+{
+ struct file_9pfs *f9pfs = file->filedata;
+
+ if ( f9pfs->fid != P9_ROOT_FID )
+ {
+ p9_clunk(f9pfs->dev, f9pfs->fid);
+ put_fid(f9pfs->dev, f9pfs->fid);
+ }
+
+ free(f9pfs);
+
+ return 0;
+}
+
static int open_9pfs(struct mount_point *mnt, const char *pathname, int flags,
mode_t mode)
{
- errno = ENOSYS;
+ int fd;
+ char *path = NULL;
+ char *p;
+ struct file *file;
+ struct file_9pfs *f9pfs;
+ uint16_t nwalk;
+ uint8_t omode;
+ int ret;
+
+ f9pfs = calloc(1, sizeof(*f9pfs));
+ f9pfs->dev = mnt->dev;
+ f9pfs->fid = P9_ROOT_FID;
+
+ fd = alloc_fd(ftype_9pfs);
+ file = get_file_from_fd(fd);
+ file->filedata = f9pfs;
+
+ switch ( flags & O_ACCMODE )
+ {
+ case O_RDONLY:
+ omode = P9_OREAD;
+ break;
+ case O_WRONLY:
+ omode = P9_OWRITE;
+ break;
+ case O_RDWR:
+ omode = P9_ORDWR;
+ break;
+ default:
+ ret = EINVAL;
+ goto err;
+ }
+
+ if ( flags & O_TRUNC )
+ omode |= P9_OTRUNC;
+ f9pfs->append = flags & O_APPEND;
+
+ nwalk = split_path(pathname + strlen(mnt->path), &path);
+
+ f9pfs->fid = get_fid(mnt->dev);
+ if ( !f9pfs->fid )
+ {
+ ret = ENFILE;
+ goto err;
+ }
+ p = path;
+ nwalk = walk_9pfs(mnt->dev, f9pfs->fid, nwalk, &p);
+ if ( nwalk )
+ {
+ if ( nwalk > 1 || !(flags & O_CREAT) )
+ {
+ ret = ENOENT;
+ goto err;
+ }
+
+ ret = p9_create(mnt->dev, f9pfs->fid, p, mode, omode);
+ if ( ret )
+ goto err;
+ goto out;
+ }
+
+ ret = p9_open(mnt->dev, f9pfs->fid, omode);
+ if ( ret )
+ goto err;
+
+ out:
+ free(path);
+
+ return fd;
+
+ err:
+ free(path);
+ close(fd);
+ errno = ret;
return -1;
}
@@ -525,6 +804,7 @@ void *init_9pfront(unsigned int id, const char *mnt)
init_waitqueue_head(&dev->waitq);
init_SEMAPHORE(&dev->ring_out_sem, 1);
init_SEMAPHORE(&dev->ring_in_sem, 1);
+ dev->fid_mask = ~0ULL;
for ( i = 0; i < N_REQS; i++ )
{
@@ -720,6 +1000,7 @@ void shutdown_9pfront(void *dev)
static const struct file_ops ops_9pfs = {
.name = "9pfs",
+ .close = close_9pfs,
};
__attribute__((constructor))
--
2.35.3
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 7/7] Mini-OS: add read and write support to 9pfsfront
2023-02-03 9:18 [PATCH 0/7] Mini-OS: ad minimal 9pfs support Juergen Gross
` (5 preceding siblings ...)
2023-02-03 9:18 ` [PATCH 6/7] Mini-OS: add open and close handling to the 9pfs frontend Juergen Gross
@ 2023-02-03 9:18 ` Juergen Gross
2023-02-03 12:52 ` Juergen Gross
2023-02-06 10:13 ` Samuel Thibault
2023-02-06 10:14 ` [PATCH 0/7] Mini-OS: ad minimal 9pfs support Samuel Thibault
7 siblings, 2 replies; 28+ messages in thread
From: Juergen Gross @ 2023-02-03 9:18 UTC (permalink / raw)
To: minios-devel, xen-devel; +Cc: samuel.thibault, wl, Juergen Gross
Add support to read from and write to a file handled by 9pfsfront.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
9pfront.c | 189 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 189 insertions(+)
diff --git a/9pfront.c b/9pfront.c
index ebe48601..45f2f9a9 100644
--- a/9pfront.c
+++ b/9pfront.c
@@ -72,7 +72,10 @@ struct file_9pfs {
#define P9_CMD_WALK 110
#define P9_CMD_OPEN 112
#define P9_CMD_CREATE 114
+#define P9_CMD_READ 116
+#define P9_CMD_WRITE 118
#define P9_CMD_CLUNK 120
+#define P9_CMD_STAT 124
/* P9 protocol open flags. */
#define P9_OREAD 0 /* read */
@@ -86,11 +89,39 @@ struct p9_header {
uint16_t tag;
} __attribute__((packed));
+struct p9_stat {
+ uint16_t size;
+ uint16_t type;
+ uint32_t dev;
+ uint8_t qid[13];
+ uint32_t mode;
+ uint32_t atime;
+ uint32_t mtime;
+ uint64_t length;
+ char *name;
+ char *uid;
+ char *gid;
+ char *muid;
+ char *extension;
+ uint32_t n_uid;
+ uint32_t n_gid;
+ uint32_t n_muid;
+};
+
#define P9_VERSION "9P2000.u"
#define P9_ROOT_FID 0
static unsigned int ftype_9pfs;
+static void free_stat(struct p9_stat *stat)
+{
+ free(stat->name);
+ free(stat->uid);
+ free(stat->gid);
+ free(stat->muid);
+ free(stat->extension);
+}
+
static unsigned int get_fid(struct dev_9pfs *dev)
{
unsigned int fid;
@@ -179,9 +210,12 @@ static void copy_from_ring(struct dev_9pfs *dev, void *data, unsigned int len)
* Only valid for sending.
* u: 2 byte unsigned integer (uint16_t)
* U: 4 byte unsigned integer (uint32_t)
+ * L: 8 byte unsigned integer (uint64_t)
* S: String (2 byte length + <length> characters)
* in the rcv_9p() case the data for string is allocated (length omitted,
* string terminated by a NUL character)
+ * D: Binary data (4 byte length + <length> bytes of data), requires a length
+ * and a buffer pointer parameter.
* Q: A 13 byte "qid", consisting of 1 byte file type, 4 byte file version
* and 8 bytes unique file id. Only valid for receiving.
*/
@@ -190,10 +224,12 @@ static void send_9p(struct dev_9pfs *dev, struct req *req, const char *fmt, ...)
struct p9_header hdr;
va_list ap, aq;
const char *f;
+ uint64_t longval;
uint32_t intval;
uint16_t shortval;
uint16_t len;
uint8_t byte;
+ uint8_t *data;
char *strval;
hdr.size = sizeof(hdr);
@@ -219,11 +255,21 @@ static void send_9p(struct dev_9pfs *dev, struct req *req, const char *fmt, ...)
hdr.size += 4;
intval = va_arg(aq, unsigned int);
break;
+ case 'L':
+ hdr.size += 8;
+ longval = va_arg(aq, uint64_t);
+ break;
case 'S':
hdr.size += 2;
strval = va_arg(aq, char *);
hdr.size += strlen(strval);
break;
+ case 'D':
+ hdr.size += 4;
+ intval = va_arg(aq, unsigned int);
+ hdr.size += intval;
+ data = va_arg(aq, uint8_t *);
+ break;
default:
printk("send_9p: unknown format character %c\n", *f);
break;
@@ -256,12 +302,22 @@ static void send_9p(struct dev_9pfs *dev, struct req *req, const char *fmt, ...)
intval = va_arg(ap, unsigned int);
copy_to_ring(dev, &intval, sizeof(intval));
break;
+ case 'L':
+ longval = va_arg(ap, uint64_t);
+ copy_to_ring(dev, &longval, sizeof(longval));
+ break;
case 'S':
strval = va_arg(ap, char *);
len = strlen(strval);
copy_to_ring(dev, &len, sizeof(len));
copy_to_ring(dev, strval, len);
break;
+ case 'D':
+ intval = va_arg(ap, unsigned int);
+ copy_to_ring(dev, &intval, sizeof(intval));
+ data = va_arg(ap, uint8_t *);
+ copy_to_ring(dev, data, intval);
+ break;
}
}
@@ -323,6 +379,8 @@ static void rcv_9p_copy(struct dev_9pfs *dev, struct req *req,
uint32_t err;
uint16_t *shortval;
uint32_t *val;
+ uint64_t *longval;
+ uint8_t *data;
char **strval;
uint8_t *qval;
@@ -387,6 +445,10 @@ static void rcv_9p_copy(struct dev_9pfs *dev, struct req *req,
val = va_arg(ap, uint32_t *);
copy_bufs(&buf1, &buf2, &len1, &len2, val, sizeof(*val));
break;
+ case 'L':
+ longval = va_arg(ap, uint64_t *);
+ copy_bufs(&buf1, &buf2, &len1, &len2, longval, sizeof(*longval));
+ break;
case 'S':
strval = va_arg(ap, char **);
copy_bufs(&buf1, &buf2, &len1, &len2, &len, sizeof(len));
@@ -394,6 +456,12 @@ static void rcv_9p_copy(struct dev_9pfs *dev, struct req *req,
copy_bufs(&buf1, &buf2, &len1, &len2, *strval, len);
(*strval)[len] = 0;
break;
+ case 'D':
+ val = va_arg(ap, uint32_t *);
+ data = va_arg(ap, uint8_t *);
+ copy_bufs(&buf1, &buf2, &len1, &len2, val, sizeof(*val));
+ copy_bufs(&buf1, &buf2, &len1, &len2, data, *val);
+ break;
case 'Q':
qval = va_arg(ap, uint8_t *);
copy_bufs(&buf1, &buf2, &len1, &len2, qval, 13);
@@ -608,6 +676,88 @@ static int p9_create(struct dev_9pfs *dev, uint32_t fid, char *path,
return ret;
}
+static int p9_stat(struct dev_9pfs *dev, uint32_t fid, struct p9_stat *stat)
+{
+ struct req *req = get_free_req(dev);
+ int ret;
+
+ if ( !req )
+ return ENOENT;
+
+ memset(stat, 0, sizeof(*stat));
+ req->cmd = P9_CMD_STAT;
+ send_9p(dev, req, "U", fid);
+ rcv_9p(dev, req, "uuUQUUULSSSSSUUU", &stat->size, &stat->type, &stat->dev,
+ stat->qid, &stat->mode, &stat->atime, &stat->mtime, &stat->length,
+ &stat->name, &stat->uid, &stat->gid, &stat->muid, &stat->extension,
+ &stat->n_uid, &stat->n_gid, &stat->n_muid);
+
+ ret = req->result;
+
+ put_free_req(dev, req);
+
+ return ret;
+}
+
+static int p9_read(struct dev_9pfs *dev, uint32_t fid, uint64_t offset,
+ uint8_t *data, uint32_t len)
+{
+ struct req *req = get_free_req(dev);
+ int ret;
+ uint32_t count;
+
+ if ( !req )
+ {
+ errno = EIO;
+ return -1;
+ }
+
+ req->cmd = P9_CMD_READ;
+ send_9p(dev, req, "ULU", fid, offset, len);
+ rcv_9p(dev, req, "D", &count, data);
+
+ if ( req->result )
+ {
+ ret = -1;
+ errno = EIO;
+ }
+ else
+ ret = count;
+
+ put_free_req(dev, req);
+
+ return ret;
+}
+
+static int p9_write(struct dev_9pfs *dev, uint32_t fid, uint64_t offset,
+ const uint8_t *data, uint32_t len)
+{
+ struct req *req = get_free_req(dev);
+ int ret;
+ uint32_t count;
+
+ if ( !req )
+ {
+ errno = EIO;
+ return -1;
+ }
+
+ req->cmd = P9_CMD_WRITE;
+ send_9p(dev, req, "ULD", fid, offset, len, data);
+ rcv_9p(dev, req, "U", &count);
+ if ( req->result )
+ {
+ ret = -1;
+ errno = EIO;
+ }
+ else
+ ret = count;
+
+ put_free_req(dev, req);
+
+ return ret;
+}
+
/*
* Walk from root <steps> levels with the levels listed in <*paths> as a
* sequence of names. Returns the number of steps not having been able to
@@ -668,6 +818,43 @@ static void intr_9pfs(evtchn_port_t port, struct pt_regs *regs, void *data)
wake_up(&dev->waitq);
}
+static int read_9pfs(struct file *file, void *buf, size_t nbytes)
+{
+ struct file_9pfs *f9pfs = file->filedata;
+ int ret;
+
+ ret = p9_read(f9pfs->dev, f9pfs->fid, file->offset, buf, nbytes);
+ if ( ret >= 0 )
+ file->offset += ret;
+
+ return ret;
+}
+
+static int write_9pfs(struct file *file, const void *buf, size_t nbytes)
+{
+ struct file_9pfs *f9pfs = file->filedata;
+ struct p9_stat stat;
+ int ret;
+
+ if ( f9pfs->append )
+ {
+ ret = p9_stat(f9pfs->dev, f9pfs->fid, &stat);
+ free_stat(&stat);
+ if ( ret )
+ {
+ errno = EIO;
+ return -1;
+ }
+ file->offset = stat.length;
+ }
+
+ ret = p9_write(f9pfs->dev, f9pfs->fid, file->offset, buf, nbytes);
+ if ( ret >= 0 )
+ file->offset += ret;
+
+ return ret;
+}
+
static int close_9pfs(struct file *file)
{
struct file_9pfs *f9pfs = file->filedata;
@@ -1000,6 +1187,8 @@ void shutdown_9pfront(void *dev)
static const struct file_ops ops_9pfs = {
.name = "9pfs",
+ .read = read_9pfs,
+ .write = write_9pfs,
.close = close_9pfs,
};
--
2.35.3
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 6/7] Mini-OS: add open and close handling to the 9pfs frontend
2023-02-03 9:18 ` [PATCH 6/7] Mini-OS: add open and close handling to the 9pfs frontend Juergen Gross
@ 2023-02-03 12:50 ` Juergen Gross
2023-02-06 10:05 ` Samuel Thibault
1 sibling, 0 replies; 28+ messages in thread
From: Juergen Gross @ 2023-02-03 12:50 UTC (permalink / raw)
To: minios-devel, xen-devel; +Cc: samuel.thibault, wl
[-- Attachment #1.1.1: Type: text/plain, Size: 391 bytes --]
On 03.02.23 10:18, Juergen Gross wrote:
> Add the open() and close() support to the 9pfs frontend. This requires
> to split the path name and to walk to the desired directory level.
>
> The opened file needs to be queried via stat in order to obtain the
> data needed for proper access (access rights, size, type of file).
Oh, sorry, this paragraph should be dropped.
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 7/7] Mini-OS: add read and write support to 9pfsfront
2023-02-03 9:18 ` [PATCH 7/7] Mini-OS: add read and write support to 9pfsfront Juergen Gross
@ 2023-02-03 12:52 ` Juergen Gross
2023-02-06 10:13 ` Samuel Thibault
1 sibling, 0 replies; 28+ messages in thread
From: Juergen Gross @ 2023-02-03 12:52 UTC (permalink / raw)
To: minios-devel, xen-devel; +Cc: samuel.thibault, wl
[-- Attachment #1.1.1: Type: text/plain, Size: 291 bytes --]
On 03.02.23 10:18, Juergen Gross wrote:
> Add support to read from and write to a file handled by 9pfsfront.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
This patch is missing the limitation of read/write messages to stay
below the max. supported message size.
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/7] Mini-OS: xenbus: add support for reading node from directory
2023-02-03 9:18 ` [PATCH 1/7] Mini-OS: xenbus: add support for reading node from directory Juergen Gross
@ 2023-02-04 14:01 ` Samuel Thibault
2023-02-06 9:23 ` Juergen Gross
0 siblings, 1 reply; 28+ messages in thread
From: Samuel Thibault @ 2023-02-04 14:01 UTC (permalink / raw)
To: Juergen Gross; +Cc: minios-devel, xen-devel, wl
Hello,
Juergen Gross, le ven. 03 févr. 2023 10:18:03 +0100, a ecrit:
> +char *xenbus_read_unsigned(xenbus_transaction_t xbt, const char *dir,
> + const char *node, unsigned int *value)
> +{
> + char path[BUFFER_SIZE];
> + char *msg;
> + char *str;
> +
> + xenbus_build_path(dir, node, path);
> + msg = xenbus_read(xbt, path, &str);
> + if ( msg )
> + return msg;
> +
> + sscanf(str, "%u", value);
I'd say better check that sscanf returned 1 and otherwise return an
error. Otherwise *value may end up uninitialized.
> + free(str);
> +
> + return NULL;
> +}
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/7] Mini-OS: add concept of mount points
2023-02-03 9:18 ` [PATCH 2/7] Mini-OS: add concept of mount points Juergen Gross
@ 2023-02-05 12:40 ` Samuel Thibault
2023-02-05 12:45 ` Samuel Thibault
1 sibling, 0 replies; 28+ messages in thread
From: Samuel Thibault @ 2023-02-05 12:40 UTC (permalink / raw)
To: Juergen Gross; +Cc: minios-devel, xen-devel, wl
Juergen Gross, le ven. 03 févr. 2023 10:18:04 +0100, a ecrit:
> Add the concept of mount points to Mini-OS. A mount point is a path
> associated with a device pointer and an open() callback. A mount point
> can be either a file (e.g. "/dev/mem") or a directory ("/var/log").
>
> This allows to replace the special casing in the generic open()
> handling with a generic mount point handling.
>
> Prepare the open() callbacks to support creating new files by adding a
> mode parameter.
>
> Additionally add a close() prototype to include/lib.h, as it is missing
> today.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Thanks!
> ---
> include/lib.h | 9 ++++++
> lib/sys.c | 80 +++++++++++++++++++++++++++++++++++++++------------
> 2 files changed, 70 insertions(+), 19 deletions(-)
>
> diff --git a/include/lib.h b/include/lib.h
> index bec99646..36d94ec4 100644
> --- a/include/lib.h
> +++ b/include/lib.h
> @@ -187,6 +187,13 @@ struct file_ops {
> bool (*select_wr)(struct file *file);
> };
>
> +struct mount_point {
> + const char *path;
> + int (*open)(struct mount_point *mnt, const char *pathname, int flags,
> + mode_t mode);
> + void *dev;
> +};
> +
> unsigned int alloc_file_type(const struct file_ops *ops);
>
> off_t lseek_default(struct file *file, off_t offset, int whence);
> @@ -198,6 +205,8 @@ int alloc_fd(unsigned int type);
> void close_all_files(void);
> extern struct thread *main_thread;
> void sparse(unsigned long data, size_t size);
> +
> +int close(int fd);
> #endif
>
> #endif /* _LIB_H_ */
> diff --git a/lib/sys.c b/lib/sys.c
> index 8f8a3de2..1475c621 100644
> --- a/lib/sys.c
> +++ b/lib/sys.c
> @@ -263,11 +263,6 @@ char *getcwd(char *buf, size_t size)
> return buf;
> }
>
> -#define LOG_PATH "/var/log/"
> -#define SAVE_PATH "/var/lib/xen"
> -#define SAVE_CONSOLE 1
> -#define RESTORE_CONSOLE 2
> -
> int mkdir(const char *pathname, mode_t mode)
> {
> errno = EIO;
> @@ -286,18 +281,30 @@ int posix_openpt(int flags)
> return fd;
> }
>
> +static int open_pt(struct mount_point *mnt, const char *pathname, int flags,
> + mode_t mode)
> +{
> + return posix_openpt(flags);
> +}
> +
> int open_savefile(const char *path, int save)
> {
> int fd;
> char nodename[64];
>
> - snprintf(nodename, sizeof(nodename), "device/console/%d", save ? SAVE_CONSOLE : RESTORE_CONSOLE);
> + snprintf(nodename, sizeof(nodename), "device/console/%d", save ? 1 : 2);
>
> fd = open_consfront(nodename);
> printk("fd(%d) = open_savefile\n", fd);
>
> return fd;
> }
> +
> +static int open_save(struct mount_point *mnt, const char *pathname, int flags,
> + mode_t mode)
> +{
> + return open_savefile(pathname, flags & O_WRONLY);
> +}
> #else
> int posix_openpt(int flags)
> {
> @@ -311,24 +318,59 @@ int open_savefile(const char *path, int save)
> }
> #endif
>
> -int open(const char *pathname, int flags, ...)
> +static int open_log(struct mount_point *mnt, const char *pathname, int flags,
> + mode_t mode)
> {
> int fd;
> +
> /* Ugly, but fine. */
> - if (!strncmp(pathname,LOG_PATH,strlen(LOG_PATH))) {
> - fd = alloc_fd(FTYPE_CONSOLE);
> - printk("open(%s) -> %d\n", pathname, fd);
> - return fd;
> + fd = alloc_fd(FTYPE_CONSOLE);
> + printk("open(%s) -> %d\n", pathname, fd);
> + return fd;
> +}
> +
> +static int open_mem(struct mount_point *mnt, const char *pathname, int flags,
> + mode_t mode)
> +{
> + int fd;
> +
> + fd = alloc_fd(FTYPE_MEM);
> + printk("open(%s) -> %d\n", pathname, fd);
> + return fd;
> +}
> +
> +static struct mount_point mount_points[] = {
> + { .path = "/var/log", .open = open_log, .dev = NULL },
> + { .path = "/dev/mem", .open = open_mem, .dev = NULL },
> +#ifdef CONFIG_CONSFRONT
> + { .path = "/dev/ptmx", .open = open_pt, .dev = NULL },
> + { .path = "/var/lib/xen", .open = open_save, .dev = NULL },
> +#endif
> +};
> +
> +int open(const char *pathname, int flags, ...)
> +{
> + unsigned int m, mlen;
> + struct mount_point *mnt;
> + mode_t mode = 0;
> + va_list ap;
> +
> + if ( flags & O_CREAT )
> + {
> + va_start(ap, flags);
> + mode = va_arg(ap, mode_t);
> + va_end(ap);
> }
> - if (!strncmp(pathname, "/dev/mem", strlen("/dev/mem"))) {
> - fd = alloc_fd(FTYPE_MEM);
> - printk("open(/dev/mem) -> %d\n", fd);
> - return fd;
> +
> + for ( m = 0; m < ARRAY_SIZE(mount_points); m++ )
> + {
> + mnt = mount_points + m;
> + mlen = strlen(mnt->path);
> + if ( !strncmp(pathname, mnt->path, mlen) &&
> + (pathname[mlen] == '/' || pathname[mlen] == 0) )
> + return mnt->open(mnt, pathname, flags, mode);
> }
> - if (!strncmp(pathname, "/dev/ptmx", strlen("/dev/ptmx")))
> - return posix_openpt(flags);
> - if (!strncmp(pathname,SAVE_PATH,strlen(SAVE_PATH)))
> - return open_savefile(pathname, flags & O_WRONLY);
> +
> errno = EIO;
> return -1;
> }
> --
> 2.35.3
>
--
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/7] Mini-OS: add support for runtime mounts
2023-02-03 9:18 ` [PATCH 3/7] Mini-OS: add support for runtime mounts Juergen Gross
@ 2023-02-05 12:42 ` Samuel Thibault
0 siblings, 0 replies; 28+ messages in thread
From: Samuel Thibault @ 2023-02-05 12:42 UTC (permalink / raw)
To: Juergen Gross; +Cc: minios-devel, xen-devel, wl
Juergen Gross, le ven. 03 févr. 2023 10:18:05 +0100, a ecrit:
> Add the support to mount a device at runtime. The number of dynamic
> mounts is limited by a #define.
>
> For devices supporting multiple files struct file is modified to hold
> a pointer to file specific data in contrast to device specific data.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> ---
> include/lib.h | 5 +++++
> lib/sys.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 54 insertions(+), 1 deletion(-)
>
> diff --git a/include/lib.h b/include/lib.h
> index 36d94ec4..fd8c36de 100644
> --- a/include/lib.h
> +++ b/include/lib.h
> @@ -172,6 +172,7 @@ struct file {
> union {
> int fd; /* Any fd from an upper layer. */
> void *dev;
> + void *filedata;
> };
> };
>
> @@ -194,6 +195,10 @@ struct mount_point {
> void *dev;
> };
>
> +int mount(const char *path, void *dev,
> + int (*open)(struct mount_point *, const char *, int, mode_t));
> +void umount(const char *path);
> +
> unsigned int alloc_file_type(const struct file_ops *ops);
>
> off_t lseek_default(struct file *file, off_t offset, int whence);
> diff --git a/lib/sys.c b/lib/sys.c
> index 1475c621..4171bfd6 100644
> --- a/lib/sys.c
> +++ b/lib/sys.c
> @@ -339,7 +339,14 @@ static int open_mem(struct mount_point *mnt, const char *pathname, int flags,
> return fd;
> }
>
> -static struct mount_point mount_points[] = {
> +#ifdef CONFIG_CONSFRONT
> +#define STATIC_MNTS 4
> +#else
> +#define STATIC_MNTS 2
> +#endif
> +#define DYNAMIC_MNTS 8
> +
> +static struct mount_point mount_points[STATIC_MNTS + DYNAMIC_MNTS] = {
> { .path = "/var/log", .open = open_log, .dev = NULL },
> { .path = "/dev/mem", .open = open_mem, .dev = NULL },
> #ifdef CONFIG_CONSFRONT
> @@ -365,6 +372,8 @@ int open(const char *pathname, int flags, ...)
> for ( m = 0; m < ARRAY_SIZE(mount_points); m++ )
> {
> mnt = mount_points + m;
> + if ( !mnt->path )
> + continue;
> mlen = strlen(mnt->path);
> if ( !strncmp(pathname, mnt->path, mlen) &&
> (pathname[mlen] == '/' || pathname[mlen] == 0) )
> @@ -375,6 +384,45 @@ int open(const char *pathname, int flags, ...)
> return -1;
> }
>
> +int mount(const char *path, void *dev,
> + int (*open)(struct mount_point *, const char *, int, mode_t))
> +{
> + unsigned int m;
> + struct mount_point *mnt;
> +
> + for ( m = 0; m < ARRAY_SIZE(mount_points); m++ )
> + {
> + mnt = mount_points + m;
> + if ( !mnt->path )
> + {
> + mnt->path = strdup(path);
> + mnt->open = open;
> + mnt->dev = dev;
> + return 0;
> + }
> + }
> +
> + errno = ENOSPC;
> + return -1;
> +}
> +
> +void umount(const char *path)
> +{
> + unsigned int m;
> + struct mount_point *mnt;
> +
> + for ( m = 0; m < ARRAY_SIZE(mount_points); m++ )
> + {
> + mnt = mount_points + m;
> + if ( mnt->path && !strcmp(mnt->path, path) )
> + {
> + free((char *)mnt->path);
> + mnt->path = NULL;
> + return;
> + }
> + }
> +}
> +
> int isatty(int fd)
> {
> return files[fd].type == FTYPE_CONSOLE;
> --
> 2.35.3
>
--
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/7] Mini-OS: add concept of mount points
2023-02-03 9:18 ` [PATCH 2/7] Mini-OS: add concept of mount points Juergen Gross
2023-02-05 12:40 ` Samuel Thibault
@ 2023-02-05 12:45 ` Samuel Thibault
2023-02-06 9:25 ` Juergen Gross
1 sibling, 1 reply; 28+ messages in thread
From: Samuel Thibault @ 2023-02-05 12:45 UTC (permalink / raw)
To: Juergen Gross; +Cc: minios-devel, xen-devel, wl
Juergen Gross, le ven. 03 févr. 2023 10:18:04 +0100, a ecrit:
> +int open(const char *pathname, int flags, ...)
> +{
> + unsigned int m, mlen;
> + struct mount_point *mnt;
> + mode_t mode = 0;
> + va_list ap;
> +
> + if ( flags & O_CREAT )
> + {
> + va_start(ap, flags);
> + mode = va_arg(ap, mode_t);
> + va_end(ap);
> }
> - if (!strncmp(pathname, "/dev/mem", strlen("/dev/mem"))) {
> - fd = alloc_fd(FTYPE_MEM);
> - printk("open(/dev/mem) -> %d\n", fd);
> - return fd;
> +
> + for ( m = 0; m < ARRAY_SIZE(mount_points); m++ )
> + {
> + mnt = mount_points + m;
> + mlen = strlen(mnt->path);
> + if ( !strncmp(pathname, mnt->path, mlen) &&
> + (pathname[mlen] == '/' || pathname[mlen] == 0) )
> + return mnt->open(mnt, pathname, flags, mode);
Thinking about it more: don't we want to pass pathname+mlen?
So that the open function doesn't have to care where it's mounted.
Samuel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 4/7] Mini-OS: add 9pfs frontend
2023-02-03 9:18 ` [PATCH 4/7] Mini-OS: add 9pfs frontend Juergen Gross
@ 2023-02-06 9:01 ` Samuel Thibault
2023-02-06 9:22 ` Juergen Gross
0 siblings, 1 reply; 28+ messages in thread
From: Samuel Thibault @ 2023-02-06 9:01 UTC (permalink / raw)
To: Juergen Gross; +Cc: minios-devel, xen-devel, wl
Juergen Gross, le ven. 03 févr. 2023 10:18:06 +0100, a ecrit:
> +void *init_9pfront(unsigned int id, const char *mnt)
> +{
[...]
> + free(xenbus_watch_path_token(XBT_NIL, bepath, bepath, &dev->events));
Better check for errors, otherwise the rest will hang without useful
feedback.
> + for ( v = version; *v; v++ )
> + {
> + if ( strtoul(v, &v, 10) == 1 )
> + {
> + v = NULL;
> + break;
This looks fragile? if version is "2.1" it will accept it apparently? I
guess better check whether strtoul did read a number, and in that case
break the loop anyway, successfully if the number is 1 and with failure
otherwise.
> + }
> + }
> + free(version);
> + if ( v )
> + {
> + reason = "version 1 not supported";
> + goto err;
> + }
This looks odd: when number 1 is detected this breaks out successfully,
while the error message otherwise says that it's version 1 which is not
supported? Is the message supposed to be "version greater than 1 not
supported"?
> + err:
> + if ( bepath[0] )
> + free(xenbus_unwatch_path_token(XBT_NIL, bepath, bepath));
> + if ( msg )
> + printk("9pfsfront add %u failed, error %s accessing Xenstore\n",
> + id, msg);
> + else
> + printk("9pfsfront add %u failed, %s\n", id, reason);
> + free_9pfront(dev);
In case of early errors, this will try to free uninitialized evtchn,
ring_ref, etc.
> + free(msg);
> + return NULL;
> +}
Samuel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 4/7] Mini-OS: add 9pfs frontend
2023-02-06 9:01 ` Samuel Thibault
@ 2023-02-06 9:22 ` Juergen Gross
2023-02-06 9:48 ` Samuel Thibault
0 siblings, 1 reply; 28+ messages in thread
From: Juergen Gross @ 2023-02-06 9:22 UTC (permalink / raw)
To: Samuel Thibault, minios-devel, xen-devel, wl
[-- Attachment #1.1.1: Type: text/plain, Size: 2065 bytes --]
On 06.02.23 10:01, Samuel Thibault wrote:
> Juergen Gross, le ven. 03 févr. 2023 10:18:06 +0100, a ecrit:
>> +void *init_9pfront(unsigned int id, const char *mnt)
>> +{
> [...]
>> + free(xenbus_watch_path_token(XBT_NIL, bepath, bepath, &dev->events));
>
> Better check for errors, otherwise the rest will hang without useful
> feedback.
This is a common pattern in Mini-OS frontends.
I can add an error check, of course.
>
>> + for ( v = version; *v; v++ )
>> + {
>> + if ( strtoul(v, &v, 10) == 1 )
>> + {
>> + v = NULL;
>> + break;
>
> This looks fragile? if version is "2.1" it will accept it apparently? I
> guess better check whether strtoul did read a number, and in that case
> break the loop anyway, successfully if the number is 1 and with failure
> otherwise.
Versions are defined to be integers.
I can add checks for sanitizing backend written data, but I'm not sure we
need that. In case the backend wants to fool us, it can easily tell us to
support version 1 even if it doesn't.
>
>> + }
>> + }
>> + free(version);
>> + if ( v )
>> + {
>> + reason = "version 1 not supported";
>> + goto err;
>> + }
>
> This looks odd: when number 1 is detected this breaks out successfully,
> while the error message otherwise says that it's version 1 which is not
> supported? Is the message supposed to be "version greater than 1 not
> supported"?
I can change the message to "Backend doesn't support version 1".
>
>> + err:
>> + if ( bepath[0] )
>> + free(xenbus_unwatch_path_token(XBT_NIL, bepath, bepath));
>> + if ( msg )
>> + printk("9pfsfront add %u failed, error %s accessing Xenstore\n",
>> + id, msg);
>> + else
>> + printk("9pfsfront add %u failed, %s\n", id, reason);
>> + free_9pfront(dev);
>
> In case of early errors, this will try to free uninitialized evtchn,
> ring_ref, etc.
Oh right, I need to check those for being not 0.
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/7] Mini-OS: xenbus: add support for reading node from directory
2023-02-04 14:01 ` Samuel Thibault
@ 2023-02-06 9:23 ` Juergen Gross
0 siblings, 0 replies; 28+ messages in thread
From: Juergen Gross @ 2023-02-06 9:23 UTC (permalink / raw)
To: Samuel Thibault, minios-devel, xen-devel, wl
[-- Attachment #1.1.1: Type: text/plain, Size: 683 bytes --]
On 04.02.23 15:01, Samuel Thibault wrote:
> Hello,
>
> Juergen Gross, le ven. 03 févr. 2023 10:18:03 +0100, a ecrit:
>> +char *xenbus_read_unsigned(xenbus_transaction_t xbt, const char *dir,
>> + const char *node, unsigned int *value)
>> +{
>> + char path[BUFFER_SIZE];
>> + char *msg;
>> + char *str;
>> +
>> + xenbus_build_path(dir, node, path);
>> + msg = xenbus_read(xbt, path, &str);
>> + if ( msg )
>> + return msg;
>> +
>> + sscanf(str, "%u", value);
>
> I'd say better check that sscanf returned 1 and otherwise return an
> error. Otherwise *value may end up uninitialized.
Okay.
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/7] Mini-OS: add concept of mount points
2023-02-05 12:45 ` Samuel Thibault
@ 2023-02-06 9:25 ` Juergen Gross
0 siblings, 0 replies; 28+ messages in thread
From: Juergen Gross @ 2023-02-06 9:25 UTC (permalink / raw)
To: Samuel Thibault, minios-devel, xen-devel, wl
[-- Attachment #1.1.1: Type: text/plain, Size: 1263 bytes --]
On 05.02.23 13:45, Samuel Thibault wrote:
> Juergen Gross, le ven. 03 févr. 2023 10:18:04 +0100, a ecrit:
>> +int open(const char *pathname, int flags, ...)
>> +{
>> + unsigned int m, mlen;
>> + struct mount_point *mnt;
>> + mode_t mode = 0;
>> + va_list ap;
>> +
>> + if ( flags & O_CREAT )
>> + {
>> + va_start(ap, flags);
>> + mode = va_arg(ap, mode_t);
>> + va_end(ap);
>> }
>> - if (!strncmp(pathname, "/dev/mem", strlen("/dev/mem"))) {
>> - fd = alloc_fd(FTYPE_MEM);
>> - printk("open(/dev/mem) -> %d\n", fd);
>> - return fd;
>> +
>> + for ( m = 0; m < ARRAY_SIZE(mount_points); m++ )
>> + {
>> + mnt = mount_points + m;
>> + mlen = strlen(mnt->path);
>> + if ( !strncmp(pathname, mnt->path, mlen) &&
>> + (pathname[mlen] == '/' || pathname[mlen] == 0) )
>> + return mnt->open(mnt, pathname, flags, mode);
>
> Thinking about it more: don't we want to pass pathname+mlen?
>
> So that the open function doesn't have to care where it's mounted.
I think both variants have their pros and cons.
As the open functions have the mount point available via the mnt parameter
I can change it.
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 5/7] Mini-OS: add 9pfs transport layer
2023-02-03 9:18 ` [PATCH 5/7] Mini-OS: add 9pfs transport layer Juergen Gross
@ 2023-02-06 9:40 ` Samuel Thibault
2023-02-06 10:23 ` Juergen Gross
0 siblings, 1 reply; 28+ messages in thread
From: Samuel Thibault @ 2023-02-06 9:40 UTC (permalink / raw)
To: Juergen Gross; +Cc: minios-devel, xen-devel, wl
Juergen Gross, le ven. 03 févr. 2023 10:18:07 +0100, a ecrit:
> +/*
> + * Using an opportunistic approach for receiving data: in case multiple
> + * requests are outstanding (which is very unlikely), we nevertheless need
> + * to consume all data available until we reach the desired request.
> + * For requests other than the one we are waiting for, we link the complete
> + * data to the request via an intermediate buffer. For our own request we can
> + * omit that buffer and directly fill the caller provided variables.
> + */
This documents the "why" which is very welcome, but does not document
what the function does exactly (AIUI, copy from buf1+buf2 into target up
to len bytes, and update buf/len accordingly).
> +static void copy_bufs(unsigned char **buf1, unsigned char **buf2,
> + unsigned int *len1, unsigned int *len2,
> + void *target, unsigned int len)
> +{
> + if ( len <= *len1 )
> + {
> + memcpy(target, *buf1, len);
> + *buf1 += len;
> + *len1 -= len;
> + }
> + else
> + {
> + memcpy(target, *buf1, *len1);
> + target = (char *)target + *len1;
> + len -= *len1;
> + *buf1 = *buf2;
> + *len1 = *len2;
> + *buf2 = NULL;
> + *len2 = 0;
> + if ( len > *len1 )
> + len = *len1;
But then this is a short copy, don't we need to error out, or at least
log something? Normally the other end is not supposed to push data
incrementaly, but better at least catch such misprogramming.
> + memcpy(target, *buf1, *len1);
> + }
> +}
> +
> +static void rcv_9p_copy(struct dev_9pfs *dev, struct req *req,
> + struct p9_header *hdr, const char *fmt, va_list ap)
Please document what this helper function does (at the very least which
way the copy happens). The hdr != NULL vs == NULL notably needs to be
explained.
> + case 'Q':
> + qval = va_arg(ap, uint8_t *);
> + copy_bufs(&buf1, &buf2, &len1, &len2, qval, 13);
I'd say make this 13 a macro.
> +static void rcv_9p(struct dev_9pfs *dev, struct req *req, const char *fmt, ...)
> +{
> + va_list ap;
> +
> + va_start(ap, fmt);
> +
> + down(&dev->ring_in_sem);
> +
> + while ( !rcv_9p_one(dev, req, fmt, ap) );
> +
> + rmb(); /* Read all data before updating ring index. */
> + dev->intf->in_cons = dev->cons_pvt_in;
Shouldn't there be an event notification after updating the consumption
index?
> + up(&dev->ring_in_sem);
> +
> + va_end(ap);
> +}
Samuel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 4/7] Mini-OS: add 9pfs frontend
2023-02-06 9:22 ` Juergen Gross
@ 2023-02-06 9:48 ` Samuel Thibault
0 siblings, 0 replies; 28+ messages in thread
From: Samuel Thibault @ 2023-02-06 9:48 UTC (permalink / raw)
To: Juergen Gross; +Cc: minios-devel, xen-devel, wl
Juergen Gross, le lun. 06 févr. 2023 10:22:10 +0100, a ecrit:
> On 06.02.23 10:01, Samuel Thibault wrote:
> > Juergen Gross, le ven. 03 févr. 2023 10:18:06 +0100, a ecrit:
> > > +void *init_9pfront(unsigned int id, const char *mnt)
> > > +{
> > [...]
> > > + free(xenbus_watch_path_token(XBT_NIL, bepath, bepath, &dev->events));
> >
> > Better check for errors, otherwise the rest will hang without useful
> > feedback.
>
> This is a common pattern in Mini-OS frontends.
Ah, indeed. Ok, then.
> > > + for ( v = version; *v; v++ )
> > > + {
> > > + if ( strtoul(v, &v, 10) == 1 )
> > > + {
> > > + v = NULL;
> > > + break;
> >
> > This looks fragile? if version is "2.1" it will accept it apparently? I
> > guess better check whether strtoul did read a number, and in that case
> > break the loop anyway, successfully if the number is 1 and with failure
> > otherwise.
>
> Versions are defined to be integers.
Ah, it's a list of versions?
(I don't know the protocol at all).
> I can add checks for sanitizing backend written data, but I'm not sure we
> need that. In case the backend wants to fool us, it can easily tell us to
> support version 1 even if it doesn't.
Not necessarily fooling, but misprogramming :)
> > > + }
> > > + }
> > > + free(version);
> > > + if ( v )
> > > + {
> > > + reason = "version 1 not supported";
> > > + goto err;
> > > + }
> >
> > This looks odd: when number 1 is detected this breaks out successfully,
> > while the error message otherwise says that it's version 1 which is not
> > supported? Is the message supposed to be "version greater than 1 not
> > supported"?
>
> I can change the message to "Backend doesn't support version 1".
Aah, yes indeed, that was the part I missed for understanding it all :)
Samuel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 6/7] Mini-OS: add open and close handling to the 9pfs frontend
2023-02-03 9:18 ` [PATCH 6/7] Mini-OS: add open and close handling to the 9pfs frontend Juergen Gross
2023-02-03 12:50 ` Juergen Gross
@ 2023-02-06 10:05 ` Samuel Thibault
2023-02-06 10:15 ` Juergen Gross
1 sibling, 1 reply; 28+ messages in thread
From: Samuel Thibault @ 2023-02-06 10:05 UTC (permalink / raw)
To: Juergen Gross; +Cc: minios-devel, xen-devel, wl
Juergen Gross, le ven. 03 févr. 2023 10:18:08 +0100, a ecrit:
> +static unsigned int get_fid(struct dev_9pfs *dev)
> +{
> + unsigned int fid;
> +
> + fid = ffs(dev->fid_mask);
> + if ( fid )
> + dev->fid_mask &= 1ULL << (fid - 1);
Isn't that missing a ~ ?
> @@ -459,6 +522,134 @@ static int p9_attach(struct dev_9pfs *dev)
> return ret;
> }
>
> +static int p9_clunk(struct dev_9pfs *dev, uint32_t fid)
> +{
> + struct req *req = get_free_req(dev);
> + int ret;
> +
> + if ( !req )
> + return ENOENT;
Rather EAGAIN? (and similar in other functions)
(actually p9_version and p9_attach could be updated too, even if in
their case it's not supposed to happen.
> + req->cmd = P9_CMD_CLUNK;
> + send_9p(dev, req, "U", fid);
> + rcv_9p(dev, req, "");
> + ret = req->result;
> +
> + put_free_req(dev, req);
> +
> + return ret;
> +}
> +static unsigned int split_path(const char *pathname, char **split_ptr)
> +{
> + unsigned int parts = 1;
> + char *p;
> +
> + *split_ptr = strdup(pathname);
> +
> + for ( p = strchr(*split_ptr, '/'); p; p = strchr(p + 1, '/') )
> + {
> + *p = 0;
> + parts++;
> + }
> +
> + return parts;
> +}
Do we need to care about an empty part if we're passed "/foo//bar"?
> static int open_9pfs(struct mount_point *mnt, const char *pathname, int flags,
> mode_t mode)
> {
> - errno = ENOSYS;
> + int fd;
> + char *path = NULL;
> + char *p;
> + struct file *file;
> + struct file_9pfs *f9pfs;
> + uint16_t nwalk;
> + uint8_t omode;
> + int ret;
> +
> + f9pfs = calloc(1, sizeof(*f9pfs));
> + f9pfs->dev = mnt->dev;
> + f9pfs->fid = P9_ROOT_FID;
> +
> + fd = alloc_fd(ftype_9pfs);
> + file = get_file_from_fd(fd);
> + file->filedata = f9pfs;
> +
> + switch ( flags & O_ACCMODE )
> + {
> + case O_RDONLY:
> + omode = P9_OREAD;
> + break;
> + case O_WRONLY:
> + omode = P9_OWRITE;
> + break;
> + case O_RDWR:
> + omode = P9_ORDWR;
> + break;
> + default:
> + ret = EINVAL;
> + goto err;
> + }
[...]
> + err:
> + free(path);
> + close(fd);
> + errno = ret;
This seems missing free(f9pfs)?
Samuel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 7/7] Mini-OS: add read and write support to 9pfsfront
2023-02-03 9:18 ` [PATCH 7/7] Mini-OS: add read and write support to 9pfsfront Juergen Gross
2023-02-03 12:52 ` Juergen Gross
@ 2023-02-06 10:13 ` Samuel Thibault
2023-02-06 10:17 ` Juergen Gross
1 sibling, 1 reply; 28+ messages in thread
From: Samuel Thibault @ 2023-02-06 10:13 UTC (permalink / raw)
To: Juergen Gross; +Cc: minios-devel, xen-devel, wl
Juergen Gross, le ven. 03 févr. 2023 10:18:09 +0100, a ecrit:
> This patch is missing the limitation of read/write messages to stay
> below the max. supported message size.
It should at least be asserted.
> +static int p9_read(struct dev_9pfs *dev, uint32_t fid, uint64_t offset,
> + uint8_t *data, uint32_t len)
> +{
> + struct req *req = get_free_req(dev);
> + int ret;
> + uint32_t count;
> +
> + if ( !req )
> + {
> + errno = EIO;
Here as well (and in p9_write) we'd want EAGAIN rather than EIO which
can be mistaken with the error case below.
> + return -1;
> + }
> +
> + req->cmd = P9_CMD_READ;
> + send_9p(dev, req, "ULU", fid, offset, len);
> + rcv_9p(dev, req, "D", &count, data);
> +
> + if ( req->result )
> + {
> + ret = -1;
> + errno = EIO;
> + }
> + else
> + ret = count;
> +
> + put_free_req(dev, req);
> +
> + return ret;
> +}
Samuel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/7] Mini-OS: ad minimal 9pfs support
2023-02-03 9:18 [PATCH 0/7] Mini-OS: ad minimal 9pfs support Juergen Gross
` (6 preceding siblings ...)
2023-02-03 9:18 ` [PATCH 7/7] Mini-OS: add read and write support to 9pfsfront Juergen Gross
@ 2023-02-06 10:14 ` Samuel Thibault
7 siblings, 0 replies; 28+ messages in thread
From: Samuel Thibault @ 2023-02-06 10:14 UTC (permalink / raw)
To: Juergen Gross; +Cc: minios-devel, xen-devel, wl
Hello,
Juergen Gross, le ven. 03 févr. 2023 10:18:02 +0100, a ecrit:
> This series is adding minimal support to use 9pfs in Mini-OS. It is
> adding a PV 9pfs frontend and the ability to open, close, read and
> write files.
Nice, thanks! :)
Samuel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 6/7] Mini-OS: add open and close handling to the 9pfs frontend
2023-02-06 10:05 ` Samuel Thibault
@ 2023-02-06 10:15 ` Juergen Gross
0 siblings, 0 replies; 28+ messages in thread
From: Juergen Gross @ 2023-02-06 10:15 UTC (permalink / raw)
To: Samuel Thibault, minios-devel, xen-devel, wl
[-- Attachment #1.1.1: Type: text/plain, Size: 2770 bytes --]
On 06.02.23 11:05, Samuel Thibault wrote:
> Juergen Gross, le ven. 03 févr. 2023 10:18:08 +0100, a ecrit:
>> +static unsigned int get_fid(struct dev_9pfs *dev)
>> +{
>> + unsigned int fid;
>> +
>> + fid = ffs(dev->fid_mask);
>> + if ( fid )
>> + dev->fid_mask &= 1ULL << (fid - 1);
>
> Isn't that missing a ~ ?
Oh, indeed.
>
>> @@ -459,6 +522,134 @@ static int p9_attach(struct dev_9pfs *dev)
>> return ret;
>> }
>>
>> +static int p9_clunk(struct dev_9pfs *dev, uint32_t fid)
>> +{
>> + struct req *req = get_free_req(dev);
>> + int ret;
>> +
>> + if ( !req )
>> + return ENOENT;
>
> Rather EAGAIN? (and similar in other functions)
> (actually p9_version and p9_attach could be updated too, even if in
> their case it's not supposed to happen.
Yes, might be better.
>
>> + req->cmd = P9_CMD_CLUNK;
>> + send_9p(dev, req, "U", fid);
>> + rcv_9p(dev, req, "");
>> + ret = req->result;
>> +
>> + put_free_req(dev, req);
>> +
>> + return ret;
>> +}
>
>
>> +static unsigned int split_path(const char *pathname, char **split_ptr)
>> +{
>> + unsigned int parts = 1;
>> + char *p;
>> +
>> + *split_ptr = strdup(pathname);
>> +
>> + for ( p = strchr(*split_ptr, '/'); p; p = strchr(p + 1, '/') )
>> + {
>> + *p = 0;
>> + parts++;
>> + }
>> +
>> + return parts;
>> +}
>
> Do we need to care about an empty part if we're passed "/foo//bar"?
I'd rather add a check for the file name to be properly canonicalized
further up in the hierarchy (either in open() directly, or in open_9pfs()).
>
>> static int open_9pfs(struct mount_point *mnt, const char *pathname, int flags,
>> mode_t mode)
>> {
>> - errno = ENOSYS;
>> + int fd;
>> + char *path = NULL;
>> + char *p;
>> + struct file *file;
>> + struct file_9pfs *f9pfs;
>> + uint16_t nwalk;
>> + uint8_t omode;
>> + int ret;
>> +
>> + f9pfs = calloc(1, sizeof(*f9pfs));
>> + f9pfs->dev = mnt->dev;
>> + f9pfs->fid = P9_ROOT_FID;
>> +
>> + fd = alloc_fd(ftype_9pfs);
>> + file = get_file_from_fd(fd);
>> + file->filedata = f9pfs;
>> +
>> + switch ( flags & O_ACCMODE )
>> + {
>> + case O_RDONLY:
>> + omode = P9_OREAD;
>> + break;
>> + case O_WRONLY:
>> + omode = P9_OWRITE;
>> + break;
>> + case O_RDWR:
>> + omode = P9_ORDWR;
>> + break;
>> + default:
>> + ret = EINVAL;
>> + goto err;
>> + }
> [...]
>> + err:
>> + free(path);
>> + close(fd);
>> + errno = ret;
>
> This seems missing free(f9pfs)?
close() does that via close_9pfs().
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 7/7] Mini-OS: add read and write support to 9pfsfront
2023-02-06 10:13 ` Samuel Thibault
@ 2023-02-06 10:17 ` Juergen Gross
2023-02-06 12:05 ` Samuel Thibault
0 siblings, 1 reply; 28+ messages in thread
From: Juergen Gross @ 2023-02-06 10:17 UTC (permalink / raw)
To: Samuel Thibault, minios-devel, xen-devel, wl
[-- Attachment #1.1.1: Type: text/plain, Size: 814 bytes --]
On 06.02.23 11:13, Samuel Thibault wrote:
>
> Juergen Gross, le ven. 03 févr. 2023 10:18:09 +0100, a ecrit:
>> This patch is missing the limitation of read/write messages to stay
>> below the max. supported message size.
>
> It should at least be asserted.
It can easily be limited by returning the resulting limit as the
number of processed bytes.
>
>> +static int p9_read(struct dev_9pfs *dev, uint32_t fid, uint64_t offset,
>> + uint8_t *data, uint32_t len)
>> +{
>> + struct req *req = get_free_req(dev);
>> + int ret;
>> + uint32_t count;
>> +
>> + if ( !req )
>> + {
>> + errno = EIO;
>
> Here as well (and in p9_write) we'd want EAGAIN rather than EIO which
> can be mistaken with the error case below.
Fine with me.
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 5/7] Mini-OS: add 9pfs transport layer
2023-02-06 9:40 ` Samuel Thibault
@ 2023-02-06 10:23 ` Juergen Gross
0 siblings, 0 replies; 28+ messages in thread
From: Juergen Gross @ 2023-02-06 10:23 UTC (permalink / raw)
To: Samuel Thibault, minios-devel, xen-devel, wl
[-- Attachment #1.1.1: Type: text/plain, Size: 2814 bytes --]
On 06.02.23 10:40, Samuel Thibault wrote:
> Juergen Gross, le ven. 03 févr. 2023 10:18:07 +0100, a ecrit:
>> +/*
>> + * Using an opportunistic approach for receiving data: in case multiple
>> + * requests are outstanding (which is very unlikely), we nevertheless need
>> + * to consume all data available until we reach the desired request.
>> + * For requests other than the one we are waiting for, we link the complete
>> + * data to the request via an intermediate buffer. For our own request we can
>> + * omit that buffer and directly fill the caller provided variables.
>> + */
>
> This documents the "why" which is very welcome, but does not document
> what the function does exactly (AIUI, copy from buf1+buf2 into target up
> to len bytes, and update buf/len accordingly).
Okay, will add some more comments.
>
>> +static void copy_bufs(unsigned char **buf1, unsigned char **buf2,
>> + unsigned int *len1, unsigned int *len2,
>> + void *target, unsigned int len)
>> +{
>> + if ( len <= *len1 )
>> + {
>> + memcpy(target, *buf1, len);
>> + *buf1 += len;
>> + *len1 -= len;
>> + }
>> + else
>> + {
>> + memcpy(target, *buf1, *len1);
>> + target = (char *)target + *len1;
>> + len -= *len1;
>> + *buf1 = *buf2;
>> + *len1 = *len2;
>> + *buf2 = NULL;
>> + *len2 = 0;
>> + if ( len > *len1 )
>> + len = *len1;
>
> But then this is a short copy, don't we need to error out, or at least
> log something? Normally the other end is not supposed to push data
> incrementaly, but better at least catch such misprogramming.
I can add a log message.
>
>> + memcpy(target, *buf1, *len1);
>> + }
>> +}
>> +
>> +static void rcv_9p_copy(struct dev_9pfs *dev, struct req *req,
>> + struct p9_header *hdr, const char *fmt, va_list ap)
>
> Please document what this helper function does (at the very least which
> way the copy happens). The hdr != NULL vs == NULL notably needs to be
> explained.
Okay.
>
>> + case 'Q':
>> + qval = va_arg(ap, uint8_t *);
>> + copy_bufs(&buf1, &buf2, &len1, &len2, qval, 13);
>
> I'd say make this 13 a macro.
Okay.
>
>
>> +static void rcv_9p(struct dev_9pfs *dev, struct req *req, const char *fmt, ...)
>> +{
>> + va_list ap;
>> +
>> + va_start(ap, fmt);
>> +
>> + down(&dev->ring_in_sem);
>> +
>> + while ( !rcv_9p_one(dev, req, fmt, ap) );
>> +
>> + rmb(); /* Read all data before updating ring index. */
>> + dev->intf->in_cons = dev->cons_pvt_in;
>
> Shouldn't there be an event notification after updating the consumption
> index?
Hmm, yes.
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 7/7] Mini-OS: add read and write support to 9pfsfront
2023-02-06 10:17 ` Juergen Gross
@ 2023-02-06 12:05 ` Samuel Thibault
2023-02-06 15:35 ` Juergen Gross
0 siblings, 1 reply; 28+ messages in thread
From: Samuel Thibault @ 2023-02-06 12:05 UTC (permalink / raw)
To: Juergen Gross; +Cc: minios-devel, xen-devel, wl
Juergen Gross, le lun. 06 févr. 2023 11:17:27 +0100, a ecrit:
> On 06.02.23 11:13, Samuel Thibault wrote:
> >
> > Juergen Gross, le ven. 03 févr. 2023 10:18:09 +0100, a ecrit:
> > > This patch is missing the limitation of read/write messages to stay
> > > below the max. supported message size.
> >
> > It should at least be asserted.
>
> It can easily be limited by returning the resulting limit as the
> number of processed bytes.
Strictly speaking, posix allows to return short reads, but that's
usually only handled by applications when they know that they may get
signals. I'd thus rather have read/write loop over the size.
Samuel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 7/7] Mini-OS: add read and write support to 9pfsfront
2023-02-06 12:05 ` Samuel Thibault
@ 2023-02-06 15:35 ` Juergen Gross
0 siblings, 0 replies; 28+ messages in thread
From: Juergen Gross @ 2023-02-06 15:35 UTC (permalink / raw)
To: Samuel Thibault, minios-devel, xen-devel, wl
[-- Attachment #1.1.1: Type: text/plain, Size: 727 bytes --]
On 06.02.23 13:05, Samuel Thibault wrote:
> Juergen Gross, le lun. 06 févr. 2023 11:17:27 +0100, a ecrit:
>> On 06.02.23 11:13, Samuel Thibault wrote:
>>>
>>> Juergen Gross, le ven. 03 févr. 2023 10:18:09 +0100, a ecrit:
>>>> This patch is missing the limitation of read/write messages to stay
>>>> below the max. supported message size.
>>>
>>> It should at least be asserted.
>>
>> It can easily be limited by returning the resulting limit as the
>> number of processed bytes.
>
> Strictly speaking, posix allows to return short reads, but that's
> usually only handled by applications when they know that they may get
> signals. I'd thus rather have read/write loop over the size.
Okay.
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2023-02-06 15:35 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-03 9:18 [PATCH 0/7] Mini-OS: ad minimal 9pfs support Juergen Gross
2023-02-03 9:18 ` [PATCH 1/7] Mini-OS: xenbus: add support for reading node from directory Juergen Gross
2023-02-04 14:01 ` Samuel Thibault
2023-02-06 9:23 ` Juergen Gross
2023-02-03 9:18 ` [PATCH 2/7] Mini-OS: add concept of mount points Juergen Gross
2023-02-05 12:40 ` Samuel Thibault
2023-02-05 12:45 ` Samuel Thibault
2023-02-06 9:25 ` Juergen Gross
2023-02-03 9:18 ` [PATCH 3/7] Mini-OS: add support for runtime mounts Juergen Gross
2023-02-05 12:42 ` Samuel Thibault
2023-02-03 9:18 ` [PATCH 4/7] Mini-OS: add 9pfs frontend Juergen Gross
2023-02-06 9:01 ` Samuel Thibault
2023-02-06 9:22 ` Juergen Gross
2023-02-06 9:48 ` Samuel Thibault
2023-02-03 9:18 ` [PATCH 5/7] Mini-OS: add 9pfs transport layer Juergen Gross
2023-02-06 9:40 ` Samuel Thibault
2023-02-06 10:23 ` Juergen Gross
2023-02-03 9:18 ` [PATCH 6/7] Mini-OS: add open and close handling to the 9pfs frontend Juergen Gross
2023-02-03 12:50 ` Juergen Gross
2023-02-06 10:05 ` Samuel Thibault
2023-02-06 10:15 ` Juergen Gross
2023-02-03 9:18 ` [PATCH 7/7] Mini-OS: add read and write support to 9pfsfront Juergen Gross
2023-02-03 12:52 ` Juergen Gross
2023-02-06 10:13 ` Samuel Thibault
2023-02-06 10:17 ` Juergen Gross
2023-02-06 12:05 ` Samuel Thibault
2023-02-06 15:35 ` Juergen Gross
2023-02-06 10:14 ` [PATCH 0/7] Mini-OS: ad minimal 9pfs support 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.