Issue 246490043: Issue #236: Step two of preg refactoring. (Closed)
|
Created:
9 years, 4 months ago by kiddi Modified:
9 years, 4 months ago Reviewers:
Joachim Metz CC:
log2timeline-dev_googlegroups.com Visibility:
Public. |
DescriptionIssue #236: Step two of preg refactoring.
Patch Set 1 #
Total comments: 75
Patch Set 2 : Uploading changes made to code. #Patch Set 3 : Uploading changes made to code. #
Total comments: 70
Patch Set 4 : Uploading changes made to code. #
Total comments: 22
Patch Set 5 : Uploading changes made to code. #Patch Set 6 : Submitted. #
MessagesTotal messages: 14
This is an early stage "preview" of the CL. Removed all tests since they have to be rewritten from almost scratch. Next steps: + Clean up the code a bit + Clean up all TODO's, especially those that are marked as "HERNA" "FIXME" and "ADDME" (this is a very much WIP CL) + Rewrite all the tests But this should give you an early peak at what is happening.
Sign in to reply to this message.
Initial nits https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py File plaso/frontend/preg.py (right): https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcode59 plaso/frontend/preg.py:59: """Yields consumed event objects.""" Make this docstring similar to others: Retrieves the consumed event object. Yields: Event objects (instances of ...) https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcode68 plaso/frontend/preg.py:68: if self._event_objects: if not self._event_objects: break and drop the else construction https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcode92 plaso/frontend/preg.py:92: """The Windows Registry plugin list.""" + (instance of PluginList) https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:172: event_queue: An event queue object (instance of Queue). This is Stick with the overall docstring format: Optional ... The default is None, which ... https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:219: BadConfigOption: If the source scanner is unable to complete due to alpha ordering ? https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:240: # Create the keyword list if plugins are used. move this to a separate method, if you need a comment to explain a codeblock it often is a good candidate for a separate method. https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:283: """ADDME HERNA.""" reviewer cannot parse ;) https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:284: if not self._image_path: restructure this function a bit to make it more clear what you are doing. https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:302: collector_name = u'VSS Store: {0:d}'.format(getattr( I opt to use: x = gettattr( ... y = '{0:s}'.format(x) code blocks inside other code blocks are harder to read, and thus bad practice https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:399: hive_helper.Close() does this close require a try - finally ? https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:468: verbose: include more verbose content, like information about sub keys. think about the docstring convention here optional ... https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:490: return return_dict return {} https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:493: return return_dict return {} https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:523: registry_helper: a Registry file object (instance of PregRegistryHelper). file object and instance of helper ? https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:524: use_plugins: a list of plugin names to use, or none if all should be used. optional ... https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:547: for weight in plugins_list.GetWeights(): move to separate method? https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:562: # Build a parser mediator. comment adds no value. https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:567: for weight in plugins: move to separate method? https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:584: def SetImagePath(self, image_path): I opt to use "source" for storage media image and single file input. https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:632: path_spec = getattr(self.file_entry, 'path_spec', None) u' or b' (repeat everywhere) https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:686: """Reset all attributes of the Registry helper.""" Reset => Initialize Also don't abbreviate "Init", this is bad readability practice. https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:695: """Detect and set the hive type.""" Set or detect? I opt that DetectHiveType is a more appropriate name if the method does more than just set a class attribute. https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:714: """Closes the Preg Registry helper.""" no need to mention Preg that is clear from the class name. https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:769: # Initialize the hive to the root key. Do you need the root key or the base key ? https://codereview.appspot.com/246490043/diff/1/tools/preg.py File tools/preg.py (right): https://codereview.appspot.com/246490043/diff/1/tools/preg.py#newcode136 tools/preg.py:136: align_length = 15 shouldn't this be a class constant? https://codereview.appspot.com/246490043/diff/1/tools/preg.py#newcode144 tools/preg.py:144: attributes = event_object.GetAttributes().difference( don't combine multiple calls, harder to debug and bad readability practice https://codereview.appspot.com/246490043/diff/1/tools/preg.py#newcode156 tools/preg.py:156: def _GetTSKPartitionIdentifiers( re-use storage media tool? https://codereview.appspot.com/246490043/diff/1/tools/preg.py#newcode434 tools/preg.py:434: event_objects_and_timestamps = {} move part of this method to sub methods, it is currently hard to read. https://codereview.appspot.com/246490043/diff/1/tools/preg.py#newcode474 tools/preg.py:474: - white line https://codereview.appspot.com/246490043/diff/1/tools/preg.py#newcode660 tools/preg.py:660: self._front_end.SetImagePath(image) why have duplicate code paths for file and image ? get a path spec and be done with it. https://codereview.appspot.com/246490043/diff/1/tools/preg.py#newcode709 tools/preg.py:709: # Scan the source. comment adds no value https://codereview.appspot.com/246490043/diff/1/tools/preg.py#newcode851 tools/preg.py:851: if key.startswith(u'\\'): move this to a class constant ? e.g. REGISTRY_KEY_PATH_SEPARATOR https://codereview.appspot.com/246490043/diff/1/tools/preg.py#newcode1172 tools/preg.py:1172: # We need to extract the front-end object from the tool. ? is this a TODO ? if not IMO use the tool object here not the front end https://codereview.appspot.com/246490043/diff/1/tools/preg.py#newcode1275 tools/preg.py:1275: """Prints the initial banner.""" - initial https://codereview.appspot.com/246490043/diff/1/tools/preg.py#newcode1324 tools/preg.py:1324: if index >= len(self._registry_helpers) or index < 0: start with checking the lower bound, then check the upper bound https://codereview.appspot.com/246490043/diff/1/tools/preg.py#newcode1359 tools/preg.py:1359: hive_path: The hive name or path as a string, this is optional name or optional ... https://codereview.appspot.com/246490043/diff/1/tools/preg.py#newcode1374 tools/preg.py:1374: r'[{color.LightBlue}\T{color.Normal}]', why use r' here ? https://codereview.appspot.com/246490043/diff/1/tools/preg.py#newcode1459 tools/preg.py:1459: console: an IPython shell object (instance of InteractiveShellEmbed). console => shell_object ?
Sign in to reply to this message.
Done, this CL should now be ready for review. I've added all the tests in again, wrote some new tests. We still need to add some tests here, especially testing storage media parsing. But this should be ready for a review round. https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py File plaso/frontend/preg.py (right): https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcode59 plaso/frontend/preg.py:59: """Yields consumed event objects.""" On 2015/06/25 11:52:27, Joachim Metz wrote: > Make this docstring similar to others: > > Retrieves the consumed event object. > > Yields: > Event objects (instances of ...) Acknowledged. https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcode68 plaso/frontend/preg.py:68: if self._event_objects: On 2015/06/25 11:52:26, Joachim Metz wrote: > if not self._event_objects: > break > > and drop the else construction Done. https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcode92 plaso/frontend/preg.py:92: """The Windows Registry plugin list.""" On 2015/06/25 11:52:27, Joachim Metz wrote: > + (instance of PluginList) Done. https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:172: event_queue: An event queue object (instance of Queue). This is On 2015/06/25 11:52:27, Joachim Metz wrote: > Stick with the overall docstring format: > > Optional ... The default is None, which ... old remnants.... https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:219: BadConfigOption: If the source scanner is unable to complete due to On 2015/06/25 11:52:26, Joachim Metz wrote: > alpha ordering ? Done... the badconfigoption was an old remnant... shouldn't be raised in the front-end anyway https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:283: """ADDME HERNA.""" On 2015/06/25 11:52:26, Joachim Metz wrote: > reviewer cannot parse ;) why not.... http://www.101languages.net/icelandic/ no more excuses https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:284: if not self._image_path: On 2015/06/25 11:52:27, Joachim Metz wrote: > restructure this function a bit to make it more clear what you are doing. this function was part of the todo list to change.... so changed (hence the HERNA marking) https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:302: collector_name = u'VSS Store: {0:d}'.format(getattr( On 2015/06/25 11:52:27, Joachim Metz wrote: > I opt to use: > > x = gettattr( ... > y = '{0:s}'.format(x) > > code blocks inside other code blocks are harder to read, and thus bad practice Done. https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:399: hive_helper.Close() On 2015/06/25 11:52:27, Joachim Metz wrote: > does this close require a try - finally ? maybe better... changed https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:468: verbose: include more verbose content, like information about sub keys. On 2015/06/25 11:52:26, Joachim Metz wrote: > think about the docstring convention here > > optional ... Done. https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:490: return return_dict On 2015/06/25 11:52:27, Joachim Metz wrote: > return {} Done. https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:493: return return_dict On 2015/06/25 11:52:26, Joachim Metz wrote: > return {} Done. https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:523: registry_helper: a Registry file object (instance of PregRegistryHelper). On 2015/06/25 11:52:26, Joachim Metz wrote: > file object and instance of helper ? Done. https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:524: use_plugins: a list of plugin names to use, or none if all should be used. On 2015/06/25 11:52:27, Joachim Metz wrote: > optional ... Done. https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:547: for weight in plugins_list.GetWeights(): On 2015/06/25 11:52:27, Joachim Metz wrote: > move to separate method? I don't know... this is not that complex... perhaps just rather remove the comment? https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:562: # Build a parser mediator. On 2015/06/25 11:52:26, Joachim Metz wrote: > comment adds no value. Done. https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:567: for weight in plugins: On 2015/06/25 11:52:27, Joachim Metz wrote: > move to separate method? don't think so, removed the comment... no need for that there. https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:584: def SetImagePath(self, image_path): On 2015/06/25 11:52:26, Joachim Metz wrote: > I opt to use "source" for storage media image and single file input. Done. https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:632: path_spec = getattr(self.file_entry, 'path_spec', None) On 2015/06/25 11:52:26, Joachim Metz wrote: > u' or b' (repeat everywhere) Done. https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:686: """Reset all attributes of the Registry helper.""" On 2015/06/25 11:52:27, Joachim Metz wrote: > Reset => Initialize > > Also don't abbreviate "Init", this is bad readability practice. Done. https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:695: """Detect and set the hive type.""" On 2015/06/25 11:52:26, Joachim Metz wrote: > Set or detect? I opt that DetectHiveType is a more appropriate name if the > method does more than just set a class attribute. Done. https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:714: """Closes the Preg Registry helper.""" On 2015/06/25 11:52:26, Joachim Metz wrote: > no need to mention Preg that is clear from the class name. Done. https://codereview.appspot.com/246490043/diff/1/plaso/frontend/preg.py#newcod... plaso/frontend/preg.py:769: # Initialize the hive to the root key. On 2015/06/25 11:52:27, Joachim Metz wrote: > Do you need the root key or the base key ? base https://codereview.appspot.com/246490043/diff/1/tools/preg.py File tools/preg.py (right): https://codereview.appspot.com/246490043/diff/1/tools/preg.py#newcode136 tools/preg.py:136: align_length = 15 On 2015/06/25 11:52:27, Joachim Metz wrote: > shouldn't this be a class constant? it definitely could be.... https://codereview.appspot.com/246490043/diff/1/tools/preg.py#newcode144 tools/preg.py:144: attributes = event_object.GetAttributes().difference( On 2015/06/25 11:52:28, Joachim Metz wrote: > don't combine multiple calls, harder to debug and bad readability practice Done. https://codereview.appspot.com/246490043/diff/1/tools/preg.py#newcode156 tools/preg.py:156: def _GetTSKPartitionIdentifiers( On 2015/06/25 11:52:27, Joachim Metz wrote: > re-use storage media tool? this is a copy of that function, slightly modified to add Windows partition detection and delay the user prompts.... (just prompt if more than a single windows partition is discovered...) https://codereview.appspot.com/246490043/diff/1/tools/preg.py#newcode434 tools/preg.py:434: event_objects_and_timestamps = {} On 2015/06/25 11:52:27, Joachim Metz wrote: > move part of this method to sub methods, it is currently hard to read. Done. https://codereview.appspot.com/246490043/diff/1/tools/preg.py#newcode474 tools/preg.py:474: On 2015/06/25 11:52:27, Joachim Metz wrote: > - white line Done. https://codereview.appspot.com/246490043/diff/1/tools/preg.py#newcode660 tools/preg.py:660: self._front_end.SetImagePath(image) On 2015/06/25 11:52:27, Joachim Metz wrote: > why have duplicate code paths for file and image ? get a path spec and be done > with it. Done. This needs to be slightly changed.... but still "better" now https://codereview.appspot.com/246490043/diff/1/tools/preg.py#newcode709 tools/preg.py:709: # Scan the source. On 2015/06/25 11:52:28, Joachim Metz wrote: > comment adds no value no, it was meant for me.... while writing hte code ;) https://codereview.appspot.com/246490043/diff/1/tools/preg.py#newcode851 tools/preg.py:851: if key.startswith(u'\\'): On 2015/06/25 11:52:27, Joachim Metz wrote: > move this to a class constant ? e.g. REGISTRY_KEY_PATH_SEPARATOR Done. https://codereview.appspot.com/246490043/diff/1/tools/preg.py#newcode1172 tools/preg.py:1172: # We need to extract the front-end object from the tool. On 2015/06/25 11:52:27, Joachim Metz wrote: > ? is this a TODO ? if not IMO use the tool object here not the front end no... the console needs access to the actual front-end to extract things... and ATM the front-end object is protected..... I'll remove the comment.... or maybe put a TODO here to find alternatives to having the front-end, such as implementing all those calls in the tool itself... https://codereview.appspot.com/246490043/diff/1/tools/preg.py#newcode1275 tools/preg.py:1275: """Prints the initial banner.""" On 2015/06/25 11:52:28, Joachim Metz wrote: > - initial Done. https://codereview.appspot.com/246490043/diff/1/tools/preg.py#newcode1324 tools/preg.py:1324: if index >= len(self._registry_helpers) or index < 0: On 2015/06/25 11:52:27, Joachim Metz wrote: > start with checking the lower bound, then check the upper bound Done. https://codereview.appspot.com/246490043/diff/1/tools/preg.py#newcode1359 tools/preg.py:1359: hive_path: The hive name or path as a string, this is optional name or On 2015/06/25 11:52:27, Joachim Metz wrote: > optional ... Done. https://codereview.appspot.com/246490043/diff/1/tools/preg.py#newcode1374 tools/preg.py:1374: r'[{color.LightBlue}\T{color.Normal}]', On 2015/06/25 11:52:28, Joachim Metz wrote: > why use r' here ? forgot.... there was a reason for that, I'm leaving it as is for now.... https://codereview.appspot.com/246490043/diff/1/tools/preg.py#newcode1459 tools/preg.py:1459: console: an IPython shell object (instance of InteractiveShellEmbed). On 2015/06/25 11:52:28, Joachim Metz wrote: > console => shell_object ? according to the documentation: http://ipython.org/ipython-doc/2/api/generated/IPython.core.hooks.html "Hooks are simple functions, but they should be declared with self as their first argument, because when activated they are registered into IPython as instance methods. The self argument will be the IPython running instance itself, so hooks have full access to the entire IPython object." so perhaps not have it as console nor shell_helper but self.... but that is still "confusing" since this is not a class function....
Sign in to reply to this message.
Code updated.
Sign in to reply to this message.
Code updated.
Sign in to reply to this message.
I definitely improvement over the old version. Some nits. https://codereview.appspot.com/246490043/diff/40001/plaso/frontend/preg.py File plaso/frontend/preg.py (right): https://codereview.appspot.com/246490043/diff/40001/plaso/frontend/preg.py#ne... plaso/frontend/preg.py:13: from plaso.engine import queue - duplicate https://codereview.appspot.com/246490043/diff/40001/plaso/frontend/preg.py#ne... plaso/frontend/preg.py:65: class PregFrontend(extraction_frontend.ExtractionFrontend): + Attributes: knowledge_base_object: ... https://codereview.appspot.com/246490043/diff/40001/plaso/frontend/preg.py#ne... plaso/frontend/preg.py:131: # TODO: Make this part of the DFVFS file system searcher, it should be The path spec contains an absolute path by design. The alternative is to use a mount point path spec. https://codereview.appspot.com/246490043/diff/40001/plaso/frontend/preg.py#ne... plaso/frontend/preg.py:183: # TODO: Remove once file system searcher are mount point aware. It is: https://github.com/log2timeline/dfvfs/blob/master/dfvfs/helpers/file_system_s... Not sure what issue you are running into. https://codereview.appspot.com/246490043/diff/40001/plaso/frontend/preg.py#ne... plaso/frontend/preg.py:252: """Retrieve a searcher for the first source path spec. path specification (repeat in all docstrings) https://codereview.appspot.com/246490043/diff/40001/plaso/frontend/preg.py#ne... plaso/frontend/preg.py:280: def BuildParserMediator(self, event_queue=None): I opt either CreatearserMediator() or NewParserMediator(). https://codereview.appspot.com/246490043/diff/40001/plaso/frontend/preg.py#ne... plaso/frontend/preg.py:426: if registry_types: flatten this nested if by using elif. https://codereview.appspot.com/246490043/diff/40001/plaso/frontend/preg.py#ne... plaso/frontend/preg.py:525: ... + or an empty dict on error? https://codereview.appspot.com/246490043/diff/40001/plaso/frontend/preg.py#ne... plaso/frontend/preg.py:534: if not registry_helper: at this point registry_helper.Open() has already raised if registry_helper is None. https://codereview.appspot.com/246490043/diff/40001/plaso/frontend/preg.py#ne... plaso/frontend/preg.py:548: if verbose: why is this controlled by verbose? Isn't that an output control flag? https://codereview.appspot.com/246490043/diff/40001/plaso/frontend/preg.py#ne... plaso/frontend/preg.py:571: A dict with plugin objects as keys and extracted event objects from each + or an empty dict on error? https://codereview.appspot.com/246490043/diff/40001/plaso/frontend/preg.py#ne... plaso/frontend/preg.py:688: def collector_name(self): alpha ordering of properties? https://codereview.appspot.com/246490043/diff/40001/plaso/frontend/preg.py#ne... plaso/frontend/preg.py:697: def __init__(self, file_entry, collector_name, codepage=u'cp1252'): I opt to put the internal functions __x__() at the top of the class. https://codereview.appspot.com/246490043/diff/40001/plaso/frontend/preg.py#ne... plaso/frontend/preg.py:709: self.file_entry = file_entry + Attributes to class docstring https://codereview.appspot.com/246490043/diff/40001/plaso/frontend/preg.py#ne... plaso/frontend/preg.py:721: def _BuildHiveCache(self): Create or New https://codereview.appspot.com/246490043/diff/40001/plaso/frontend/preg.py#ne... plaso/frontend/preg.py:728: def _ResetAttributes(self): Maybe just Reset() since the method would apply to the object anyway? Also try to match the naming of the corresponding initialize method which looks like BuildHiveCache(). E.g. Start/Stop, Begin/End, Open/Close, etc. https://codereview.appspot.com/246490043/diff/40001/tools/preg.py File tools/preg.py (right): https://codereview.appspot.com/246490043/diff/40001/tools/preg.py#newcode101 tools/preg.py:101: DEFAULT_FORMAT_ALIGN_LENGTH = 15 Since only used internally I opt: _DEFAULT_FORMAT_ALIGN_LENGTH https://codereview.appspot.com/246490043/diff/40001/tools/preg.py#newcode131 tools/preg.py:131: self.list_plugins = False + Attributes section in class docstring https://codereview.appspot.com/246490043/diff/40001/tools/preg.py#newcode177 tools/preg.py:177: SourceScannerError: if the format of or within the source alpha ordering? https://codereview.appspot.com/246490043/diff/40001/tools/preg.py#newcode281 tools/preg.py:281: show_hex: boolean, if set to True hex dump of the value is included in optional https://codereview.appspot.com/246490043/diff/40001/tools/preg.py#newcode281 tools/preg.py:281: show_hex: boolean, if set to True hex dump of the value is included in hex dump => hexadecimal representation https://codereview.appspot.com/246490043/diff/40001/tools/preg.py#newcode423 tools/preg.py:423: Defaults to None. The default is None. https://codereview.appspot.com/246490043/diff/40001/tools/preg.py#newcode452 tools/preg.py:452: self.PrintParsedRegistryKey(key_data, file_entry) you are passing a kwarg so use file_entry= https://codereview.appspot.com/246490043/diff/40001/tools/preg.py#newcode481 tools/preg.py:481: Defaults to None. The default is None. https://codereview.appspot.com/246490043/diff/40001/tools/preg.py#newcode498 tools/preg.py:498: if len(event_objects) == 1 and event_objects[0] is None: Explain this check https://codereview.appspot.com/246490043/diff/40001/tools/preg.py#newcode773 tools/preg.py:773: parsed_data, registry_helper.file_entry) you are passing a kwarg so use file_entry= https://codereview.appspot.com/246490043/diff/40001/tools/preg.py#newcode978 tools/preg.py:978: # TODO: Implement this again. improve this comment what is this? and why needs it to be implemented again? https://codereview.appspot.com/246490043/diff/40001/tools/preg.py#newcode1067 tools/preg.py:1067: parsed_data, current_helper.file_entry) you are passing a kwarg so use file_entry= https://codereview.appspot.com/246490043/diff/40001/tools/preg.py#newcode1161 tools/preg.py:1161: parsed_data, current_helper.file_entry) you are passing a kwarg so use file_entry= https://codereview.appspot.com/246490043/diff/40001/tools/preg.py#newcode1300 tools/preg.py:1300: """Checks if a Windows Registry file is loaded.""" + Returns: https://codereview.appspot.com/246490043/diff/40001/tools/preg.py#newcode1383 tools/preg.py:1383: """Write a list of all available registry helpers to an output writer.""" an output writer ? where is that passed? https://codereview.appspot.com/246490043/diff/40001/tools/preg_test.py File tools/preg_test.py (right): https://codereview.appspot.com/246490043/diff/40001/tools/preg_test.py#newcod... tools/preg_test.py:108: for line in output.split(b'\n'): Maybe move the string parsing to a separate method? https://codereview.appspot.com/246490043/diff/40001/tools/preg_test.py#newcod... tools/preg_test.py:151: _BANNER_HEADER = ( This is hard to change/maintain, I opt to use a join and list entry per line https://codereview.appspot.com/246490043/diff/40001/tools/test_lib.py File tools/test_lib.py (right): https://codereview.appspot.com/246490043/diff/40001/tools/test_lib.py#newcode29 tools/test_lib.py:29: A dfVFS file_entry object (instance of dfvfs.FileEntry). A dfVFS file_entry object => A file entry object or None https://codereview.appspot.com/246490043/diff/40001/tools/test_lib.py#newcode34 tools/test_lib.py:34: file_entry = path_spec_resolver.Resolver.OpenFileEntry(path_spec) return path_spec_resolver.Resolver.OpenFileEntry(path_spec)
Sign in to reply to this message.
done https://codereview.appspot.com/246490043/diff/40001/plaso/frontend/preg.py File plaso/frontend/preg.py (right): https://codereview.appspot.com/246490043/diff/40001/plaso/frontend/preg.py#ne... plaso/frontend/preg.py:13: from plaso.engine import queue On 2015/06/30 05:38:28, Joachim Metz wrote: > - duplicate that is quite true.... not sure how that happened https://codereview.appspot.com/246490043/diff/40001/plaso/frontend/preg.py#ne... plaso/frontend/preg.py:65: class PregFrontend(extraction_frontend.ExtractionFrontend): On 2015/06/30 05:38:28, Joachim Metz wrote: > + Attributes: > knowledge_base_object: ... Done. https://codereview.appspot.com/246490043/diff/40001/plaso/frontend/preg.py#ne... plaso/frontend/preg.py:131: # TODO: Make this part of the DFVFS file system searcher, it should be On 2015/06/30 05:38:27, Joachim Metz wrote: > The path spec contains an absolute path by design. The alternative is to use a > mount point path spec. Acknowledged. https://codereview.appspot.com/246490043/diff/40001/plaso/frontend/preg.py#ne... plaso/frontend/preg.py:183: # TODO: Remove once file system searcher are mount point aware. On 2015/06/30 05:38:27, Joachim Metz wrote: > It is: > https://github.com/log2timeline/dfvfs/blob/master/dfvfs/helpers/file_system_s... > > Not sure what issue you are running into. Acknowledged. https://codereview.appspot.com/246490043/diff/40001/plaso/frontend/preg.py#ne... plaso/frontend/preg.py:280: def BuildParserMediator(self, event_queue=None): On 2015/06/30 05:38:27, Joachim Metz wrote: > I opt either CreatearserMediator() or NewParserMediator(). Done. https://codereview.appspot.com/246490043/diff/40001/plaso/frontend/preg.py#ne... plaso/frontend/preg.py:426: if registry_types: On 2015/06/30 05:38:27, Joachim Metz wrote: > flatten this nested if by using elif. Done. https://codereview.appspot.com/246490043/diff/40001/plaso/frontend/preg.py#ne... plaso/frontend/preg.py:525: ... On 2015/06/30 05:38:27, Joachim Metz wrote: > + or an empty dict on error? Done. https://codereview.appspot.com/246490043/diff/40001/plaso/frontend/preg.py#ne... plaso/frontend/preg.py:534: if not registry_helper: On 2015/06/30 05:38:27, Joachim Metz wrote: > at this point registry_helper.Open() has already raised if registry_helper is > None. that is true, moving up... https://codereview.appspot.com/246490043/diff/40001/plaso/frontend/preg.py#ne... plaso/frontend/preg.py:548: if verbose: On 2015/06/30 05:38:28, Joachim Metz wrote: > why is this controlled by verbose? Isn't that an output control flag? that is a very good point, and yes, this is "how it was done" and really shouldn't be done anymore... removing the verbose flag and just having this as something the output controls https://codereview.appspot.com/246490043/diff/40001/plaso/frontend/preg.py#ne... plaso/frontend/preg.py:571: A dict with plugin objects as keys and extracted event objects from each On 2015/06/30 05:38:27, Joachim Metz wrote: > + or an empty dict on error? Done. https://codereview.appspot.com/246490043/diff/40001/plaso/frontend/preg.py#ne... plaso/frontend/preg.py:688: def collector_name(self): On 2015/06/30 05:38:28, Joachim Metz wrote: > alpha ordering of properties? sure, why not... https://codereview.appspot.com/246490043/diff/40001/plaso/frontend/preg.py#ne... plaso/frontend/preg.py:697: def __init__(self, file_entry, collector_name, codepage=u'cp1252'): On 2015/06/30 05:38:27, Joachim Metz wrote: > I opt to put the internal functions __x__() at the top of the class. so before properties? https://codereview.appspot.com/246490043/diff/40001/plaso/frontend/preg.py#ne... plaso/frontend/preg.py:709: self.file_entry = file_entry On 2015/06/30 05:38:28, Joachim Metz wrote: > + Attributes to class docstring Done. https://codereview.appspot.com/246490043/diff/40001/plaso/frontend/preg.py#ne... plaso/frontend/preg.py:721: def _BuildHiveCache(self): On 2015/06/30 05:38:27, Joachim Metz wrote: > Create or New Done. https://codereview.appspot.com/246490043/diff/40001/plaso/frontend/preg.py#ne... plaso/frontend/preg.py:728: def _ResetAttributes(self): On 2015/06/30 05:38:27, Joachim Metz wrote: > Maybe just Reset() since the method would apply to the object anyway? > > Also try to match the naming of the corresponding initialize method which looks > like BuildHiveCache(). E.g. Start/Stop, Begin/End, Open/Close, etc. Done. https://codereview.appspot.com/246490043/diff/40001/tools/preg.py File tools/preg.py (right): https://codereview.appspot.com/246490043/diff/40001/tools/preg.py#newcode101 tools/preg.py:101: DEFAULT_FORMAT_ALIGN_LENGTH = 15 On 2015/06/30 05:38:28, Joachim Metz wrote: > Since only used internally I opt: _DEFAULT_FORMAT_ALIGN_LENGTH Done. https://codereview.appspot.com/246490043/diff/40001/tools/preg.py#newcode131 tools/preg.py:131: self.list_plugins = False On 2015/06/30 05:38:28, Joachim Metz wrote: > + Attributes section in class docstring Done. https://codereview.appspot.com/246490043/diff/40001/tools/preg.py#newcode177 tools/preg.py:177: SourceScannerError: if the format of or within the source On 2015/06/30 05:38:28, Joachim Metz wrote: > alpha ordering? Done..... but since this got copied over from another source, changed the source as well. https://codereview.appspot.com/246490043/diff/40001/tools/preg.py#newcode281 tools/preg.py:281: show_hex: boolean, if set to True hex dump of the value is included in On 2015/06/30 05:38:28, Joachim Metz wrote: > hex dump => hexadecimal representation Done. https://codereview.appspot.com/246490043/diff/40001/tools/preg.py#newcode281 tools/preg.py:281: show_hex: boolean, if set to True hex dump of the value is included in On 2015/06/30 05:38:29, Joachim Metz wrote: > optional Done. https://codereview.appspot.com/246490043/diff/40001/tools/preg.py#newcode423 tools/preg.py:423: Defaults to None. On 2015/06/30 05:38:28, Joachim Metz wrote: > The default is None. Done. https://codereview.appspot.com/246490043/diff/40001/tools/preg.py#newcode452 tools/preg.py:452: self.PrintParsedRegistryKey(key_data, file_entry) On 2015/06/30 05:38:29, Joachim Metz wrote: > you are passing a kwarg so use file_entry= Done. https://codereview.appspot.com/246490043/diff/40001/tools/preg.py#newcode481 tools/preg.py:481: Defaults to None. On 2015/06/30 05:38:29, Joachim Metz wrote: > The default is None. Done. https://codereview.appspot.com/246490043/diff/40001/tools/preg.py#newcode498 tools/preg.py:498: if len(event_objects) == 1 and event_objects[0] is None: On 2015/06/30 05:38:28, Joachim Metz wrote: > Explain this check well... this means that sometimes there have been cases (probably) where we get back a list with a single item in it that is None.... why that has happened I'm not sure of.... this is old remnants of a moved piece of code. I don't really see any reasons for this check anymore.... I put a TODO to remove it, but I can also just remove it for now.... (I believe this may have been remnants from how events were pulled in previously, before the event consumer) https://codereview.appspot.com/246490043/diff/40001/tools/preg.py#newcode773 tools/preg.py:773: parsed_data, registry_helper.file_entry) On 2015/06/30 05:38:28, Joachim Metz wrote: > you are passing a kwarg so use file_entry= Done. https://codereview.appspot.com/246490043/diff/40001/tools/preg.py#newcode978 tools/preg.py:978: # TODO: Implement this again. On 2015/06/30 05:38:28, Joachim Metz wrote: > improve this comment what is this? and why needs it to be implemented again? elaborated https://codereview.appspot.com/246490043/diff/40001/tools/preg.py#newcode1067 tools/preg.py:1067: parsed_data, current_helper.file_entry) On 2015/06/30 05:38:28, Joachim Metz wrote: > you are passing a kwarg so use file_entry= Done. https://codereview.appspot.com/246490043/diff/40001/tools/preg.py#newcode1161 tools/preg.py:1161: parsed_data, current_helper.file_entry) On 2015/06/30 05:38:28, Joachim Metz wrote: > you are passing a kwarg so use file_entry= Done. https://codereview.appspot.com/246490043/diff/40001/tools/preg.py#newcode1300 tools/preg.py:1300: """Checks if a Windows Registry file is loaded.""" On 2015/06/30 05:38:28, Joachim Metz wrote: > + Returns: Done. https://codereview.appspot.com/246490043/diff/40001/tools/preg.py#newcode1383 tools/preg.py:1383: """Write a list of all available registry helpers to an output writer.""" On 2015/06/30 05:38:29, Joachim Metz wrote: > an output writer ? where is that passed? well, this is what the function does... the output writer is not configurable though... added a TODO for that. https://codereview.appspot.com/246490043/diff/40001/tools/preg_test.py File tools/preg_test.py (right): https://codereview.appspot.com/246490043/diff/40001/tools/preg_test.py#newcod... tools/preg_test.py:108: for line in output.split(b'\n'): On 2015/06/30 05:38:29, Joachim Metz wrote: > Maybe move the string parsing to a separate method? Acknowledged. https://codereview.appspot.com/246490043/diff/40001/tools/preg_test.py#newcod... tools/preg_test.py:151: _BANNER_HEADER = ( On 2015/06/30 05:38:29, Joachim Metz wrote: > This is hard to change/maintain, I opt to use a join and list entry per line Done. https://codereview.appspot.com/246490043/diff/40001/tools/test_lib.py File tools/test_lib.py (right): https://codereview.appspot.com/246490043/diff/40001/tools/test_lib.py#newcode29 tools/test_lib.py:29: A dfVFS file_entry object (instance of dfvfs.FileEntry). On 2015/06/30 05:38:29, Joachim Metz wrote: > A dfVFS file_entry object => A file entry object > > or None copied from another source, changing that as well https://codereview.appspot.com/246490043/diff/40001/tools/test_lib.py#newcode34 tools/test_lib.py:34: file_entry = path_spec_resolver.Resolver.OpenFileEntry(path_spec) On 2015/06/30 05:38:29, Joachim Metz wrote: > return path_spec_resolver.Resolver.OpenFileEntry(path_spec) done and in original source as well
Sign in to reply to this message.
Code updated.
Sign in to reply to this message.
https://codereview.appspot.com/246490043/diff/40001/plaso/frontend/preg.py File plaso/frontend/preg.py (right): https://codereview.appspot.com/246490043/diff/40001/plaso/frontend/preg.py#ne... plaso/frontend/preg.py:697: def __init__(self, file_entry, collector_name, codepage=u'cp1252'): Yes largely since these are meta-methods. https://codereview.appspot.com/246490043/diff/60001/plaso/frontend/preg.py File plaso/frontend/preg.py (right): https://codereview.appspot.com/246490043/diff/60001/plaso/frontend/preg.py#ne... plaso/frontend/preg.py:283: """Create and return back a parser mediator object. Creates a parser mediator object. "and return back" is already covered by "Returns" IMO no need to repeat that. https://codereview.appspot.com/246490043/diff/60001/plaso/frontend/preg.py#ne... plaso/frontend/preg.py:545: return_dict[key_path][u'subkeys'] = [ return_dict[key_path][u'subkeys'] = list(key.GetSubkeys()) ? https://codereview.appspot.com/246490043/diff/60001/plaso/frontend/preg.py#ne... plaso/frontend/preg.py:567: A dict with plugin objects as keys and extracted event objects from each dict => dictionary https://codereview.appspot.com/246490043/diff/60001/tests/parsers/test_lib.py File tests/parsers/test_lib.py (right): https://codereview.appspot.com/246490043/diff/60001/tests/parsers/test_lib.py... tests/parsers/test_lib.py:154: """Creates a file_entry that references a file in the test dir. file_entry => file entry (remove underscore) https://codereview.appspot.com/246490043/diff/60001/tests/parsers/test_lib.py... tests/parsers/test_lib.py:160: A file_entry object (instance of dfvfs.FileEntry). file_entry => file entry (remove underscore) https://codereview.appspot.com/246490043/diff/60001/tools/preg.py File tools/preg.py (right): https://codereview.appspot.com/246490043/diff/60001/tools/preg.py#newcode509 tools/preg.py:509: # TODO: Is this check really necessary anymore? Can this be removed. => TODO: Remove this check if not needed, otherwise describe what it is supposed to be checking. https://codereview.appspot.com/246490043/diff/60001/tools/preg_test.py File tools/preg_test.py (right): https://codereview.appspot.com/246490043/diff/60001/tools/preg_test.py#newcode18 tools/preg_test.py:18: def _StringToPluginsAndKeys(self, string): ExtractPluginsAndKey or ParsePluginsAndKey ? https://codereview.appspot.com/246490043/diff/60001/tools/preg_test.py#newcode19 tools/preg_test.py:19: """Takes a string and returns two sets with names of plugins and keys.""" + args, also describe how you expect string to look like. + returns https://codereview.appspot.com/246490043/diff/60001/tools/preg_test.py#newcod... tools/preg_test.py:159: (u'******** Welcome to PREG - home of the Plaso Windows Registry ' There are still some \n left in the text have a look at: https://github.com/log2timeline/plaso/blob/master/tests/cli/storage_media_too... https://codereview.appspot.com/246490043/diff/60001/tools/test_lib.py File tools/test_lib.py (right): https://codereview.appspot.com/246490043/diff/60001/tools/test_lib.py#newcode23 tools/test_lib.py:23: """Creates a file_entry that references a file in the test dir. file_entry remove underscore https://codereview.appspot.com/246490043/diff/60001/tools/test_lib.py#newcode29 tools/test_lib.py:29: A file_entry object (instance of dfvfs.FileEntry). underscore
Sign in to reply to this message.
done. https://codereview.appspot.com/246490043/diff/60001/plaso/frontend/preg.py File plaso/frontend/preg.py (right): https://codereview.appspot.com/246490043/diff/60001/plaso/frontend/preg.py#ne... plaso/frontend/preg.py:283: """Create and return back a parser mediator object. On 2015/06/30 17:57:37, Joachim Metz wrote: > Creates a parser mediator object. > > "and return back" is already covered by "Returns" IMO no need to repeat that. Acknowledged. https://codereview.appspot.com/246490043/diff/60001/plaso/frontend/preg.py#ne... plaso/frontend/preg.py:545: return_dict[key_path][u'subkeys'] = [ On 2015/06/30 17:57:37, Joachim Metz wrote: > return_dict[key_path][u'subkeys'] = list(key.GetSubkeys()) ? yes, both work https://codereview.appspot.com/246490043/diff/60001/plaso/frontend/preg.py#ne... plaso/frontend/preg.py:567: A dict with plugin objects as keys and extracted event objects from each On 2015/06/30 17:57:36, Joachim Metz wrote: > dict => dictionary Done. https://codereview.appspot.com/246490043/diff/60001/tests/parsers/test_lib.py File tests/parsers/test_lib.py (right): https://codereview.appspot.com/246490043/diff/60001/tests/parsers/test_lib.py... tests/parsers/test_lib.py:154: """Creates a file_entry that references a file in the test dir. On 2015/06/30 17:57:37, Joachim Metz wrote: > file_entry => file entry (remove underscore) Done. https://codereview.appspot.com/246490043/diff/60001/tests/parsers/test_lib.py... tests/parsers/test_lib.py:160: A file_entry object (instance of dfvfs.FileEntry). On 2015/06/30 17:57:37, Joachim Metz wrote: > file_entry => file entry (remove underscore) Done. https://codereview.appspot.com/246490043/diff/60001/tools/preg.py File tools/preg.py (right): https://codereview.appspot.com/246490043/diff/60001/tools/preg.py#newcode509 tools/preg.py:509: # TODO: Is this check really necessary anymore? Can this be removed. On 2015/06/30 17:57:37, Joachim Metz wrote: > => TODO: Remove this check if not needed, otherwise describe what it is supposed > to be checking. removed it.... This was due to the "yield/return" section in the consumer. If there were no event objects it would first yield None and then return.... replaced that with a StopIterator, which fixed this. https://codereview.appspot.com/246490043/diff/60001/tools/preg_test.py File tools/preg_test.py (right): https://codereview.appspot.com/246490043/diff/60001/tools/preg_test.py#newcode18 tools/preg_test.py:18: def _StringToPluginsAndKeys(self, string): On 2015/06/30 17:57:37, Joachim Metz wrote: > ExtractPluginsAndKey or ParsePluginsAndKey ? Done. https://codereview.appspot.com/246490043/diff/60001/tools/preg_test.py#newcode19 tools/preg_test.py:19: """Takes a string and returns two sets with names of plugins and keys.""" On 2015/06/30 17:57:37, Joachim Metz wrote: > + args, also describe how you expect string to look like. > + returns Done. https://codereview.appspot.com/246490043/diff/60001/tools/preg_test.py#newcod... tools/preg_test.py:159: (u'******** Welcome to PREG - home of the Plaso Windows Registry ' On 2015/06/30 17:57:37, Joachim Metz wrote: > There are still some \n left in the text > have a look at: > https://github.com/log2timeline/plaso/blob/master/tests/cli/storage_media_too... ok, I'll add some blanks in there to remove the \n's https://codereview.appspot.com/246490043/diff/60001/tools/test_lib.py File tools/test_lib.py (right): https://codereview.appspot.com/246490043/diff/60001/tools/test_lib.py#newcode23 tools/test_lib.py:23: """Creates a file_entry that references a file in the test dir. On 2015/06/30 17:57:37, Joachim Metz wrote: > file_entry remove underscore Done. https://codereview.appspot.com/246490043/diff/60001/tools/test_lib.py#newcode29 tools/test_lib.py:29: A file_entry object (instance of dfvfs.FileEntry). On 2015/06/30 17:57:37, Joachim Metz wrote: > underscore Done.
Sign in to reply to this message.
Code updated.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
