|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 2/6] xen: add basic hypervisor filesystem support
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.
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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |