[RFC] Shutdown rewrite for 1.5.x

  On Sat, Jan 13, 2007 at 09:48:47PM +0100, Udo Richter wrote:
> >A more important suggestion: Could you please add a notification method
> >to status.h for notifying plugins whether VDR is currently in
> >interactive mode?  
> 
> You can poll this state on your own by calling 
> Shutdown.IsUserInactive().

Thanks, this seems to work: the display is powered off after the
"press any button to cancel shutdown" timeout.  However, after a
failed shutdown attempt (renaming the shutdown script so that
it won't be found), vdr does something to the file descriptor that
my plugin opens, and the relay control will fail.

This might have something to do with the fact that it is file descriptor 0.
There is no stdin, because my runvdr script closes it:

exec <&- >> /tmp/vdr.log 2>&1

The problem seems to lie in shutdown.c, in SystemExecSession().
It won't close file descriptor 0, but it will close anything above
STDERR_FILENO (which is normally 2).  This function appears to show
up in strace output as "clone()".  Adding close(0) to the function
fixes the problem.  Do you see any reason why the shutdown script
would need to access stdin?  (I can see reasons why it would want to
access stdout or stderr.)

I made a simple patch to softdevice (attached) that seems to work.
A similar patch would be needed for the subtitles plugin, to prevent
subtitles from appearing on the suspended video screen.

	Marko
  

Marko Mäkelä wrote:
> The problem seems to lie in shutdown.c, in SystemExecSession().
> It won't close file descriptor 0, but it will close anything above
> STDERR_FILENO (which is normally 2).  This function appears to show
> up in strace output as "clone()".  Adding close(0) to the function
> fixes the problem.  Do you see any reason why the shutdown script
> would need to access stdin?  (I can see reasons why it would want to
> access stdout or stderr.)

Actually, the SystemExecSession() is just a modified version of 
SystemExec() in thread.c, that was used before to call the shutdown 
script. They only differ in the fact that VDR continues instead of 
waiting for the child process, and that the child process is detached 
into its own session. There's no serious difference regarding the file 
handles.
Since the shutdown script runs in the background, the stdin could 
probably be closed. After all, VDR should continue to 'own' stdin, and 
running in the background would probably cause a conflict. However, 
running the script with a closed file handle may cause other side effects.

Cheers,

Udo
  

On Tue, Jan 23, 2007 at 10:19:57PM +0100, Udo Richter wrote:
> Since the shutdown script runs in the background, the stdin could 
> probably be closed. After all, VDR should continue to 'own' stdin, and 
> running in the background would probably cause a conflict. However, 
> running the script with a closed file handle may cause other side effects.

I can't think of any side effect.  The shutdown script is not supposed
to be interactive, and system tools generally do not read stdin.

There are two reasons why my runvdr script closes stdin and redirects
stdout and stderr to a file.  First, scrolling console output on the bitmap
framebuffer could interfere with softdevice-dfb or slow down the playback.
Second, I may occasionally launch the runvdr script via an ssh session,
and the ssh session would refuse to close if stdin, stdout, or stderr are
being used after exiting the shell.

I guess I could work around this bug by leaving file descriptor 0 open
before starting vdr, e.g., by replacing the <&- with </dev/null in my
runvdr script.  But I would very much appreciate it if you could add
close(STDIN_FILENO) after the fork() call.

Are you planning to maintain the 1.4 branch of the patch?  If yes
(and I hope so), can plugins use some #ifdef to see if
Shutdown.IsUserInactive() is available?

	Marko
  

Marko Mäkelä wrote:
> On Tue, Jan 23, 2007 at 10:19:57PM +0100, Udo Richter wrote:
>> Since the shutdown script runs in the background, the stdin could 
>> probably be closed. After all, VDR should continue to 'own' stdin, and 
>> running in the background would probably cause a conflict. However, 
>> running the script with a closed file handle may cause other side effects.
> 
> I can't think of any side effect.  The shutdown script is not supposed
> to be interactive, and system tools generally do not read stdin.

Well, strange things happen. I've had such a side effect on my runvdr 
extreme with a closed stdout, see here: (German)
http://www.vdr-portal.de/board/thread.php?postid=542948#post542948

In this case, printf on kanotix complained heavily on a closed stdout, 
and the only workaround was to replace >&- with the traditional >/dev/null.

> Are you planning to maintain the 1.4 branch of the patch?  If yes
> (and I hope so), can plugins use some #ifdef to see if
> Shutdown.IsUserInactive() is available?

For now I'll maintain it, currently the differences are only minor. 
Adding some #ifdef detection is a good idea, I'll add that for the next 
1.4 patch.

Cheers,

Udo
  

On Wed, Jan 24, 2007 at 06:49:10PM +0100, Udo Richter wrote:
> Well, strange things happen. I've had such a side effect on my runvdr 
> extreme with a closed stdout, see here: (German)
> http://www.vdr-portal.de/board/thread.php?postid=542948#post542948
> 
> In this case, printf on kanotix complained heavily on a closed stdout, 
> and the only workaround was to replace >&- with the traditional >/dev/null.

Closing stdout is a different matter.  But I agree with you, it may be
not a wise idea to close stdin either.  The workaround </dev/null
did not occur to me until today.  And even without the workaround, it's
not that bad, because usually the shutdown does succeed.

> >Are you planning to maintain the 1.4 branch of the patch?  If yes
> >(and I hope so), can plugins use some #ifdef to see if
> >Shutdown.IsUserInactive() is available?
> 
> For now I'll maintain it, currently the differences are only minor. 
> Adding some #ifdef detection is a good idea, I'll add that for the next 
> 1.4 patch.

Thank you!  I plan to keep using 1.4 at least until 1.5 supports
DVB subtitles (which I hope it will support at some point) or UTF-8.

	Marko