> On 7 Apr 2021, at 09:10, Jan Beulich <jbeul...@suse.com> 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
