Re: [PATCH v9 04/12] xen: add basic hypervisor filesystem support

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.05.2020 13:58, Juergen Gross wrote:
> --- /dev/null
> +++ b/xen/common/hypfs.c
> @@ -0,0 +1,418 @@
> +/******************************************************************************
> + *
> + * hypfs.c
> + *
> + * Simple sysfs-like file system for the hypervisor.
> + */
> +
> +#include <xen/err.h>
> +#include <xen/guest_access.h>
> +#include <xen/hypercall.h>
> +#include <xen/hypfs.h>
> +#include <xen/lib.h>
> +#include <xen/rwlock.h>
> +#include <public/hypfs.h>
> +
> +#ifdef CONFIG_COMPAT
> +#include <compat/hypfs.h>
> +CHECK_hypfs_dirlistentry;
> +#endif
> +
> +#define DIRENTRY_NAME_OFF offsetof(struct xen_hypfs_dirlistentry, name)
> +#define DIRENTRY_SIZE(name_len) \
> +    (DIRENTRY_NAME_OFF +        \
> +     ROUNDUP((name_len) + 1, alignof(struct xen_hypfs_direntry)))
> +
> +static DEFINE_RWLOCK(hypfs_lock);
> +enum hypfs_lock_state {
> +    hypfs_unlocked,
> +    hypfs_read_locked,
> +    hypfs_write_locked
> +};
> +static DEFINE_PER_CPU(enum hypfs_lock_state, hypfs_locked);
> +
> +HYPFS_DIR_INIT(hypfs_root, "");
> +
> +static void hypfs_read_lock(void)
> +{
> +    read_lock(&hypfs_lock);
> +    this_cpu(hypfs_locked) = hypfs_read_locked;
> +}

Perhaps at least

    ASSERT(this_cpu(hypfs_locked) != hypfs_write_locked);

first thing in the function?

> +static void hypfs_write_lock(void)
> +{
> +    write_lock(&hypfs_lock);
> +    this_cpu(hypfs_locked) = hypfs_write_locked;
> +}

If so,

    ASSERT(this_cpu(hypfs_locked) == hypfs_unlocked);

here then.

> +static struct hypfs_entry *hypfs_get_entry_rel(struct hypfs_entry_dir *dir,
> +                                               const char *path)
> +{
> +    const char *end;
> +    struct hypfs_entry *entry;
> +    unsigned int name_len;
> +    bool again = true;
> +
> +    while ( again )
> +    {
> +        if ( dir->e.type != XEN_HYPFS_TYPE_DIR )
> +            return NULL;
> +
> +        if ( !*path )
> +            return &dir->e;
> +
> +        end = strchr(path, '/');
> +        if ( !end )
> +            end = strchr(path, '\0');
> +        name_len = end - path;
> +
> +     again = false;

Hard tab slipped in.

With at least the latter taken care of, non-XSM pieces
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan