sccm parser and small fix for #181

These files are missing unit tests:
 + plaso/formatters/__init__.py + plaso/frontend/presets.py + plaso/parsers/__init__.py

[kiddi, 2015-05-18 21:08:57]

First batch of comments

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

https://codereview.appspot.com/239830043/diff/1/plaso/formatters/sccm.py#newc...
plaso/formatters/sccm.py:21: SOURCE_SHORT = u'SCCM'
keep this as LOG

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm.py
File plaso/parsers/sccm.py (right):

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm.py#newcode14
plaso/parsers/sccm.py:14: DATA_TYPE = "software_management:sccm:log"
<nitpick>empty line before DATA_TYPE</nitpick>

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm.py#newcode18
plaso/parsers/sccm.py:18: """Parses the SCCM Logs."""
Parser for Windows System Center Configuration Manager (SCCM) logs.

Or something like that.

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm.py#newcode19
plaso/parsers/sccm.py:19: NAME = 'sccm'
<nitpick>empty line before NAME</nitpick>

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm.py#newcode24
plaso/parsers/sccm.py:24: _parsing_components = {
this is a constant, so it should be uppercase

_PARSING_COM....

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm.py#newcode25
plaso/parsers/sccm.py:25: 'msgLeftDelim': pyparsing.Literal(u'<![LOG['),
this is not part of the format itself, so let's keep this according to our
naming scheme...

msg_left_delimiter

(same with others)

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm.py#newcode29
plaso/parsers/sccm.py:29: 'day':
pyparsing.Regex(r'\d{1,2}').setResultsName('day'),
why not use the pyparsing constants? The TWO_DIGITS or ONE_OR_TWO_DIGTIS?

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm.py#newcode48
plaso/parsers/sccm.py:48: _parsing_components[component] = word
I opt not to do this... rather set these explicitly int he above constant.

This is a piece of code inside the class declaration, which we try to limit as
much as possible.

Also, why not use the pyparsing constants? the TWO_DIGITS or ONE_OR_TWO_DIGITS?
Can these be single digits? As in is the date always HH:MM:SS or can it be H:M:S
as in 3:1:1?

Or should it be

_PARSING_COMP...['hour'] = text_parser.PyPa...TWO_DIGITS.setResultName('hour')

or something like that

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm.py#newcode72
plaso/parsers/sccm.py:72: self.ENCODING = 'utf-8-sig'
here you are changing a class constant.... which is not what you are supposed to
do (it's a constant)

what you want is

self.encoding = u'utf-8-sig'

BUT you want to do that AFTER calling super(), otherwise it will get overwritten

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm.py#newcode73
plaso/parsers/sccm.py:73: #SCCM messages can be quite large.
space between # and start of comment.

Also could you elaborate a bit...

Increasing buffer size due to SCCM messages being longer than the default
buffer..... or something like that....

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm.py#newcode74
plaso/parsers/sccm.py:74: self.BUFFER_SIZE = 16384
same here... you are overwriting a class constant... don't do that.

what you want to do here is

self._buffer_size = 16.....

OR change the actual constant in the parser definition, and not overload the
__init__ function at all... you can just do

BUFFER_SIZE = 16...
ENCODING = u'utf-8...

in the class declaration and not do the __init__ function at all.

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm.py#newcode78
plaso/parsers/sccm.py:78: """Ensure the file is a log we want to parse."""
could you fix this docstring a bit

also missing Args: section

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm.py#newcode92
plaso/parsers/sccm.py:92: #remove the microsecond dash or plus if it is present
space between # and start of comment, also start with capital letter and end
with a dot

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm.py#newcode94
plaso/parsers/sccm.py:94: structure.microsecond =
structure.microsecond.replace('+', '')
why is this needed? perhaps we need to elaborate on that in the comment

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm.py#newcode96
plaso/parsers/sccm.py:96: # sometimes, additional precision is specified
same comment about a comment (and could you elaborate?)

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm.py#newcode100
plaso/parsers/sccm.py:100: #month should be two digits
comment about a comment

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm.py#newcode108
plaso/parsers/sccm.py:108: timestamp =
timelib.Timestamp.FromTimeParts(int(structure.year),
rather have the indentation

timestamp = timelib.....Parts(
    int(structure.year), int(structure.month)......

Also... I'm not sure why you are doing all these castings, could you not do that
in pyparsing? And also why doing the check of day and month, you check if day or
month is a single digit or double, then you add the 0 in front of it only to
cast it to an integer two lines later... making it completely unnecessary.

And in addition, when you do int casting, please specify the base, as in

int(string, 10)

And havfe it surrounded with a try/except

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm.py#newcode116
plaso/parsers/sccm.py:116: return  SCCMLogEvent(timestamp, 0, structure)
aren't there two spaces after return?

and why set the offset to always zero? this is a text file, you should be able
to read the offset...

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm_test.py
File plaso/parsers/sccm_test.py (right):

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm_test.py#ne...
plaso/parsers/sccm_test.py:21: # Test that 10 messages are parsed.
no need for this comment

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm_test.py#ne...
plaso/parsers/sccm_test.py:24: #Test timestamps with six digits after seconds.
no need for this comment

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm_test.py#ne...
plaso/parsers/sccm_test.py:29: #Test timestamps with seven digits after seconds.
space between # and start (same below)

[buia, 2015-06-06 07:11:57]

Addressed comments.  Prepping for update.

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

https://codereview.appspot.com/239830043/diff/1/plaso/formatters/sccm.py#newc...
plaso/formatters/sccm.py:21: SOURCE_SHORT = u'SCCM'
On 2015/05/18 21:08:55, kiddi wrote:
> keep this as LOG

Done.

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm.py
File plaso/parsers/sccm.py (right):

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm.py#newcode14
plaso/parsers/sccm.py:14: DATA_TYPE = "software_management:sccm:log"
On 2015/05/18 21:08:56, kiddi wrote:
> <nitpick>empty line before DATA_TYPE</nitpick>

Done.

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm.py#newcode18
plaso/parsers/sccm.py:18: """Parses the SCCM Logs."""
On 2015/05/18 21:08:56, kiddi wrote:
> Parser for Windows System Center Configuration Manager (SCCM) logs.
> 
> Or something like that.

Done.

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm.py#newcode19
plaso/parsers/sccm.py:19: NAME = 'sccm'
On 2015/05/18 21:08:56, kiddi wrote:
> <nitpick>empty line before NAME</nitpick>

Done.

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm.py#newcode24
plaso/parsers/sccm.py:24: _parsing_components = {
On 2015/05/18 21:08:56, kiddi wrote:
> this is a constant, so it should be uppercase
> 
> _PARSING_COM....

Done.

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm.py#newcode25
plaso/parsers/sccm.py:25: 'msgLeftDelim': pyparsing.Literal(u'<![LOG['),
On 2015/05/18 21:08:57, kiddi wrote:
> this is not part of the format itself, so let's keep this according to our
> naming scheme...
> 
> msg_left_delimiter
> 
> (same with others)

Done.

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm.py#newcode29
plaso/parsers/sccm.py:29: 'day':
pyparsing.Regex(r'\d{1,2}').setResultsName('day'),
On 2015/05/18 21:08:56, kiddi wrote:
> why not use the pyparsing constants? The TWO_DIGITS or ONE_OR_TWO_DIGTIS?

Done.

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm.py#newcode48
plaso/parsers/sccm.py:48: _parsing_components[component] = word
On 2015/05/18 21:08:56, kiddi wrote:
> I opt not to do this... rather set these explicitly int he above constant.
> 
> This is a piece of code inside the class declaration, which we try to limit as
> much as possible.
> 
> Also, why not use the pyparsing constants? the TWO_DIGITS or
ONE_OR_TWO_DIGITS?
> Can these be single digits? As in is the date always HH:MM:SS or can it be
H:M:S
> as in 3:1:1?
> 
> Or should it be
> 
> _PARSING_COMP...['hour'] = text_parser.PyPa...TWO_DIGITS.setResultName('hour')
> 
> or something like that

Done. Set explicitly as suggested with the appropriate constant.

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm.py#newcode72
plaso/parsers/sccm.py:72: self.ENCODING = 'utf-8-sig'
On 2015/05/18 21:08:56, kiddi wrote:
> here you are changing a class constant.... which is not what you are supposed
to
> do (it's a constant)
> 
> what you want is
> 
> self.encoding = u'utf-8-sig'
> 
> BUT you want to do that AFTER calling super(), otherwise it will get
overwritten

Updated the super class constructor to take optional parameters (default to
constants) for encoding and buffer_size, as these are ultimately private members
of the _text_reader member.

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm.py#newcode73
plaso/parsers/sccm.py:73: #SCCM messages can be quite large.
On 2015/05/18 21:08:57, kiddi wrote:
> space between # and start of comment.
> 
> Also could you elaborate a bit...
> 
> Increasing buffer size due to SCCM messages being longer than the default
> buffer..... or something like that....

Done.

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm.py#newcode74
plaso/parsers/sccm.py:74: self.BUFFER_SIZE = 16384
On 2015/05/18 21:08:57, kiddi wrote:
> same here... you are overwriting a class constant... don't do that.
> 
> what you want to do here is
> 
> self._buffer_size = 16.....
> 
> OR change the actual constant in the parser definition, and not overload the
> __init__ function at all... you can just do
> 
> BUFFER_SIZE = 16...
> ENCODING = u'utf-8...
> 
> in the class declaration and not do the __init__ function at all.

As stated above, updated super class constructor - i don't think updating the
constants or private members would make sense because they are passed to the
constructor of the _text_reader member, during the constructor of the super
class.

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm.py#newcode78
plaso/parsers/sccm.py:78: """Ensure the file is a log we want to parse."""
On 2015/05/18 21:08:56, kiddi wrote:
> could you fix this docstring a bit
> 
> also missing Args: section

Done.

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm.py#newcode92
plaso/parsers/sccm.py:92: #remove the microsecond dash or plus if it is present
On 2015/05/18 21:08:56, kiddi wrote:
> space between # and start of comment, also start with capital letter and end
> with a dot

Done.

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm.py#newcode94
plaso/parsers/sccm.py:94: structure.microsecond =
structure.microsecond.replace('+', '')
On 2015/05/18 21:08:56, kiddi wrote:
> why is this needed? perhaps we need to elaborate on that in the comment

Done.

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm.py#newcode96
plaso/parsers/sccm.py:96: # sometimes, additional precision is specified
On 2015/05/18 21:08:56, kiddi wrote:
> same comment about a comment (and could you elaborate?)

Done.

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm.py#newcode100
plaso/parsers/sccm.py:100: #month should be two digits
On 2015/05/18 21:08:56, kiddi wrote:
> comment about a comment

Acknowledged.

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm.py#newcode108
plaso/parsers/sccm.py:108: timestamp =
timelib.Timestamp.FromTimeParts(int(structure.year),
On 2015/05/18 21:08:56, kiddi wrote:
> rather have the indentation
> 
> timestamp = timelib.....Parts(
>     int(structure.year), int(structure.month)......
> 
> Also... I'm not sure why you are doing all these castings, could you not do
that
> in pyparsing? And also why doing the check of day and month, you check if day
or
> month is a single digit or double, then you add the 0 in front of it only to
> cast it to an integer two lines later... making it completely unnecessary.
> 
> And in addition, when you do int casting, please specify the base, as in
> 
> int(string, 10)
> 
> And havfe it surrounded with a try/except

Yeah...checking the number of digits makes no sense here, that was carry over
from my own experimental stand-alone parser which was string-based, not
int-based. 

Removing unneeded castings.

wrapped it with a try/except - just not sure if the logic I supplied is keeping
with the spirit of how we handle these issues.

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm.py#newcode116
plaso/parsers/sccm.py:116: return  SCCMLogEvent(timestamp, 0, structure)
On 2015/05/18 21:08:56, kiddi wrote:
> aren't there two spaces after return?
> 
> and why set the offset to always zero? this is a text file, you should be able
> to read the offset...

The reason I am setting this to zero is that it appears that the superclass
method that calls this method (ParseRecord) has TODO code for setting the event
offset, probably because the offset is maintained by the private _text_reader
object.  So, setting it to 0 because I am making an assumption that the
superclass code will be fixed as a TODO.  

Should I open an issue for the TODO?

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm_test.py
File plaso/parsers/sccm_test.py (right):

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm_test.py#ne...
plaso/parsers/sccm_test.py:21: # Test that 10 messages are parsed.
On 2015/05/18 21:08:57, kiddi wrote:
> no need for this comment

Done.

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm_test.py#ne...
plaso/parsers/sccm_test.py:24: #Test timestamps with six digits after seconds.
On 2015/05/18 21:08:57, kiddi wrote:
> no need for this comment

Done.

https://codereview.appspot.com/239830043/diff/1/plaso/parsers/sccm_test.py#ne...
plaso/parsers/sccm_test.py:29: #Test timestamps with seven digits after seconds.
On 2015/05/18 21:08:57, kiddi wrote:
> space between # and start (same below)

Done.

[buia, 2015-06-06 07:14:36]

Code updated.

[kiddi, 2015-06-19 00:15:52]

Few more comments

https://codereview.appspot.com/239830043/diff/20001/plaso/parsers/sccm.py
File plaso/parsers/sccm.py (right):

https://codereview.appspot.com/239830043/diff/20001/plaso/parsers/sccm.py#new...
plaso/parsers/sccm.py:88: buffer_size=16384)
but why are you doing this? AS in you already changed the value of ENCODING and
BUFFER_SIZE by changing the constants... why add this to the super class and
pass these values here?

we can discuss this further if you want, but this is completely unnecessary
since you already overwrite the class constants

https://codereview.appspot.com/239830043/diff/20001/plaso/parsers/sccm.py#new...
plaso/parsers/sccm.py:129: int_microsecond = int(structure.microsecond)
explicit base, (int..., 10)

and why not just microsecond, instead of int_microsecond... you never use that
elsewhere

https://codereview.appspot.com/239830043/diff/20001/plaso/parsers/sccm.py#new...
plaso/parsers/sccm.py:131: logging.warning(u'unable to interpret microseconds:
{0:s}'.
start with capital

u'Unable to....

and do we want this to be a warning message? Or is this not rather a parser
error? and you want to produce a parser error through the mediator?

https://codereview.appspot.com/239830043/diff/20001/plaso/parsers/sccm.py#new...
plaso/parsers/sccm.py:136: timestamp =
timelib.Timestamp.FromTimeParts(structure.year,
let's have the indent

timelibe....Parts(
    structure..., structu....)

https://codereview.appspot.com/239830043/diff/20001/plaso/parsers/text_parser.py
File plaso/parsers/text_parser.py (right):

https://codereview.appspot.com/239830043/diff/20001/plaso/parsers/text_parser...
plaso/parsers/text_parser.py:969: if encoding == None:
I don't agree with these changes here... 

I mean the implementation of the class will overwrite the class constants and
thus changes these values... why adding it here too?

https://codereview.appspot.com/239830043/diff/20001/tests/formatters/sccm.py
File tests/formatters/sccm.py (right):

https://codereview.appspot.com/239830043/diff/20001/tests/formatters/sccm.py#...
tests/formatters/sccm.py:12: """Tests for the SkyDrive log event formatter."""
skydrive?

[buia, 2015-07-05 20:42:27]

Reverted the wonky stuff with passing encoding and buffer size to the multiline
text reader superclass as we discussed.  Addressed the other minor issues.

https://codereview.appspot.com/239830043/diff/20001/plaso/parsers/sccm.py
File plaso/parsers/sccm.py (right):

https://codereview.appspot.com/239830043/diff/20001/plaso/parsers/sccm.py#new...
plaso/parsers/sccm.py:88: buffer_size=16384)
On 2015/06/19 00:15:52, kiddi wrote:
> but why are you doing this? AS in you already changed the value of ENCODING
and
> BUFFER_SIZE by changing the constants... why add this to the super class and
> pass these values here?
> 
> we can discuss this further if you want, but this is completely unnecessary
> since you already overwrite the class constants

I'm in agreement, not worth getting into my flawed thought process here...

https://codereview.appspot.com/239830043/diff/20001/plaso/parsers/sccm.py#new...
plaso/parsers/sccm.py:129: int_microsecond = int(structure.microsecond)
On 2015/06/19 00:15:52, kiddi wrote:
> explicit base, (int..., 10)
> 
> and why not just microsecond, instead of int_microsecond... you never use that
> elsewhere

Done.

https://codereview.appspot.com/239830043/diff/20001/plaso/parsers/sccm.py#new...
plaso/parsers/sccm.py:131: logging.warning(u'unable to interpret microseconds:
{0:s}'.
On 2015/06/19 00:15:52, kiddi wrote:
> start with capital
> 
> u'Unable to....
> 
> and do we want this to be a warning message? Or is this not rather a parser
> error? and you want to produce a parser error through the mediator?

Done.

https://codereview.appspot.com/239830043/diff/20001/plaso/parsers/sccm.py#new...
plaso/parsers/sccm.py:136: timestamp =
timelib.Timestamp.FromTimeParts(structure.year,
On 2015/06/19 00:15:52, kiddi wrote:
> let's have the indent
> 
> timelibe....Parts(
>     structure..., structu....)

Done.

https://codereview.appspot.com/239830043/diff/20001/plaso/parsers/text_parser.py
File plaso/parsers/text_parser.py (right):

https://codereview.appspot.com/239830043/diff/20001/plaso/parsers/text_parser...
plaso/parsers/text_parser.py:969: if encoding == None:
On 2015/06/19 00:15:51, kiddi wrote:
> I don't agree with these changes here... 
> 
> I mean the implementation of the class will overwrite the class constants and
> thus changes these values... why adding it here too?

Acknowledged and agree.  Fixed.

https://codereview.appspot.com/239830043/diff/20001/tests/formatters/sccm.py
File tests/formatters/sccm.py (right):

https://codereview.appspot.com/239830043/diff/20001/tests/formatters/sccm.py#...
tests/formatters/sccm.py:12: """Tests for the SkyDrive log event formatter."""
On 2015/06/19 00:15:52, kiddi wrote:
> skydrive?

Done.

[buia, 2015-07-05 20:50:35]

Code updated.

[kiddi, 2015-07-31 20:50:54]

Minor comments, then I'll do a final review, but it should hopefully be mostly
there

https://codereview.appspot.com/239830043/diff/40001/plaso/parsers/sccm.py
File plaso/parsers/sccm.py (right):

https://codereview.appspot.com/239830043/diff/40001/plaso/parsers/sccm.py#new...
plaso/parsers/sccm.py:136: structure.minute, structure.second, microsecond)
#pylint: disable=bad-continuation
this is all weird ;)

timestamp = timelib.Timestamp.FromTimeParts(
    structure.year, structure.month, ....,
    structure.min......)

that looks nicer, doesn't go over the 80 char lines and no need to do pylint
overrides

https://codereview.appspot.com/239830043/diff/40001/tests/parsers/sccm.py
File tests/parsers/sccm.py (right):

https://codereview.appspot.com/239830043/diff/40001/tests/parsers/sccm.py#new...
tests/parsers/sccm.py:54: 
two empty lines here

[buia, 2015-09-26 02:18:18]

Addressed the two minor fixes. Also had to make some slight changes to the code
given some of the recent refactoring (plaso/parsers/sccm.py)

https://codereview.appspot.com/239830043/diff/40001/plaso/parsers/sccm.py
File plaso/parsers/sccm.py (right):

https://codereview.appspot.com/239830043/diff/40001/plaso/parsers/sccm.py#new...
plaso/parsers/sccm.py:136: structure.minute, structure.second, microsecond)
#pylint: disable=bad-continuation
On 2015/07/31 20:50:54, kiddi wrote:
> this is all weird ;)
> 
> timestamp = timelib.Timestamp.FromTimeParts(
>     structure.year, structure.month, ....,
>     structure.min......)
> 
> that looks nicer, doesn't go over the 80 char lines and no need to do pylint
> overrides

Done.

https://codereview.appspot.com/239830043/diff/40001/tests/parsers/sccm.py
File tests/parsers/sccm.py (right):

https://codereview.appspot.com/239830043/diff/40001/tests/parsers/sccm.py#new...
tests/parsers/sccm.py:54: 
On 2015/07/31 20:50:54, kiddi wrote:
> two empty lines here

Done.

[buia, 2015-09-26 02:18:20]

Addressed the two minor fixes. Also had to make some slight changes to the code
given some of the recent refactoring (plaso/parsers/sccm.py)