Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add python bindings #222

Merged
merged 4 commits into from
Mar 17, 2022
Merged

Add python bindings #222

merged 4 commits into from
Mar 17, 2022

Conversation

jimmylomro
Copy link
Contributor

@jimmylomro jimmylomro commented Mar 15, 2022

Adds python bindings for libnethogs using pybind11. Didn't touch any of the existing code, so does not break anything that already exists.

To compile/install this people will need to also install pybind11-dev deb package. It uses setuptools to install, so you need to run pip install . and it will compile everything and install.

To use:

import nethogs
import threading
def callback(action: int, record: nethogs.NethogsMonitorRecord) -> None:
    do_whatever_with_record(record)
    return
th = threading.Thread(target=nethogs.nethogsmonitor_loop, args(callback, filter, to_ms))
th.start()
do_whatever_you_need_to_do()
nethogs.nethogsmonitor_breakloop()
th.join()

As a general comment, there's some not very good use of global variables, callbacks should be std::function instead, and a lot of other things mostly related with newer c++ standards. Probably a good idea to start a ToDo list in the readme :) specially for such a widely used codebase.

There are a few things to notice:

  • There are only bindings for nethogsmonitor_loop and nethogsmonitor_breakloop.
  • At the moment, only one instance of the loop can be run at a time. Callbacks would get completely messed up if more than one is run.
  • There is no way of setting pidsToWatch, but it is not difficult to implement if anyone wants to do it :)
  • The package version is set in setup.py using the script determineVersion.sh, but it is very hacky as a PEP compatible string is needed. Something stronger should be implemented, ideally using the regex module.
  • Ideally this should not be run in the main thread, stopping the nethogsmonitor_loop with a SIGINT or SIGTERM is a bit hacky and could break things.

Copy link
Owner

@raboof raboof left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this!

Might be good to add some docs?

src/bindings.cpp Outdated
@@ -0,0 +1,68 @@
#include <pybind11/pybind11.h>
Copy link
Owner

Choose a reason for hiding this comment

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

Do I understand correctly that because of the way src/MakeApp.mk is set up, this file is only compiled when building the python bindings, and not when nethogs is 'regularly' compiled? Perhaps it would be neat to put it in a separate directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is right, the src/bindings.cpp file should only be compiled when building the python bindings i.e. pip install .. I see MakeApp.mk and MakeLib.mk both explicitly define the list of files to be built, so keeping src/bindings.cpp won't affect the make compilation, but I see the point, I can move it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could move it to a python directory where I can add a readme with some info (will be similar to the PR original comment). The setup.py and pyproject.toml will have to stay in the root though.

Copy link
Owner

Choose a reason for hiding this comment

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

that sounds fine to me!

I guess this makes the contrib script obsolete?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would, yes, but for those who still want to load the lib directly I can also move this file into the python directory.

@raboof raboof merged commit a663b3a into raboof:main Mar 17, 2022
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.

None yet

2 participants