Received-SPF: pass (google.com: domain of linux-kernel+bounces-140381-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <316aba6b-2384-4867-b634-89f9c07f03ca@intel.com>
Date: Thu, 11 Apr 2024 13:48:52 +0200
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] kernel-doc: fix struct_group_tagged() parsing
Content-Language: en-US
To: Alexander Lobakin <aleksander.lobakin@intel.com>
CC: Jonathan Corbet <corbet@lwn.net>, Kees Cook <keescook@chromium.org>, Jakub
 Kicinski <kuba@kernel.org>, Dan Williams <dan.j.williams@intel.com>, "Gustavo
 A. R. Silva" <gustavoars@kernel.org>, Keith Packard <keithp@keithp.com>,
	<nex.sw.ncis.osdt.itp.upstreaming@intel.com>, <linux-doc@vger.kernel.org>,
	<netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <20240411093208.2483580-1-aleksander.lobakin@intel.com>
 <b9804c32-9ba2-493b-bfb8-a6ac4f108175@intel.com>
 <80d765f5-1436-4799-95fe-ba05c0681d5c@intel.com>
From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
In-Reply-To: <80d765f5-1436-4799-95fe-ba05c0681d5c@intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UktuL3NmVHRwa2dXL09xOGFKRndLTUx4UGpsbUp6UXMzaGV5d3FidFh4ckp6?=
 =?utf-8?B?M0o5N015aDg1VU1hTCtSVGhLYlRsYWt4WTd6K0JUZGs0eVkrQ3hhVGxOWVJr?=
 =?utf-8?B?WnZEOGtJQklBdXlPaG8xZlpXK0phekI1RHpRQjlLTFdncVJYaGhubFB4eXVn?=
 =?utf-8?B?czNqd3RMRVhYTjUvaGg3TmhqVEdQdklxZE9yb2EyTVBwejlPaXpnTUFnbkZo?=
 =?utf-8?B?L2NEbDhBSjVFNXVyeFhXZGRvczlhV1hQek1kREV2M0tSODlUSFpSWk5tQ09i?=
 =?utf-8?B?YkxSTU1BQkdUSSs5WWpwQlpnWnVsQklTcGhCaCtnT1gwd3RrcHh0Yk1BWUdz?=
 =?utf-8?B?NWVVRjhIeXcwdHNidmVTZHhwWHI2YTU5MWVjVmZGWlNnSmxVNm9IYWMwbW1N?=
 =?utf-8?B?QWhwTHVjdVRLL3dlbTNkMXZqTzlHN2I0Tk5XeEh5Q2IyNGFDWU1kZ0dzSkFL?=
 =?utf-8?B?Yi9MYjMxbCtvc0JtbFlTTmZ4QlNZMHRYTUEvTTlrYURnZHY4a2R4c09lRE9D?=
 =?utf-8?B?WVppS0VmT1pPYXZLUnVuQ3gyNm9nTEdiNWlxK3dNRXJ4UGhnRzd1d1l1RC9S?=
 =?utf-8?B?aEw3NUw2LzVRb2E2U1BiRThoRkNKMmltY25YQjllSGR1K0hpYk91cXRTWEdh?=
 =?utf-8?B?STVua1hoaWtDM21kbUxlM2pvVDdPL3pjOFExMGtIZ3FNZVJpVGRaeU5uM1JN?=
 =?utf-8?B?OXVSNmR4NjRtTXgxZ2psWXl1SjY3RlpqcWhCQ0swMm5CSmt0aUN0d1ZWWWkw?=
 =?utf-8?B?MlVzOG1YQU9GcGhreDRROXV4cDM1bERBU3Q4SkdMTDF2N003R0Izd0xNb1hM?=
 =?utf-8?B?WE1EdDdHTDhxL09yQUMrbit0ZldrV0pNU2JhL1JKVGpuR1dxbjR0RzVYZmNC?=
 =?utf-8?B?NzhXQ1RUa3NRWWtsQVBwQWg4aklMSzdhaSt5VU5HTFFOTk51Qm1hNlBRZ3ds?=
 =?utf-8?B?NE93ckFoWVVKcnV2ZXpKRGFJN1pEZ1RQejJwVHRYNjM4ai9ZSzVYdkhJNkk4?=
 =?utf-8?B?bHdnVjByZjNJZ000b1NMK3dWWDRqTU9Dam5JT1l3andjTjYyOFpHZG1mT3pZ?=
 =?utf-8?B?VERCQWlQRG4rV1VDbC9uK2trNEs0eWYyZ0JHdFNWdTBYUnl2SzJMbjE5NFJD?=
 =?utf-8?B?cjAwdVNmVzJ6aGo3cExuWW1iejdUbmNwZHVpQWdoYXF4d3ZoeTFmZmRyS1po?=
 =?utf-8?B?Ry81bjI4c3BRN2gvbVEzaC95UWVHc0tOV0tvQ1NkNU0zR3Z4WC9La014bzNL?=
 =?utf-8?B?aUZZM2V6OTMvSnppdjZzK0VtaHRwQTAvV2I2ZDQvYTd5aG1ITmY0UGhaeS92?=
 =?utf-8?B?RFdmTXhLTWVBN1k0NVdXSVBNMVNhR3ljQTBxaWtydm9ldk5ESW9QRko0dENy?=
 =?utf-8?B?cXJpdDNEMXhtR2wyNDhNdGhTTXEycThSb3hjTkNxUkdPT0hMZTZjTG1zTWRk?=
 =?utf-8?B?KzBieGtHdDlxcC8zc2lpdVc2dzYwWUZVcVNvM1F0TEp0dkpMQ0szMThNNFVC?=
 =?utf-8?B?VFZqeURmUWRxbmpvN3QvdzlySEFReGxTMDU4UjJKZ3huVGZ6L3FUdFA3U0Qv?=
 =?utf-8?B?VWo3bGxtbTZCUGFRUFR6aTkrT1BoOWVIbFQxbFBsckt5a1dUYXE2ZWxUWUxD?=
 =?utf-8?B?azg2WE5CTkFyd1h5MWxNSWU5aTRTYktxcVI1QUIxMkMwRU5KQTdJVW0wY1g2?=
 =?utf-8?B?LzVSZ3BlSXNNTGUwNDBVUFUvMEVHL2RacDB6N08ycSsxWjVWYWNIb3Nra2NY?=
 =?utf-8?B?VjJsZ214TEppaTJQWi9TQitWV3kzRGRoSmI4OWdpUDllVEp5NHVSL01zRGlB?=
 =?utf-8?B?cXpRUWI2Rk5sUVNDWmpOWWhNOEZLRmpsRzVaNGF1WXlIaVVZczRrY1ZXMDZs?=
 =?utf-8?B?VFMvSjhEZDhubWRMS0RIRHBQVkxmVjNyQTdvQmdXMm5mcHFqLzBqTnF1eVEv?=
 =?utf-8?B?WFVpbXdUSXZCVGdYSjVyQnRFYWhRT1B0aFdMSlpjMEczU2ZqTEZFZTI2eExL?=
 =?utf-8?B?ZTRNZXBoeUs3c01LQjVERVM1dVhvMVI4REdtdkJPZnBUS3FGMW1mV1MxRm0r?=
 =?utf-8?B?WERlK2QxZks3VGF5SFo1czRrV2F2QWJteWVyT2RoekNPUTJlRHl5WTlBOUZD?=
 =?utf-8?B?bGdDQWdlYVVLM21Edkl3OWV3cGhvaG94WHUwQnNPeXo4VWdrU29vaXdoeGNI?=
 =?utf-8?Q?k7i9YOvZDaKfvdD6XsEGd48=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 993930b7-203f-407d-79d9-08dc5a1d5f06
