[vdr] Re: lock handling in sections.c

[Rainer Zocholl <UseNet-Posting-Nospam-74308-@zocki.toppoint.de>]

(Jouni Karvo)  07.01.05 20:32


>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 too found the original code "suspicious", 
but had no time to analyze it further.

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

As long as "(round++ < 20)" is "TRUE" of course
"(DeviceHasLock = device->HasLock())==false)"
will be executed to.

You should never "assume" any timing order a programs runs just
because you wrote it this way in C...
Modern compilers may change the execution order...
That's where "volatile" can (somethimes) help, but AFAIK, gcc 
does not really support it(it's simply ignored IIRC).
It's not worth the afford to "hand optimize" C-code (except by
using an entire better algorithm).
If "hand optimized C-code" is used, that code must be
fixed to that compiler and must be documented in the code...


IMHO it's not good to simply discard the "round" value
and maybe work with invalid data...
Even it's not likely ever to occur or it's not clear how to
handle the condition gracefully, the least thing
to be done is: Make a logfile entry (not just a "todo" comment).
But not simply ignore the (unexpected, forbidden) timeout condition.
And: "likeliness" is no good paradigm for stable programms.
(See Murphy's law: "If it can happen, it will happen".)


If below a ("unstructed") "continue" is used, it would 
be clearer if a "continue" is used here too, IMHO.

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

ACK. That's still a problem that should be solved.



Rainer