[vdr] Re: lock handling in sections.c

[Klaus Schmidinger <Klaus.Schmidinger@cadsoft.de>]

Jouni Karvo wrote:
> Klaus Schmidinger writes:
>  > > +         int round=0; 
>  > > +         bool DeviceHasLock;
>  > > +         while (((DeviceHasLock = device->HasLock())==false) && 
>  > > +                (round++ < 20)) {
>  > > +           cCondWait::SleepMs(100);
>  > > +         }
>  > >             for (int i = 0; i < NumFilters; i++) {
>  > >                 if (pfd[i].revents & POLLIN) {
>  > >                    cFilterHandle *fh = NULL;
>  > 
>  > I wonder why this would change anything.
>
> Not necessarily by any reason.  I am not certain.  More "reasoning"
> follows... 
>  > Besides, after the last cCondWait::SleepMs(100) your code still doesn't
>  > re-check device->HasLock().
>
> I do think (DeviceHasLock = device->HasLock()) is executed every time
> the condition is evaluated (i.e. before the last check whether we
> bother to wait once more).  I do not have my Stroustrup now at hand so
> I cannot check if it is allowed to depend on the order in which the
> conditions are written, though.
Oh, sorry, you're of course right here.

> There is, of course, still the possibility that the lock status
> changes between this loop and the actual !DeviceHasLock -> continue
> check, but at least now there is no 0.1s time window in between.
>
>  > Are you really sure that this change actually helped?
>
> Not 100%, but I do think I did not (this time) mess up with other
> changes simultaneously.
>
> I do not understand the flow of actions here, but the thing that came
> in to my mind is that perhaps if here the lock is checked (and found
> false) and the remainings of the function are left undone, and
> meanwhile the lock has been achieved, then later there would be some
> confusion because there is a lock but the settings in the end of this
> function have not been executed.  Fixing this (not necessarily with the
> while() I wrote) at least shortens this time window.
>
> If the code is left this way, I think the whole sleep is unnecessary
> since it does not change anything in this function (a no-op) anyway.
> If there is a need of such a wait without a re-check, then it perhaps
> should be located somewhere else in the code so that its meaning is
> more clearly seen from the code.  And, in that case, you can move the
> check to the LOCK_THREADed part of the function.
>
> Come to think about that, you could even implement it without the
> DeviceHasLock variable:
>
> before LOCK_THREAD:
>
> if (!device->HasLock()) cCondWait::SleepMs(100);
>
> (so that it tries to wait once for device lock before going to
> protected area (I guess LOCK_THREAD is supposed to provide
> protection)) 
> And then, in the actual test phase (to make sure the info on lock is
> as fresh as possible):
>
>  if (!device->HasLock())
>                         continue; // we do the read anyway, to flush
>                         any data that might have come from a different
>                         transponder
>
> (this I have not tested, though)
>
> yours,
> 		Jouni
I must admit that I'm having a hard time understanding why it would
be so important "to make sure the info on lock is as fresh as possible"?

The section data is repeated over and over again. It  doesn't really
matter whether it is received some 100ms earlier or later.

I believe it is important to check and remember the DeviceHasLock condition
_before_ actually reading the data. Otherwise we might read data that came
in while the device didn't have a lock yet (i.e. garbage), and after reading
the data, but before processing it, the device has obtained a lock. Then
we would process data that in fact is garbage.

Klaus