[Vol-dev] Pylint and code questions

Hiya guys, First off thanks for the warm reception, everyone's been really friendly on IRC, etc. It feels like a really nice community. 5:) People on IRC suggest that the mailing list really is the right place for patches (even code clean-up patches), so I've unleashed my giant pylint patch on it. As I mentioned I've run pylint against most it (I use pydev with eclipse, which has pylint support built in, and went through all the files). Unfortunately that led to a horribly large 6000+ line patch (attached) and I'm really, really sorry it's such an unweildy nasty patch. 5:( It applies to the current volatility HEAD (r16), but it has a *lot* of changes it in, so I'd really appreciate it if someone could look over it (with me, if that helps). Mostly they should be indentation, spacing, unused imports and unused variables, but it could be that the change to explicit imports or prepending unused variable names with an _ might have an unexpected effect. I don't have that many memory images to run this against, so anyone who has more do please let me know if there's any problems. It might be easiest if I cut this down into file-by-file patches (although scan.py and vmodules.py were both huge). Would that be useful, or are people happy to deal with it as is? I did spot a couple of issues as I was going through, there's a method without the self argument in addrspace, and a few others which I was able to fix myself. Some of them though I don't know the code well enough to sort myself and I've listed here: * In scan.py, check_connection_poolprevioussize, seems always to return False, and sets PreviousSize seemingly for no reason, does the function have side effects besides simply returning? * In scan2.py, BaseMemoryScanner.Process uses a default value of {} which is generally a bad idea (having a mutable object as a default means that it may not always be what you expect when you call the function[1]). This turned up in a few other places later on as well, but I think I've fixed those up. * In tasks.py, find_dtb appears to reopen an existing address space rather than using the one it was provided with, and I can't quite figure out why? * In object.py, get_obj_offset, there's a variable that could potentially be used before it's defined (I think it's conditional and gets defined lower in the loop, so as long as the condition is never met first time round, everything will work fine). Is there a coding standard or anything that you guys are working to? So, yeah, I'm not sure the best way of approaching all this, but I thought I'd best post it to get it out there. I've also got a fair few questions about some bits and pieces I'd like to work on (with varying levels of viability) include: * Removing the big chunks of data table sat in crash_dump.py (I've already slimmed them down in another patch that'd need applying after the pylint patch, but I'm thinking of loading it all from a resource file or something). * I'd also like to remove the use of globals. I haven't figured out exactly where this gets used (it seems mostly to be various tables in meta_info.py), but I think it's generally discouraged. * It felt like there was an awful lot of repetition going on in vmodules.py, and I wondered if the functions couldn't be turned into classes and dumped in an internal-plugins location, to save hardwiring in everything in to "modules" at the start of the volatility script. * Also, the namespace seemed a bit all over the place, format_time was defined in two places, and most of the scripts seemed to just "from forensics.win32.blah import *", which imported all the imports, so lots of them were getting gmtime and strftime from modules other than time. Unfortunately I'm not around much this weekend, so if there's any issues or fixes or anything, they'll have to wait until Sunday, but hopefully I'll chat to people on the list or IRC. Thanks, Mike 5:) [1] http://effbot.org/zone/default-values.htm