Output rendering in plugins with inheritance

[GoogleCodeExporter]

Hey guys, I don't think this is a big problem currently, but just FYI because 
it came up recently. If a plugin doesn't support output in HTML (or any other 
format), but inherits from a plugin that does support it...then we'll see type 
and/or tuple unpack exceptions. For example:

class OnePlugin:
    def calculate(self):
        yield a,b,c
    def render_text(self, outfd, data):
        pass
    def render_html(self, outfd, data):
        for a,b,c in data:
            pass

class TwoPlugin(OnePlugin):
    def calculate(self):
        yield a,b,c,d,e,f,g
    def render_text(self, outfd, data):
        pass

So if someone tries to do "volatility.py twoplugin --output=html" then 
OnePlugin.render_html() will be executed with the data yielded by 
TwoPlugin.calculate() - and it will result is some exceptions. 

Original issue reported on code.google.com by michael.hale@gmail.com on 9 Dec 2010 at 3:45

[GoogleCodeExporter]

Ok, so solving this one's not that easy.  You could check the class's __dict__ 
to see whether calculate and render_blah were defined in their own class, or in 
some superclass.  That leaves us with four possibilities:

  | Calculate | Render_blah |  Meaning
----------------------------------------
A |  Defined  |   Defined   | Render_blah written with Calculate in mind
B |  Defined  |  Inherited  | Calculate possibly changed from Render_blah's 
expectations
C | Inherited |   Defined   | Render_blah possibly changed from calculate's 
expectations
D | Inherited |  Inherited  | Not our problem, since we haven't changed either

Now we could easily put in blocks where calculate and render_blah aren't both 
defined or inherited, and that should ensure no exceptions occur (unless 
they're both inherited and one of the super's didn't break the rules).  
Unfortunately that's going to leave developers scratching their heads why 
inheritance isn't working when they've defined a perfectly valid render_blah 
function, but it isn't getting passed the data from super's calculate.  Also, 
if someone subclasses from a broken class, the protection wouldn't be extended 
(as mentioned above, the supers have to not break the rules in order for 
inherited + inherited to work properly).

We could build up a better picture by tracing back exactly where each function 
is defined in the inheritance tree, but given python supports multiple 
inheritance that's going to be sticky, and most of this comes about from 
subclassing being abused a little bit...

Generally people subclass because they want additional/existing functions from 
another plugin (such as their calculate method, or helper functions for that 
calculate method), and I think this really boils down to the rendering 
functions needing to be separated from the plugin calculations (which is issue 
11).  So ideally the plugins produce some form of standardized output, and then 
renderers can be written to handle that output form and deal with it in some 
way.

The following idea probably belongs in issue 11, but another option might be 
split away the plugins from the tasks they're trying to complete.  So we've got 
framework functions for getting task lists and dealing with the registry, but 
we're writing complex functionality into the plugins, rather than just simple 
connect-the-dots between functions from the framework.  If we produced 
plugin-function-libraries, as well as plugin-commands, then you wouldn't get 
the render/calculate issues, but you'd also find the API being a bit of a 
moving target, because *everything's* so extensible.  It would also be 
difficult to ensure different plugin writers don't write overlapping library 
functions with different outputs.

Not a simple problem, and not one I propose to solve in this already rather 
full comment, but just to say I'm marking this as blocking against issue 11, 
since I think if we get 11 sorted, this problem will go away.  Issue 11 is a 
whole other discussion in itself...

Original comment by mike.auty@gmail.com on 11 Dec 2010 at 8:30

[GoogleCodeExporter]

I dont see this as an issue at all. If you inherit from a class you have an 
implicit contract to maintain the external interfaces of the class working 
(i.e. not broken). Since render_* are public methods you need to ensure they 
all still work appropriately. On some level calculate() is also a public method 
and should probably return the same data format (so that the old render_* 
methods would still work) but that would make it hard to inherit from classes 
in the general case (for doing something different).

Maybe we need to make calculate() a private method but even then render_* 
should all be made to work.

If you are going to extend a class you need to make sure all the render methods 
is supports are not broken.

I dont think issue 11 is blocking because issue 11 is simply a generic renderer 
and does not preclude us from using specific renderers.

Original comment by scude...@gmail.com on 11 Dec 2010 at 10:31

[GoogleCodeExporter]

Well, so that seems like the debate at the heart of the matter.  Should 
calculate have a better interface, or should all subclasses fix up any 
remaining renderers?

Since new render methods could be added in the base class later on (perhaps 
PluginTwo was defined before PluginOne got render_html support), it really 
seems to suggest that calculate should have a more defined output interface.

Making calculate private is unlikely to help.  People need access to the 
functions so they don't duplicate code, if calculate's private (and maintains 
the bulk of the code people want to reuse) they'll start instantiating plugins 
within plugins, and that'll get ugly.  It's not clear that many Commands need 
their calculate functions to be part of the class, I think very few of them end 
up using self at all (apart, now, for configs).  However, making them class 
methods will require all the helper functions to become class methods, and so 
also gets sticky.

We could let people just drop raw library code into the plugins folders, but 
then why not roll it into the framework proper, rather than having a plugins 
folder?

I think probably the best move is just to put a bit more restriction on 
calculate functions, so that they provide a more standardized result.

I've removed blocking on issue 11, but I'm not sure what to set the status of 
this issue to?  Invalid, Wontfix, Later or just leave it as accepted?

Original comment by mike.auty@gmail.com on 11 Dec 2010 at 10:51

[GoogleCodeExporter]

Thats a good point. IMHO its best to leave calculate public but maybe refactor 
the other functionality into better modules. For example right now the only 
inheritance thats really going on is pslist etc inheriting from a common base 
in order to borrow some common functions. This inheritance hierarchy might be 
refactored a bit better (possibly even with multiple inheritance). If we keep 
calculate as public it should be implicit that extension of the class will 
preserve the results from calculate().

I think this bug can be fixed by refactoring all inheritances.

Original comment by scude...@gmail.com on 11 Dec 2010 at 11:02

[GoogleCodeExporter]

Original comment by mike.auty@gmail.com on 21 Jan 2011 at 8:20

[GoogleCodeExporter]

per dev call agreement, this is no longer an issue

Original comment by michael.hale@gmail.com on 15 Dec 2011 at 7:50

• Changed state: Done