[mythtv-commits] Ticket #7762: Autoshutdown slave feature enhanced

[MythTV]

#7762: Autoshutdown slave feature enhanced
-------------------------------+--------------------------------------------
 Reporter:  gmembre@…          |       Owner:  cpinkham
     Type:  enhancement        |      Status:  assigned
 Priority:  minor              |   Milestone:          
Component:  MythTV - General   |     Version:  unknown 
 Severity:  medium             |     Mlocked:  0       
-------------------------------+--------------------------------------------

Comment(by cpinkham):

 Thanks for the feature enhancement, but there are a few things that I
 think need to be corrected before this patch can be accepted.  Here are
 the issues I noticed:

 1) You can't instantiate a Scheduler anywhere but the master.   Just
 calling DisableScheduling() still lets the main parts of the scheduler run
 but it doesn't fire off any recordings.  The
 Scheduler code is not guaranteed to be 'multi-backend-safe', meaning that
 it potentially could
 do things to the database that should only be done from the master
 Scheduler.  I don't think
 there is any code like this currently, but it's not something we want to
 deal with in the
 future.  Even the "mythbackend --printsched" functionality opens a
 connection to the master
 scheduler and downloads the list of scheduled recordings.  If
 Scheduler::CheckShutdownServer() is the only thing you need from the
 scheduler, then you can make this method static and call it as
 "bool status = Scheduler::CheckShutdownServer(0, idleSince,
 blockShutdown);" in your code.

 2) There are a couple reasons that EncoderLink::GoToSleep() can return
 false other than when the slave returns a non "OK" status, so you can't
 rip out the while loop that I have that sets the slave's status to
 undefined.  It sounds like EncoderLink::GoToSleep() and
 PlaybackSock::GoToSleep() both need to return an int status.  Something
 like -1 == error, set status to undefined, 0 == OK slave is going to
 sleep, and 1 == slave was busy and can't go to sleep.

 3) The changes to MainServer::HandleGoToSleep() seem wrong in a couple
 ways.  a) you are calling SendResponse() twice in the case where 'status'
 is true, but the SleepCommand being empty.  b) you don't need to get the
 value of blockSDWUwithoutClient prior to calling CheckShutdownServer.  The
 'blockshutdown' parameter to CheckShutdownServer is passed by reference,
 and updated inside the method so that the caller can get the value.  You
 can just declare "bool blockShutdown;" without the GetNumSetting since you
 don't need the value of blockShutdown.

 4) please observe myth coding style and put brackets { and } on their own
 lines.

 5) the Feb 9 version of the patch still uses the AllowSlaveToSleep setting
 in one place.

 6) since Scheduler::WakeUpSlaves is now checking if a slave IsAwake(), it
 probably should also check IsWaking() in case we just told it to wake up a
 minute ago and the slave's mythbackend isn't up and running yet.

 I think that's about all I noticed from my first pass through the patch.

-- 
Ticket URL: <http://svn.mythtv.org/trac/ticket/7762#comment:6>
MythTV <http://www.mythtv.org/>
MythTV