[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Luca Fancellu <luca.fancellu@xxxxxxx>
  • Date: Wed, 7 Apr 2021 09:42:23 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=btZT0UAffeikLWFNW3XKx6Hyd5QUnNhsO2TFqindxKk=; b=H/B7ZfdhyNFLo0pIC+CD7xjqsZD9F5XHoVtM8uwlyoDhJ/1FedFsxQQ0SZm4pJUEWHXpB9yAj5JnZlEhMS1A+KWa/ihkLs+cSzlYBBUDf4el94uElabvKoGCXdpsCiO1uQvhfBXqee2VHE8Yb4lhYHGGuGZesIPXcxpJ1t6f/WTiSFhm7I40Uph21fkug62AcSVDE2eVddUGXsS6XbtJe0BLNkjaqCK+lUFhzFThIkVavWDSmynEQ4ddY72jgG9O74wzE4KAy2wO/J0CUwmp0dX31/Yx0C93vYMND3Yat+soH3L7bDdVO0FVvxUTud85KPANJ80v95QGypP83tW4ig==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=GIu4+qlBt6KDKu9+xHorjNAt/hKtmyHeKI+5ReHuodWuv7Lcm394CfZIi9ThxCxqNLPBvkFcsIja+yQbxWVCCPppCaGsYAdVqVCglcU42PCUzJ5jz8EZr9NeYojhqEwVE7VJRvSSzNxAAutg4iuePK1aNvC2j9j2wU9+2BhdOZZXhu7FdvkyMbBlnccLBHmfcQfyhEZR0hOYhG5HLX7qNLhP9JzR3cvw/v8PHw/iediUChsxO0k+gJ5OMXI/fwfdNAilf5/LBp8ariGV4l0h9tnUn5XIsgJ30oMQDHdd4OvlaIlQ/iA/PSDbPRZZpSph2Z2EW2mCfL4/xiXVu23hKw==
  • Authentication-results-original: suse.com; dkim=none (message not signed) header.d=none;suse.com; dmarc=none action=none header.from=arm.com;
  • Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>, Bertrand Marquis <bertrand.marquis@xxxxxxx>, wei.chen@xxxxxxx, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Wei Liu <wl@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Wed, 07 Apr 2021 08:43:06 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Nodisclaimer: true
  • Original-authentication-results: suse.com; dkim=none (message not signed) header.d=none;suse.com; dmarc=none action=none header.from=arm.com;


> 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




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.