Issue 13078043: added basic event-type pairing
|
Created:
10 years, 8 months ago by Oliver Modified:
8 years, 9 months ago CC:
log2timeline-dev_googlegroups.com Visibility:
Public. |
Description.
Patch Set 1 #
Total comments: 17
Patch Set 2 : . #
Total comments: 14
MessagesTotal messages: 8
First small batch of comments. https://codereview.appspot.com/13078043/diff/1/plaso/frontend/plasm.py File plaso/frontend/plasm.py (right): https://codereview.appspot.com/13078043/diff/1/plaso/frontend/plasm.py#newcod... plaso/frontend/plasm.py:313: timestamp = 0 is this a class constant? Then it should be uppercase, if this is not, then I opt to define this as "self.timestamp" inside the init function. https://codereview.appspot.com/13078043/diff/1/plaso/frontend/plasm.py#newcod... plaso/frontend/plasm.py:370: @staticmethod any particular reason you like this to be static? https://codereview.appspot.com/13078043/diff/1/plaso/frontend/plasm.py#newcod... plaso/frontend/plasm.py:384: @staticmethod same here. https://codereview.appspot.com/13078043/diff/1/plaso/frontend/plasm.py#newcod... plaso/frontend/plasm.py:630: # clean up memory a little comment start with uppercase and end with dot (same applies everywhere) https://codereview.appspot.com/13078043/diff/1/plaso/frontend/plasm.py#newcod... plaso/frontend/plasm.py:642: pickle.dump(evttypes, x) write a todo to not use pickle https://codereview.appspot.com/13078043/diff/1/plaso/frontend/plasm.py#newcod... plaso/frontend/plasm.py:674: self.plaso_hash, str(threshold), str(closeness)) closeness - is that always an ASCII string? (otherwise this will fail) https://codereview.appspot.com/13078043/diff/1/plaso/frontend/plasm.py#newcod... plaso/frontend/plasm.py:677: x = open(pairs_filename, 'rb') x is not a very good attribute name, nor descriptive. https://codereview.appspot.com/13078043/diff/1/plaso/frontend/plasm.py#newcod... plaso/frontend/plasm.py:688: # remove anything "too old" from the buffer same here, start with uppercase and end with a dot.
Sign in to reply to this message.
https://codereview.appspot.com/13078043/diff/1/plaso/frontend/plasm.py File plaso/frontend/plasm.py (right): https://codereview.appspot.com/13078043/diff/1/plaso/frontend/plasm.py#newcod... plaso/frontend/plasm.py:313: timestamp = 0 On 2013/08/16 23:09:46, kiddi wrote: > is this a class constant? Then it should be uppercase, if this is not, then I > opt to define this as "self.timestamp" inside the init function. Done. https://codereview.appspot.com/13078043/diff/1/plaso/frontend/plasm.py#newcod... plaso/frontend/plasm.py:370: @staticmethod On 2013/08/16 23:09:46, kiddi wrote: > any particular reason you like this to be static? It does not interact with any data attached to the class - it just outputs a representation of its input (much like HashFile, StringJoin, etc.). https://codereview.appspot.com/13078043/diff/1/plaso/frontend/plasm.py#newcod... plaso/frontend/plasm.py:384: @staticmethod On 2013/08/16 23:09:46, kiddi wrote: > same here. same as above https://codereview.appspot.com/13078043/diff/1/plaso/frontend/plasm.py#newcod... plaso/frontend/plasm.py:630: # clean up memory a little On 2013/08/16 23:09:46, kiddi wrote: > comment start with uppercase and end with dot (same applies everywhere) Done. https://codereview.appspot.com/13078043/diff/1/plaso/frontend/plasm.py#newcod... plaso/frontend/plasm.py:642: pickle.dump(evttypes, x) On 2013/08/16 23:09:46, kiddi wrote: > write a todo to not use pickle It's on line 530 (the first time I use pickle). https://codereview.appspot.com/13078043/diff/1/plaso/frontend/plasm.py#newcod... plaso/frontend/plasm.py:674: self.plaso_hash, str(threshold), str(closeness)) On 2013/08/16 23:09:46, kiddi wrote: > closeness - is that always an ASCII string? (otherwise this will fail) It's always an integer (see line 742), so str() should be fine. https://codereview.appspot.com/13078043/diff/1/plaso/frontend/plasm.py#newcod... plaso/frontend/plasm.py:677: x = open(pairs_filename, 'rb') On 2013/08/16 23:09:46, kiddi wrote: > x is not a very good attribute name, nor descriptive. Done. https://codereview.appspot.com/13078043/diff/1/plaso/frontend/plasm.py#newcod... plaso/frontend/plasm.py:688: # remove anything "too old" from the buffer On 2013/08/16 23:09:46, kiddi wrote: > same here, start with uppercase and end with a dot. Done.
Sign in to reply to this message.
Few more comments. https://codereview.appspot.com/13078043/diff/1/plaso/frontend/plasm.py File plaso/frontend/plasm.py (right): https://codereview.appspot.com/13078043/diff/1/plaso/frontend/plasm.py#newcod... plaso/frontend/plasm.py:370: @staticmethod ack On 2013/08/16 23:56:50, Oliver wrote: > On 2013/08/16 23:09:46, kiddi wrote: > > any particular reason you like this to be static? > > It does not interact with any data attached to the class - it just outputs a > representation of its input (much like HashFile, StringJoin, etc.). https://codereview.appspot.com/13078043/diff/6001/plaso/frontend/plasm.py File plaso/frontend/plasm.py (right): https://codereview.appspot.com/13078043/diff/6001/plaso/frontend/plasm.py#new... plaso/frontend/plasm.py:334: timestamp = 0 shouldn't this be a self.timestamp? otherwise the scope of it is local and not accessible outside of __init__ https://codereview.appspot.com/13078043/diff/6001/plaso/frontend/plasm.py#new... plaso/frontend/plasm.py:366: second: second string. missing a Returns section https://codereview.appspot.com/13078043/diff/6001/plaso/frontend/plasm.py#new... plaso/frontend/plasm.py:380: dictionary: a python dictionary missing a dot in the end, and missing a Returns section. https://codereview.appspot.com/13078043/diff/6001/plaso/frontend/plasm.py#new... plaso/frontend/plasm.py:394: attribute: the corresponding event_object attribute. missing a returns section https://codereview.appspot.com/13078043/diff/6001/plaso/frontend/plasm.py#new... plaso/frontend/plasm.py:465: update_timestamp: (optional) updates classwide timestamp for each event missing a Yields section https://codereview.appspot.com/13078043/diff/6001/plaso/frontend/plasm.py#new... plaso/frontend/plasm.py:486: dump_filename: the filename of the Plaso Storage to be deduped. missing Returns section https://codereview.appspot.com/13078043/diff/6001/plaso/frontend/plasm.py#new... plaso/frontend/plasm.py:526: vector_size: size of this vector. missing a REturns section https://codereview.appspot.com/13078043/diff/6001/plaso/frontend/plasm.py#new... plaso/frontend/plasm.py:537: vector = pickle.load(var_dump) should we have some sort of checks here to verify the object, so that we are not loading up something potentially malicious? (same applies everywhere pickle is used). What I'm thinking is perhaps a simple method that takes in the filename, and what it should return (int, dict, EventObject, etc) and then verify that the unpickled object is of that type? To have at least some minimum checks.... not perfect by a long shot, but perhaps at least better than nothing at all... (although such checks can be easily fooled so perhaps that is almost not doing anything for us) https://codereview.appspot.com/13078043/diff/6001/plaso/frontend/plasm.py#new... plaso/frontend/plasm.py:563: vector: (optional) vector of hash tallies. missing Returns section https://codereview.appspot.com/13078043/diff/6001/plaso/frontend/plasm.py#new... plaso/frontend/plasm.py:608: frequent_words: the set of attributes not to ignore. missing Returns section https://codereview.appspot.com/13078043/diff/6001/plaso/frontend/plasm.py#new... plaso/frontend/plasm.py:683: association_window = closeness * 1000 why this multiple? What's behind this number 1000? Perhaps add a comment here. https://codereview.appspot.com/13078043/diff/6001/plaso/frontend/plasm.py#new... plaso/frontend/plasm.py:688: # Remove anything "too old" from the buffer. what is the definition of "too old"? https://codereview.appspot.com/13078043/diff/6001/plaso/frontend/plasm.py#new... plaso/frontend/plasm.py:708: pickle.dump(pairs, var_dump) I'm also wondering if we should "abstract" all these pickle calls so that we don't really expose pickle everywhere in the code. That way we could perhpas easily add more checks, swap out pickle for something else without making changes everywhere in the code. as in ... "Serialize(pairs, var_dump)" and then "DeSerialize(...)" or something like that.... could be simple methods in the file, no need to go for a full class/interface implementation. https://codereview.appspot.com/13078043/diff/6001/plaso/frontend/plasm.py#new... plaso/frontend/plasm.py:713: """Iterates through a tagged Plaso Store file, attempting to cluster events the first line should end with a dot and be no longer than 80 char. Then a space and then the full text.
Sign in to reply to this message.
Oliver is this CL going anywhere?
Sign in to reply to this message.
|
