Skip to content

Handle marine loaded already#36

Merged
tomlegkov merged 3 commits intomasterfrom
bugfix/marine-loaded-twice
Jul 14, 2020
Merged

Handle marine loaded already#36
tomlegkov merged 3 commits intomasterfrom
bugfix/marine-loaded-twice

Conversation

@domerd
Copy link
Copy Markdown
Collaborator

@domerd domerd commented Jul 12, 2020

There is a flag in marine-core that's preventing Marine from initializing twice.
In case Marine was already started, init_marine() returns -2.

Copy link
Copy Markdown
Collaborator

@shaharyarn shaharyarn left a comment

Choose a reason for hiding this comment

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

Just adjust accordingly to the CR of marine-core and it's good.

marine/marine.py Outdated

from . import encap_consts

MARINE_ALREADY_INITIALIZED_ERROR_CODE = -2
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Remember to switch to the const from marine.h.

@yehonatanz yehonatanz linked an issue Jul 12, 2020 that may be closed by this pull request
marine/marine.py Outdated
self._marine.marine_free.argtypes = [MARINE_RESULT_POINTER]
return_code = self._marine.init_marine()
if return_code < 0:
if return_code == c_int.in_dll(self._marine, 'MARINE_ALREADY_INITIALIZED_ERROR_CODE').value:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Run black on your code please :)

marine/marine.py Outdated
self._marine.marine_free.argtypes = [MARINE_RESULT_POINTER]
return_code = self._marine.init_marine()
if return_code < 0:
if return_code == c_int.in_dll(self._marine, 'MARINE_ALREADY_INITIALIZED_ERROR_CODE').value:
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I really don't like this magic string.. :(

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Suggestions?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I like the const that was defined. I think it's better than a long magic string.

Another option might be to expose the error code as the return value of a function (get_marine_already_initialized_err_code()) and not a const but that's weird

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yeah that a bit weird, @yehonatanz @shaharyarn what do you guys think?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@tomlegkov name of a function is no less magic than name of a const, it bothers you just because the latter is in quotes.
An AttributeError will be raised if this const symbol won't exist anyway and neither way is detected by static analysis.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Ok, good point.

@domerd domerd requested a review from shaharyarn July 14, 2020 11:30
@tomlegkov tomlegkov merged commit 79033c7 into master Jul 14, 2020
@tomlegkov tomlegkov deleted the bugfix/marine-loaded-twice branch July 14, 2020 14:44
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.

Marine craches with SIGTRAP when init is called twice

4 participants