Added initial winevt-rc support - step 4 #99.

[kiddi, 2015-02-23 23:11:30]

Few comments

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

https://codereview.appspot.com/200460043/diff/1/plaso/formatters/mediator.py#...
plaso/formatters/mediator.py:15: _WINEVT_RC_DATABASE = u'winevt-rc.db'
do we want to keep all these parser specific events at the top level mediator?

... at least something to keep in mind, I don't mind this now, and I'm not sure
how much more we'll add, but if we do, then we may need to rethink this
approach,

so, this is food for thoughts, but fine with this as is for now

https://codereview.appspot.com/200460043/diff/1/plaso/formatters/mediator.py#...
plaso/formatters/mediator.py:45: """Retrieves a specific message for a specific
Windows Event Log source.
or Retrieves the message string for a specific Window.....

https://codereview.appspot.com/200460043/diff/1/plaso/formatters/mediator.py#...
plaso/formatters/mediator.py:58: if self.lcid != self.DEFAULT_LCID:
can this ever come up??? I mean self.lcid is a property with only a getter ,not
a setter and it returns self.DEFAULT_LCID, so thsi should always return back the
default one, and thus never not be the same....

perhaps better to have self.lcid as a regular attribute to the class and not
define it as a property? And have it being set to the default value in the
initialization? 

or define a SETTER, but then you have to have another attribute, since youa re
not going to overwrite a class constant

https://codereview.appspot.com/200460043/diff/1/plaso/formatters/winevt.py
File plaso/formatters/winevt.py (right):

https://codereview.appspot.com/200460043/diff/1/plaso/formatters/winevt.py#ne...
plaso/formatters/winevt.py:25: u'Message string: {message_string}']
should the message string come first?

https://codereview.appspot.com/200460043/diff/1/plaso/formatters/winevt.py#ne...
plaso/formatters/winevt.py:107: strings = event_values.get(u'strings', None)
this should perhaps default to [], since you are doing *strings

In [1]: a = None

In [2]: 'asdf {}'.format(*a)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-2-3f661634f1b1> in <module>()
----> 1 'asdf {}'.format(*a)

TypeError: format() argument after * must be a sequence, not NoneType

https://codereview.appspot.com/200460043/diff/1/plaso/formatters/winevt.py#ne...
plaso/formatters/winevt.py:109: message_string =
formatter_mediator.GetWinevtMessage(
should this also perhaps be GetWindowsEventMessage instead of hte WinEvt
shortening

https://codereview.appspot.com/200460043/diff/1/plaso/formatters/winevt.py#ne...
plaso/formatters/winevt.py:115: for string in strings:
also if strings is  None here, this will fail too

https://codereview.appspot.com/200460043/diff/1/plaso/formatters/winevt_rc.py
File plaso/formatters/winevt_rc.py (right):

https://codereview.appspot.com/200460043/diff/1/plaso/formatters/winevt_rc.py...
plaso/formatters/winevt_rc.py:10: """Class that defines a sqlite3 database
file."""
This seems like a very generic SQL database reader, should we perhaps at least
have a TODO here to move this to a more generic location if/when other parts of
the tool may need/want access to sqlite databases?

https://codereview.appspot.com/200460043/diff/1/plaso/formatters/winevt_rc.py...
plaso/formatters/winevt_rc.py:62: has_table = True
or just

if self._cursor.fetchone():
  return True

return False

and save yourself the allocation of the has_table attribute

https://codereview.appspot.com/200460043/diff/1/plaso/formatters/winevt_rc.py...
plaso/formatters/winevt_rc.py:97: yield values
why not just use the:

self._connection.row_factory = sqlite3.Row

and that way the results will be

for row in sql_results:
  row['column_name']

https://docs.python.org/2/library/sqlite3.html#sqlite3.Row

that is then I don't think you need this step... since that is already done in
the returned row

https://codereview.appspot.com/200460043/diff/1/plaso/formatters/winevtx.py
File plaso/formatters/winevtx.py (right):

https://codereview.appspot.com/200460043/diff/1/plaso/formatters/winevtx.py#n...
plaso/formatters/winevtx.py:53: strings = event_values.get(u'strings', None)
same comment as before, that ist he default value being []

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

https://codereview.appspot.com/200460043/diff/1/plaso/lib/event.py#newcode256
plaso/lib/event.py:256: timelib.Timestamp.CopyToIsoFormat(self.timestamp)))
why do you want to remove the calculated message string out of the unicode
function?

[Joachim Metz, 2015-02-24 07:07:49]

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

https://codereview.appspot.com/200460043/diff/1/plaso/formatters/mediator.py#...
plaso/formatters/mediator.py:15: _WINEVT_RC_DATABASE = u'winevt-rc.db'
On 2015/02/23 23:11:29, kiddi wrote:
> do we want to keep all these parser specific events at the top level mediator?
> 
> ... at least something to keep in mind, I don't mind this now, and I'm not
sure
> how much more we'll add, but if we do, then we may need to rethink this
> approach,
> 
> so, this is food for thoughts, but fine with this as is for now

> do we want to keep all these parser specific events at the top level mediator?

Not sure what you mean this is a formatting resource/system shared by 2 parsers.
If it should be merged then with other resource/system data. It is the role of
the mediator to mediate between the formatters and other parts of plaso.

https://codereview.appspot.com/200460043/diff/1/plaso/formatters/mediator.py#...
plaso/formatters/mediator.py:45: """Retrieves a specific message for a specific
Windows Event Log source.
On 2015/02/23 23:11:29, kiddi wrote:
> or Retrieves the message string for a specific Window.....

Done.

https://codereview.appspot.com/200460043/diff/1/plaso/formatters/mediator.py#...
plaso/formatters/mediator.py:58: if self.lcid != self.DEFAULT_LCID:
On 2015/02/23 23:11:29, kiddi wrote:
> can this ever come up??? I mean self.lcid is a property with only a getter
,not
> a setter and it returns self.DEFAULT_LCID, so thsi should always return back
the
> default one, and thus never not be the same....
> 
> perhaps better to have self.lcid as a regular attribute to the class and not
> define it as a property? And have it being set to the default value in the
> initialization? 
> 
> or define a SETTER, but then you have to have another attribute, since youa re
> not going to overwrite a class constant

I've changed the code a bit but the idea is that lcid comes from the user
preferences, but I'm not implementing this in this CL.

https://codereview.appspot.com/200460043/diff/1/plaso/formatters/winevt.py
File plaso/formatters/winevt.py (right):

https://codereview.appspot.com/200460043/diff/1/plaso/formatters/winevt.py#ne...
plaso/formatters/winevt.py:25: u'Message string: {message_string}']
On 2015/02/23 23:11:30, kiddi wrote:
> should the message string come first?

For now I don't really mind, we can change it later.

https://codereview.appspot.com/200460043/diff/1/plaso/formatters/winevt.py#ne...
plaso/formatters/winevt.py:107: strings = event_values.get(u'strings', None)
On 2015/02/23 23:11:30, kiddi wrote:
> this should perhaps default to [], since you are doing *strings
> 
> In [1]: a = None
> 
> In [2]: 'asdf {}'.format(*a)
> ---------------------------------------------------------------------------
> TypeError                                 Traceback (most recent call last)
> <ipython-input-2-3f661634f1b1> in <module>()
> ----> 1 'asdf {}'.format(*a)
> 
> TypeError: format() argument after * must be a sequence, not NoneType

good catch

