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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.04.2021 11:42, Luca Fancellu wrote:
>> On 20 Apr 2021, at 10:14, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> On 20.04.2021 10:46, Luca Fancellu wrote:
>>>> On 19 Apr 2021, at 12:05, Jan Beulich <jbeulich@xxxxxxxx> 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.

>>>>> @@ -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.

Jan