diff mbox

vdr shutdown handling / streamdev plugin

Message ID 457155BC.6000303@cadsoft.de
State New
Headers

Commit Message

Klaus Schmidinger Dec. 2, 2006, 10:30 a.m. UTC 
  Udo Richter wrote:
> Klaus Schmidinger wrote:
>> Are you sure?
>> ACTIVITYTIMEOUT is 60 seconds, so wouldn't your change just
>> avoid the calls until 60 seconds after the last user activity?
> 
> Hmm, you're right. Its not a test on Setup.MinUserInactivity * 60. My 
> patch would just avoid these calls the usual 60 seconds after a key press.
> 
> However, I still think that Active() should be called only if VDR is 
> really willing to shut down, and not while running normally. Or is 
> Active() supposed to also delay any housekeeping tasks?

Since cPlugin::Active() is only intended to be a means of preventing
VDR from shutting down, I'd say you're right - it should only be called
when VDR is actually trying to shut down.

The attached patch makes it call cPluginManager::Active() only
if it really wants to shut down, and if there is actually an option
for plugins to delay the shutdown (i.e. this is not a forced shutdown).

If cPluginManager::Active() returns 'true' once, it waits for SHUTDOWNRETRY
minutes before trying again.

Please give this a try and let me know whether it works.

Klaus

Comments

Klaus Schmidinger Dec. 2, 2006, 11:27 a.m. UTC | #1 
Klaus Schmidinger wrote:
> ...
> --- vdr.c	2006/10/14 10:01:32	1.280
> +++ vdr.c	2006/12/02 10:20:30
> @@ -1154,11 +1154,15 @@
> ...
> +                    LastActivity = time(NULL) - Setup.MinUserInactivity * 60 + SHUTDOWNRETRY; // try again later

Just in case somebody noticed: I just realized that 'Now' can be used
instead of 'time(NULL)' here.

Klaus
Udo Richter Dec. 2, 2006, 3:09 p.m. UTC | #2 
Klaus Schmidinger wrote:
> The attached patch makes it call cPluginManager::Active() only
> if it really wants to shut down, and if there is actually an option
> for plugins to delay the shutdown (i.e. this is not a forced shutdown).
> 
> If cPluginManager::Active() returns 'true' once, it waits for SHUTDOWNRETRY
> minutes before trying again.

I'd suggest moving the if block a bit down in the code, at least behind 
the manual start check, so plugins cannot interfere with this. I think 
it should be ok to move it down right before the "ForceShutdown = 
false;" line - thats the point where the "Press any key to cancel 
shutdown" will be asked for sure.

SHUTDOWNRETRY defaults to 5 minutes, thats ok, though I would go for 
something around 1 minute.

Cheers,

Udo
Matthias Schwarzott Dec. 2, 2006, 3:24 p.m. UTC | #3 
On Saturday 02 December 2006 16:09, Udo Richter wrote:
> Klaus Schmidinger wrote:
> > The attached patch makes it call cPluginManager::Active() only
> > if it really wants to shut down, and if there is actually an option
> > for plugins to delay the shutdown (i.e. this is not a forced shutdown).
> >
> > If cPluginManager::Active() returns 'true' once, it waits for
> > SHUTDOWNRETRY minutes before trying again.
>
> I'd suggest moving the if block a bit down in the code, at least behind
> the manual start check, so plugins cannot interfere with this. I think

To manual start check I have another idea. Perhaps vdr can get a parameter for 
setting explicitly manual-start or start based on timer. Then the startskript 
could check the time that was written into nvram/acpi/wherever and tell vdr 
if start was manual or not.

Matthias
Klaus Schmidinger Dec. 2, 2006, 3:26 p.m. UTC | #4 
Udo Richter wrote:
> Klaus Schmidinger wrote:
>> The attached patch makes it call cPluginManager::Active() only
>> if it really wants to shut down, and if there is actually an option
>> for plugins to delay the shutdown (i.e. this is not a forced shutdown).
>>
>> If cPluginManager::Active() returns 'true' once, it waits for 
>> SHUTDOWNRETRY
>> minutes before trying again.
> 
> I'd suggest moving the if block a bit down in the code, at least behind 
> the manual start check, so plugins cannot interfere with this. I think 
> it should be ok to move it down right before the "ForceShutdown = 
> false;" line - thats the point where the "Press any key to cancel 
> shutdown" will be asked for sure.

I agree to move it down until before the line

    if (timer && Delta < Setup.MinEventTimeout * 60 && ForceShutdown) {

in order to not interfere with the "assuming manual start of VDR" stuff.
But if we move it further down, the dsyslog("reboot at %s", *TimeToString(Next))
will be executed, even if the plugin's activity prevents that from
happening.

> SHUTDOWNRETRY defaults to 5 minutes, thats ok, though I would go for 
> something around 1 minute.

Well, this is only important if VDR shuts down by itself, unattended.
So I guess it dosn't really matter if it stays up a little longer ;-)

Klaus
Udo Richter Dec. 2, 2006, 3:53 p.m. UTC | #5 
Klaus Schmidinger wrote:
> I agree to move it down until before the line
> 
>    if (timer && Delta < Setup.MinEventTimeout * 60 && ForceShutdown) {
> 
> in order to not interfere with the "assuming manual start of VDR" stuff.
> But if we move it further down, the dsyslog("reboot at %s", 
> *TimeToString(Next))
> will be executed, even if the plugin's activity prevents that from
> happening.

No, I thought of that too. This code part explicitly requests "&& 
ForceShutdown", while the new code requires "!ForceShutdown", so they 
exclude each other.

Moving it down to before "ForceShutdown = false;" will also prevent 
Active() from being called within the MinEventTimeout gap.


The whole shutdown code is an ugly mess, and could use a clean rewrite 
in the 1.5.x cycle. I'll volunteer if that helps...

Cheers,

Udo
Klaus Schmidinger Dec. 2, 2006, 4:25 p.m. UTC | #6 
Udo Richter wrote:
> Klaus Schmidinger wrote:
>> I agree to move it down until before the line
>>
>>    if (timer && Delta < Setup.MinEventTimeout * 60 && ForceShutdown) {
>>
>> in order to not interfere with the "assuming manual start of VDR" stuff.
>> But if we move it further down, the dsyslog("reboot at %s", 
>> *TimeToString(Next))
>> will be executed, even if the plugin's activity prevents that from
>> happening.
> 
> No, I thought of that too. This code part explicitly requests "&& 
> ForceShutdown", while the new code requires "!ForceShutdown", so they 
> exclude each other.

Oh, I must have missed that.

> Moving it down to before "ForceShutdown = false;" will also prevent 
> Active() from being called within the MinEventTimeout gap.

Ok, I'll move it before the line

   if (!Next || Delta > Setup.MinEventTimeout * 60 || ForceShutdown) {

then.

> The whole shutdown code is an ugly mess, and could use a clean rewrite 
> in the 1.5.x cycle. I'll volunteer if that helps...

Sure, once the 1.5 cycle has been opened, I'll be glad to see
how you can do this cleaner ;-)

Klaus
diff mbox

Patch

--- vdr.c	2006/10/14 10:01:32	1.280
+++ vdr.c	2006/12/02 10:20:30
@@ -1154,11 +1154,15 @@ 
                  Skins.Message(mtInfo, tr("Editing process finished"));
               }
            }
-        if (!Interact && ((!cRecordControls::Active() && !cCutter::Active() && !cPluginManager::Active() && (!Interface->HasSVDRPConnection() || UserShutdown)) || ForceShutdown)) {
+        if (!Interact && ((!cRecordControls::Active() && !cCutter::Active() && (!Interface->HasSVDRPConnection() || UserShutdown)) || ForceShutdown)) {
            time_t Now = time(NULL);
            if (Now - LastActivity > ACTIVITYTIMEOUT) {
               // Shutdown:
               if (Shutdown && (Setup.MinUserInactivity || LastActivity == 1) && Now - LastActivity > Setup.MinUserInactivity * 60) {
+                 if (!ForceShutdown && cPluginManager::Active()) {
+                    LastActivity = time(NULL) - Setup.MinUserInactivity * 60 + SHUTDOWNRETRY; // try again later
+                    continue;
+                    }
                  cTimer *timer = Timers.GetNextActiveTimer();
                  time_t Next  = timer ? timer->StartTime() : 0;
                  time_t Delta = timer ? Next - Now : 0;