[bug] Shutdown during an active timer is broken
Commit Message
Hi list,
Based on a first patch by Helmut Auer, here's a patch to solve the
shutdown on running timers issue. I did some simple tests and it seemed
to work, however the patch is still experimental.
The patch basically disables the offending timers up to MinEventPause
minutes in the future. There's also some endless loop detection that
imho should be impossible to trigger.
Some issues:
- If you decide to cancel the shutdown after confirming it first, timers
will remain disabled. Thats probably not what the user expects.
- Disabling pending timers up to MinEventPause may not what the user
expects either. I have set MinEventPause to 11 minutes (5 for shutdown,
5 for wakeup, 1 to avoid race conditions). But if an user has set this
to 120 minutes for example, then the question "Recording in 100 minutes,
shut down anyway?" doesn't sound like VDR wont wake up for it.
- A clean implementation should merge both loops into one function and
move the code into cTimers (API change!). (the two loops are required to
skip negative values for "Recording in xx minutes")
Cheers,
Udo
Comments
Udo Richter wrote:
> Hi list,
>
> Based on a first patch by Helmut Auer, here's a patch to solve the
> shutdown on running timers issue. I did some simple tests and it seemed
> to work, however the patch is still experimental.
>
> The patch basically disables the offending timers up to MinEventPause
> minutes in the future. There's also some endless loop detection that
> imho should be impossible to trigger.
>
> Some issues:
> - If you decide to cancel the shutdown after confirming it first, timers
> will remain disabled. Thats probably not what the user expects.
>
> - Disabling pending timers up to MinEventPause may not what the user
> expects either. I have set MinEventPause to 11 minutes (5 for shutdown,
> 5 for wakeup, 1 to avoid race conditions). But if an user has set this
> to 120 minutes for example, then the question "Recording in 100 minutes,
> shut down anyway?" doesn't sound like VDR wont wake up for it.
>
> - A clean implementation should merge both loops into one function and
> move the code into cTimers (API change!). (the two loops are required to
> skip negative values for "Recording in xx minutes")
>
> Cheers,
>
> Udo
>
>
> ------------------------------------------------------------------------
>
> --- vdr-1.4.1-orig/vdr.c 2006-07-25 15:48:14.000000000 +0200
> +++ vdr-1.4.1/vdr.c 2006-07-25 16:39:38.943900248 +0200
> @@ -1007,6 +1007,27 @@
> if (cRecordControls::Active()) {
> if (!Interface->Confirm(tr("Recording - shut down anyway?")))
> break;
> + // Stop all running timers
> + time_t Now = time(NULL);
> + cTimer *timer = Timers.GetNextActiveTimer();
> + time_t Next = timer ? timer->StartTime() : 0;
> + while (timer && Next - Now < 0) {
> + if (timer->IsSingleEvent())
> + timer->ClrFlags(tfActive);
> + else
> + timer->Skip();
> + timer->Matches();
> +
> + cTimer *nextTimer = Timers.GetNextActiveTimer();
> + time_t nextNext = nextTimer ? nextTimer->StartTime() : 0;
> + if (nextNext < Next || (nextNext == Next && nextTimer == timer)) {
> + esyslog("Loop detected while disabling running timers");
> + break;
> + }
> + Next=nextNext;
> + timer=nextTimer;
> + }
> + Timers.SetModified();
> }
> if (cPluginManager::Active(tr("shut down anyway?")))
> break;
> @@ -1020,6 +1041,27 @@
> free(buf);
> if (!confirm)
> break;
> + // Stop all pending timers until MinEventTimeout
> + time_t Now = time(NULL);
> + cTimer *timer = Timers.GetNextActiveTimer();
> + time_t Next = timer ? timer->StartTime() : 0;
> + while (timer && Next - Now <= Setup.MinEventTimeout * 60) {
> + if (timer->IsSingleEvent())
> + timer->ClrFlags(tfActive);
> + else
> + timer->Skip();
> + timer->Matches();
> +
> + cTimer *nextTimer = Timers.GetNextActiveTimer();
> + time_t nextNext = nextTimer ? nextTimer->StartTime() : 0;
> + if (nextNext < Next || (nextNext == Next && nextTimer == timer)) {
> + esyslog("Loop detected while disabling pending timers");
> + break;
> + }
> + Next=nextNext;
> + timer=nextTimer;
> + }
> + Timers.SetModified();
> }
> ForceShutdown = true;
> break;
This is getting *way* too complex for my taste.
How about this: if the user presses the "Power" key to turn off VDR,
and VDR tells him/her that there is a recording going on, or a timer
will hit in a few minutes, and the user still insists in shutting
down VDR *now*, what could be the reason for this? IMHO the only real
reason is that the user wants to do some maintenance, for which the
device must be turned off (like, for instance, install some hardware
or relocate it to a different place). At any rate, chances are the user
will turn it on again anyway after the action that was so important
that it was worth interrupting an ongoing recording is finished.
As a safety precaution, in case the user turned off VDR and ignored
all the warnings, let's just tell the shutdown script to reboot it
at, say, five minutes in the future (in case a recording is going on)
or whenever the next timer hits after that time.
If the user doesn't want this to happen, then he/she can simply go
into the Timers menu and turn timers off as desired. I don't think
VDR should make that decision behind the user's back.
Klaus
Klaus Schmidinger wrote:
> As a safety precaution, in case the user turned off VDR and ignored
> all the warnings, let's just tell the shutdown script to reboot it
> at, say, five minutes in the future (in case a recording is going on)
> or whenever the next timer hits after that time.
I'm not sure whether I like the idea that VDR starts again after 5
minutes, probably just in that moment when I am plugging a PCI card or
something like that. (Yes, you're *supposed* to pull the plug first. You
really do? ;) )
Anyway, 5 mins is too short, anything less that 10 mins will be denied
by nvram-wakeup default configuration.
> If the user doesn't want this to happen, then he/she can simply go
> into the Timers menu and turn timers off as desired.
I don't have a problem with disabling the offending timers manually. I
just wrote the patch because of the request.
Its just a bit strange that VDR requires extra confirmation, and then
nvram-wakeup denies shutdown anyway. (or other shoudown scrips doing
even more strange things)
On a side note: With some scripting-fu, it should be possible to disable
the offending timers from the shutdown script via SVDRP, and then
request the new reboot time also via SVDRP.
Cheers,
Udo
On Sat, 29 Jul 2006, Udo Richter (UR) wrote:
UR>
UR> Its just a bit strange that VDR requires extra confirmation, and then
UR> nvram-wakeup denies shutdown anyway. (or other shoudown scrips doing even
UR> more strange things)
as far as I remember, one of the parameters passed to the shutdown script
indicates if the shutdown is requested by the user.
in this case you could just call nvram-wakeup -d and shutdown. keeping in
mind, that the machine will not wakeup again at all.
Sergei
@@ -1007,6 +1007,27 @@
if (cRecordControls::Active()) {
if (!Interface->Confirm(tr("Recording - shut down anyway?")))
break;
+ // Stop all running timers
+ time_t Now = time(NULL);
+ cTimer *timer = Timers.GetNextActiveTimer();
+ time_t Next = timer ? timer->StartTime() : 0;
+ while (timer && Next - Now < 0) {
+ if (timer->IsSingleEvent())
+ timer->ClrFlags(tfActive);
+ else
+ timer->Skip();
+ timer->Matches();
+
+ cTimer *nextTimer = Timers.GetNextActiveTimer();
+ time_t nextNext = nextTimer ? nextTimer->StartTime() : 0;
+ if (nextNext < Next || (nextNext == Next && nextTimer == timer)) {
+ esyslog("Loop detected while disabling running timers");
+ break;
+ }
+ Next=nextNext;
+ timer=nextTimer;
+ }
+ Timers.SetModified();
}
if (cPluginManager::Active(tr("shut down anyway?")))
break;
@@ -1020,6 +1041,27 @@
free(buf);
if (!confirm)
break;
+ // Stop all pending timers until MinEventTimeout
+ time_t Now = time(NULL);
+ cTimer *timer = Timers.GetNextActiveTimer();
+ time_t Next = timer ? timer->StartTime() : 0;
+ while (timer && Next - Now <= Setup.MinEventTimeout * 60) {
+ if (timer->IsSingleEvent())
+ timer->ClrFlags(tfActive);
+ else
+ timer->Skip();
+ timer->Matches();
+
+ cTimer *nextTimer = Timers.GetNextActiveTimer();
+ time_t nextNext = nextTimer ? nextTimer->StartTime() : 0;
+ if (nextNext < Next || (nextNext == Next && nextTimer == timer)) {
+ esyslog("Loop detected while disabling pending timers");
+ break;
+ }
+ Next=nextNext;
+ timer=nextTimer;
+ }
+ Timers.SetModified();
}
ForceShutdown = true;
break;