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

[Luca Fancellu <luca.fancellu@xxxxxxx>]

> On 7 Apr 2021, at 09:10, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> 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.

Hi Jan,

Thank you for your feedback, I agree with you all that “a” is not really a good 
name,
I will be happy to change it if we define a pattern.

Just to be sure that we are in the same page, are you suggesting to modify the 
name
In this way?

struct gnttab_cache_flush {
-    union {
+    union xen_gnttab_cache_flush_a {
        uint64_t dev_bus_addr;
        grant_ref_t ref;
    } a;

Following this kind of pattern: xen_<upper struct name>_<member name> ?

Cheers,
Luca

> 
> Jan