[Merge] lp:~jamesodhunt/ubuntu/trusty/sysvinit/log-open-files-on-shutdown into lp:ubuntu/sysvinit

Steve Langasek steve.langasek at canonical.com
Thu Mar 6 23:30:51 UTC 2014


Review: Needs Fixing

Hi James,

Apologies again for the delay in reviewing.  Some comments:
 - lsof and awk are both in /usr/bin.  If /usr is on a separate filesystem, this function is going to fail from /etc/init.d/umountroot.  We should really try to make it more robust.  Should we be using fuser -vm here (part of psmisc, more likely to be installed), instead of lsof?  fuser is located in /bin, and more directly addresses what we're looking to find out.
 - Your /etc/init.d/umountroot change defines the do_log() function but does not invoke it.  This looks like a bug to me.
 - If we're going to print anything to the console, then we should also include some sort of pause to give the user a chance to react before the system hard-reboots out from under them.  Perhaps either a 'sleep 5' or a 'Press any key to continue' && read in the error case.
 - I think we want to invoke this logging code *only* when there is a failure unmounting the filesystem or mounting the root filesystem read-write.  If the root filesystem is successfully mounted ro, and all the other filesystems are successfully unmounted, then there's no reason for us to care what files are open, and spamming this to the console on shutdown (because there will always be a non-zero number of files) is going to be unwelcome noise.  And in all the cases where it matters, we know in fact that the root filesystem is *not* remounted ro, so we can reliably log to the root filesystem instead of the console which is better for debugging purposes.

Can you please adjust your patch in light of the above?
-- 
https://code.launchpad.net/~jamesodhunt/ubuntu/trusty/sysvinit/log-open-files-on-shutdown/+merge/196141
Your team Ubuntu branches is subscribed to branch lp:ubuntu/sysvinit.



More information about the Ubuntu-reviews mailing list