added in/outdev getters and performance improvements.#87
added in/outdev getters and performance improvements.#87DOWRIGHTTV wants to merge 1 commit intooremanj:masterfrom
Conversation
oremanj
left a comment
oremanj
left a comment
There was a problem hiding this comment.
Thank you for your contribution! I left some comments below; please also update the API reference in the README file to match this change.
| """ | ||
|
|
||
| # Constants for module users | ||
| import Cython |
There was a problem hiding this comment.
These imports are placed awkwardly relative to the comment that is now above them but describes the constants below
| Copyright: (c) 2011, Kerkhoff Technologies Inc. | ||
| License: MIT; see LICENSE.txt | ||
|
|
||
| Expanded features and performance improvements from downstream development |
There was a problem hiding this comment.
I would prefer that something like this be more specific about what you changed; this is a fairly small change and I think the description makes it sound bigger than it is.
There was a problem hiding this comment.
tbh, i dont really care about the formalities. some of this stuff was a side effect of my heavily worked on fork where i removed the gil and some other things.
| return 1 | ||
| packet = Packet() | ||
|
|
||
| # skipping call to __init__. |
There was a problem hiding this comment.
the init method is inherently called when instantiating the Packet class and is a pure python method. calling new will return a new instance while reducing unnecessary code execution.
see: https://cython.readthedocs.io/en/latest/src/userguide/special_methods.html?highlight=performance
| cdef nfqnl_msg_packet_hw *hw | ||
| cdef nfqnl_msg_packet_hdr *hdr | ||
|
|
||
| self._nfa = nfa |
There was a problem hiding this comment.
This is not safe; the nfq_data pointer only lives as long as the callback, but the Packet object can be retained longer. Not saving nfa to the object is a deliberate choice. You should instead extract the in/out device numbers here (and convert them to names only if requested, since that part is likely to be slower).
There was a problem hiding this comment.
the way i did this was for performance. this will not punish users who dont care about the in/out interfaces.
also, i dont feel the safety is relevant here. the intention is for the callback to store the in/out values on their side.
once the callback is called, the user can decide what information they want to query and store the returns locally in callback. this makes everything inherently safe because we dont need to rely on dead pointers and subsequent calls dont need to reach into Cython.
|
|
||
| self._verdict_is_set = True | ||
|
|
||
| cpdef get_indev(self, bint name=False): |
There was a problem hiding this comment.
I think this should be two functions, one to get the index and one to get the name. (Or an attribute for the index and a function for the name, which might make more sense especially if you're saving the indices on the object anyway like I suggested above.) Parameters that change the return type of a function are awkward; they'll almost certainly be passed as constants, and they create a hazard for calling code to not name the parameter resulting in difficult-to-understand things like packet.get_indev(True).
There was a problem hiding this comment.
tbh, i dont use the name functionality. i added that to make it more complete as a lib.
|
here is an example of what i am talking about. it is a derivative work of this repo from last year that actually takes on the basic protocol parsing for the user and releases the GIL prior to parsing. https://github.com/DOWRIGHTTV/dnx_nfqueue/blob/master/dnx_nfqueue.pyx |
|
I recently added indev/outdev attributes using a different approach. Closing this since you didn't respond to my comments. |
2 participants
No description provided.