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

[Luca Fancellu <luca.fancellu@arm.com>]

> On 20 Apr 2021, at 11:27, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 20.04.2021 11:42, Luca Fancellu wrote:
>>> On 20 Apr 2021, at 10:14, Jan Beulich <jbeulich@suse.com> wrote:
>>> On 20.04.2021 10:46, Luca Fancellu wrote:
>>>>> On 19 Apr 2021, at 12:05, Jan Beulich <jbeulich@suse.com> wrote:
>>>>> On 19.04.2021 11:12, Luca Fancellu wrote:
>>>>>> - */
>>>>>> -
>>>>>> -/*
>>>>>> - * Reference to a grant entry in a specified domain's grant table.
>>>>>> - */
>>>>>> -typedef uint32_t grant_ref_t;
>>>>> 
>>>>> Why does this get moved ...
>>>>> 
>>>>>> -
>>>>>> -/*
>>>>>> + *
>>>>>> * A grant table comprises a packed array of grant entries in one or more
>>>>>> * page frames shared between Xen and a guest.
>>>>>> + *
>>>>>> * [XEN]: This field is written by Xen and read by the sharing guest.
>>>>>> + *
>>>>>> * [GST]: This field is written by the guest and read by Xen.
>>>>>> + *
>>>>>> + * @addtogroup grant_table Grant Tables
>>>>>> + * @{
>>>>>> */
>>>>>> 
>>>>>> -/*
>>>>>> - * Version 1 of the grant table entry structure is maintained purely
>>>>>> - * for backwards compatibility.  New guests should use version 2.
>>>>>> +/**
>>>>>> + * Reference to a grant entry in a specified domain's grant table.
>>>>>> */
>>>>>> +typedef uint32_t grant_ref_t;
>>>>> 
>>>>> ... here, past a comment unrelated to it?
>>>> 
>>>> The comment “Version 1 of the grant table entry […]” was moved on top of the struct grant_entry_v1, in this way
>>>> Doxygen associate the comment to that structure.
>>>> 
>>>> The comment “Reference to a grant entry in a specified domain's grant table.” Is moved on top of typedef uint32_t grant_ref_t
>>>> for the same reason above
>>> 
>>> But it's the other comment ("A grant table comprises ...") that I
>>> was asking about.
>> 
>> I thought it was part of the brief about grant tables, am I wrong? For that reason I moved it.
> 
> Well, the comment talks about grant table entries (the layout of which
> gets defined immediately below, as visible in the original patch), not
> grant references.

Hi Jan,

Of course this can be reverted back if it is wrong. 

I’ve prepared a page with the output of all my commits (some of them are not yet in the ML),
so anyone can have a look on what is the output and how can sphinx+doxygen improve
the quality of the documentation.

Here: 

https://luca.fancellu.gitlab.io/xen-docs/hypercall-interfaces/arm64.html

the document Is trying to reproduce this other one from the current docs:

https://xenbits.xen.org/docs/unstable/hypercall/arm/index.html

You will receive a warning from the browser because the gitlab certificate has problems when
the username has a dot in it.

> 
>>>>>> @@ -243,23 +258,27 @@ union grant_entry_v2 {
>>>>>>    * In that case, the frame field has the same semantics as the
>>>>>>    * field of the same name in the V1 entry structure.
>>>>>>    */
>>>>>> +    /** @cond skip anonymous struct/union for doxygen */
>>>>>>   struct {
>>>>>>       grant_entry_header_t hdr;
>>>>>>       uint32_t pad0;
>>>>>>       uint64_t frame;
>>>>>>   } full_page;
>>>>>> +    /** @endcond */
>>>>>> 
>>>>>>   /*
>>>>>>    * If the grant type is GTF_grant_access and GTF_sub_page is set,
>>>>>>    * @domid is allowed to access bytes [@page_off,@page_off+@length)
>>>>>>    * in frame @frame.
>>>>>>    */
>>>>>> +    /** @cond skip anonymous struct/union for doxygen */
>>>>>>   struct {
>>>>>>       grant_entry_header_t hdr;
>>>>>>       uint16_t page_off;
>>>>>>       uint16_t length;
>>>>>>       uint64_t frame;
>>>>>>   } sub_page;
>>>>>> +    /** @endcond */
>>>>>> 
>>>>>>   /*
>>>>>>    * If the grant is GTF_transitive, @domid is allowed to use the
>>>>>> @@ -270,12 +289,14 @@ union grant_entry_v2 {
>>>>>>    * The current version of Xen does not allow transitive grants
>>>>>>    * to be mapped.
>>>>>>    */
>>>>>> +    /** @cond skip anonymous struct/union for doxygen */
>>>>>>   struct {
>>>>>>       grant_entry_header_t hdr;
>>>>>>       domid_t trans_domid;
>>>>>>       uint16_t pad0;
>>>>>>       grant_ref_t gref;
>>>>>>   } transitive;
>>>>>> +    /** @endcond */
>>>>> 
>>>>> While already better than the introduction of strange struct tags,
>>>>> I'm still not convinced we want this extra clutter (sorry). Plus -
>>>>> don't these additions mean the sub-structures then won't be
>>>>> represented in the generated doc, rendering it (partly) useless?
>>>> 
>>>> Are you suggesting to remove the structure from docs?
>>> 
>>> Just yet I'm not suggesting anything here - I was merely guessing at
>>> the effect of "@cond" and the associated "skip ..." to mean that this
>>> construct makes doxygen skip the enclosed section. If that's not the
>>> effect, then maybe the comment wants rewording? (And then - what does
>>> @cond mean?)
>> 
>> Yes you were right, in the documentation the structure grant_entry_v2 won’t display the
>> anonymous union.
> 
> In which case I'm now completely unclear about your prior question.

We have to choose just if the struct is useful even if it’s partially documented or if
it’s better to hide it completely from the docs since it can’t be fully documented.

Cheers,
Luca

> 
> Jan