Skip to content

remove unnecessary print#9

Open
psifertex wants to merge 3 commits into
CySHell:mainfrom
psifertex:patch-2
Open

remove unnecessary print#9
psifertex wants to merge 3 commits into
CySHell:mainfrom
psifertex:patch-2

Conversation

@psifertex
Copy link
Copy Markdown
Contributor

Convert an unnecessary print. Really should probably just be removed, but I changed it to a log_debug so if you really want to still see the message you can in debug mode.

Convert an unnecessary print (really should probably just be removed but I changed it to a log_debug so if you really want to still see the message you can in debug mode.
@psifertex
Copy link
Copy Markdown
Contributor Author

Actually, it's worse than that. If you make a new file it won't have an arch at all and you fill your logs with errors:

  File "/Users/jwiens/Library/Application Support/Binary Ninja/repositories/community/plugins/CySHell_ClassyPP/StartInspection.py", line 13, in is_bv_valid_for_plugin
    if bv.arch.name == "x86_64" or bv.arch.name == "x86":
       ^^^^^^^^^^^^
AttributeError: 'NoneType' object has no attribute 'name'
Traceback (most recent call last):
  File "/Applications/Binary Ninja-dev.app/Contents/MacOS/plugins/../../Resources/python/binaryninja/plugin.py", line 229, in _default_is_valid
    return is_valid(view_obj)
           ^^^^^^^^^^^^^^^^^^
  File "/Users/jwiens/Library/Application Support/Binary Ninja/repositories/community/plugins/CySHell_ClassyPP/StartInspection.py", line 13, in is_bv_valid_for_plugin
    if bv.arch.name == "x86_64" or bv.arch.name == "x86":
       ^^^^^^^^^^^^
AttributeError: 'NoneType' object has no attribute 'name'

I'll amend my PR to guard for that too.

psifertex added 2 commits June 9, 2023 11:18
On second though, removing the log entirely as in the case of no arch there isn't even a name to print. Best to just remove it.
Comment thread StartInspection.py
return True
else:
print(f'ClassyPP: Executable CPU Arch is: {bv.arch.name}. This plugin supports only x86 32/64 bit executables.')
return False
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to remove the return False?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I don't think so, I think I was planning on just moving it to the top level but looks like it got eaten. It might default to false on the core side so it might have just worked accidentally, but agree leaving it is best.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants