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...

[Martin Harvey <martin.harvey@xxxxxxxxxx>]

-----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".

> > +    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"?

MH.