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_moisie...@epam.com>
>
> ...
>
> > > > 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
