RE: [PATCH 1/3] Prevent a xenagent timeout on S3/S4 transition.

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Owen Smith <owen.smith@xxxxxxxxxx>
> Sent: 03 September 2020 16:07
> To: Troy Crosley <troycrosley@xxxxxxxxx>;
win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: paul@xxxxxxx; Ben Chalmers <ben.chalmers@xxxxxxxxxx>
> Subject: RE: [PATCH 1/3] Prevent a xenagent timeout on S3/S4 transition.
> 
> 
> 
> > -----Original Message-----
> > From: Troy Crosley <troycrosley@xxxxxxxxx>
> > Sent: 01 September 2020 18:28
> > To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> > Cc: paul@xxxxxxx; Ben Chalmers <ben.chalmers@xxxxxxxxxx>; Owen Smith
> > <owen.smith@xxxxxxxxxx>; Troy Crosley <troycrosley@xxxxxxxxx>
> > Subject: [PATCH 1/3] Prevent a xenagent timeout on S3/S4 transition.
> >
> > [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open
attachments
> > unless you have verified the sender and know the content is safe.
> >
> > Prevent a xenagent timeout (and live kernel dump) on S3/S4 transition
by
> > changing CXenIfaceCreator::Log to use TryEnterCriticalSection.
> > Otherwise, a timeout occurs when the service control handler fails to
return due
> > to attempting to enter a critical section object that the main service
thread
> > already owns while responding to the control/shutdown xenstore watch.
> >
> > Signed-off-by: Troy Crosley <troycrosley@xxxxxxxxx>
> > ---
> >  src/xenagent/service.cpp | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/xenagent/service.cpp b/src/xenagent/service.cpp index
> > 4d62e09..e595093 100644
> > --- a/src/xenagent/service.cpp
> > +++ b/src/xenagent/service.cpp
> > @@ -107,9 +107,9 @@ void CXenIfaceCreator::OnPowerEvent(DWORD evt,
> > LPVOID data)  void CXenIfaceCreator::Log(const char* message)  {
> >      // if possible, send to xeniface to forward to logs
> > -    CCritSec crit(&m_crit);
> > -    if (m_device) {
> > +    if (TryEnterCriticalSection(&m_crit) && m_device) {
> 
> If the TryEnterCriticalSection succeeds, but m_device is null, doesn't
this leave the critical section
> held?

It does appear that way. I think it is sufficient to re-order the if
clause such that m_device is evaluated first. I'll fix on commit.

  Paul


> 
> >          m_device->Log(message);
> > +        LeaveCriticalSection(&m_crit);
> >      }
> >  }
> >
> > --
> > 2.20.1
> >