https://codereview.appspot.com/200460043/diff/1/plaso/formatters/winevt.py#ne...
plaso/formatters/winevt.py:109: message_string =
formatter_mediator.GetWinevtMessage(
On 2015/02/23 23:11:30, kiddi wrote:
> should this also perhaps be GetWindowsEventMessage instead of hte WinEvt
> shortening

Done.

https://codereview.appspot.com/200460043/diff/1/plaso/formatters/winevt.py#ne...
plaso/formatters/winevt.py:115: for string in strings:
On 2015/02/23 23:11:30, kiddi wrote:
> also if strings is  None here, this will fail too

Done.

https://codereview.appspot.com/200460043/diff/1/plaso/formatters/winevt_rc.py
File plaso/formatters/winevt_rc.py (right):

https://codereview.appspot.com/200460043/diff/1/plaso/formatters/winevt_rc.py...
plaso/formatters/winevt_rc.py:10: """Class that defines a sqlite3 database
file."""
On 2015/02/23 23:11:30, kiddi wrote:
> This seems like a very generic SQL database reader, should we perhaps at least
> have a TODO here to move this to a more generic location if/when other parts
of
> the tool may need/want access to sqlite databases?

Done.

https://codereview.appspot.com/200460043/diff/1/plaso/formatters/winevt_rc.py...
plaso/formatters/winevt_rc.py:62: has_table = True
On 2015/02/23 23:11:30, kiddi wrote:
> or just
> 
> if self._cursor.fetchone():
>   return True
> 
> return False
> 
> and save yourself the allocation of the has_table attribute

Done.

https://codereview.appspot.com/200460043/diff/1/plaso/formatters/winevt_rc.py...
plaso/formatters/winevt_rc.py:97: yield values
On 2015/02/23 23:11:30, kiddi wrote:
> why not just use the:
> 
> self._connection.row_factory = sqlite3.Row
> 
> and that way the results will be
> 
> for row in sql_results:
>   row['column_name']
> 
> https://docs.python.org/2/library/sqlite3.html#sqlite3.Row
> 
> that is then I don't think you need this step... since that is already done in
> the returned row

I'll have a look.

https://codereview.appspot.com/200460043/diff/1/plaso/formatters/winevtx.py
File plaso/formatters/winevtx.py (right):

https://codereview.appspot.com/200460043/diff/1/plaso/formatters/winevtx.py#n...
plaso/formatters/winevtx.py:53: strings = event_values.get(u'strings', None)
On 2015/02/23 23:11:30, kiddi wrote:
> same comment as before, that ist he default value being []

Done.

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

https://codereview.appspot.com/200460043/diff/1/plaso/lib/event.py#newcode256
plaso/lib/event.py:256: timelib.Timestamp.CopyToIsoFormat(self.timestamp)))
On 2015/02/23 23:11:30, kiddi wrote:
> why do you want to remove the calculated message string out of the unicode
> function? 

This should not be here. The event object does not know anything about
formatting, neither should it. The message string should be in a different part
of the code. Having the formatting here breaks "loose coupling and high
coherence" and makes things unnecessarily complex.

[kiddi, 2015-02-24 21:46:42]

don't see any new file update here, perhaps forgot to upload the latest patch
set?

[Joachim Metz, 2015-02-25 06:58:29]

Needed to change a test. CL is now synced with head.

[Joachim Metz, 2015-02-25 07:00:39]

Code updated.

[kiddi, 2015-02-27 22:25:40]

LGTM

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

https://codereview.appspot.com/200460043/diff/1/plaso/lib/event.py#newcode256
plaso/lib/event.py:256: timelib.Timestamp.CopyToIsoFormat(self.timestamp)))
On 2015/02/24 07:07:48, Joachim Metz wrote:
> On 2015/02/23 23:11:30, kiddi wrote:
> > why do you want to remove the calculated message string out of the unicode
> > function? 
> 
> This should not be here. The event object does not know anything about
> formatting, neither should it. The message string should be in a different
part
> of the code. Having the formatting here breaks "loose coupling and high
> coherence" and makes things unnecessarily complex.

Acknowledged.

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

https://codereview.appspot.com/200460043/diff/20001/plaso/formatters/mediator...
plaso/formatters/mediator.py:60: if self.lcid != self.DEFAULT_LCID:
why not just compare against self._lcid in this case? Instead of relying on the
property...

[Joachim Metz, 2015-02-28 07:10:58]

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

https://codereview.appspot.com/200460043/diff/20001/plaso/formatters/mediator...
plaso/formatters/mediator.py:60: if self.lcid != self.DEFAULT_LCID:
On 2015/02/27 22:25:40, kiddi wrote:
> why not just compare against self._lcid in this case? Instead of relying on
the
> property...

true, will change it.

[Joachim Metz, 2015-02-28 07:12:43]

Code Submitted.