Re: [RFC v2 1/8] xen/hypfs: support fo nested dynamic hypfs nodes

[Juergen Gross <jgross@xxxxxxxx>]

On 11.02.22 09:16, Oleksii Moisieiev wrote:
> Hi Juergen,
>
> On Thu, Feb 10, 2022 at 08:34:08AM +0100, Juergen Gross wrote:
> > On 08.02.22 19:00, Oleksii Moisieiev wrote:
> > > Add new api:
> > > - hypfs_read_dyndir_entry
> > > - hypfs_gen_dyndir_entry
> > > which are the extension of the dynamic hypfs nodes support, presented in
> > > 0b3b53be8cf226d947a79c2535a9efbb2dd7bc38.
> > > This allows nested dynamic nodes to be added. Also input parameter is
> > > hypfs_entry, so properties can also be generated dynamically.
> > >
> > > Generating mixed list of dirs and properties is also supported.
> > > Same as to the dynamic hypfs nodes, this is anchored in percpu pointer,
> > > which can be retriewed on any level of the dynamic entries.
> > > This handle should be allocated on enter() callback and released on
> > > exit() callback. When using nested dynamic dirs and properties handle
> > > should be allocated on the first enter() call and released on the last
> > > exit() call.
> > >
> > > Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@xxxxxxxx>
...

> > > diff --git a/xen/include/xen/hypfs.h b/xen/include/xen/hypfs.h
> > > index e9d4c2555b..5d2728b963 100644
> > > --- a/xen/include/xen/hypfs.h
> > > +++ b/xen/include/xen/hypfs.h
> > > @@ -79,8 +79,8 @@ struct hypfs_entry_dir {
> > >    struct hypfs_dyndir_id {
> > Please rename to struct hypfs_dyndir.
> Ok, thanks.
>
> > >        struct hypfs_entry_dir dir;             /* Modified copy of template. */
> > >        struct hypfs_funcs funcs;               /* Dynamic functions. */
> > > -    const struct hypfs_entry_dir *template; /* Template used. */
> > > -#define HYPFS_DYNDIR_ID_NAMELEN 12
> > > +    const struct hypfs_entry *template; /* Template used. */
> > > +#define HYPFS_DYNDIR_ID_NAMELEN 32
> > >        char name[HYPFS_DYNDIR_ID_NAMELEN];     /* Name of hypfs entry. */
> > >        unsigned int id;                        /* Numerical id. */
> > What about the following change instead:
> >
> > -    const struct hypfs_entry_dir *template; /* Template used. */
> > -#define HYPFS_DYNDIR_ID_NAMELEN 12
> > -    char name[HYPFS_DYNDIR_ID_NAMELEN];     /* Name of hypfs entry. */
> > -
> > -    unsigned int id;                        /* Numerical id. */
> > -    void *data;                             /* Data associated with id. */
> > +    const struct hypfs_entry *template;  /* Template used. */
> > +    union {
> > +#define HYPFS_DYNDIR_NAMELEN    32
> > +        char name[HYPFS_DYNDIR_NAMELEN]; /* Name of hypfs entry. */
> > +        struct {
> > +#define HYPFS_DYNDIR_ID_NAMELEN 12
> > +            char name[HYPFS_DYNDIR_ID_NAMELEN]; /* Name of id entry. */
> > +            unsigned int id;                    /* Numerical id. */
> > +        } id;
> > +    };
> > +    void*data;                          /* Data associated with entry. */
> I'm not sure I see the benefit from this union. The only one I see is
> that struct hypds_dyndir will be smaller by sizeof(unsigned int).
> Could you explain please?
My main concern is that it is not obvious to a user that the
numerical id is needed only for a special case. Putting it into
the union makes this much more clear.

> Also what do you think about the following change:
> -    char name[HYPFS_DYNDIR_ID_NAMELEN];     /* Name of hypfs entry. */
> -
> -    unsigned int id;                        /* Numerical id. */
> -    void *data;                             /* Data associated with id. */
> +    char name[HYPFS_DYNDIR_ID_NAMELEN];     /* Name of hypfs entry. */
> +
> +    unsigned int id;                        /* Numerical id. */
> +    union {
> +       const void *content;
> +       void *data;                             /* Data associated with id. */
> +    }
> This change is similar to the hypfs_entry_leaf. In this case we can
> store const pointer for read-only entries and use data when write access
> is needed?
Sure, if you need that.

> PS: I will address all your comments in v3.
Thanks,


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature