Skip to content

Conversation

@RasmusWind
Copy link
Contributor

Please check if this is okay.
Currently seems able to start and stop thread.
username, password, use_ssl, port, skip_verify and verbose are optional. The latter 4 will default to 0.

@RasmusWind RasmusWind requested a review from kivkiv12345 May 28, 2024 05:21
Copy link
Contributor

@kivkiv12345 kivkiv12345 left a comment

Choose a reason for hiding this comment

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

As I understand it, reference counting on Python globals is not strictly required, but it still seems preferred as indicated by the macro Py_RETURN_NONE, which will "return Py_NewRef(Py_None)".

Whenever an exposed function returns an object to Python, it is expected to be a new reference. Python therefore decrements the reference count of the returned object, when it falls out of scope.
Suffice to say, not incrementing the reference count when returning, steals a reference. This will eventually cause the object to be deallocated while it is still in use, causing a segmentation fault.
Fortunately, I don't think this is the case with builtin globals (such as None), which Python knows to never deallocate. Nevertheless, it doesn't hurt to reference count correctly.
Occurrences of "return Py_None" (that return to Python) therefore ought to be replaced with either "Py_RETURN_NONE" or "return Py_NewRef(Py_None)".

@kivkiv12345
Copy link
Contributor

There are also some places in victoria_metrics.c where we don't check the return value of strdup(), which will be NULL when we run out of memory.
This is more so a problem in CSH, than this PR, which we may end up solving in libcsh.

@kivkiv12345
Copy link
Contributor

Remind me to not forget about this PR.
csh_si has bindings for a Host class (with PDU channels and such), which would ideally subclass a Node class from this commit.
It's made a bit tricky with how the memory is reallocated in C however.

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants