Re: [PATCH 3/3] docs/doxygen: doxygen documentation for grant_table.h

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.04.2021 23:46, Stefano Stabellini wrote:
> On Tue, 6 Apr 2021, Jan Beulich wrote:
>> On 06.04.2021 12:36, Luca Fancellu wrote:
>>> Modification to include/public/grant_table.h:
>>>
>>> 1) Change anonymous structure to be named structure,
>>>    because doxygen can't deal with them.
>>
>> Especially in the form presented (adding further name space clutter
>> for consumers to fall over) I object to this, most notably ...
>>
>>> @@ -584,7 +599,7 @@ DEFINE_XEN_GUEST_HANDLE(gnttab_swap_grant_ref_t);
>>>   * page granted to the calling domain by a foreign domain.
>>>   */
>>>  struct gnttab_cache_flush {
>>> -    union {
>>> +    union a {
>>
>> ... this one.
> 
> It is unfortunate that none of these tools support anonymous structs or
> unions well. (You might recall we also had issues with the older
> kernel-doc series too, although a bit different.)

While I wouldn't veto such changes, I think it is a very bad approach
to make adjustments like this to cover for a docs tool shortcoming.
Is it entirely unreasonable to have the tool fixed? In fact, if the
issue was run into before, isn't it a bad sign that the tool hasn't
been fixed already, and we merely need to require a certain minimum
version?

> It is difficult to provide a good name here, a suggestion would be more
> than welcome. I agree with you that calling it "a" is a bad idea: "a"
> becomes a globally-visible union name.
> 
> Maybe we could call it: StructName_MemberName, so in this case:
> 
>   union gnttab_cache_flush_a
> 
> It makes sure it is unique and doesn't risk clashing with anything else.
> We can follow this pattern elsewhere as well.
> 
> Any better suggestions?

First and foremost any new additions ought to use a xen_, Xen_, or
XEN_ prefix. For the specific case here, since "a" is already a
rather bad choice for a member name (What does it stand for? In lieu
of any better name we typically use "u" in such cases.), the badness
shouldn't be extended. Sadly the ref as being one way of expressing
the target MFN is also not accompanied by a GFN (as it ought to be),
but an address. Otherwise I would have suggested to abstract the
similar union also used by struct gnttab_copy. In the end I can't
see many alternatives to something like xen_gnttab_cache_flush_target
or xen_gnttab_cache_flush_ref_or_addr.

Jan