Re: [Xen-devel] [PATCH v2 2/6] xen: add basic hypervisor filesystem support

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.11.2019 15:40, Jürgen Groß wrote:
> On 13.11.19 15:07, Jan Beulich wrote:
>> On 12.11.2019 17:06, Jürgen Groß wrote:
>>> On 12.11.19 14:48, Jan Beulich wrote:
>>>> On 02.10.2019 13:20, Juergen Gross wrote:
>>>>> +static unsigned int hypfs_get_entry_len(struct hypfs_entry *entry)
>>>>> +{
>>>>> +    unsigned int len = 0;
>>>>> +
>>>>> +    switch ( entry->type )
>>>>> +    {
>>>>> +    case hypfs_type_dir:
>>>>> +        len = entry->dir->content_size;
>>>>> +        break;
>>>>> +    case hypfs_type_string:
>>>>> +        len = strlen(entry->str_val) + 1;
>>>>> +        break;
>>>>> +    case hypfs_type_uint:
>>>>> +        len = 11;      /* longest possible printed value + 1 */
>>>>
>>>> Why would uint values be restricted to 32 bits? There are plenty of
>>>> 64-bit numbers that might be of interest exposing through this
>>>> interface (and even more so if down the road we were to re-use this
>>>> for something debugfs-like). And even without this I think it would
>>>> be better to not have a literal number here - it'll be close to
>>>> unnoticeable (without someone remembering) when porting to an arch
>>>> with unsigned int wider than 32 bits.
>>>
>>> Support of 64-bit numbers would add "hypfs_type_ulong".
>>
>> At this layer I dislike making assumptions on the bitness of int
>> or long. Can we settle on either a type that's suitable for all
>> sensible values (would likely be unsigned long long) or use fixed
>> with identifications (hypfs_type_u32 et al)?
> 
> This is a problem with e.g. runtime parameters. The current int type
> parameters are unsigned int. So changing the type to hypfs_type_u32
> would then make assumptions about unsigned int bitness.
> 
> My plan was to have hypfs_type_* according to the definitions of the
> variables pointed to. Maybe the sensible way to handle that would be
> to have hypfs_type_u(sz) similar to boot/runtime parameter handling.

Agreed.

>>>>> +    union {
>>>>> +        void *content;
>>>>
>>>> const?
>>>>
>>>>> +        struct hypfs_dir *dir;
>>>>
>>>> const?
>>>
>>> As already said above: mixing const and non-const pointers in a
>>> union looks fishy to me.
>>
>> Hmm, well, I can see your point, but I think it still can be viewed
>> to have its (perhaps largely documentation) value.
> 
> So the void pointer shouldn't be const IMO as it can be used as a
> replacement for all of the other union members.

But this was exactly the reason why I considered it to become
const - to disallow such use when it's about changing a value.

> And the dir member is
> used as non const in case of adding an entry.

Well, if there indeed is such a use (which I had looked for but
apparently overlooked), then of course const shouldn't be added.
Hence the question mark in my initial reply.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel