Add the infrastructure for the hypervisor filesystem.
This includes the hypercall interface and the base functions for
entry creation, deletion and modification.
Initially we support string and unsigned integer entry types. The saved
entry size is an upper bound, so for unsigned integer entries we always
set the value "11".
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V1:
- rename files from filesystem.* to hypfs.*
- add dummy write entry support
- rename hypercall filesystem_op to hypfs_op
- add support for unsigned integer entries
V2:
- test new entry name to be valid
---
xen/arch/arm/traps.c | 1 +
xen/arch/x86/hvm/hypercall.c | 1 +
xen/arch/x86/hypercall.c | 1 +
xen/arch/x86/pv/hypercall.c | 1 +
xen/common/Makefile | 1 +
xen/common/hypfs.c | 318 +++++++++++++++++++++++++++++++++++++++++++
xen/include/public/errno.h | 1 +
xen/include/public/hypfs.h | 123 +++++++++++++++++
xen/include/public/xen.h | 1 +
xen/include/xen/hypercall.h | 8 ++
xen/include/xen/hypfs.h | 40 ++++++
11 files changed, 496 insertions(+)
create mode 100644 xen/common/hypfs.c
create mode 100644 xen/include/public/hypfs.h
create mode 100644 xen/include/xen/hypfs.h
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index a3b961bd06..4b1acd67a2 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1401,6 +1401,7 @@ static arm_hypercall_t arm_hypercall_table[] = {
#ifdef CONFIG_ARGO
HYPERCALL(argo_op, 5),
#endif
+ HYPERCALL(hypfs_op, 5),
};
#ifndef NDEBUG
diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
index 33dd2d99d2..210dda4f38 100644
--- a/xen/arch/x86/hvm/hypercall.c
+++ b/xen/arch/x86/hvm/hypercall.c
@@ -144,6 +144,7 @@ static const hypercall_table_t hvm_hypercall_table[] = {
#endif
HYPERCALL(xenpmu_op),
COMPAT_CALL(dm_op),
+ HYPERCALL(hypfs_op),
HYPERCALL(arch_1)
};
diff --git a/xen/arch/x86/hypercall.c b/xen/arch/x86/hypercall.c
index d483dbaa6b..8a8f223481 100644
--- a/xen/arch/x86/hypercall.c
+++ b/xen/arch/x86/hypercall.c
@@ -73,6 +73,7 @@ const hypercall_args_t hypercall_args_table[NR_hypercalls] =
ARGS(hvm_op, 2),
ARGS(dm_op, 3),
#endif
+ ARGS(hypfs_op, 5),
ARGS(mca, 1),
ARGS(arch_1, 1),
};
diff --git a/xen/arch/x86/pv/hypercall.c b/xen/arch/x86/pv/hypercall.c
index 0c84c0b3a0..e6860247cc 100644
--- a/xen/arch/x86/pv/hypercall.c
+++ b/xen/arch/x86/pv/hypercall.c
@@ -84,6 +84,7 @@ const hypercall_table_t pv_hypercall_table[] = {
HYPERCALL(hvm_op),
COMPAT_CALL(dm_op),
#endif
+ HYPERCALL(hypfs_op),
HYPERCALL(mca),
HYPERCALL(arch_1),
};
diff --git a/xen/common/Makefile b/xen/common/Makefile
index 62b34e69e9..a3f66aa0c0 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -11,6 +11,7 @@ obj-y += domain.o
obj-y += event_2l.o
obj-y += event_channel.o
obj-y += event_fifo.o
+obj-y += hypfs.o
obj-$(CONFIG_CRASH_DEBUG) += gdbstub.o
obj-$(CONFIG_GRANT_TABLE) += grant_table.o
obj-y += guestcopy.o
diff --git a/xen/common/hypfs.c b/xen/common/hypfs.c
new file mode 100644
index 0000000000..c8c5c504cf
--- /dev/null
+++ b/xen/common/hypfs.c
@@ -0,0 +1,318 @@
+/******************************************************************************
+ *
+ * hypfs.c
+ *
+ * Simple sysfs-like file system for the hypervisor.
+ */
+
+#include <xen/lib.h>
+#include <xen/hypfs.h>
+#include <xen/guest_access.h>
+#include <xen/hypercall.h>
+#include <public/hypfs.h>
+
+static DEFINE_SPINLOCK(hypfs_lock);
+
+struct hypfs_dir hypfs_root = {
+ .list = LIST_HEAD_INIT(hypfs_root.list),
+};
+
+static struct hypfs_entry hypfs_root_entry = {
+ .type = hypfs_type_dir,
+ .name = "",
+ .list = LIST_HEAD_INIT(hypfs_root_entry.list),
+ .parent = &hypfs_root,
+ .dir = &hypfs_root,
+};
+
+static int hypfs_add_entry(struct hypfs_dir *parent, struct hypfs_entry *new)
+{
+ int ret = -ENOENT;
+ struct list_head *l;
+
+ if ( !new->content )
+ return -EINVAL;
+
+ spin_lock(&hypfs_lock);
+
+ list_for_each ( l, &parent->list )
+ {
+ struct hypfs_entry *e = list_entry(l, struct hypfs_entry, list);
+ int cmp = strcmp(e->name, new->name);
+
+ if ( cmp > 0 )
+ {
+ ret = 0;
+ list_add_tail(&new->list, l);
+ break;
+ }
+ if ( cmp == 0 )
+ {
+ ret = -EEXIST;
+ break;
+ }
+ }
+
+ if ( ret == -ENOENT )
+ {
+ ret = 0;
+ list_add_tail(&new->list, &parent->list);
+ }
+
+ if ( !ret )
+ {
+ unsigned int sz = strlen(new->name) + 1;
+
+ parent->content_size += sizeof(struct xen_hypfs_direntry) +
+ ROUNDUP(sz, 4);
+ new->parent = parent;
+ }
+
+ spin_unlock(&hypfs_lock);
+
+ return ret;
+}
+
+int hypfs_new_entry_any(struct hypfs_dir *parent, const char *name,
+ enum hypfs_entry_type type, void *content)
+{
+ int ret;
+ struct hypfs_entry *new;
+
+ if ( strchr(name, '/') || !strcmp(name, ".") || !strcmp(name, "..") )
+ return -EINVAL;
+
+ new = xzalloc(struct hypfs_entry);
+ if ( !new )
+ return -ENOMEM;
+
+ new->name = name;
+ new->type = type;
+ new->content = content;
+
+ ret = hypfs_add_entry(parent, new);
+
+ if ( ret )
+ xfree(new);
+
+ return ret;
+}
+
+int hypfs_new_entry_string(struct hypfs_dir *parent, const char *name,
+ char *val)
+{
+ return hypfs_new_entry_any(parent, name, hypfs_type_string, val);
+}
+
+int hypfs_new_entry_uint(struct hypfs_dir *parent, const char *name,
+ unsigned int *val)
+{
+ return hypfs_new_entry_any(parent, name, hypfs_type_uint, val);
+}
+
+int hypfs_new_dir(struct hypfs_dir *parent, const char *name,
+ struct hypfs_dir *dir)
+{
+ if ( !dir )
+ dir = xzalloc(struct hypfs_dir);
+
+ return hypfs_new_entry_any(parent, name, hypfs_type_dir, dir);
+}
+
+static int hypfs_get_path_user(char *buf, XEN_GUEST_HANDLE_PARAM(void) uaddr,
+ unsigned long len)
+{
+ if ( len > XEN_HYPFS_MAX_PATHLEN )
+ return -EINVAL;
+
+ if ( copy_from_guest(buf, uaddr, len) )
+ return -EFAULT;
+
+ buf[len - 1] = 0;
+
+ return 0;
+}
+
+static struct hypfs_entry *hypfs_get_entry_rel(struct hypfs_entry *dir,
+ char *path)
+{
+ char *slash;
+ struct hypfs_entry *entry;
+ struct list_head *l;
+ unsigned int name_len;
+
+ if ( *path == 0 )
+ return dir;
+
+ if ( dir->type != hypfs_type_dir )
+ return NULL;
+
+ slash = strchr(path, '/');
+ if ( !slash )
+ slash = strchr(path, '\0');
+ name_len = slash - path;
+
+ list_for_each ( l, &dir->dir->list )
+ {
+ int cmp;
+
+ entry = list_entry(l, struct hypfs_entry, list);
+ cmp = strncmp(path, entry->name, name_len);
+ if ( cmp < 0 )
+ return NULL;
+ if ( cmp > 0 )
+ continue;
+ if ( strlen(entry->name) == name_len )
+ return *slash ? hypfs_get_entry_rel(entry, slash + 1) : entry;
+ }
+
+ return NULL;
+}
+
+struct hypfs_entry *hypfs_get_entry(char *path)
+{
+ if ( path[0] != '/' )
+ return NULL;
+
+ return hypfs_get_entry_rel(&hypfs_root_entry, path + 1);
+}
+
+static unsigned int hypfs_get_entry_len(struct hypfs_entry *entry)
+{
+ unsigned int len = 0;
+
+ switch ( entry->type )
+ {
+ case hypfs_type_dir:
+ len = entry->dir->content_size;
+ break;
+ case hypfs_type_string:
+ len = strlen(entry->str_val) + 1;
+ break;
+ case hypfs_type_uint:
+ len = 11; /* longest possible printed value + 1 */
+ break;
+ }
+
+ return len;
+}
+
+long do_hypfs_op(unsigned int cmd,
+ XEN_GUEST_HANDLE_PARAM(void) arg1, unsigned long arg2,
+ XEN_GUEST_HANDLE_PARAM(void) arg3, unsigned long arg4)
+{
+ int ret;
+ struct hypfs_entry *entry;
+ unsigned int len;
+ static char path[XEN_HYPFS_MAX_PATHLEN];
+
+ if ( !is_control_domain(current->domain) &&
+ !is_hardware_domain(current->domain) )
+ return -EPERM;
+
+ spin_lock(&hypfs_lock);
+
+ ret = hypfs_get_path_user(path, arg1, arg2);
+ if ( ret )
+ goto out;
+
+ entry = hypfs_get_entry(path);
+ if ( !entry )
+ {
+ ret = -ENOENT;
+ goto out;
+ }
+
+ switch ( cmd )
+ {
+ case XEN_HYPFS_OP_read_contents:
+ {
+ char buf[12];
+ char *val = buf;
+
+ len = hypfs_get_entry_len(entry);
+ if ( len > arg4 )
+ {
+ ret = len;
+ break;
+ }
+
+ switch ( entry->type )
+ {
+ case hypfs_type_dir:
+ ret = -EISDIR;
+ break;
+ case hypfs_type_string:
+ val = entry->str_val;
+ break;
+ case hypfs_type_uint:
+ len = snprintf(buf, sizeof(buf), "%u", *entry->uint_val) + 1;
+ break;
+ }
+
+ if ( !ret && copy_to_guest(arg3, val, len) )
+ ret = -EFAULT;
+
+ break;
+ }
+
+ case XEN_HYPFS_OP_read_dir:
+ {
+ struct list_head *l;
+
+ if ( entry->type != hypfs_type_dir )
+ {
+ ret = -ENOTDIR;
+ break;
+ }
+
+ len = entry->dir->content_size;
+ if ( len > arg4 )
+ {
+ ret = len;
+ break;
+ }
+
+ list_for_each ( l, &entry->dir->list )
+ {
+ struct xen_hypfs_direntry direntry;
+ struct hypfs_entry *e = list_entry(l, struct hypfs_entry, list);
+ unsigned int e_len = strlen(e->name) + 1;
+
+ e_len = sizeof(direntry) + ROUNDUP(e_len, 4);
+ direntry.flags = (e->type == hypfs_type_dir) ? XEN_HYPFS_ISDIR : 0;
+ direntry.off_next = list_is_last(l, &entry->dir->list) ? 0 : e_len;
+ direntry.content_len = hypfs_get_entry_len(e);
+ if ( copy_to_guest(arg3, &direntry, 1) )
+ {
+ ret = -EFAULT;
+ goto out;
+ }
+
+ if ( copy_to_guest_offset(arg3, sizeof(direntry), e->name,
+ strlen(e->name) + 1) )
+ {
+ ret = -EFAULT;
+ goto out;
+ }
+
+ guest_handle_add_offset(arg3, e_len);
+ }
+
+ break;
+ }
+
+ case XEN_HYPFS_OP_write_contents:
+ ret = -EACCES;
+ break;
+
+ default:
+ ret = -ENOSYS;
+ break;
+ }
+
+ out:
+ spin_unlock(&hypfs_lock);
+
+ return ret;
+}
diff --git a/xen/include/public/errno.h b/xen/include/public/errno.h
index e1d02fcddf..5c53af6af9 100644
--- a/xen/include/public/errno.h
+++ b/xen/include/public/errno.h
@@ -78,6 +78,7 @@ XEN_ERRNO(EBUSY, 16) /* Device or resource busy */
XEN_ERRNO(EEXIST, 17) /* File exists */
XEN_ERRNO(EXDEV, 18) /* Cross-device link */
XEN_ERRNO(ENODEV, 19) /* No such device */
+XEN_ERRNO(ENOTDIR, 20) /* Not a directory */
XEN_ERRNO(EISDIR, 21) /* Is a directory */
XEN_ERRNO(EINVAL, 22) /* Invalid argument */
XEN_ERRNO(ENFILE, 23) /* File table overflow */
diff --git a/xen/include/public/hypfs.h b/xen/include/public/hypfs.h
new file mode 100644
index 0000000000..8822e2fb15
--- /dev/null
+++ b/xen/include/public/hypfs.h
@@ -0,0 +1,123 @@
+/******************************************************************************
+ * Xen Hypervisor Filesystem
+ *
+ * Copyright (c) 2019, SUSE Software Solutions Germany GmbH
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
+ */
+
+#ifndef __XEN_PUBLIC_HYPFS_H__
+#define __XEN_PUBLIC_HYPFS_H__
+
+#include "xen.h"
+
+/*
+ * Definitions for the __HYPERVISOR_hypfs_op hypercall.
+ */
+
+/* Maximum length of a path in the filesystem. */
+#define XEN_HYPFS_MAX_PATHLEN 1024
+
+struct xen_hypfs_direntry {
+ uint16_t flags;
+#define XEN_HYPFS_ISDIR 0x0001
+#define XEN_HYPFS_WRITEABLE 0x0002
+ /* Offset in bytes to next entry (0 == this is the last entry). */
+ uint16_t off_next;
+ uint32_t content_len;
+ char name[XEN_FLEX_ARRAY_DIM];
+};
+
+/*
+ * Hypercall operations.
+ */
+
+/*
+ * XEN_HYPFS_OP_read_contents
+ *
+ * Read contents of a filesystem entry.
+ *
+ * Returns the contents of an entry in the buffer supplied by the caller.
+ * Only text data with a trailing zero byte is returned.
+ *
+ * arg1: XEN_GUEST_HANDLE(path name)
+ * arg2: length of path name (including trailing zero byte)
+ * arg3: XEN_GUEST_HANDLE(content buffer)
+ * arg4: content buffer size
+ *
+ * Possible return values:
+ * 0: success
+ * -EPERM: operation not permitted
+ * -ENOENT: entry not found
+ * -EACCESS: access to entry not permitted
+ * -EISDIR: entry is a directory
+ * -EINVAL: invalid parameter
+ * positive value: content buffer was too small, returned value is needed size
+ */
+#define XEN_HYPFS_OP_read_contents 1
+
+/*
+ * XEN_HYPFS_OP_read_dir
+ *
+ * Read directory entries of a directory.
+ *
+ * Returns a struct xen_fs_direntry for each entry in a directory.
+ *
+ * arg1: XEN_GUEST_HANDLE(path name)
+ * arg2: length of path name (including trailing zero byte)
+ * arg3: XEN_GUEST_HANDLE(content buffer)
+ * arg4: content buffer size
+ *
+ * Possible return values:
+ * 0: success
+ * -EPERM: operation not permitted
+ * -ENOENT: entry not found
+ * -EACCESS: access to entry not permitted
+ * -ENOTDIR: entry is not a directory
+ * -EINVAL: invalid parameter
+ * positive value: content buffer was too small, returned value is needed size
+ */
+#define XEN_HYPFS_OP_read_dir 2
+
+/*
+ * XEN_HYPFS_OP_read_contents
+ *
+ * Write contents of a filesystem entry.
+ *
+ * Writes an entry with the contents of a buffer supplied by the caller.
+ * Only text data with a trailing zero byte can be written.
+ *
+ * arg1: XEN_GUEST_HANDLE(path name)
+ * arg2: length of path name (including trailing zero byte)
+ * arg3: XEN_GUEST_HANDLE(content buffer)
+ * arg4: content buffer size
+ *
+ * Possible return values:
+ * 0: success
+ * -EPERM: operation not permitted
+ * -ENOENT: entry not found
+ * -EACCESS: access to entry not permitted
+ * -EISDIR: entry is a directory
+ * -EINVAL: invalid parameter
+ * -ENOMEM: memory shortage in the hypervisor
+ */
+#define XEN_HYPFS_OP_write_contents 3
+
+#endif /* __XEN_PUBLIC_HYPFS_H__ */
diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index d2198dffad..bf80f1da8c 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -130,6 +130,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
#define __HYPERVISOR_argo_op 39
#define __HYPERVISOR_xenpmu_op 40
#define __HYPERVISOR_dm_op 41
+#define __HYPERVISOR_hypfs_op 42
/* Architecture-specific hypercall definitions. */
#define __HYPERVISOR_arch_0 48
diff --git a/xen/include/xen/hypercall.h b/xen/include/xen/hypercall.h
index ad8ad27b23..349a0f6487 100644
--- a/xen/include/xen/hypercall.h
+++ b/xen/include/xen/hypercall.h
@@ -150,6 +150,14 @@ do_dm_op(
unsigned int nr_bufs,
XEN_GUEST_HANDLE_PARAM(xen_dm_op_buf_t) bufs);
+extern long
+do_hypfs_op(
+ unsigned int cmd,
+ XEN_GUEST_HANDLE_PARAM(void) arg1,
+ unsigned long arg2,
+ XEN_GUEST_HANDLE_PARAM(void) arg3,
+ unsigned long arg4);
+
#ifdef CONFIG_COMPAT
extern int
diff --git a/xen/include/xen/hypfs.h b/xen/include/xen/hypfs.h
new file mode 100644
index 0000000000..d2537211f6
--- /dev/null
+++ b/xen/include/xen/hypfs.h
@@ -0,0 +1,40 @@
+#ifndef __XEN_HYPFS_H__
+#define __XEN_HYPFS_H__
+
+#include <xen/list.h>
+
+struct hypfs_dir {
+ unsigned int content_size;
+ struct list_head list;
+};
+
+enum hypfs_entry_type {
+ hypfs_type_dir,
+ hypfs_type_string,
+ hypfs_type_uint
+};
+
+struct hypfs_entry {
+ enum hypfs_entry_type type;
+ const char *name;
+ struct list_head list;
+ struct hypfs_dir *parent;
+ union {
+ void *content;
+ struct hypfs_dir *dir;
+ char *str_val;
+ unsigned int *uint_val;
+ };
+};
+
+extern struct hypfs_dir hypfs_root;
+
+int hypfs_new_dir(struct hypfs_dir *parent, const char *name,
+ struct hypfs_dir *dir);
+int hypfs_new_entry_string(struct hypfs_dir *parent, const char *name,
+ char *val);
+int hypfs_new_entry_uint(struct hypfs_dir *parent, const char *name,
+ unsigned int *val);
+struct hypfs_entry *hypfs_get_entry(char *path);
+
+#endif /* __XEN_HYPFS_H__ */
--
2.16.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-develOn 02.10.2019 13:20, Juergen Gross wrote:
> --- /dev/null
> +++ b/xen/common/hypfs.c
> @@ -0,0 +1,318 @@
> +/******************************************************************************
> + *
> + * hypfs.c
> + *
> + * Simple sysfs-like file system for the hypervisor.
> + */
> +
> +#include <xen/lib.h>
> +#include <xen/hypfs.h>
> +#include <xen/guest_access.h>
> +#include <xen/hypercall.h>
> +#include <public/hypfs.h>
> +
> +static DEFINE_SPINLOCK(hypfs_lock);
> +
> +struct hypfs_dir hypfs_root = {
> + .list = LIST_HEAD_INIT(hypfs_root.list),
> +};
> +
> +static struct hypfs_entry hypfs_root_entry = {
> + .type = hypfs_type_dir,
> + .name = "",
> + .list = LIST_HEAD_INIT(hypfs_root_entry.list),
> + .parent = &hypfs_root,
> + .dir = &hypfs_root,
> +};
This looks to be used only in hypfs_get_entry(). Unless there are
plans to have further uses, it should be moved there.
I'm also somewhat puzzled by "name" being an empty string; this
too would look less suspicious if this wasn't a file scope variable.
> +static int hypfs_add_entry(struct hypfs_dir *parent, struct hypfs_entry *new)
> +{
> + int ret = -ENOENT;
> + struct list_head *l;
> +
> + if ( !new->content )
> + return -EINVAL;
> +
> + spin_lock(&hypfs_lock);
> +
> + list_for_each ( l, &parent->list )
> + {
> + struct hypfs_entry *e = list_entry(l, struct hypfs_entry, list);
const?
> + int cmp = strcmp(e->name, new->name);
> +
> + if ( cmp > 0 )
> + {
> + ret = 0;
> + list_add_tail(&new->list, l);
> + break;
> + }
> + if ( cmp == 0 )
> + {
> + ret = -EEXIST;
> + break;
> + }
> + }
> +
> + if ( ret == -ENOENT )
> + {
> + ret = 0;
> + list_add_tail(&new->list, &parent->list);
> + }
> +
> + if ( !ret )
> + {
> + unsigned int sz = strlen(new->name) + 1;
> +
> + parent->content_size += sizeof(struct xen_hypfs_direntry) +
> + ROUNDUP(sz, 4);
What is this literal 4 coming from? DYM alignof(struct xen_hypfs_direntry)?
> + new->parent = parent;
> + }
> +
> + spin_unlock(&hypfs_lock);
> +
> + return ret;
> +}
> +
> +int hypfs_new_entry_any(struct hypfs_dir *parent, const char *name,
> + enum hypfs_entry_type type, void *content)
Perhaps drop the _any suffix?
> +{
> + int ret;
> + struct hypfs_entry *new;
> +
> + if ( strchr(name, '/') || !strcmp(name, ".") || !strcmp(name, "..") )
> + return -EINVAL;
> +
> + new = xzalloc(struct hypfs_entry);
> + if ( !new )
> + return -ENOMEM;
> +
> + new->name = name;
> + new->type = type;
> + new->content = content;
> +
> + ret = hypfs_add_entry(parent, new);
> +
> + if ( ret )
> + xfree(new);
> +
> + return ret;
> +}
> +
> +int hypfs_new_entry_string(struct hypfs_dir *parent, const char *name,
> + char *val)
The last parameter here and below being non-const is because of the
intended write support?
> +{
> + return hypfs_new_entry_any(parent, name, hypfs_type_string, val);
> +}
> +
> +int hypfs_new_entry_uint(struct hypfs_dir *parent, const char *name,
> + unsigned int *val)
> +{
> + return hypfs_new_entry_any(parent, name, hypfs_type_uint, val);
> +}
> +
> +int hypfs_new_dir(struct hypfs_dir *parent, const char *name,
> + struct hypfs_dir *dir)
> +{
> + if ( !dir )
> + dir = xzalloc(struct hypfs_dir);
> +
> + return hypfs_new_entry_any(parent, name, hypfs_type_dir, dir);
> +}
> +
> +static int hypfs_get_path_user(char *buf, XEN_GUEST_HANDLE_PARAM(void) uaddr,
> + unsigned long len)
> +{
> + if ( len > XEN_HYPFS_MAX_PATHLEN )
> + return -EINVAL;
> +
> + if ( copy_from_guest(buf, uaddr, len) )
> + return -EFAULT;
> +
> + buf[len - 1] = 0;
In the public interface description you have "including trailing zero
byte". I think instead of putting one there you should check there's
one.
> + return 0;
> +}
> +
> +static struct hypfs_entry *hypfs_get_entry_rel(struct hypfs_entry *dir,
> + char *path)
const?
> +{
> + char *slash;
const?
> + struct hypfs_entry *entry;
> + struct list_head *l;
> + unsigned int name_len;
> +
> + if ( *path == 0 )
Please either use !*path or be consistent with code a few lines
down and use '\0'.
> + return dir;
> +
> + if ( dir->type != hypfs_type_dir )
> + return NULL;
> +
> + slash = strchr(path, '/');
> + if ( !slash )
> + slash = strchr(path, '\0');
With this better name the variable "end" or some such?
> + name_len = slash - path;
> +
> + list_for_each ( l, &dir->dir->list )
> + {
> + int cmp;
> +
> + entry = list_entry(l, struct hypfs_entry, list);
Why not list_for_each_entry(), eliminating the need for the "l"
helper variable?
> + cmp = strncmp(path, entry->name, name_len);
> + if ( cmp < 0 )
> + return NULL;
> + if ( cmp > 0 )
> + continue;
> + if ( strlen(entry->name) == name_len )
> + return *slash ? hypfs_get_entry_rel(entry, slash + 1) : entry;
Perhaps slightly shorter
if ( cmp == 0 && strlen(entry->name) == name_len )
return *slash ? hypfs_get_entry_rel(entry, slash + 1) : entry;
?
> + }
> +
> + return NULL;
> +}
> +
> +struct hypfs_entry *hypfs_get_entry(char *path)
const?
> +{
> + if ( path[0] != '/' )
> + return NULL;
> +
> + return hypfs_get_entry_rel(&hypfs_root_entry, path + 1);
> +}
> +
> +static unsigned int hypfs_get_entry_len(struct hypfs_entry *entry)
> +{
> + unsigned int len = 0;
> +
> + switch ( entry->type )
> + {
> + case hypfs_type_dir:
> + len = entry->dir->content_size;
> + break;
> + case hypfs_type_string:
> + len = strlen(entry->str_val) + 1;
> + break;
> + case hypfs_type_uint:
> + len = 11; /* longest possible printed value + 1 */
Why would uint values be restricted to 32 bits? There are plenty of
64-bit numbers that might be of interest exposing through this
interface (and even more so if down the road we were to re-use this
for something debugfs-like). And even without this I think it would
be better to not have a literal number here - it'll be close to
unnoticeable (without someone remembering) when porting to an arch
with unsigned int wider than 32 bits.
> + break;
> + }
> +
> + return len;
> +}
> +
> +long do_hypfs_op(unsigned int cmd,
> + XEN_GUEST_HANDLE_PARAM(void) arg1, unsigned long arg2,
> + XEN_GUEST_HANDLE_PARAM(void) arg3, unsigned long arg4)
> +{
> + int ret;
> + struct hypfs_entry *entry;
> + unsigned int len;
> + static char path[XEN_HYPFS_MAX_PATHLEN];
> +
> + if ( !is_control_domain(current->domain) &&
> + !is_hardware_domain(current->domain) )
> + return -EPERM;
Replace by an XSM check?
> + spin_lock(&hypfs_lock);
Wouldn't this better be an r/w lock from the beginning, requiring
only read access here?
> + ret = hypfs_get_path_user(path, arg1, arg2);
> + if ( ret )
> + goto out;
> +
> + entry = hypfs_get_entry(path);
> + if ( !entry )
> + {
> + ret = -ENOENT;
> + goto out;
> + }
> +
> + switch ( cmd )
> + {
> + case XEN_HYPFS_OP_read_contents:
> + {
> + char buf[12];
> + char *val = buf;
const void *?
> + len = hypfs_get_entry_len(entry);
> + if ( len > arg4 )
> + {
> + ret = len;
> + break;
> + }
> +
> + switch ( entry->type )
> + {
> + case hypfs_type_dir:
> + ret = -EISDIR;
> + break;
> + case hypfs_type_string:
> + val = entry->str_val;
> + break;
> + case hypfs_type_uint:
> + len = snprintf(buf, sizeof(buf), "%u", *entry->uint_val) + 1;
> + break;
> + }
> +
> + if ( !ret && copy_to_guest(arg3, val, len) )
> + ret = -EFAULT;
> +
> + break;
> + }
> +
> + case XEN_HYPFS_OP_read_dir:
> + {
> + struct list_head *l;
> +
> + if ( entry->type != hypfs_type_dir )
> + {
> + ret = -ENOTDIR;
> + break;
> + }
> +
> + len = entry->dir->content_size;
> + if ( len > arg4 )
> + {
> + ret = len;
> + break;
> + }
> +
> + list_for_each ( l, &entry->dir->list )
list_for_each_entry() again?
> + {
> + struct xen_hypfs_direntry direntry;
> + struct hypfs_entry *e = list_entry(l, struct hypfs_entry, list);
> + unsigned int e_len = strlen(e->name) + 1;
> +
> + e_len = sizeof(direntry) + ROUNDUP(e_len, 4);
Literal 4 again. Perhaps you want to put the entire ROUNDUP(, 4)
construct in a macro or function?
> + direntry.flags = (e->type == hypfs_type_dir) ? XEN_HYPFS_ISDIR : 0;
> + direntry.off_next = list_is_last(l, &entry->dir->list) ? 0 : e_len;
> + direntry.content_len = hypfs_get_entry_len(e);
> + if ( copy_to_guest(arg3, &direntry, 1) )
> + {
> + ret = -EFAULT;
> + goto out;
> + }
> +
> + if ( copy_to_guest_offset(arg3, sizeof(direntry), e->name,
> + strlen(e->name) + 1) )
You calculate the length once already a few lines up. Please store into
another local variable and reuse here.
> + {
> + ret = -EFAULT;
> + goto out;
> + }
> +
> + guest_handle_add_offset(arg3, e_len);
I think it would be good to assert somewhere here that you don't go
beyond the dir's stored content_size.
> --- /dev/null
> +++ b/xen/include/public/hypfs.h
> @@ -0,0 +1,123 @@
> +/******************************************************************************
> + * Xen Hypervisor Filesystem
> + *
> + * Copyright (c) 2019, SUSE Software Solutions Germany GmbH
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to
> + * deal in the Software without restriction, including without limitation the
> + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
> + * sell copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + *
> + */
> +
> +#ifndef __XEN_PUBLIC_HYPFS_H__
> +#define __XEN_PUBLIC_HYPFS_H__
> +
> +#include "xen.h"
> +
> +/*
> + * Definitions for the __HYPERVISOR_hypfs_op hypercall.
> + */
> +
> +/* Maximum length of a path in the filesystem. */
> +#define XEN_HYPFS_MAX_PATHLEN 1024
> +
> +struct xen_hypfs_direntry {
> + uint16_t flags;
> +#define XEN_HYPFS_ISDIR 0x0001
> +#define XEN_HYPFS_WRITEABLE 0x0002
> + /* Offset in bytes to next entry (0 == this is the last entry). */
> + uint16_t off_next;
> + uint32_t content_len;
> + char name[XEN_FLEX_ARRAY_DIM];
> +};
Are you certain we won't soon need further fields here? Expressing
symlinks can perhaps be done via a new XEN_HYPFS_* flag, but there
may be other things that would be better to provide for even if
there's no implementation from the beginning. It's just the case
that changing this structure after the fact is going to be
impossible, and it'll take new sub-ops then instead.
> +/*
> + * Hypercall operations.
> + */
> +
> +/*
> + * XEN_HYPFS_OP_read_contents
> + *
> + * Read contents of a filesystem entry.
> + *
> + * Returns the contents of an entry in the buffer supplied by the caller.
> + * Only text data with a trailing zero byte is returned.
> + *
> + * arg1: XEN_GUEST_HANDLE(path name)
> + * arg2: length of path name (including trailing zero byte)
> + * arg3: XEN_GUEST_HANDLE(content buffer)
> + * arg4: content buffer size
> + *
> + * Possible return values:
> + * 0: success
> + * -EPERM: operation not permitted
> + * -ENOENT: entry not found
> + * -EACCESS: access to entry not permitted
> + * -EISDIR: entry is a directory
> + * -EINVAL: invalid parameter
I'm not convinced enumerating possible return value is a good idea.
Down the road we're certain to forget extending this list. Plus
extension would, strictly speaking, not even be allowed if these
enumerations are considered part of the interface.
> + * positive value: content buffer was too small, returned value is needed size
Positive return values are problematic when reaching INT_MAX. Are you
convinced we want to have yet another instance?
> + */
> +#define XEN_HYPFS_OP_read_contents 1
> +
> +/*
> + * XEN_HYPFS_OP_read_dir
> + *
> + * Read directory entries of a directory.
> + *
> + * Returns a struct xen_fs_direntry for each entry in a directory.
> + *
> + * arg1: XEN_GUEST_HANDLE(path name)
> + * arg2: length of path name (including trailing zero byte)
> + * arg3: XEN_GUEST_HANDLE(content buffer)
> + * arg4: content buffer size
> + *
> + * Possible return values:
> + * 0: success
> + * -EPERM: operation not permitted
> + * -ENOENT: entry not found
> + * -EACCESS: access to entry not permitted
> + * -ENOTDIR: entry is not a directory
> + * -EINVAL: invalid parameter
> + * positive value: content buffer was too small, returned value is needed size
> + */
> +#define XEN_HYPFS_OP_read_dir 2
> +
> +/*
> + * XEN_HYPFS_OP_read_contents
XEN_HYPFS_OP_write_contents
> + * Write contents of a filesystem entry.
> + *
> + * Writes an entry with the contents of a buffer supplied by the caller.
> + * Only text data with a trailing zero byte can be written.
> + *
> + * arg1: XEN_GUEST_HANDLE(path name)
> + * arg2: length of path name (including trailing zero byte)
> + * arg3: XEN_GUEST_HANDLE(content buffer)
> + * arg4: content buffer size
The latest here (in contrast to the read counterpart) I think it becomes
desirable to identify what's IN and what's OUT.
> --- /dev/null
> +++ b/xen/include/xen/hypfs.h
> @@ -0,0 +1,40 @@
> +#ifndef __XEN_HYPFS_H__
> +#define __XEN_HYPFS_H__
> +
> +#include <xen/list.h>
> +
> +struct hypfs_dir {
> + unsigned int content_size;
> + struct list_head list;
> +};
> +
> +enum hypfs_entry_type {
> + hypfs_type_dir,
> + hypfs_type_string,
> + hypfs_type_uint
> +};
> +
> +struct hypfs_entry {
> + enum hypfs_entry_type type;
> + const char *name;
> + struct list_head list;
> + struct hypfs_dir *parent;
Afaict you set this field, but you never use it anywhere. Why do you
add it in the first place? (Initially I meant to ask whether this
can be pointer-to-const.)
> + union {
> + void *content;
const?
> + struct hypfs_dir *dir;
const?
> + char *str_val;
> + unsigned int *uint_val;
> + };
> +};
> +
> +extern struct hypfs_dir hypfs_root;
> +
> +int hypfs_new_dir(struct hypfs_dir *parent, const char *name,
> + struct hypfs_dir *dir);
> +int hypfs_new_entry_string(struct hypfs_dir *parent, const char *name,
> + char *val);
> +int hypfs_new_entry_uint(struct hypfs_dir *parent, const char *name,
> + unsigned int *val);
Thinking about the lack of const on the last parameters here again -
if these are for the pointed to values to be modifiable through
this interface, then how would the "owning" component learn of the
value having changed? Not everyone may need this, but I think there
would want to be a callback. Until then perhaps better to add const
here, promising that the values won't change behind the backs of
the owners.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 12.11.19 14:48, Jan Beulich wrote:
> On 02.10.2019 13:20, Juergen Gross wrote:
>> --- /dev/null
>> +++ b/xen/common/hypfs.c
>> @@ -0,0 +1,318 @@
>> +/******************************************************************************
>> + *
>> + * hypfs.c
>> + *
>> + * Simple sysfs-like file system for the hypervisor.
>> + */
>> +
>> +#include <xen/lib.h>
>> +#include <xen/hypfs.h>
>> +#include <xen/guest_access.h>
>> +#include <xen/hypercall.h>
>> +#include <public/hypfs.h>
>> +
>> +static DEFINE_SPINLOCK(hypfs_lock);
>> +
>> +struct hypfs_dir hypfs_root = {
>> + .list = LIST_HEAD_INIT(hypfs_root.list),
>> +};
>> +
>> +static struct hypfs_entry hypfs_root_entry = {
>> + .type = hypfs_type_dir,
>> + .name = "",
>> + .list = LIST_HEAD_INIT(hypfs_root_entry.list),
>> + .parent = &hypfs_root,
>> + .dir = &hypfs_root,
>> +};
>
> This looks to be used only in hypfs_get_entry(). Unless there are
> plans to have further uses, it should be moved there.
Okay.
>
> I'm also somewhat puzzled by "name" being an empty string; this
> too would look less suspicious if this wasn't a file scope variable.
>
>> +static int hypfs_add_entry(struct hypfs_dir *parent, struct hypfs_entry *new)
>> +{
>> + int ret = -ENOENT;
>> + struct list_head *l;
>> +
>> + if ( !new->content )
>> + return -EINVAL;
>> +
>> + spin_lock(&hypfs_lock);
>> +
>> + list_for_each ( l, &parent->list )
>> + {
>> + struct hypfs_entry *e = list_entry(l, struct hypfs_entry, list);
>
> const?
Hmm, is this true when I add a new entry to it? l is part of *e
after all.
>
>> + int cmp = strcmp(e->name, new->name);
>> +
>> + if ( cmp > 0 )
>> + {
>> + ret = 0;
>> + list_add_tail(&new->list, l);
>> + break;
>> + }
>> + if ( cmp == 0 )
>> + {
>> + ret = -EEXIST;
>> + break;
>> + }
>> + }
>> +
>> + if ( ret == -ENOENT )
>> + {
>> + ret = 0;
>> + list_add_tail(&new->list, &parent->list);
>> + }
>> +
>> + if ( !ret )
>> + {
>> + unsigned int sz = strlen(new->name) + 1;
>> +
>> + parent->content_size += sizeof(struct xen_hypfs_direntry) +
>> + ROUNDUP(sz, 4);
>
> What is this literal 4 coming from? DYM alignof(struct xen_hypfs_direntry)?
Yes.
>
>> + new->parent = parent;
>> + }
>> +
>> + spin_unlock(&hypfs_lock);
>> +
>> + return ret;
>> +}
>> +
>> +int hypfs_new_entry_any(struct hypfs_dir *parent, const char *name,
>> + enum hypfs_entry_type type, void *content)
>
> Perhaps drop the _any suffix?
Okay.
>
>> +{
>> + int ret;
>> + struct hypfs_entry *new;
>> +
>> + if ( strchr(name, '/') || !strcmp(name, ".") || !strcmp(name, "..") )
>> + return -EINVAL;
>> +
>> + new = xzalloc(struct hypfs_entry);
>> + if ( !new )
>> + return -ENOMEM;
>> +
>> + new->name = name;
>> + new->type = type;
>> + new->content = content;
>> +
>> + ret = hypfs_add_entry(parent, new);
>> +
>> + if ( ret )
>> + xfree(new);
>> +
>> + return ret;
>> +}
>> +
>> +int hypfs_new_entry_string(struct hypfs_dir *parent, const char *name,
>> + char *val)
>
> The last parameter here and below being non-const is because of the
> intended write support?
Yes.
>
>> +{
>> + return hypfs_new_entry_any(parent, name, hypfs_type_string, val);
>> +}
>> +
>> +int hypfs_new_entry_uint(struct hypfs_dir *parent, const char *name,
>> + unsigned int *val)
>> +{
>> + return hypfs_new_entry_any(parent, name, hypfs_type_uint, val);
>> +}
>> +
>> +int hypfs_new_dir(struct hypfs_dir *parent, const char *name,
>> + struct hypfs_dir *dir)
>> +{
>> + if ( !dir )
>> + dir = xzalloc(struct hypfs_dir);
>> +
>> + return hypfs_new_entry_any(parent, name, hypfs_type_dir, dir);
>> +}
>> +
>> +static int hypfs_get_path_user(char *buf, XEN_GUEST_HANDLE_PARAM(void) uaddr,
>> + unsigned long len)
>> +{
>> + if ( len > XEN_HYPFS_MAX_PATHLEN )
>> + return -EINVAL;
>> +
>> + if ( copy_from_guest(buf, uaddr, len) )
>> + return -EFAULT;
>> +
>> + buf[len - 1] = 0;
>
> In the public interface description you have "including trailing zero
> byte". I think instead of putting one there you should check there's
> one.
Okay.
>
>> + return 0;
>> +}
>> +
>> +static struct hypfs_entry *hypfs_get_entry_rel(struct hypfs_entry *dir,
>> + char *path)
>
> const?
Yes.
>
>> +{
>> + char *slash;
>
> const?
Okay.
>
>> + struct hypfs_entry *entry;
>> + struct list_head *l;
>> + unsigned int name_len;
>> +
>> + if ( *path == 0 )
>
> Please either use !*path or be consistent with code a few lines
> down and use '\0'.
Okay.
>
>> + return dir;
>> +
>> + if ( dir->type != hypfs_type_dir )
>> + return NULL;
>> +
>> + slash = strchr(path, '/');
>> + if ( !slash )
>> + slash = strchr(path, '\0');
>
> With this better name the variable "end" or some such?
Fine with me.
>
>> + name_len = slash - path;
>> +
>> + list_for_each ( l, &dir->dir->list )
>> + {
>> + int cmp;
>> +
>> + entry = list_entry(l, struct hypfs_entry, list);
>
> Why not list_for_each_entry(), eliminating the need for the "l"
> helper variable?
Ah, of course!
>
>> + cmp = strncmp(path, entry->name, name_len);
>> + if ( cmp < 0 )
>> + return NULL;
>> + if ( cmp > 0 )
>> + continue;
>> + if ( strlen(entry->name) == name_len )
>> + return *slash ? hypfs_get_entry_rel(entry, slash + 1) : entry;
>
> Perhaps slightly shorter
>
> if ( cmp == 0 && strlen(entry->name) == name_len )
> return *slash ? hypfs_get_entry_rel(entry, slash + 1) : entry;
>
> ?
Okay.
>
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +struct hypfs_entry *hypfs_get_entry(char *path)
>
> const?
Yes.
>
>> +{
>> + if ( path[0] != '/' )
>> + return NULL;
>> +
>> + return hypfs_get_entry_rel(&hypfs_root_entry, path + 1);
>> +}
>> +
>> +static unsigned int hypfs_get_entry_len(struct hypfs_entry *entry)
>> +{
>> + unsigned int len = 0;
>> +
>> + switch ( entry->type )
>> + {
>> + case hypfs_type_dir:
>> + len = entry->dir->content_size;
>> + break;
>> + case hypfs_type_string:
>> + len = strlen(entry->str_val) + 1;
>> + break;
>> + case hypfs_type_uint:
>> + len = 11; /* longest possible printed value + 1 */
>
> Why would uint values be restricted to 32 bits? There are plenty of
> 64-bit numbers that might be of interest exposing through this
> interface (and even more so if down the road we were to re-use this
> for something debugfs-like). And even without this I think it would
> be better to not have a literal number here - it'll be close to
> unnoticeable (without someone remembering) when porting to an arch
> with unsigned int wider than 32 bits.
Support of 64-bit numbers would add "hypfs_type_ulong".
So basically something like "(sizeof(type) * 8 * 3 + 9) / 10 + 1" ?
(equivalent to: 10 bits make roughly 3 digits, round that up and
add 0-Byte). This is correct for 1, 2, 4 and 8 byte values. For 16
byte values the result is 40, but it should be 41.
>
>> + break;
>> + }
>> +
>> + return len;
>> +}
>> +
>> +long do_hypfs_op(unsigned int cmd,
>> + XEN_GUEST_HANDLE_PARAM(void) arg1, unsigned long arg2,
>> + XEN_GUEST_HANDLE_PARAM(void) arg3, unsigned long arg4)
>> +{
>> + int ret;
>> + struct hypfs_entry *entry;
>> + unsigned int len;
>> + static char path[XEN_HYPFS_MAX_PATHLEN];
>> +
>> + if ( !is_control_domain(current->domain) &&
>> + !is_hardware_domain(current->domain) )
>> + return -EPERM;
>
> Replace by an XSM check?
Yes, but I could need some help here. How do I add a new hypercall
in XSM?
>
>> + spin_lock(&hypfs_lock);
>
> Wouldn't this better be an r/w lock from the beginning, requiring
> only read access here?
Depending on the further use cases we might even end up with per-element
locks. I'm fine using a r/w lock for now.
>
>> + ret = hypfs_get_path_user(path, arg1, arg2);
>> + if ( ret )
>> + goto out;
>> +
>> + entry = hypfs_get_entry(path);
>> + if ( !entry )
>> + {
>> + ret = -ENOENT;
>> + goto out;
>> + }
>> +
>> + switch ( cmd )
>> + {
>> + case XEN_HYPFS_OP_read_contents:
>> + {
>> + char buf[12];
>> + char *val = buf;
>
> const void *?
Why void *? The result is always a string.
>
>> + len = hypfs_get_entry_len(entry);
>> + if ( len > arg4 )
>> + {
>> + ret = len;
>> + break;
>> + }
>> +
>> + switch ( entry->type )
>> + {
>> + case hypfs_type_dir:
>> + ret = -EISDIR;
>> + break;
>> + case hypfs_type_string:
>> + val = entry->str_val;
>> + break;
>> + case hypfs_type_uint:
>> + len = snprintf(buf, sizeof(buf), "%u", *entry->uint_val) + 1;
>> + break;
>> + }
>> +
>> + if ( !ret && copy_to_guest(arg3, val, len) )
>> + ret = -EFAULT;
>> +
>> + break;
>> + }
>> +
>> + case XEN_HYPFS_OP_read_dir:
>> + {
>> + struct list_head *l;
>> +
>> + if ( entry->type != hypfs_type_dir )
>> + {
>> + ret = -ENOTDIR;
>> + break;
>> + }
>> +
>> + len = entry->dir->content_size;
>> + if ( len > arg4 )
>> + {
>> + ret = len;
>> + break;
>> + }
>> +
>> + list_for_each ( l, &entry->dir->list )
>
> list_for_each_entry() again?
Yes.
>
>> + {
>> + struct xen_hypfs_direntry direntry;
>> + struct hypfs_entry *e = list_entry(l, struct hypfs_entry, list);
>> + unsigned int e_len = strlen(e->name) + 1;
>> +
>> + e_len = sizeof(direntry) + ROUNDUP(e_len, 4);
>
> Literal 4 again. Perhaps you want to put the entire ROUNDUP(, 4)
> construct in a macro or function?
Yes, might be better in a function.
>
>> + direntry.flags = (e->type == hypfs_type_dir) ? XEN_HYPFS_ISDIR : 0;
>> + direntry.off_next = list_is_last(l, &entry->dir->list) ? 0 : e_len;
>> + direntry.content_len = hypfs_get_entry_len(e);
>> + if ( copy_to_guest(arg3, &direntry, 1) )
>> + {
>> + ret = -EFAULT;
>> + goto out;
>> + }
>> +
>> + if ( copy_to_guest_offset(arg3, sizeof(direntry), e->name,
>> + strlen(e->name) + 1) )
>
> You calculate the length once already a few lines up. Please store into
> another local variable and reuse here.
Okay.
>
>> + {
>> + ret = -EFAULT;
>> + goto out;
>> + }
>> +
>> + guest_handle_add_offset(arg3, e_len);
>
> I think it would be good to assert somewhere here that you don't go
> beyond the dir's stored content_size.
Fine with me.
>
>> --- /dev/null
>> +++ b/xen/include/public/hypfs.h
>> @@ -0,0 +1,123 @@
>> +/******************************************************************************
>> + * Xen Hypervisor Filesystem
>> + *
>> + * Copyright (c) 2019, SUSE Software Solutions Germany GmbH
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (the "Software"), to
>> + * deal in the Software without restriction, including without limitation the
>> + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
>> + * sell copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
>> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
>> + * DEALINGS IN THE SOFTWARE.
>> + *
>> + */
>> +
>> +#ifndef __XEN_PUBLIC_HYPFS_H__
>> +#define __XEN_PUBLIC_HYPFS_H__
>> +
>> +#include "xen.h"
>> +
>> +/*
>> + * Definitions for the __HYPERVISOR_hypfs_op hypercall.
>> + */
>> +
>> +/* Maximum length of a path in the filesystem. */
>> +#define XEN_HYPFS_MAX_PATHLEN 1024
>> +
>> +struct xen_hypfs_direntry {
>> + uint16_t flags;
>> +#define XEN_HYPFS_ISDIR 0x0001
>> +#define XEN_HYPFS_WRITEABLE 0x0002
>> + /* Offset in bytes to next entry (0 == this is the last entry). */
>> + uint16_t off_next;
>> + uint32_t content_len;
>> + char name[XEN_FLEX_ARRAY_DIM];
>> +};
>
> Are you certain we won't soon need further fields here? Expressing
> symlinks can perhaps be done via a new XEN_HYPFS_* flag, but there
> may be other things that would be better to provide for even if
> there's no implementation from the beginning. It's just the case
> that changing this structure after the fact is going to be
> impossible, and it'll take new sub-ops then instead.
Okay, I'll make flags uint32_t and add a "uint16_t off_name". This
allows adding more fields later (e.g. depending on flags).
>
>> +/*
>> + * Hypercall operations.
>> + */
>> +
>> +/*
>> + * XEN_HYPFS_OP_read_contents
>> + *
>> + * Read contents of a filesystem entry.
>> + *
>> + * Returns the contents of an entry in the buffer supplied by the caller.
>> + * Only text data with a trailing zero byte is returned.
>> + *
>> + * arg1: XEN_GUEST_HANDLE(path name)
>> + * arg2: length of path name (including trailing zero byte)
>> + * arg3: XEN_GUEST_HANDLE(content buffer)
>> + * arg4: content buffer size
>> + *
>> + * Possible return values:
>> + * 0: success
>> + * -EPERM: operation not permitted
>> + * -ENOENT: entry not found
>> + * -EACCESS: access to entry not permitted
>> + * -EISDIR: entry is a directory
>> + * -EINVAL: invalid parameter
>
> I'm not convinced enumerating possible return value is a good idea.
> Down the road we're certain to forget extending this list. Plus
> extension would, strictly speaking, not even be allowed if these
> enumerations are considered part of the interface.
Okay, I'll drop listing them.
>
>> + * positive value: content buffer was too small, returned value is needed size
>
> Positive return values are problematic when reaching INT_MAX. Are you
> convinced we want to have yet another instance?
Are you convinced we want to return more then 2G long strings in one go?
>
>> + */
>> +#define XEN_HYPFS_OP_read_contents 1
>> +
>> +/*
>> + * XEN_HYPFS_OP_read_dir
>> + *
>> + * Read directory entries of a directory.
>> + *
>> + * Returns a struct xen_fs_direntry for each entry in a directory.
>> + *
>> + * arg1: XEN_GUEST_HANDLE(path name)
>> + * arg2: length of path name (including trailing zero byte)
>> + * arg3: XEN_GUEST_HANDLE(content buffer)
>> + * arg4: content buffer size
>> + *
>> + * Possible return values:
>> + * 0: success
>> + * -EPERM: operation not permitted
>> + * -ENOENT: entry not found
>> + * -EACCESS: access to entry not permitted
>> + * -ENOTDIR: entry is not a directory
>> + * -EINVAL: invalid parameter
>> + * positive value: content buffer was too small, returned value is needed size
>> + */
>> +#define XEN_HYPFS_OP_read_dir 2
>> +
>> +/*
>> + * XEN_HYPFS_OP_read_contents
>
> XEN_HYPFS_OP_write_contents
Oh, of yourse!
>
>> + * Write contents of a filesystem entry.
>> + *
>> + * Writes an entry with the contents of a buffer supplied by the caller.
>> + * Only text data with a trailing zero byte can be written.
>> + *
>> + * arg1: XEN_GUEST_HANDLE(path name)
>> + * arg2: length of path name (including trailing zero byte)
>> + * arg3: XEN_GUEST_HANDLE(content buffer)
>> + * arg4: content buffer size
>
> The latest here (in contrast to the read counterpart) I think it becomes
> desirable to identify what's IN and what's OUT.
Yes.
>
>> --- /dev/null
>> +++ b/xen/include/xen/hypfs.h
>> @@ -0,0 +1,40 @@
>> +#ifndef __XEN_HYPFS_H__
>> +#define __XEN_HYPFS_H__
>> +
>> +#include <xen/list.h>
>> +
>> +struct hypfs_dir {
>> + unsigned int content_size;
>> + struct list_head list;
>> +};
>> +
>> +enum hypfs_entry_type {
>> + hypfs_type_dir,
>> + hypfs_type_string,
>> + hypfs_type_uint
>> +};
>> +
>> +struct hypfs_entry {
>> + enum hypfs_entry_type type;
>> + const char *name;
>> + struct list_head list;
>> + struct hypfs_dir *parent;
>
> Afaict you set this field, but you never use it anywhere. Why do you
> add it in the first place? (Initially I meant to ask whether this
> can be pointer-to-const.)
It will be needed for cases where the entry is being changed, e.g.
when support for custom runtime parameters is added.
>
>> + union {
>> + void *content;
>
> const?
>
>> + struct hypfs_dir *dir;
>
> const?
As already said above: mixing const and non-const pointers in a
union looks fishy to me.
>
>> + char *str_val;
>> + unsigned int *uint_val;
>> + };
>> +};
>> +
>> +extern struct hypfs_dir hypfs_root;
>> +
>> +int hypfs_new_dir(struct hypfs_dir *parent, const char *name,
>> + struct hypfs_dir *dir);
>> +int hypfs_new_entry_string(struct hypfs_dir *parent, const char *name,
>> + char *val);
>> +int hypfs_new_entry_uint(struct hypfs_dir *parent, const char *name,
>> + unsigned int *val);
>
> Thinking about the lack of const on the last parameters here again -
> if these are for the pointed to values to be modifiable through
> this interface, then how would the "owning" component learn of the
> value having changed? Not everyone may need this, but I think there
> would want to be a callback. Until then perhaps better to add const
> here, promising that the values won't change behind the backs of
> the owners.
That's what hypfs_lock is for (and maybe later per-element locks).
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 12.11.2019 17:06, Jürgen Groß wrote:
> On 12.11.19 14:48, Jan Beulich wrote:
>> On 02.10.2019 13:20, Juergen Gross wrote:
>>> +static int hypfs_add_entry(struct hypfs_dir *parent, struct hypfs_entry *new)
>>> +{
>>> + int ret = -ENOENT;
>>> + struct list_head *l;
>>> +
>>> + if ( !new->content )
>>> + return -EINVAL;
>>> +
>>> + spin_lock(&hypfs_lock);
>>> +
>>> + list_for_each ( l, &parent->list )
>>> + {
>>> + struct hypfs_entry *e = list_entry(l, struct hypfs_entry, list);
>>
>> const?
>
> Hmm, is this true when I add a new entry to it? l is part of *e
> after all.
But you don't use e in a way requiring it to be non-const. Question
is (as I did ask elsewhere already) whether you wouldn't better use
list_for_each_entry() here in the first place.
>>> +static unsigned int hypfs_get_entry_len(struct hypfs_entry *entry)
>>> +{
>>> + unsigned int len = 0;
>>> +
>>> + switch ( entry->type )
>>> + {
>>> + case hypfs_type_dir:
>>> + len = entry->dir->content_size;
>>> + break;
>>> + case hypfs_type_string:
>>> + len = strlen(entry->str_val) + 1;
>>> + break;
>>> + case hypfs_type_uint:
>>> + len = 11; /* longest possible printed value + 1 */
>>
>> Why would uint values be restricted to 32 bits? There are plenty of
>> 64-bit numbers that might be of interest exposing through this
>> interface (and even more so if down the road we were to re-use this
>> for something debugfs-like). And even without this I think it would
>> be better to not have a literal number here - it'll be close to
>> unnoticeable (without someone remembering) when porting to an arch
>> with unsigned int wider than 32 bits.
>
> Support of 64-bit numbers would add "hypfs_type_ulong".
At this layer I dislike making assumptions on the bitness of int
or long. Can we settle on either a type that's suitable for all
sensible values (would likely be unsigned long long) or use fixed
with identifications (hypfs_type_u32 et al)?
> So basically something like "(sizeof(type) * 8 * 3 + 9) / 10 + 1" ?
> (equivalent to: 10 bits make roughly 3 digits, round that up and
> add 0-Byte). This is correct for 1, 2, 4 and 8 byte values. For 16
> byte values the result is 40, but it should be 41.
That's one option. The other - especially worthwhile to consider
for large numbers - is to represent them in hex.
>>> + break;
>>> + }
>>> +
>>> + return len;
>>> +}
>>> +
>>> +long do_hypfs_op(unsigned int cmd,
>>> + XEN_GUEST_HANDLE_PARAM(void) arg1, unsigned long arg2,
>>> + XEN_GUEST_HANDLE_PARAM(void) arg3, unsigned long arg4)
>>> +{
>>> + int ret;
>>> + struct hypfs_entry *entry;
>>> + unsigned int len;
>>> + static char path[XEN_HYPFS_MAX_PATHLEN];
>>> +
>>> + if ( !is_control_domain(current->domain) &&
>>> + !is_hardware_domain(current->domain) )
>>> + return -EPERM;
>>
>> Replace by an XSM check?
>
> Yes, but I could need some help here. How do I add a new hypercall
> in XSM?
I guess we should rope in Daniel (now Cc-ed).
>>
>>> + spin_lock(&hypfs_lock);
>>
>> Wouldn't this better be an r/w lock from the beginning, requiring
>> only read access here?
>
> Depending on the further use cases we might even end up with per-element
> locks. I'm fine using a r/w lock for now.
Indeed I was thinking that write-value support would want a per-entry
lock, with the global one only guarding the directory tree.
>>> + ret = hypfs_get_path_user(path, arg1, arg2);
>>> + if ( ret )
>>> + goto out;
>>> +
>>> + entry = hypfs_get_entry(path);
>>> + if ( !entry )
>>> + {
>>> + ret = -ENOENT;
>>> + goto out;
>>> + }
>>> +
>>> + switch ( cmd )
>>> + {
>>> + case XEN_HYPFS_OP_read_contents:
>>> + {
>>> + char buf[12];
>>> + char *val = buf;
>>
>> const void *?
>
> Why void *? The result is always a string.
const char * might be fine too, but the code really doesn't depend
on this being other than void afaics.
>>> + * positive value: content buffer was too small, returned value is needed size
>>
>> Positive return values are problematic when reaching INT_MAX. Are you
>> convinced we want to have yet another instance?
>
> Are you convinced we want to return more then 2G long strings in one go?
Counter question: Are you convinced we'll stick to just strings?
See the gzip-ing question on the later patch for example.
>>> +struct hypfs_entry {
>>> + enum hypfs_entry_type type;
>>> + const char *name;
>>> + struct list_head list;
>>> + struct hypfs_dir *parent;
>>
>> Afaict you set this field, but you never use it anywhere. Why do you
>> add it in the first place? (Initially I meant to ask whether this
>> can be pointer-to-const.)
>
> It will be needed for cases where the entry is being changed, e.g.
> when support for custom runtime parameters is added.
Can we defer its introduction until then?
>>> + union {
>>> + void *content;
>>
>> const?
>>
>>> + struct hypfs_dir *dir;
>>
>> const?
>
> As already said above: mixing const and non-const pointers in a
> union looks fishy to me.
Hmm, well, I can see your point, but I think it still can be viewed
to have its (perhaps largely documentation) value.
>>> + char *str_val;
>>> + unsigned int *uint_val;
>>> + };
>>> +};
>>> +
>>> +extern struct hypfs_dir hypfs_root;
>>> +
>>> +int hypfs_new_dir(struct hypfs_dir *parent, const char *name,
>>> + struct hypfs_dir *dir);
>>> +int hypfs_new_entry_string(struct hypfs_dir *parent, const char *name,
>>> + char *val);
>>> +int hypfs_new_entry_uint(struct hypfs_dir *parent, const char *name,
>>> + unsigned int *val);
>>
>> Thinking about the lack of const on the last parameters here again -
>> if these are for the pointed to values to be modifiable through
>> this interface, then how would the "owning" component learn of the
>> value having changed? Not everyone may need this, but I think there
>> would want to be a callback. Until then perhaps better to add const
>> here, promising that the values won't change behind the backs of
>> the owners.
>
> That's what hypfs_lock is for (and maybe later per-element locks).
I don't understand: Are you intending random code to acquire this
lock?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 13.11.19 15:07, Jan Beulich wrote:
> On 12.11.2019 17:06, Jürgen Groß wrote:
>> On 12.11.19 14:48, Jan Beulich wrote:
>>> On 02.10.2019 13:20, Juergen Gross wrote:
>>>> +static int hypfs_add_entry(struct hypfs_dir *parent, struct hypfs_entry *new)
>>>> +{
>>>> + int ret = -ENOENT;
>>>> + struct list_head *l;
>>>> +
>>>> + if ( !new->content )
>>>> + return -EINVAL;
>>>> +
>>>> + spin_lock(&hypfs_lock);
>>>> +
>>>> + list_for_each ( l, &parent->list )
>>>> + {
>>>> + struct hypfs_entry *e = list_entry(l, struct hypfs_entry, list);
>>>
>>> const?
>>
>> Hmm, is this true when I add a new entry to it? l is part of *e
>> after all.
>
> But you don't use e in a way requiring it to be non-const. Question
> is (as I did ask elsewhere already) whether you wouldn't better use
> list_for_each_entry() here in the first place.
I already agreed on doing that.
>
>>>> +static unsigned int hypfs_get_entry_len(struct hypfs_entry *entry)
>>>> +{
>>>> + unsigned int len = 0;
>>>> +
>>>> + switch ( entry->type )
>>>> + {
>>>> + case hypfs_type_dir:
>>>> + len = entry->dir->content_size;
>>>> + break;
>>>> + case hypfs_type_string:
>>>> + len = strlen(entry->str_val) + 1;
>>>> + break;
>>>> + case hypfs_type_uint:
>>>> + len = 11; /* longest possible printed value + 1 */
>>>
>>> Why would uint values be restricted to 32 bits? There are plenty of
>>> 64-bit numbers that might be of interest exposing through this
>>> interface (and even more so if down the road we were to re-use this
>>> for something debugfs-like). And even without this I think it would
>>> be better to not have a literal number here - it'll be close to
>>> unnoticeable (without someone remembering) when porting to an arch
>>> with unsigned int wider than 32 bits.
>>
>> Support of 64-bit numbers would add "hypfs_type_ulong".
>
> At this layer I dislike making assumptions on the bitness of int
> or long. Can we settle on either a type that's suitable for all
> sensible values (would likely be unsigned long long) or use fixed
> with identifications (hypfs_type_u32 et al)?
This is a problem with e.g. runtime parameters. The current int type
parameters are unsigned int. So changing the type to hypfs_type_u32
would then make assumptions about unsigned int bitness.
My plan was to have hypfs_type_* according to the definitions of the
variables pointed to. Maybe the sensible way to handle that would be
to have hypfs_type_u(sz) similar to boot/runtime parameter handling.
>> So basically something like "(sizeof(type) * 8 * 3 + 9) / 10 + 1" ?
>> (equivalent to: 10 bits make roughly 3 digits, round that up and
>> add 0-Byte). This is correct for 1, 2, 4 and 8 byte values. For 16
>> byte values the result is 40, but it should be 41.
>
> That's one option. The other - especially worthwhile to consider
> for large numbers - is to represent them in hex.
Hmm, with your request regarding .config, maybe just transferring the
binary value and size might be the best option.
>
>>>> + break;
>>>> + }
>>>> +
>>>> + return len;
>>>> +}
>>>> +
>>>> +long do_hypfs_op(unsigned int cmd,
>>>> + XEN_GUEST_HANDLE_PARAM(void) arg1, unsigned long arg2,
>>>> + XEN_GUEST_HANDLE_PARAM(void) arg3, unsigned long arg4)
>>>> +{
>>>> + int ret;
>>>> + struct hypfs_entry *entry;
>>>> + unsigned int len;
>>>> + static char path[XEN_HYPFS_MAX_PATHLEN];
>>>> +
>>>> + if ( !is_control_domain(current->domain) &&
>>>> + !is_hardware_domain(current->domain) )
>>>> + return -EPERM;
>>>
>>> Replace by an XSM check?
>>
>> Yes, but I could need some help here. How do I add a new hypercall
>> in XSM?
>
> I guess we should rope in Daniel (now Cc-ed).
>
>>>
>>>> + spin_lock(&hypfs_lock);
>>>
>>> Wouldn't this better be an r/w lock from the beginning, requiring
>>> only read access here?
>>
>> Depending on the further use cases we might even end up with per-element
>> locks. I'm fine using a r/w lock for now.
>
> Indeed I was thinking that write-value support would want a per-entry
> lock, with the global one only guarding the directory tree.
>
>>>> + ret = hypfs_get_path_user(path, arg1, arg2);
>>>> + if ( ret )
>>>> + goto out;
>>>> +
>>>> + entry = hypfs_get_entry(path);
>>>> + if ( !entry )
>>>> + {
>>>> + ret = -ENOENT;
>>>> + goto out;
>>>> + }
>>>> +
>>>> + switch ( cmd )
>>>> + {
>>>> + case XEN_HYPFS_OP_read_contents:
>>>> + {
>>>> + char buf[12];
>>>> + char *val = buf;
>>>
>>> const void *?
>>
>> Why void *? The result is always a string.
>
> const char * might be fine too, but the code really doesn't depend
> on this being other than void afaics.
>
>>>> + * positive value: content buffer was too small, returned value is needed size
>>>
>>> Positive return values are problematic when reaching INT_MAX. Are you
>>> convinced we want to have yet another instance?
>>
>> Are you convinced we want to return more then 2G long strings in one go?
>
> Counter question: Are you convinced we'll stick to just strings?
> See the gzip-ing question on the later patch for example.
Nevertheless I'm questioning the idea to support GB sized buffers. This
seems to be a perfect way to ask for problems.
But with binary value support we'd need a size reported anyway, so this
should be settled then.
>
>>>> +struct hypfs_entry {
>>>> + enum hypfs_entry_type type;
>>>> + const char *name;
>>>> + struct list_head list;
>>>> + struct hypfs_dir *parent;
>>>
>>> Afaict you set this field, but you never use it anywhere. Why do you
>>> add it in the first place? (Initially I meant to ask whether this
>>> can be pointer-to-const.)
>>
>> It will be needed for cases where the entry is being changed, e.g.
>> when support for custom runtime parameters is added.
>
> Can we defer its introduction until then?
Okay.
>
>>>> + union {
>>>> + void *content;
>>>
>>> const?
>>>
>>>> + struct hypfs_dir *dir;
>>>
>>> const?
>>
>> As already said above: mixing const and non-const pointers in a
>> union looks fishy to me.
>
> Hmm, well, I can see your point, but I think it still can be viewed
> to have its (perhaps largely documentation) value.
So the void pointer shouldn't be const IMO as it can be used as a
replacement for all of the other union members. And the dir member is
used as non const in case of adding an entry.
>
>>>> + char *str_val;
>>>> + unsigned int *uint_val;
>>>> + };
>>>> +};
>>>> +
>>>> +extern struct hypfs_dir hypfs_root;
>>>> +
>>>> +int hypfs_new_dir(struct hypfs_dir *parent, const char *name,
>>>> + struct hypfs_dir *dir);
>>>> +int hypfs_new_entry_string(struct hypfs_dir *parent, const char *name,
>>>> + char *val);
>>>> +int hypfs_new_entry_uint(struct hypfs_dir *parent, const char *name,
>>>> + unsigned int *val);
>>>
>>> Thinking about the lack of const on the last parameters here again -
>>> if these are for the pointed to values to be modifiable through
>>> this interface, then how would the "owning" component learn of the
>>> value having changed? Not everyone may need this, but I think there
>>> would want to be a callback. Until then perhaps better to add const
>>> here, promising that the values won't change behind the backs of
>>> the owners.
>>
>> That's what hypfs_lock is for (and maybe later per-element locks).
>
> I don't understand: Are you intending random code to acquire this
> lock?
Oh, now I understand your initial question better.
You are right, in general cases a callback might be needed (just the
same as with custom parameters).
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 13.11.2019 15:40, Jürgen Groß wrote:
> On 13.11.19 15:07, Jan Beulich wrote:
>> On 12.11.2019 17:06, Jürgen Groß wrote:
>>> On 12.11.19 14:48, Jan Beulich wrote:
>>>> On 02.10.2019 13:20, Juergen Gross wrote:
>>>>> +static unsigned int hypfs_get_entry_len(struct hypfs_entry *entry)
>>>>> +{
>>>>> + unsigned int len = 0;
>>>>> +
>>>>> + switch ( entry->type )
>>>>> + {
>>>>> + case hypfs_type_dir:
>>>>> + len = entry->dir->content_size;
>>>>> + break;
>>>>> + case hypfs_type_string:
>>>>> + len = strlen(entry->str_val) + 1;
>>>>> + break;
>>>>> + case hypfs_type_uint:
>>>>> + len = 11; /* longest possible printed value + 1 */
>>>>
>>>> Why would uint values be restricted to 32 bits? There are plenty of
>>>> 64-bit numbers that might be of interest exposing through this
>>>> interface (and even more so if down the road we were to re-use this
>>>> for something debugfs-like). And even without this I think it would
>>>> be better to not have a literal number here - it'll be close to
>>>> unnoticeable (without someone remembering) when porting to an arch
>>>> with unsigned int wider than 32 bits.
>>>
>>> Support of 64-bit numbers would add "hypfs_type_ulong".
>>
>> At this layer I dislike making assumptions on the bitness of int
>> or long. Can we settle on either a type that's suitable for all
>> sensible values (would likely be unsigned long long) or use fixed
>> with identifications (hypfs_type_u32 et al)?
>
> This is a problem with e.g. runtime parameters. The current int type
> parameters are unsigned int. So changing the type to hypfs_type_u32
> would then make assumptions about unsigned int bitness.
>
> My plan was to have hypfs_type_* according to the definitions of the
> variables pointed to. Maybe the sensible way to handle that would be
> to have hypfs_type_u(sz) similar to boot/runtime parameter handling.
Agreed.
>>>>> + union {
>>>>> + void *content;
>>>>
>>>> const?
>>>>
>>>>> + struct hypfs_dir *dir;
>>>>
>>>> const?
>>>
>>> As already said above: mixing const and non-const pointers in a
>>> union looks fishy to me.
>>
>> Hmm, well, I can see your point, but I think it still can be viewed
>> to have its (perhaps largely documentation) value.
>
> So the void pointer shouldn't be const IMO as it can be used as a
> replacement for all of the other union members.
But this was exactly the reason why I considered it to become
const - to disallow such use when it's about changing a value.
> And the dir member is
> used as non const in case of adding an entry.
Well, if there indeed is such a use (which I had looked for but
apparently overlooked), then of course const shouldn't be added.
Hence the question mark in my initial reply.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
© 2016 - 2026 Red Hat, Inc.