X-MS-Exchange-CrossTenant-AuthSource: MN6PR11MB8102.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Apr 2024 11:48:57.6122
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: icqVYj6bJgTbEdc80+2i/tLLrJBedsq1lFo4ozO9FNYA4EzLYykO8rTEXER/eBSXCdsmccrp5yK9v8xEXxIuTjqT8aDq2Omi4h97fd8gf9g=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR11MB7841
X-OriginatorOrg: intel.com

On 4/11/24 13:14, Alexander Lobakin wrote:
> From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Date: Thu, 11 Apr 2024 13:14:20 +0200
> 
>> On 4/11/24 11:32, Alexander Lobakin wrote:
>>> From: Kees Cook <keescook@chromium.org>
>>>
>>> kernel-doc emits a warning on struct_group_tagged() if you describe your
>>> struct group member:
>>>
>>> include/net/libeth/rx.h:69: warning: Excess struct member 'fp'
>>> description in 'libeth_fq'
>>>
>>> The code:
>>>
>>> /**
>>>    * struct libeth_fq - structure representing a buffer queue
>>>    * @fp: hotpath part of the structure
>>>    * @pp: &page_pool for buffer management
>>> [...]
>>>    */
>>> struct libeth_fq {
>>>      struct_group_tagged(libeth_fq_fp, fp,
>>>          struct page_pool    *pp;
>>> [...]
>>>      );
>>>
>>> When a struct_group_tagged() is encountered, we need to build a
>>> `struct TAG NAME;` from it, so that it will be treated as a valid
>>> embedded struct.
>>> Decouple the regex and do the replacement there. As far as I can see,
>>> this doesn't produce any new warnings on the current mainline tree.
>>>
>>> Reported-by: Jakub Kicinski <kuba@kernel.org>
>>> Closes: https://lore.kernel.org/netdev/20240405212513.0d189968@kernel.org
>>> Fixes: 50d7bd38c3aa ("stddef: Introduce struct_group() helper macro")
>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>> Co-developed-by: Alexander Lobakin <aleksander.lobakin@intel.com>
>>> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
>>> ---
>>>    scripts/kernel-doc | 3 ++-
>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/scripts/kernel-doc b/scripts/kernel-doc
>>> index 43a30f2de513..01ac8f794b30 100755
>>> --- a/scripts/kernel-doc
>>> +++ b/scripts/kernel-doc
>>> @@ -1152,7 +1152,8 @@ sub dump_struct($$) {
>>>            # - first eat non-declaration parameters and rewrite for
>>> final match
>>>            # - then remove macro, outer parens, and trailing semicolon
>>>            $members =~ s/\bstruct_group\s*\(([^,]*,)/STRUCT_GROUP(/gos;
>>> -        $members =~
>>> s/\bstruct_group_(attr|tagged)\s*\(([^,]*,){2}/STRUCT_GROUP(/gos;
>>> +        $members =~
>>> s/\bstruct_group_attr\s*\(([^,]*,){2}/STRUCT_GROUP(/gos;
>>> +        $members =~
>>> s/\bstruct_group_tagged\s*\(([^,]*),([^,]*),/struct $1 $2;
>>> STRUCT_GROUP(/gos;
>>>            $members =~
>>> s/\b__struct_group\s*\(([^,]*,){3}/STRUCT_GROUP(/gos;
>>>            $members =~
>>> s/\bSTRUCT_GROUP(\(((?:(?>[^)(]+)|(?1))*)\))[^;]*;/$2/gos;
>>>    
>>
>> I would complain on code that matches `[^,]*` part with 0 characters,
>> meaning no tag for struct_group_tagged(), or no attrs for
>> struct_group_attrs(). In such cases simpler struct_group() call should
>> be suggested. However, that issue was presented prior to your patch.
> 
> Rather a subject for checkpatch, not kernel-doc?

Good point.

But that reminds me that getting patches accepted into checkpatch is not
possible for mere mortals :/ (exaggerating here, but just a little)

> 
>>
>> This is clearly an improvement, so:
>> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> 
> Thanks!
> Olek