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

[Oleksii Moisieiev <Oleksii_Moisieiev@xxxxxxxx>]

On Fri, Feb 11, 2022 at 02:28:49PM +0100, Juergen Gross wrote:
> 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.
> 

This make sense. I'll make this union. Thanks.

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

Thanks I will do this as well.

Best regards,
Oleksii
> 
> > 
> > PS: I will address all your comments in v3.
> 
> Thanks,
> 
> 
> Juergen