Re: [PATCH 3/3] docs/doxygen: doxygen documentation for grant_table.h
> On 8 Apr 2021, at 12:50, Julien Grall wrote:
>
>
>
> On 08/04/2021 12:40, Julien Grall wrote:
>> Hi Luca,
>> On 08/04/2021 12:02, Luca Fancellu wrote:
>>>
>>>
On 7 Apr 2021, at 22:26, Stefano Stabellini wrote:
On Wed, 7 Apr 2021, Jan Beulich wrote:
> On 07.04.2021 10:42, Luca Fancellu wrote:
>> 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__ ?
>
> While in general I would be fine with this scheme, for field names like
> "a" or "u" it doesn't fit well imo.
"a" is a bad name anyway, even for the member. We can take the
opportunity to find a better name. Almost anything would be better than
"a". Maybe "refaddr"?
> I'm also unconvinced this would be
> scalable to the case where there's further struct/union nesting.
How many of these instances of multilevel nesting do we have? Luca might
know. Probably not many? They could be special-cased.
>>>
>>> There are not many multilevel nesting instances of anonymous struct/union
>>> and the maximum level of nesting I found in the public headers is 2:
>>>
>>> union {
>>> union/struct {
>>> …
>>> }
>>> }
>>>
>>> I also see that in the majority of cases the unions have not meaningful
>>> names like “a” or “u” as member name, instead struct names are fine,
>>> It could be fine to keep the meaningful name the same for the struct type
>>> name and use the pattern for the non-meaningful ones as long
>>> as the names doesn’t create compilation errors?
>>>
>>> Example:
>>>
>>> struct upper_level {
>>> union {
>>> struct {
>>>
>>> } meaningful_name1;
>>> struct {
>>>
>>> } meaningful_name2;
>>> } u;
>>> };
>>>
>>> becomes:
>>>
>>> struct upper_level {
>>> union upper_level_u {
>>> struct meaningful_name1 {
>>>
>>> } meaningful_name1;
>>> struct meaningful_name2 {
>>>
>>> } meaningful_name2;
>>> } u;
>>> };
>> If I understand correctly your proposal, the name of the structure would be
>> the name of the field. The name of the fields are usually pretty generic so
>> you will likely end up to redefine the structure name.
>> Unless we want to provide random name, the only safe naming would be to
>> define the structure as upper_level_u_meaningful_name{1, 2}. But, this is
>> going to be pretty awful to read.
>> But I am still a bit puzzled by the fact doxygen is not capable to deal with
>> anynomous/unamed union. How do other projects deal with them?
>
> While going through the list of anynomous union in Xen, I noticed we also
> have something like:
>
> struct test {
>union {
>int a;
>int b;
>};
> };
>
> We can't name them because of syntactic reasons. What's your plan for them?
I would say that if the fields a and b are not meant to be described in the
documentation, they can be hidden, so there is no need to change the structure
itself but we might just add some comment containing
Doxygen tags for skipping them.
In Zephyr they have some kind of structures like that.
>
> Cheers,
>
> --
> Julien Grall
Re: [PATCH 3/3] docs/doxygen: doxygen documentation for grant_table.h
> On 8 Apr 2021, at 12:40, Julien Grall wrote:
>
> Hi Luca,
>
> On 08/04/2021 12:02, Luca Fancellu wrote:
>>> On 7 Apr 2021, at 22:26, Stefano Stabellini wrote:
>>>
>>> On Wed, 7 Apr 2021, Jan Beulich wrote:
On 07.04.2021 10:42, Luca Fancellu wrote:
> 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__ ?
While in general I would be fine with this scheme, for field names like
"a" or "u" it doesn't fit well imo.
>>>
>>> "a" is a bad name anyway, even for the member. We can take the
>>> opportunity to find a better name. Almost anything would be better than
>>> "a". Maybe "refaddr"?
>>>
>>>
I'm also unconvinced this would be
scalable to the case where there's further struct/union nesting.
>>>
>>> How many of these instances of multilevel nesting do we have? Luca might
>>> know. Probably not many? They could be special-cased.
>> There are not many multilevel nesting instances of anonymous struct/union
>> and the maximum level of nesting I found in the public headers is 2:
>> union {
>> union/struct {
>> …
>> }
>> }
>> I also see that in the majority of cases the unions have not meaningful
>> names like “a” or “u” as member name, instead struct names are fine,
>> It could be fine to keep the meaningful name the same for the struct type
>> name and use the pattern for the non-meaningful ones as long
>> as the names doesn’t create compilation errors?
>> Example:
>> struct upper_level {
>> union {
>> struct {
>>
>> } meaningful_name1;
>> struct {
>>
>> } meaningful_name2;
>> } u;
>> };
>> becomes:
>> struct upper_level {
>> union upper_level_u {
>> struct meaningful_name1 {
>>
>> } meaningful_name1;
>> struct meaningful_name2 {
>>
>> } meaningful_name2;
>> } u;
>> };
>
> If I understand correctly your proposal, the name of the structure would be
> the name of the field. The name of the fields are usually pretty generic so
> you will likely end up to redefine the structure name.
>
> Unless we want to provide random name, the only safe naming would be to
> define the structure as upper_level_u_meaningful_name{1, 2}. But, this is
> going to be pretty awful to read.
>
> But I am still a bit puzzled by the fact doxygen is not capable to deal with
> anynomous/unamed union. How do other projects deal with them?
>
>> Doing this will help a lot the documentation side because the html page will
>> show the "struct upper_level" with inside the “union upper_level_u" and
>> inside again
>> the two struct “meaningful_name1" and “meaningful_name2", and from the point
>> of view of the developer it can tell her/him exactly the name of the member
>> to
>> access when writing code (apart from the upper_level_u that can be accessed
>> by u, but we can’t clearly change it).
>
> I don't quite understand the last point. Wouldn't the developper see the
> field name? So how is it going to be different from seeing the structure name?
The developer, that is using the documentation generated with sphinx+doxygen,
will see the structure name and not the field name because this is the way
sphinx+doxygen is rendering the code structures. You can see it in the
generated documentation using this serie.
>
>> If this sounds difficult to understand when reading, please generate the
>> documentation and have a look on the page in one side and the source code in
>> another.
>
> Just to be clear, do you mean understanding what you wrote or a developper
> trying to understand the code?
I meant understanding what I wrote, because I know it’s difficult to describe
the concept without visualising the html page, it would have been much easier
to attach
an image to clarify.
>
> Cheers,
>
> --
> Julien Grall
Re: [PATCH 3/3] docs/doxygen: doxygen documentation for grant_table.h
On 08/04/2021 12:40, Julien Grall wrote:
Hi Luca,
On 08/04/2021 12:02, Luca Fancellu wrote:
On 7 Apr 2021, at 22:26, Stefano Stabellini
wrote:
On Wed, 7 Apr 2021, Jan Beulich wrote:
On 07.04.2021 10:42, Luca Fancellu wrote:
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__name> ?
While in general I would be fine with this scheme, for field names like
"a" or "u" it doesn't fit well imo.
"a" is a bad name anyway, even for the member. We can take the
opportunity to find a better name. Almost anything would be better than
"a". Maybe "refaddr"?
I'm also unconvinced this would be
scalable to the case where there's further struct/union nesting.
How many of these instances of multilevel nesting do we have? Luca might
know. Probably not many? They could be special-cased.
There are not many multilevel nesting instances of anonymous
struct/union and the maximum level of nesting I found in the public
headers is 2:
union {
union/struct {
…
}
}
I also see that in the majority of cases the unions have not
meaningful names like “a” or “u” as member name, instead struct names
are fine,
It could be fine to keep the meaningful name the same for the struct
type name and use the pattern for the non-meaningful ones as long
as the names doesn’t create compilation errors?
Example:
struct upper_level {
union {
struct {
} meaningful_name1;
struct {
} meaningful_name2;
} u;
};
becomes:
struct upper_level {
union upper_level_u {
struct meaningful_name1 {
} meaningful_name1;
struct meaningful_name2 {
} meaningful_name2;
} u;
};
If I understand correctly your proposal, the name of the structure would
be the name of the field. The name of the fields are usually pretty
generic so you will likely end up to redefine the structure name.
Unless we want to provide random name, the only safe naming would be to
define the structure as upper_level_u_meaningful_name{1, 2}. But, this
is going to be pretty awful to read.
But I am still a bit puzzled by the fact doxygen is not capable to deal
with anynomous/unamed union. How do other projects deal with them?
While going through the list of anynomous union in Xen, I noticed we
also have something like:
struct test {
union {
int a;
int b;
};
};
We can't name them because of syntactic reasons. What's your plan for them?
Cheers,
--
Julien Grall
Re: [PATCH 3/3] docs/doxygen: doxygen documentation for grant_table.h
Hi Luca,
On 08/04/2021 12:02, Luca Fancellu wrote:
On 7 Apr 2021, at 22:26, Stefano Stabellini wrote:
On Wed, 7 Apr 2021, Jan Beulich wrote:
On 07.04.2021 10:42, Luca Fancellu wrote:
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__ ?
While in general I would be fine with this scheme, for field names like
"a" or "u" it doesn't fit well imo.
"a" is a bad name anyway, even for the member. We can take the
opportunity to find a better name. Almost anything would be better than
"a". Maybe "refaddr"?
I'm also unconvinced this would be
scalable to the case where there's further struct/union nesting.
How many of these instances of multilevel nesting do we have? Luca might
know. Probably not many? They could be special-cased.
There are not many multilevel nesting instances of anonymous struct/union and
the maximum level of nesting I found in the public headers is 2:
union {
union/struct {
…
}
}
I also see that in the majority of cases the unions have not meaningful names
like “a” or “u” as member name, instead struct names are fine,
It could be fine to keep the meaningful name the same for the struct type name
and use the pattern for the non-meaningful ones as long
as the names doesn’t create compilation errors?
Example:
struct upper_level {
union {
struct {
} meaningful_name1;
struct {
} meaningful_name2;
} u;
};
becomes:
struct upper_level {
union upper_level_u {
struct meaningful_name1 {
} meaningful_name1;
struct meaningful_name2 {
} meaningful_name2;
} u;
};
If I understand correctly your proposal, the name of the structure would
be the name of the field. The name of the fields are usually pretty
generic so you will likely end up to redefine the structure name.
Unless we want to provide random name, the only safe naming would be to
define the structure as upper_level_u_meaningful_name{1, 2}. But, this
is going to be pretty awful to read.
But I am still a bit puzzled by the fact doxygen is not capable to deal
with anynomous/unamed union. How do other projects deal with them?
Doing this will help a lot the documentation side because the html page will show the
"struct upper_level" with inside the “union upper_level_u" and inside again
the two struct “meaningful_name1" and “meaningful_name2", and from the point of
view of the developer it can tell her/him exactly the name of the member to
access when writing code (apart from the upper_level_u that can be accessed by
u, but we can’t clearly change it).
I don't quite understand the last point. Wouldn't the developper see the
field name? So how is it going to be different from seeing the structure
name?
If this sounds difficult to understand when reading, please generate the
documentation and have a look on the page in one side and the source code in
another.
Just to be clear, do you mean understanding what you wrote or a
developper trying to understand the code?
Cheers,
--
Julien Grall
Re: [PATCH 3/3] docs/doxygen: doxygen documentation for grant_table.h
On 08.04.2021 13:02, Luca Fancellu wrote:
>
>
>> On 7 Apr 2021, at 22:26, Stefano Stabellini wrote:
>>
>> On Wed, 7 Apr 2021, Jan Beulich wrote:
>>> On 07.04.2021 10:42, Luca Fancellu wrote:
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__ ?
>>>
>>> While in general I would be fine with this scheme, for field names like
>>> "a" or "u" it doesn't fit well imo.
>>
>> "a" is a bad name anyway, even for the member. We can take the
>> opportunity to find a better name. Almost anything would be better than
>> "a". Maybe "refaddr"?
>>
>>
>>> I'm also unconvinced this would be
>>> scalable to the case where there's further struct/union nesting.
>>
>> How many of these instances of multilevel nesting do we have? Luca might
>> know. Probably not many? They could be special-cased.
>
> There are not many multilevel nesting instances of anonymous struct/union and
> the maximum level of nesting I found in the public headers is 2:
>
> union {
> union/struct {
> …
> }
> }
>
> I also see that in the majority of cases the unions have not meaningful names
> like “a” or “u” as member name, instead struct names are fine,
> It could be fine to keep the meaningful name the same for the struct type
> name and use the pattern for the non-meaningful ones as long
> as the names doesn’t create compilation errors?
>
> Example:
>
> struct upper_level {
> union {
> struct {
>
> } meaningful_name1;
> struct {
>
> } meaningful_name2;
> } u;
> };
>
> becomes:
>
> struct upper_level {
> union upper_level_u {
> struct meaningful_name1 {
>
> } meaningful_name1;
> struct meaningful_name2 {
>
> } meaningful_name2;
> } u;
> };
As you say - as long as there aren't any compilation issues. Two
top level struct-s could have identically named struct/union fields
without tag, in which case your approach outlined above will fail.
And even if there was no such case right now, the case would need
to be covered in whatever naming model was to be used (except, of
course, the one without any names).
Jan
Re: [PATCH 3/3] docs/doxygen: doxygen documentation for grant_table.h
> On 7 Apr 2021, at 22:26, Stefano Stabellini wrote:
>
> On Wed, 7 Apr 2021, Jan Beulich wrote:
>> On 07.04.2021 10:42, Luca Fancellu wrote:
>>> 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__ ?
>>
>> While in general I would be fine with this scheme, for field names like
>> "a" or "u" it doesn't fit well imo.
>
> "a" is a bad name anyway, even for the member. We can take the
> opportunity to find a better name. Almost anything would be better than
> "a". Maybe "refaddr"?
>
>
>> I'm also unconvinced this would be
>> scalable to the case where there's further struct/union nesting.
>
> How many of these instances of multilevel nesting do we have? Luca might
> know. Probably not many? They could be special-cased.
There are not many multilevel nesting instances of anonymous struct/union and
the maximum level of nesting I found in the public headers is 2:
union {
union/struct {
…
}
}
I also see that in the majority of cases the unions have not meaningful names
like “a” or “u” as member name, instead struct names are fine,
It could be fine to keep the meaningful name the same for the struct type name
and use the pattern for the non-meaningful ones as long
as the names doesn’t create compilation errors?
Example:
struct upper_level {
union {
struct {
} meaningful_name1;
struct {
} meaningful_name2;
} u;
};
becomes:
struct upper_level {
union upper_level_u {
struct meaningful_name1 {
} meaningful_name1;
struct meaningful_name2 {
} meaningful_name2;
} u;
};
Doing this will help a lot the documentation side because the html page will
show the "struct upper_level" with inside the “union upper_level_u" and inside
again
the two struct “meaningful_name1" and “meaningful_name2", and from the point of
view of the developer it can tell her/him exactly the name of the member to
access when writing code (apart from the upper_level_u that can be accessed by
u, but we can’t clearly change it).
If this sounds difficult to understand when reading, please generate the
documentation and have a look on the page in one side and the source code in
another.
Moreover the change should be back-compatible since we are not changing member
names.
Cheers,
Luca
Re: [PATCH 3/3] docs/doxygen: doxygen documentation for grant_table.h
On 07.04.2021 23:26, Stefano Stabellini wrote:
> On Wed, 7 Apr 2021, Jan Beulich wrote:
>> On 07.04.2021 10:42, Luca Fancellu wrote:
>>> 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__ ?
>>
>> While in general I would be fine with this scheme, for field names like
>> "a" or "u" it doesn't fit well imo.
>
> "a" is a bad name anyway, even for the member. We can take the
> opportunity to find a better name. Almost anything would be better than
> "a". Maybe "refaddr"?
We need to be careful with changing _anything_ in the public interface.
Consumers importing our headers directly (as was e.g. done for the old
linux-2.6.18-xen.hg tree) could break with a field name change as much
as with any other changes to what had been made available to them.
Jan
Re: [PATCH 3/3] docs/doxygen: doxygen documentation for grant_table.h
On Wed, 7 Apr 2021, Jan Beulich wrote:
> On 07.04.2021 10:42, Luca Fancellu wrote:
> > 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__ ?
>
> While in general I would be fine with this scheme, for field names like
> "a" or "u" it doesn't fit well imo.
"a" is a bad name anyway, even for the member. We can take the
opportunity to find a better name. Almost anything would be better than
"a". Maybe "refaddr"?
> I'm also unconvinced this would be
> scalable to the case where there's further struct/union nesting.
How many of these instances of multilevel nesting do we have? Luca might
know. Probably not many? They could be special-cased.
Re: [PATCH 3/3] docs/doxygen: doxygen documentation for grant_table.h
Bertrand Marquis writes ("Re: [PATCH 3/3] docs/doxygen: doxygen documentation
for grant_table.h"):
> Somehow in the documentation when you have a union you will need to document
> that it is a union and the possible entries.
I would expect to find the documentation for an anonymous struct or
union folded into the documentation of the containing structure, just
as it is in the source.
> One way or an other most standards like MISRA are forbidding anonymous
> entries as they cannot be referred to.
An anonymous union or struct like this is always the type of a single
field in a containing aggegate type. So if one needs to speak of it,
one can specify the container's type and the field. So it *can* be
named.
I am assuming we don't have *unused* anonymous structs and unions.
Ian.
Re: [PATCH 3/3] docs/doxygen: doxygen documentation for grant_table.h
Hi,
> On 7 Apr 2021, at 16:54, Jan Beulich wrote:
>
> On 07.04.2021 17:29, Bertrand Marquis wrote:
>>> On 7 Apr 2021, at 16:19, Ian Jackson wrote:
>>>
>>> Luca Fancellu writes ("Re: [PATCH 3/3] docs/doxygen: doxygen documentation
>>> for grant_table.h"):
>>>> The problem is that Doxygen can’t generate proper documentation for
>>>> anonymous union/structure, it ends up with warning and/or producing wrong
>>>> documentation like
>>>> changing names or giving field description to the wrong field.
>>>
>>> This does not seem like it would be an impossibly hard feature to add
>>> to doxygen.
>>
>> Modifying doxygen is not really in our planned efforts and if someone does
>> it that would put an hard constraint on the version of doxygen possible to
>> use.
>>
>> But is adding names to anonymous elements really an issue here ?
>
> It's clutter in the code base, making things harder to read (even if
> just slightly). It's certainly odd to make such source changes just
> for a doc tool. If changing doxygen is not an option for you, how
> about pre-processing the header and inserting the names the tool
> wants, before handing the result as input to it?
Introducing a new tool or find a pre-processing solution is for sure possible
but will be more complex and less error prone.
Also as said in my mail to Julien, passing this issue of anonymous entries now
might just push the problem as we might have to solve it later if we want to
become
MISRA compliant (which is also something we are looking in FuSa).
Regards
Bertrand
>
> Jan
>
Re: [PATCH 3/3] docs/doxygen: doxygen documentation for grant_table.h
Hi Julien,
> On 7 Apr 2021, at 16:55, Julien Grall wrote:
>
>
>
> On 07/04/2021 16:29, Bertrand Marquis wrote:
>> Hi Ian,
>>> On 7 Apr 2021, at 16:19, Ian Jackson wrote:
>>>
>>> Luca Fancellu writes ("Re: [PATCH 3/3] docs/doxygen: doxygen documentation
>>> for grant_table.h"):
>>>> The problem is that Doxygen can’t generate proper documentation for
>>>> anonymous union/structure, it ends up with warning and/or producing wrong
>>>> documentation like
>>>> changing names or giving field description to the wrong field.
>>>
>>> This does not seem like it would be an impossibly hard feature to add
>>> to doxygen.
>> Modifying doxygen is not really in our planned efforts and if someone does
>> it that would put an hard constraint on the version of doxygen possible to
>> use.
>
> Are you saying that anyone who want to use doxygen has to waive off the use
> of anonymous union/struct? Is it the only thing doxygen can't deal with?
That is the main one we came into while doing this but there might be other
going forward, hard to be sure at this stage.
>
>> But is adding names to anonymous elements really an issue here ?
>> If we agree on names or on a convention for name the result will not impact
>> the code or backward compatibility.
>
> I think the naming is only the tip of the problem. One advantage of anymous
> union/struct is you make clear they are not meant to be used outside of the
> context. So they should mostly be seen as an easy way to access some part of
> the "parent" structure directly. Therefore, IMHO, they don't deserve to be
> documented separately.
Somehow in the documentation when you have a union you will need to document
that it is a union and the possible entries.
Having a name to refer to it sounds to me a lot easier than making it anonymous.
One way or an other most standards like MISRA are forbidding anonymous entries
as they cannot be referred to.
>
> In fact, this is the first thing I noticed when building the documentation
> because 'union a' was in global index.
Definitely I agree the “a” is not a good solution and we need to find
meaningful names.
But this is in fact true for the sub-element in the structure (from which the
name was taken), using “a” as an identifier is not really explanatory of what
that is.
“u” for union can be see as a standard though.
This is why i think we should put names which a meaning but this is not always
easy to find.
Cheers
Bertrand
>
> Cheers,
>
> --
> Julien Grall
Re: [PATCH 3/3] docs/doxygen: doxygen documentation for grant_table.h
On 07/04/2021 16:29, Bertrand Marquis wrote:
Hi Ian,
On 7 Apr 2021, at 16:19, Ian Jackson wrote:
Luca Fancellu writes ("Re: [PATCH 3/3] docs/doxygen: doxygen documentation for
grant_table.h"):
The problem is that Doxygen can’t generate proper documentation for anonymous
union/structure, it ends up with warning and/or producing wrong documentation
like
changing names or giving field description to the wrong field.
This does not seem like it would be an impossibly hard feature to add
to doxygen.
Modifying doxygen is not really in our planned efforts and if someone does it
that would put an hard constraint on the version of doxygen possible to use.
Are you saying that anyone who want to use doxygen has to waive off the
use of anonymous union/struct? Is it the only thing doxygen can't deal with?
But is adding names to anonymous elements really an issue here ?
If we agree on names or on a convention for name the result will not impact the
code or backward compatibility.
I think the naming is only the tip of the problem. One advantage of
anymous union/struct is you make clear they are not meant to be used
outside of the context. So they should mostly be seen as an easy way to
access some part of the "parent" structure directly. Therefore, IMHO,
they don't deserve to be documented separately.
In fact, this is the first thing I noticed when building the
documentation because 'union a' was in global index.
Cheers,
--
Julien Grall
Re: [PATCH 3/3] docs/doxygen: doxygen documentation for grant_table.h
On 07.04.2021 17:29, Bertrand Marquis wrote:
>> On 7 Apr 2021, at 16:19, Ian Jackson wrote:
>>
>> Luca Fancellu writes ("Re: [PATCH 3/3] docs/doxygen: doxygen documentation
>> for grant_table.h"):
>>> The problem is that Doxygen can’t generate proper documentation for
>>> anonymous union/structure, it ends up with warning and/or producing wrong
>>> documentation like
>>> changing names or giving field description to the wrong field.
>>
>> This does not seem like it would be an impossibly hard feature to add
>> to doxygen.
>
> Modifying doxygen is not really in our planned efforts and if someone does it
> that would put an hard constraint on the version of doxygen possible to use.
>
> But is adding names to anonymous elements really an issue here ?
It's clutter in the code base, making things harder to read (even if
just slightly). It's certainly odd to make such source changes just
for a doc tool. If changing doxygen is not an option for you, how
about pre-processing the header and inserting the names the tool
wants, before handing the result as input to it?
Jan
Re: [PATCH 3/3] docs/doxygen: doxygen documentation for grant_table.h
Hi Ian,
> On 7 Apr 2021, at 16:19, Ian Jackson wrote:
>
> Luca Fancellu writes ("Re: [PATCH 3/3] docs/doxygen: doxygen documentation
> for grant_table.h"):
>> The problem is that Doxygen can’t generate proper documentation for
>> anonymous union/structure, it ends up with warning and/or producing wrong
>> documentation like
>> changing names or giving field description to the wrong field.
>
> This does not seem like it would be an impossibly hard feature to add
> to doxygen.
Modifying doxygen is not really in our planned efforts and if someone does it
that would put an hard constraint on the version of doxygen possible to use.
But is adding names to anonymous elements really an issue here ?
If we agree on names or on a convention for name the result will not impact the
code or backward compatibility.
Regards
Bertrand
>
> Ian.
Re: [PATCH 3/3] docs/doxygen: doxygen documentation for grant_table.h
Luca Fancellu writes ("Re: [PATCH 3/3] docs/doxygen: doxygen documentation for
grant_table.h"):
> The problem is that Doxygen can’t generate proper documentation for anonymous
> union/structure, it ends up with warning and/or producing wrong documentation
> like
> changing names or giving field description to the wrong field.
This does not seem like it would be an impossibly hard feature to add
to doxygen.
Ian.
Re: [PATCH 3/3] docs/doxygen: doxygen documentation for grant_table.h
> On 7 Apr 2021, at 14:56, Julien Grall wrote:
>
> Hi Luca,
>
> On 07/04/2021 14:19, Luca Fancellu wrote:
>>> On 7 Apr 2021, at 14:13, Julien Grall wrote:
>>>
>>> Hi,
>>>
>>> On 06/04/2021 11: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.
>>>
>>> What do you mean by can't deal with them? I had a quick try with doxygen
>>> build and couldn't find any failure (although I haven't looked at the
>>> output).
>>>
>> Hi Julien,
>> The problem is that Doxygen can’t generate proper documentation for
>> anonymous union/structure, it ends up with warning and/or producing wrong
>> documentation like
>> changing names or giving field description to the wrong field.
>
> I might do something wrong because I cannot spot any significant difference
> in the doxygen ouput if I switch back to anonymous union. Would you mind to
> post more details (such as a diff) on how doxygen doesn't generate properly
> documentation?
>
Hi Julien,
Here the explanation of the problem:
https://vovkos.github.io/doxyrest/manual/unnamed-structs.html
Clearly the proposed solution is not suitable because they are just hiding the
anonymous union/struct field from the documentation.
I did two test:
1) with the anonymous structure:
struct gnttab_cache_flush {
union {
uint64_t dev_bus_addr;
grant_ref_t ref;
} a;
I get a warning from sphinx (because the XML output of Doxygen is not in a good
shape) when I generate the documentation, here follow the output:
$ make -C docs XEN_TARGET_ARCH="arm64" sphinx-html
make: Entering directory '/home/user/prj_xen/xen/docs'
Generating xen.doxyfile
mv xen.doxyfile.tmp xen.doxyfile
Generating doxygen_input.h
/usr/bin/doxygen xen.doxyfile
Generating hypercall-interfaces/index.rst
XEN_ROOT=/home/user/prj_xen/xen /usr/bin/sphinx-build -b html . sphinx/html
Running Sphinx v1.6.7
making output directory...
loading pickled environment... not yet created
building [mo]: targets for 0 po files that are out of date
building [html]: targets for 21 source files that are out of date
updating environment: 21 added, 0 changed, 0 removed
reading sources... [100%] misc/xen-makefiles/makefiles
/home/user/prj_xen/xen/docs/hypercall-interfaces/arm64/grant_tables.rst:6:
WARNING: Invalid definition: Expected end of definition. [error at 18]
gnttab_cache_flush.a
--^
looking for now-outdated files... none found
pickling environment... done
checking consistency...
/home/user/prj_xen/xen/docs/hypercall-interfaces/arm32.rst: WARNING: document
isn't included in any toctree
/home/user/prj_xen/xen/docs/hypercall-interfaces/x86_64.rst: WARNING: document
isn't included in any toctree
/home/user/prj_xen/xen/docs/misc/kconfig.rst: WARNING: document isn't included
in any toctree
/home/user/prj_xen/xen/docs/misc/kconfig-language.rst: WARNING: document isn't
included in any toctree
/home/user/prj_xen/xen/docs/misc/kconfig-macro-language.rst: WARNING: document
isn't included in any toctree
/home/user/prj_xen/xen/docs/misc/xen-makefiles/makefiles.rst: WARNING: document
isn't included in any toctree
done
preparing documents... done
writing output... [100%] misc/xen-makefiles/makefiles
generating indices... genindex
writing additional pages... search
copying images... [100%] admin-guide/xen-overview.drawio.svg
copying static files... done
copying extra files... done
dumping search index in English (code: en) ... done
dumping object inventory... done
build succeeded, 7 warnings.
make: Leaving directory '/home/user/prj_xen/xen/docs’
And checking the generated html page
html/hypercall-interfaces/arm64/grant_tables.html you can see that there is a
union without name or fields right above "union
__guest_handle_gnttab_cache_flush_t".
2) without the anonymous structure:
struct gnttab_cache_flush {
-union {
+union a {
uint64_t dev_bus_addr;
grant_ref_t ref;
} a;
I don’t get the warning from sphinx and looking in the
html/hypercall-interfaces/arm64/grant_tables.html page I can see the proper
documentation for the union a right above "union
__guest_handle_gnttab_cache_flush_t”.
Let me know if you get different results.
Cheers,
Luca
> Cheers,
>
> --
> Julien Grall
Re: [PATCH 3/3] docs/doxygen: doxygen documentation for grant_table.h
Hi Luca, On 07/04/2021 14:19, Luca Fancellu wrote: On 7 Apr 2021, at 14:13, Julien Grall wrote: Hi, On 06/04/2021 11: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. What do you mean by can't deal with them? I had a quick try with doxygen build and couldn't find any failure (although I haven't looked at the output). Hi Julien, The problem is that Doxygen can’t generate proper documentation for anonymous union/structure, it ends up with warning and/or producing wrong documentation like changing names or giving field description to the wrong field. I might do something wrong because I cannot spot any significant difference in the doxygen ouput if I switch back to anonymous union. Would you mind to post more details (such as a diff) on how doxygen doesn't generate properly documentation? Cheers, -- Julien Grall
Re: [PATCH 3/3] docs/doxygen: doxygen documentation for grant_table.h
> On 7 Apr 2021, at 14:13, Julien Grall wrote: > > Hi, > > On 06/04/2021 11: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. > > What do you mean by can't deal with them? I had a quick try with doxygen > build and couldn't find any failure (although I haven't looked at the output). > Hi Julien, The problem is that Doxygen can’t generate proper documentation for anonymous union/structure, it ends up with warning and/or producing wrong documentation like changing names or giving field description to the wrong field. Cheers, Luca > Cheers, > > -- > Julien Grall
Re: [PATCH 3/3] docs/doxygen: doxygen documentation for grant_table.h
Hi, On 06/04/2021 11: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. What do you mean by can't deal with them? I had a quick try with doxygen build and couldn't find any failure (although I haven't looked at the output). Cheers, -- Julien Grall
Re: [PATCH 3/3] docs/doxygen: doxygen documentation for grant_table.h
On 07.04.2021 10:42, Luca Fancellu wrote:
> 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__ ?
While in general I would be fine with this scheme, for field names like
"a" or "u" it doesn't fit well imo. I'm also unconvinced this would be
scalable to the case where there's further struct/union nesting.
Jan
Re: [PATCH 3/3] docs/doxygen: doxygen documentation for grant_table.h
> On 7 Apr 2021, at 09:10, Jan Beulich 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__ ?
Cheers,
Luca
>
> Jan
Re: [PATCH 3/3] docs/doxygen: doxygen documentation for grant_table.h
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
Re: [PATCH 3/3] docs/doxygen: doxygen documentation for grant_table.h
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.
Hi Jan,
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.)
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?
Re: [PATCH 3/3] docs/doxygen: doxygen documentation for grant_table.h
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.
Jan
[PATCH 3/3] docs/doxygen: doxygen documentation for grant_table.h
Modification to include/public/grant_table.h: 1) Change anonymous structure to be named structure, because doxygen can't deal with them. 2) Add doxygen tags to: - Create Grant tables section - include variables in the generated documentation 3) Add .rst file for grant table for Arm64 Signed-off-by: Luca Fancellu --- docs/hypercall-interfaces/arm64.rst | 1 + .../arm64/grant_tables.rst| 8 ++ docs/xen-doxygen/doxy_input.list | 1 + xen/include/public/grant_table.h | 79 --- 4 files changed, 59 insertions(+), 30 deletions(-) create mode 100644 docs/hypercall-interfaces/arm64/grant_tables.rst diff --git a/docs/hypercall-interfaces/arm64.rst b/docs/hypercall-interfaces/arm64.rst index 5e701a2adc..c30a7142b1 100644 --- a/docs/hypercall-interfaces/arm64.rst +++ b/docs/hypercall-interfaces/arm64.rst @@ -8,6 +8,7 @@ Starting points .. toctree:: :maxdepth: 2 + arm64/grant_tables Functions diff --git a/docs/hypercall-interfaces/arm64/grant_tables.rst b/docs/hypercall-interfaces/arm64/grant_tables.rst new file mode 100644 index 00..8955ec5812 --- /dev/null +++ b/docs/hypercall-interfaces/arm64/grant_tables.rst @@ -0,0 +1,8 @@ +.. SPDX-License-Identifier: CC-BY-4.0 + +Grant Tables + + +.. doxygengroup:: grant_table + :project: Xen + :members: diff --git a/docs/xen-doxygen/doxy_input.list b/docs/xen-doxygen/doxy_input.list index e69de29bb2..233d692fa7 100644 --- a/docs/xen-doxygen/doxy_input.list +++ b/docs/xen-doxygen/doxy_input.list @@ -0,0 +1 @@ +xen/include/public/grant_table.h diff --git a/xen/include/public/grant_table.h b/xen/include/public/grant_table.h index 84b1d26b36..b506e09ed3 100644 --- a/xen/include/public/grant_table.h +++ b/xen/include/public/grant_table.h @@ -25,15 +25,19 @@ * Copyright (c) 2004, K A Fraser */ -#ifndef __XEN_PUBLIC_GRANT_TABLE_H__ +/** + * @file + * @brief Interface for granting foreign access to page frames, and receiving + * page-ownership transfers. + */ + +#if !defined(__XEN_PUBLIC_GRANT_TABLE_H__) || defined(DOXYGEN) #define __XEN_PUBLIC_GRANT_TABLE_H__ #include "xen.h" -/* - * `incontents 150 gnttab Grant Tables - * - * Xen's grant tables provide a generic mechanism to memory sharing +/** + * @brief Xen's grant tables provide a generic mechanism to memory sharing * between domains. This shared memory interface underpins the split * device drivers for block and network IO. * @@ -51,13 +55,10 @@ * know the real machine address of a page it is sharing. This makes * it possible to share memory correctly with domains running in * fully virtualised memory. - */ - -/*** + * * GRANT TABLE REPRESENTATION - */ - -/* Some rough guidelines on accessing and updating grant-table entries + * + * Some rough guidelines on accessing and updating grant-table entries * in a concurrency-safe manner. For more information, Linux contains a * reference implementation for guest OSes (drivers/xen/grant_table.c, see * http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=drivers/xen/grant-table.c;hb=HEAD @@ -66,6 +67,7 @@ * compiler barrier will still be required. * * Introducing a valid entry into the grant table: + * @code * 1. Write ent->domid. * 2. Write ent->frame: * GTF_permit_access: Frame to which access is permitted. @@ -73,20 +75,25 @@ * frame, or zero if none. * 3. Write memory barrier (WMB). * 4. Write ent->flags, inc. valid type. + * @endcode * * Invalidating an unused GTF_permit_access entry: + * @code * 1. flags = ent->flags. * 2. Observe that !(flags & (GTF_reading|GTF_writing)). * 3. Check result of SMP-safe CMPXCHG(&ent->flags, flags, 0). * NB. No need for WMB as reuse of entry is control-dependent on success of * step 3, and all architectures guarantee ordering of ctrl-dep writes. + * @endcode * * Invalidating an in-use GTF_permit_access entry: + * * This cannot be done directly. Request assistance from the domain controller * which can set a timeout on the use of a grant entry and take necessary * action. (NB. This is not yet implemented!). * * Invalidating an unused GTF_accept_transfer entry: + * @code * 1. flags = ent->flags. * 2. Observe that !(flags & GTF_transfer_committed). [*] * 3. Check result of SMP-safe CMPXCHG(&ent->flags, flags, 0). @@ -97,47 +104,55 @@ * transferred frame is written. It is safe for the guest to spin waiting * for this to occur (detect by observing GTF_transfer_completed in * ent->flags). + * @endcode * * Invalidating a committed GTF_accept_transfer entry: * 1. Wait for (ent->flags & GTF_transfer_completed). * * Changing a GTF_permit_access from writable to read-only: + * * Use SMP-safe CMPXCHG to set GTF_readonly, while checking !GTF_writing. * * Changing a GTF_permit_access from read-only to writable: + *
