Fix analysis plugins, move analysis plugins to use CLI helpers, fix typos.

Fix analysis plugins, move analysis plugins to use CLI helpers, fix typos.

[Joachim Metz, 2015-05-18 21:00:34]

Couple of things we need to discuss but overall good.

https://codereview.appspot.com/232560043/diff/1/plaso/analysis/mediator.py
File plaso/analysis/mediator.py (right):

https://codereview.appspot.com/232560043/diff/1/plaso/analysis/mediator.py#ne...
plaso/analysis/mediator.py:49: def GetDataLocation(self):
Why not make this a property named: data_location ?

https://codereview.appspot.com/232560043/diff/1/plaso/analysis/mediator.py#ne...
plaso/analysis/mediator.py:66: def GetPlatform(self):
property?

https://codereview.appspot.com/232560043/diff/1/plaso/analysis/mediator.py#ne...
plaso/analysis/mediator.py:199: def CloseQueue(self):
+ docstring

Also let's talk about this approach tomorrow, seeing recent queue issues

https://codereview.appspot.com/232560043/diff/1/plaso/analysis/windows_servic...
File plaso/analysis/windows_services.py (right):

https://codereview.appspot.com/232560043/diff/1/plaso/analysis/windows_servic...
plaso/analysis/windows_services.py:90: return 
human_readable_service_enums.SERVICE_ENUMS['Type'].get(
u' (and repeat that across the file)

https://codereview.appspot.com/232560043/diff/1/plaso/analysis/windows_servic...
plaso/analysis/windows_services.py:205: def SetOutputFormat(self,
output_format):
Why not do this for all the interface?

https://codereview.appspot.com/232560043/diff/1/plaso/frontend/psort.py
File plaso/frontend/psort.py (right):

https://codereview.appspot.com/232560043/diff/1/plaso/frontend/psort.py#newco...
plaso/frontend/psort.py:152: analysis_process.join(60)
timeout=60

why wait a full minute?

https://codereview.appspot.com/232560043/diff/1/plaso/frontend/psort.py#newco...
plaso/frontend/psort.py:336: # Do any configuration required.
This should be done in the tool not in the front-end. Let's discuss this. The
front-end should be CLI independent.

https://codereview.appspot.com/232560043/diff/1/plaso/lib/errors.py
File plaso/lib/errors.py (right):

https://codereview.appspot.com/232560043/diff/1/plaso/lib/errors.py#newcode92
plaso/lib/errors.py:92: class WrongHelper(Error):
Where is this used? This should be replaced by BadConfigObject.

https://codereview.appspot.com/232560043/diff/1/plaso/lib/event.py
File plaso/lib/event.py (right):

https://codereview.appspot.com/232560043/diff/1/plaso/lib/event.py#newcode18
plaso/lib/event.py:18: Attributes:
What is the reason for this docstring change? sphinx?

[onager, 2015-05-27 13:26:57]

Code updated.

[onager, 2015-05-27 13:28:13]

https://codereview.appspot.com/232560043/diff/1/plaso/analysis/mediator.py
File plaso/analysis/mediator.py (right):

https://codereview.appspot.com/232560043/diff/1/plaso/analysis/mediator.py#ne...
plaso/analysis/mediator.py:49: def GetDataLocation(self):
On 2015/05/18 21:00:34, Joachim Metz wrote:
> Why not make this a property named: data_location ?

Done.

https://codereview.appspot.com/232560043/diff/1/plaso/analysis/mediator.py#ne...
plaso/analysis/mediator.py:66: def GetPlatform(self):
On 2015/05/18 21:00:34, Joachim Metz wrote:
> property?

Done.

https://codereview.appspot.com/232560043/diff/1/plaso/analysis/mediator.py#ne...
plaso/analysis/mediator.py:199: def CloseQueue(self):
On 2015/05/18 21:00:34, Joachim Metz wrote:
> + docstring
> 
> Also let's talk about this approach tomorrow, seeing recent queue issues

Acknowledged.

https://codereview.appspot.com/232560043/diff/1/plaso/analysis/windows_servic...
File plaso/analysis/windows_services.py (right):

https://codereview.appspot.com/232560043/diff/1/plaso/analysis/windows_servic...
plaso/analysis/windows_services.py:90: return 
human_readable_service_enums.SERVICE_ENUMS['Type'].get(
On 2015/05/18 21:00:34, Joachim Metz wrote:
> u' (and repeat that across the file)

Done.

https://codereview.appspot.com/232560043/diff/1/plaso/analysis/windows_servic...
plaso/analysis/windows_services.py:205: def SetOutputFormat(self,
output_format):
On 2015/05/18 21:00:34, Joachim Metz wrote:
> Why not do this for all the interface?

At the moment, no other plugins have any other output options. I don't want to
add methods to the interface that won't work for all plugins (or most, more
realistically).

https://codereview.appspot.com/232560043/diff/1/plaso/frontend/psort.py
File plaso/frontend/psort.py (right):

https://codereview.appspot.com/232560043/diff/1/plaso/frontend/psort.py#newco...
plaso/frontend/psort.py:152: analysis_process.join(60)
On 2015/05/18 21:00:34, Joachim Metz wrote:
> timeout=60
> 
> why wait a full minute?

This the maximum time that a plugin's CompileReport method will take to
complete. At the moment, these methods aren't too complex, so this is
reasonable. In the future, if these methods end up doing a lot of work, we might
have to revisit this.

https://codereview.appspot.com/232560043/diff/1/plaso/frontend/psort.py#newco...
plaso/frontend/psort.py:336: # Do any configuration required.
On 2015/05/18 21:00:34, Joachim Metz wrote:
> This should be done in the tool not in the front-end. Let's discuss this. The
> front-end should be CLI independent.

I'd like to do this, but this requires a very large refactor of the frontend.
I'll add a TODO, as for the output args just above.

https://codereview.appspot.com/232560043/diff/1/plaso/lib/errors.py
File plaso/lib/errors.py (right):

https://codereview.appspot.com/232560043/diff/1/plaso/lib/errors.py#newcode92
plaso/lib/errors.py:92: class WrongHelper(Error):
On 2015/05/18 21:00:34, Joachim Metz wrote:
> Where is this used? This should be replaced by BadConfigObject.

Done.

https://codereview.appspot.com/232560043/diff/1/plaso/lib/event.py
File plaso/lib/event.py (right):

https://codereview.appspot.com/232560043/diff/1/plaso/lib/event.py#newcode18
plaso/lib/event.py:18: Attributes:
On 2015/05/18 21:00:34, Joachim Metz wrote:
> What is the reason for this docstring change? sphinx?

This is actually part of the style guide we haven't been following. See
https://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Comments in the
classes section.

[Joachim Metz, 2015-05-27 14:24:15]

Style nits remaining and let's make sure to sync the docs on the psort queueing,
otherwise no objections.

https://codereview.appspot.com/232560043/diff/20001/plaso/analysis/manager.py
File plaso/analysis/manager.py (right):

https://codereview.appspot.com/232560043/diff/20001/plaso/analysis/manager.py...
plaso/analysis/manager.py:43: def ListAllPluginInfo(cls, show_all=True):
Info => Information

List or Get?

https://codereview.appspot.com/232560043/diff/20001/plaso/analysis/manager.py...
plaso/analysis/manager.py:56: doc_string, _, _ =
cls_obj.__doc__.partition(u'\n')
Can you add a todo to turn docstring into description.

https://codereview.appspot.com/232560043/diff/20001/plaso/analysis/manager.py...
plaso/analysis/manager.py:89: for plugin_object in
cls._plugin_classes.itervalues():
I opt to use Python 3 compatible style:

iter(cls._plugin_classes.values())

https://codereview.appspot.com/232560043/diff/20001/plaso/analysis/mediator.py
File plaso/analysis/mediator.py (right):

https://codereview.appspot.com/232560043/diff/20001/plaso/analysis/mediator.p...
plaso/analysis/mediator.py:21: is complete.
Event => event

+ (instance of EventObject) ?

https://codereview.appspot.com/232560043/diff/20001/plaso/analysis/mediator.p...
plaso/analysis/mediator.py:75: Returns:
I opt to remove the Return, since it adds no value.

https://codereview.appspot.com/232560043/diff/20001/plaso/analysis/windows_se...
File plaso/analysis/windows_services.py (right):

https://codereview.appspot.com/232560043/diff/20001/plaso/analysis/windows_se...
plaso/analysis/windows_services.py:90: return 
human_readable_service_enums.SERVICE_ENUMS[u'Type'].get(
- 1 space between return and human

https://codereview.appspot.com/232560043/diff/20001/plaso/analysis/windows_se...
plaso/analysis/windows_services.py:206: """Sets the output format of the
generated report."""
+ Args:

https://codereview.appspot.com/232560043/diff/20001/plaso/cli/helpers/windows...
File plaso/cli/helpers/windows_services_analysis.py (right):

https://codereview.appspot.com/232560043/diff/20001/plaso/cli/helpers/windows...
plaso/cli/helpers/windows_services_analysis.py:30: type=unicode, action='store',
default=u'text',
u'store'

https://codereview.appspot.com/232560043/diff/20001/plaso/cli/helpers/windows...
plaso/cli/helpers/windows_services_analysis.py:31: choices=[u'text', u'yaml'],
help=u'Specify how the results should be '
you are breaking the string enclose in ()
help=(
    u'...')

https://codereview.appspot.com/232560043/diff/20001/plaso/engine/single_proce...
File plaso/engine/single_process.py (right):

https://codereview.appspot.com/232560043/diff/20001/plaso/engine/single_proce...
plaso/engine/single_process.py:366: """Included for compatibility with the
Multiprocessing queue."""
Add the functional description, the comment is fine for a successive line.

Closes the queue backing this producer.

Included for compatibility with the Multiprocessing queue."""

https://codereview.appspot.com/232560043/diff/20001/plaso/frontend/psort.py
File plaso/frontend/psort.py (right):

https://codereview.appspot.com/232560043/diff/20001/plaso/frontend/psort.py#n...
plaso/frontend/psort.py:153: if completion_event.wait(60):
Add comment regarding the wait or maybe make 60 a class constant e.g.
MAXIMUM_WAIT_TIME or equiv.

https://codereview.appspot.com/232560043/diff/20001/plaso/frontend/psort.py#n...
plaso/frontend/psort.py:156: logging.warning(u'Analysis process {0:s} failed to
compile its report '
I opt to break line after (

Otherwise you get these code blocks on the right having the reader skip a lot of
white space. Also to keep it consistent with the previous lines of code.

https://codereview.appspot.com/232560043/diff/20001/plaso/frontend/psort.py#n...
plaso/frontend/psort.py:531: 
+ 1 white line

https://codereview.appspot.com/232560043/diff/20001/plaso/frontend/psort.py#n...
plaso/frontend/psort.py:536: completion_event: An Event object that indicates
whether the process is
Event object => event object (instance of EventObject)

https://codereview.appspot.com/232560043/diff/20001/plaso/frontend/psort.py#n...
plaso/frontend/psort.py:539: process: The instance of Multiprocessing.Process
that encapsulates the
The process (instance of Multiprocessing.Process) that ...

https://codereview.appspot.com/232560043/diff/20001/plaso/lib/event.py
File plaso/lib/event.py (right):

https://codereview.appspot.com/232560043/diff/20001/plaso/lib/event.py#newcode68
plaso/lib/event.py:68: """Sets the list of event tags that relate to the
report."""
+ Args:

[onager, 2015-05-27 14:48:26]

Code updated.

[onager, 2015-05-27 14:54:01]

Addressed these comments, still updating process management doc.

https://codereview.appspot.com/232560043/diff/20001/plaso/analysis/manager.py
File plaso/analysis/manager.py (right):

https://codereview.appspot.com/232560043/diff/20001/plaso/analysis/manager.py...
plaso/analysis/manager.py:43: def ListAllPluginInfo(cls, show_all=True):
On 2015/05/27 14:24:14, Joachim Metz wrote:
> Info => Information
> 
> List or Get?

Done.

https://codereview.appspot.com/232560043/diff/20001/plaso/analysis/manager.py...
plaso/analysis/manager.py:56: doc_string, _, _ =
cls_obj.__doc__.partition(u'\n')
On 2015/05/27 14:24:14, Joachim Metz wrote:
> Can you add a todo to turn docstring into description.

Done.

https://codereview.appspot.com/232560043/diff/20001/plaso/analysis/manager.py...
plaso/analysis/manager.py:89: for plugin_object in
cls._plugin_classes.itervalues():
On 2015/05/27 14:24:14, Joachim Metz wrote:
> I opt to use Python 3 compatible style:
> 
> iter(cls._plugin_classes.values())

Done.

https://codereview.appspot.com/232560043/diff/20001/plaso/analysis/mediator.py
File plaso/analysis/mediator.py (right):

https://codereview.appspot.com/232560043/diff/20001/plaso/analysis/mediator.p...
plaso/analysis/mediator.py:21: is complete.
On 2015/05/27 14:24:14, Joachim Metz wrote:
> Event => event
> 
> + (instance of EventObject) ?

Done.

https://codereview.appspot.com/232560043/diff/20001/plaso/analysis/mediator.p...
plaso/analysis/mediator.py:75: Returns:
On 2015/05/27 14:24:14, Joachim Metz wrote:
> I opt to remove the Return, since it adds no value.

Done.

https://codereview.appspot.com/232560043/diff/20001/plaso/analysis/windows_se...
File plaso/analysis/windows_services.py (right):

https://codereview.appspot.com/232560043/diff/20001/plaso/analysis/windows_se...
plaso/analysis/windows_services.py:90: return 
human_readable_service_enums.SERVICE_ENUMS[u'Type'].get(
On 2015/05/27 14:24:14, Joachim Metz wrote:
> - 1 space between return and human

Done.

https://codereview.appspot.com/232560043/diff/20001/plaso/analysis/windows_se...
plaso/analysis/windows_services.py:206: """Sets the output format of the
generated report."""
On 2015/05/27 14:24:14, Joachim Metz wrote:
> + Args:

Done.

https://codereview.appspot.com/232560043/diff/20001/plaso/cli/helpers/windows...
File plaso/cli/helpers/windows_services_analysis.py (right):

https://codereview.appspot.com/232560043/diff/20001/plaso/cli/helpers/windows...
plaso/cli/helpers/windows_services_analysis.py:30: type=unicode, action='store',
default=u'text',
On 2015/05/27 14:24:15, Joachim Metz wrote:
> u'store'

Done.

https://codereview.appspot.com/232560043/diff/20001/plaso/cli/helpers/windows...
plaso/cli/helpers/windows_services_analysis.py:31: choices=[u'text', u'yaml'],
help=u'Specify how the results should be '
On 2015/05/27 14:24:15, Joachim Metz wrote:
> you are breaking the string enclose in ()
> help=(
>     u'...')

Done.

https://codereview.appspot.com/232560043/diff/20001/plaso/engine/single_proce...
File plaso/engine/single_process.py (right):

https://codereview.appspot.com/232560043/diff/20001/plaso/engine/single_proce...
plaso/engine/single_process.py:366: """Included for compatibility with the
Multiprocessing queue."""
On 2015/05/27 14:24:14, Joachim Metz wrote:
> Add the functional description, the comment is fine for a successive line.
> 
> Closes the queue backing this producer.
> 
> Included for compatibility with the Multiprocessing queue."""

But this method doesn't do anything? It doesn't close any queue. Also, this is
the queue class, not the producer.

https://codereview.appspot.com/232560043/diff/20001/plaso/frontend/psort.py
File plaso/frontend/psort.py (right):

https://codereview.appspot.com/232560043/diff/20001/plaso/frontend/psort.py#n...
plaso/frontend/psort.py:153: if completion_event.wait(60):
On 2015/05/27 14:24:15, Joachim Metz wrote:
> Add comment regarding the wait or maybe make 60 a class constant e.g.
> MAXIMUM_WAIT_TIME or equiv.

Done.

https://codereview.appspot.com/232560043/diff/20001/plaso/frontend/psort.py#n...
plaso/frontend/psort.py:156: logging.warning(u'Analysis process {0:s} failed to
compile its report '
On 2015/05/27 14:24:15, Joachim Metz wrote:
> I opt to break line after (
> 
> Otherwise you get these code blocks on the right having the reader skip a lot
of
> white space. Also to keep it consistent with the previous lines of code.

Done.

https://codereview.appspot.com/232560043/diff/20001/plaso/frontend/psort.py#n...
plaso/frontend/psort.py:531: 
On 2015/05/27 14:24:15, Joachim Metz wrote:
> + 1 white line

Done.

https://codereview.appspot.com/232560043/diff/20001/plaso/frontend/psort.py#n...
plaso/frontend/psort.py:536: completion_event: An Event object that indicates
whether the process is
On 2015/05/27 14:24:14, Joachim Metz wrote:
> Event object => event object (instance of EventObject) 

Done.

https://codereview.appspot.com/232560043/diff/20001/plaso/frontend/psort.py#n...
plaso/frontend/psort.py:539: process: The instance of Multiprocessing.Process
that encapsulates the
On 2015/05/27 14:24:14, Joachim Metz wrote:
> The process (instance of Multiprocessing.Process) that ...

Done.

https://codereview.appspot.com/232560043/diff/20001/plaso/lib/event.py
File plaso/lib/event.py (right):

https://codereview.appspot.com/232560043/diff/20001/plaso/lib/event.py#newcode68
plaso/lib/event.py:68: """Sets the list of event tags that relate to the
report."""
On 2015/05/27 14:24:15, Joachim Metz wrote:
> + Args:

Done.

[Joachim Metz, 2015-05-27 15:32:26]

LGTM

https://codereview.appspot.com/232560043/diff/20001/plaso/engine/single_proce...
File plaso/engine/single_process.py (right):

https://codereview.appspot.com/232560043/diff/20001/plaso/engine/single_proce...
plaso/engine/single_process.py:366: """Included for compatibility with the
Multiprocessing queue."""
Please add the functional description, I agree that the method is a stub, but
you are implementing an interface. I opt to still describe what the interface is
supposed to do, even if it does nothing effectively.

https://codereview.appspot.com/232560043/diff/40001/plaso/analysis/mediator.py
File plaso/analysis/mediator.py (right):

https://codereview.appspot.com/232560043/diff/40001/plaso/analysis/mediator.p...
plaso/analysis/mediator.py:20: completion_event: An optional Event object
(instance of
"Event" why the capital?

[onager, 2015-05-27 15:38:38]

Code updated.

[onager, 2015-05-27 15:44:09]

Processing doc is updated.

https://codereview.appspot.com/232560043/diff/20001/plaso/engine/single_proce...
File plaso/engine/single_process.py (right):

https://codereview.appspot.com/232560043/diff/20001/plaso/engine/single_proce...
plaso/engine/single_process.py:366: """Included for compatibility with the
Multiprocessing queue."""
On 2015/05/27 15:32:26, Joachim Metz wrote:
> Please add the functional description, I agree that the method is a stub, but
> you are implementing an interface. I opt to still describe what the interface
is
> supposed to do, even if it does nothing effectively.

Done.

https://codereview.appspot.com/232560043/diff/40001/plaso/analysis/mediator.py
File plaso/analysis/mediator.py (right):

https://codereview.appspot.com/232560043/diff/40001/plaso/analysis/mediator.p...
plaso/analysis/mediator.py:20: completion_event: An optional Event object
(instance of
On 2015/05/27 15:32:26, Joachim Metz wrote:
> "Event" why the capital?

Done.

[onager, 2015-05-27 15:46:24]

Code Submitted.