tking2 / volatility

Automatically exported from code.google.com/p/volatility
GNU General Public License v2.0
0 stars 1 forks source link

Confusion about SanityCheckExceptions #292

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
From a comment in r1984 by scudette:

"Im a little confused about this new SanityCheckException exception. Its 
inconsistent with the NoneObject design pattern. In most other cases we return 
a NoneObject in case of errors, but here we raise an exception.

The original purpose of the NoneObject design was to avoid having to catch 
exceptions all the time just like the code above:

try:
    dos_header = obj.Object("_IMAGE_DOS_HEADER", offset = self.DllBase,
                            vm = self.obj_native_vm)
    return dos_header.get_nt_header()
...
except exceptions.SanityCheckException:
   return obj.NoneObject("Failed initial sanity checks. Try -u or --unsafe")

Are we changing the NoneObject design in favour of exceptions now?"

Original issue reported on code.google.com by mike.auty@gmail.com on 8 Jul 2012 at 8:35

GoogleCodeExporter commented 9 years ago
Thanks for your comment.  Could I ask you in future to post such things to the 
issue tracker please?  It means that everyone can see it, and we can give the 
idea the attention it needs, rather than being buried so that only the 
committer knows it exists.

In answer to your confusion, this patch was to replace existing exceptions that 
were being thrown, and to get it in before the 2.1 code-freeze it needed to be 
a straight-forward replacement.

If you don't understand the reasoning behind a commit, it may be better to ask, 
rather than making assumptions and assertions as you have here.  It's rather 
poor issue tracker etiquette, since they could easily be misread as arrogance 
at knowing better than the committing developer, when in fact you may simply 
not be aware of the reasoning.

Unfortunately your suggestion came a little after the commit for issue 243 was 
closed, and hence the new issue.  However, if you'd like to get this fixed more 
quickly, do please develop a patch indicating where you think NoneObjects 
should be used, and we can get it included in the next release.

Original comment by mike.auty@gmail.com on 8 Jul 2012 at 9:13

GoogleCodeExporter commented 9 years ago
This sounds a bit rude dont you think? This is not a suggestion or an
idea but it is a question about a issue I spotted in the commit log
while reviewing it - hence I did ask in the first obvious spot I saw
(just under the commit log). As per your request I did repost to the
issue tracker.

If the project policy is not to use the feature of commenting against
the commit logs than I am happy to never do this again and use the
issue tracker exclusively. I was just not aware of this policy.

I am just hoping not to get an abrupt and rude response to a simple question.

Original comment by scude...@gmail.com on 8 Jul 2012 at 9:46

GoogleCodeExporter commented 9 years ago
Sorry this is so frustrating guys. I'm going to copy the technical part of this 
to issue #291 to kind of start with a clean slate discussing the real problem 
over there. 

Original comment by michael.hale@gmail.com on 8 Jul 2012 at 3:22