RE: [PATCH 3/6] In addition to preceding changes to ring disconnects, and associated logging, we also add some logging to check whether state change notifications are being sent in a timely manner between frontend and backend. Also a great a...

["Durrant, Paul" <pdurrant@xxxxxxxxxxxx>]

> -----Original Message-----
> From: win-pv-devel <win-pv-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of 
> Martin Harvey
> Sent: 22 July 2021 13:46
> To: paul@xxxxxxx; win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: RE: [EXTERNAL] [PATCH 3/6] In addition to preceding changes to ring 
> disconnects, and
> associated logging, we also add some logging to check whether state change 
> notifications are being
> sent in a timely manner between frontend and backend. Also a great a...
> 
> 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.
> 
> 
> 
> -----Original Message-----
> From: win-pv-devel <win-pv-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of 
> Paul Durrant
> Sent: 21 July 2021 18:18
> To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH 3/6] In addition to preceding changes to ring 
> disconnects, and associated logging,
> we also add some logging to check whether state change notifications are 
> being sent in a timely manner
> between frontend and backend. Also a great assistance t...
> 
> > On 20/07/2021 14:29, Martin Harvey wrote:
> > > Signed-off-by: Martin Harvey <martin.harvey@xxxxxxxxxx>
> > > +    const ULONGLONG             TimeoutDelta = 120000;
> >
> > NIT: I don't think you really need 'Delta' in the name, just 'Timeout'
> > would suffice (and I think it would be more consistent with other code).
> 
> Yes, but unfortunately that conflicts with the timeout used in the KeWaitFor 
> ...
> 
> Perhaps more logical and less confusing would be "TotalTimeout".

Yes, that looks better.

> 
> > > +    if (TimeDelta >= TimeoutDelta) {
> > > +        Error("%s timed out.\n", __FrontendGetBackendPath(Frontend));
> > > +    }
> 
> > No need for braces on single-line ifs and no full stop needed in the log 
> > message. Since it's not in
> context and I can't remember... does > the path include the actual 'state' 
> key name? It's a lack of
> change in that particular key we're triggering on here.
> 
> Ah. It's pulled out of xenbus store, so I think not. Checking he preceding 
> code, the store watch is
> added to "state", which is a level below the backend path, which also seems 
> to indicate not.
> 
> Could add something extra in the dbg message about "state change timed out"?
> 

Yes, that's clearer.

  Paul

> MH.