RE: [PATCH] Refactor Wmi.c

[Owen Smith <owen.smith@xxxxxxxxxx>]

-----Original Message-----
From: win-pv-devel <win-pv-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of 
Durrant, Paul
Sent: Wednesday, March 30, 2022 11:11 AM
To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
Subject: Re: [PATCH] Refactor Wmi.c

[CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments 
unless you have verified the sender and know the content is safe.

On 30/03/2022 11:07, Owen Smith wrote:
> -----Original Message-----
> From: Durrant, Paul <pdurrant@xxxxxxxxxxxx>
> Sent: 30 March 2022 10:40
> To: Owen Smith <owen.smith@xxxxxxxxxx>; 
> win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: RE: [PATCH] Refactor Wmi.c
> 
> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments 
> unless you have verified the sender and know the content is safe.
> 
>> -----Original Message-----
>> From: win-pv-devel <win-pv-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On 
>> Behalf Of Owen Smith
>> Sent: 30 March 2022 10:32
>> To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
>> Cc: Owen Smith <owen.smith@xxxxxxxxxx>
>> Subject: [EXTERNAL] [PATCH] Refactor Wmi.c
>>
>> CAUTION: This email originated from outside of the organization. Do 
>> not click links or open attachments unless you can confirm the sender and 
>> know the content is safe.
>>
>>
>>
>> * Moves functions to be in related locations in file
>> * Formats code to appropriate code style
>> * Inlines some functions that are only called from 1 location
>> * Uses LIST_ENTRY macros to access linked lists
>>
>> Signed-off-by: Owen Smith <owen.smith@xxxxxxxxxx>
>> ---
>>   src/xeniface/wmi.c | 3794
>> ++++++++++++++++++++++----------------------
>>   1 file changed, 1907 insertions(+), 1887 deletions(-)
> 
> Eek. 3794 lines of change! That's almost impossible to review. Is there any 
> chance you can break this down?
> 
>    Paul
> 
> 
> Its not easy to break down a refactor like this, 1 small change has many 
> knock on changes throughout the file.
> Its probably easier to apply locally and compare side by side, but I'm 
> not expecting this to be an easy or quick review - I probably should 
> have posted 1st with RFC
> 

Can you assert that there is no functional change? I will consider taking it on 
trust if so.

   Paul


I've not made any functional changes, except for changes to how the LIST_ENTRY 
members are accessed - using the
standard LIST_ENTRY macros/functions rather than casting (this should not 
affect functionality, as the LIST_ENTRY
items are the 1st entry in their respective structures).

I have run these changes, as part of the Citrix driver package (we don’t patch 
this file, but do rename/re-GUID the WMI
objects), through automated testing which validates the WMI interfaces.

This patch was originally to clean up the code with the intent to track down a 
0x139 (KERNEL_SECURITY_CHECK_FAILURE)
bugcheck, that indicated there was occasional LIST_ENTRY corruption. After 
these changes, I have not yet reproduced
the bugcheck

Owen