Code review comment for lp:~jamesodhunt/ubuntu/saucy/sysvinit/log-processes-and-open-files-on-shutdown

Thanks for taking this on, James.

I would like to see a few improvements here:
 - These logs are written unconditionally. I would recommend that we write them *only* in the error case, by doing some analysis of the open files first and logging only if we find an issue with files open for writing.
 - Fixing this to only write the logs in case of error means that the sysvinit package itself can be made to trigger apport in the case of a failure, instead of relying on this being picked up by an apport hook later. I would like to see this done. (This does raise the bar for correctness of our checks... the last time something like this was enabled in sendsigs, the false positive rate was terrible and it had to be disabled again. But we should be able to get this done accurately if we're properly checking only for files open for writing and files open on non-root filesystems.)
 - I think logging the process list is redundant. We are going to need the lsof logs for analysis, and if lsof is unavailable, I don't think the ps output is going to be sufficient for debugging: it may let us point the finger at a particular process, but won't tell us anything about why.
 - This output is being written to /var/log in the umountfs script. It's possible (though unlikely) for a file to be opened for writing *after* umountfs has run, but before umountroot is run. I think we want to capture this data at that later point, which is more definitive? Also, since /var/log may not be on the root filesystem, we can't rely on writing there from umountroot. This suggests to me that for best results, we either need to designate some place guaranteed to be on the root filesystem to write a log from umountroot (either directly in / analagous to the /forcefsck flag, or under /etc), or log this information to the console rather than the filesystem. The latter would be /potentially/ useful in the case of a read-only root filesystem, except in that case umountroot can never fail so we don't need to log anything anyway. Oh, and /usr may *also* not be on the root filesystem, which makes it problematic to call /usr/bin/lsof. Perhaps the best approach is to log any problems to /var/log from umountfs, and then again log any problems to the console from umountroot if possible.

Finally, the exact patch here is not very idiomatic:

+ # Log open files as quickly as possible
+ { `command -v lsof` && lsof -lnP > /var/log/shutdown-lsof.log && sync; } || :

This script is not 'set -e', so the { ;} || : is unnecessary and inconsistent with the style of the rest of the script. `command -v lsof` is actually buggy, because this takes the output of 'command -v' and *runs* it, resulting in a duplicate call to lsof (without the speed optimizations). And chaining with '&&' makes the flow less obvious than it could be.

I recommend writing this as:

        if command -v lsof >/dev/null 2>&1; then
            lsof -lnP > /var/log/shutdown-lsof.log
            sync
        fi