Re: [PATCH] Refactor Wmi.c

["Durrant, Paul" <xadimgnik@xxxxxxxxx>]

On 31/03/2022 08:12, Owen Smith wrote:
> -----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
Ok. I'll take it as-is.

Acked-by: Paul Durrant <paul@xxxxxxx>

> Owen