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

[Jürgen Groß <jgross@xxxxxxxx>]

On 18.05.20 16:27, Jan Beulich wrote:
> 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?
Yes, good idea.

> > +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.
Okay.

> > +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.
Oh, sorry.

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


Juergen