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

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