Fixes SIGSEGV and improve thread safety of journal.Reader class #144
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
Filter by extension
Only manifest files
Viewed files
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| Expand Up | @@ -64,6 +64,8 @@ | |
| typedef struct { | ||
| PyObject_HEAD | ||
| sd_journal *j; | ||
| unsigned closed; | ||
| unsigned ref_count; | ||
|
Comment thread
Copy link
Copy Markdown
Member
There was a problem hiding this comment. In the future, please explain the reasoning behind a patch in the commit message. For something like thread safety, this is particularly important. Without that, the reader is left to guess. I don't understand how this is supposed to help. This is a python object, and PyObject_HEAD above already embeds a reference counter. A second reference counter in the same struct is unexpected.
Sorry, something went wrong.
Copy link
Copy Markdown
Member
There was a problem hiding this comment. Oh, you did provide an explanation, but in the pull request ticket. I was looking at the patches and missed that. Sorry. (The explanation still belongs in the commit message, not on a pull request.) I'll reply further in #143, because you make some good points there.
Sorry, something went wrong.
Copy link
Copy Markdown
Author
There was a problem hiding this comment.
Well, matter of taste either (many repo-owners like to hold the commit messages as short as possible)...
Sorry, something went wrong.
Copy link
Copy Markdown
Member
There was a problem hiding this comment.
Not this repo certainly. Frankly, I don't think that'd make any sense. One can always not look at the commit descriptions if they don't want to.
This doesn't work properly. When one looks for a reason for a bug, e.g. browsing the history of a file, or using git blame, or git bisect, they'll see the commit. Obviously, with enough effort, one can find the commit message, and the discussion on github, or even the mailing list thread, but those are all inferior options.
In general, I'd like to see the PR rebased until it looks properly, per the usual open-source workflow. In this particuluar case, let's not rebase yet. I have more comments cooking.
Sorry, something went wrong.
Copy link
Copy Markdown
Author
There was a problem hiding this comment.
No, by default git blame as well as git bisect would show/use the merge commit firstly. Even blaming on github shows firstly the merge point and only with "Blame prior to change" you'd get the original commit. But by merging of PR, GH also allows to squash everything to single commit instead of merge, and one can rewrite the message.
OK, no problem.
Sorry, something went wrong.
Copy link
Copy Markdown
Member
There was a problem hiding this comment.
I don't think so. For an arbitrary file I have here, git blame src/core/main.c|cut -d' ' -f1 |sort -u|xargs -n1 git log --pretty=oneline -1 doesn't show even one merge commit.
Sure, but that's only useful for PRs that should be one commit. Here we have additional changes…
Sorry, something went wrong.
Copy link
Copy Markdown
Author
There was a problem hiding this comment.
I think it is git blame --first-parent $file... Many UIs (and GH) doing it by default, so I assumed cmd-line would do that too.
Sorry, something went wrong. |
||
| } Reader; | ||
| static PyTypeObject ReaderType; | ||
|
|
||
| Expand All | @@ -89,6 +91,31 @@ static PyStructSequence_Desc Monotonic_desc = { | |
| 2, | ||
| }; | ||
|
|
||
| static PyObject *Reader_new(PyTypeObject *type, PyObject *args, PyObject *kwds) { | ||
| Reader *self = (Reader *)PyType_GenericNew(type, args, kwds); | ||
| self->j = NULL; | ||
| self->closed = 0; | ||
| self->ref_count = 1; /* initial reference */ | ||
| return (PyObject *)self; | ||
| } | ||
|
|
||
| static inline void decr_ref_count(Reader *self) { | ||
| if (!self->ref_count) return; | ||
| if (!--self->ref_count && self->j) { | ||
| sd_journal_close(self->j); | ||
| self->j = NULL; | ||
| } | ||
| } | ||
|
|
||
| #define INCR_REF_BEGIN_ALLOW_THREADS(self) \ | ||
| self->ref_count++; \ | ||
| Py_BEGIN_ALLOW_THREADS | ||
| #define DECR_REF_END_ALLOW_THREADS(self) \ | ||
| Py_END_ALLOW_THREADS \ | ||
| decr_ref_count(self); | ||
|
|
||
|
|
||
|
|
||
| /** | ||
| * Convert a str or bytes object into a C-string path. | ||
| * Returns NULL on error. | ||
| Expand Down Expand Up | @@ -220,7 +247,9 @@ static int intlist_converter(PyObject* obj, int **_result, size_t *_len) { | |
| } | ||
|
|
||
| static void Reader_dealloc(Reader* self) { | ||
| if (self->j) { | ||
| sd_journal_close(self->j); | ||
| } | ||
| Py_TYPE(self)->tp_free((PyObject*)self); | ||
| } | ||
|
|
||
| Expand Down Expand Up | @@ -271,9 +300,9 @@ static int Reader_init(Reader *self, PyObject *args, PyObject *keywds) { | |
| return -1; | ||
|
|
||
| #if HAVE_JOURNAL_OPEN_DIRECTORY_FD | ||
| INCR_REF_BEGIN_ALLOW_THREADS(self) | ||
| r = sd_journal_open_directory_fd(&self->j, (int) fd, flags); | ||
| DECR_REF_END_ALLOW_THREADS(self) | ||
| #else | ||
| r = -ENOSYS; | ||
| #endif | ||
| Expand All | @@ -285,9 +314,9 @@ static int Reader_init(Reader *self, PyObject *args, PyObject *keywds) { | |
| if (!path) | ||
| return -1; | ||
|
|
||
| INCR_REF_BEGIN_ALLOW_THREADS(self) | ||
| r = sd_journal_open_directory(&self->j, path, flags); | ||
| DECR_REF_END_ALLOW_THREADS(self) | ||
| } | ||
| } else if (_files) { | ||
| _cleanup_Py_DECREF_ PyObject *item0 = NULL; | ||
| Expand All | @@ -300,9 +329,9 @@ static int Reader_init(Reader *self, PyObject *args, PyObject *keywds) { | |
| return -1; | ||
|
|
||
| #if HAVE_JOURNAL_OPEN_FILES | ||
| INCR_REF_BEGIN_ALLOW_THREADS(self) | ||
| r = sd_journal_open_files(&self->j, (const char**) files, flags); | ||
| DECR_REF_END_ALLOW_THREADS(self) | ||
| #else | ||
| r = -ENOSYS; | ||
| #endif | ||
| Expand All | @@ -314,9 +343,9 @@ static int Reader_init(Reader *self, PyObject *args, PyObject *keywds) { | |
| return -1; | ||
|
|
||
| #if HAVE_JOURNAL_OPEN_DIRECTORY_FD | ||
| INCR_REF_BEGIN_ALLOW_THREADS(self) | ||
| r = sd_journal_open_files_fd(&self->j, fds, n_fds, flags); | ||
| DECR_REF_END_ALLOW_THREADS(self) | ||
| #else | ||
| r = -ENOSYS; | ||
| #endif | ||
| Expand All | @@ -329,16 +358,16 @@ static int Reader_init(Reader *self, PyObject *args, PyObject *keywds) { | |
| if (!namespace) | ||
| return -1; | ||
|
|
||
| INCR_REF_BEGIN_ALLOW_THREADS(self) | ||
| r = sd_journal_open_namespace(&self->j, namespace, flags); | ||
| DECR_REF_END_ALLOW_THREADS(self) | ||
| #else | ||
| r = -ENOSYS; | ||
| #endif | ||
| } else { | ||
| INCR_REF_BEGIN_ALLOW_THREADS(self) | ||
| r = sd_journal_open(&self->j, flags); | ||
| DECR_REF_END_ALLOW_THREADS(self) | ||
| } | ||
|
|
||
| return set_error(r, NULL, "Opening the journal failed"); | ||
| Expand Down Expand Up | @@ -438,8 +467,10 @@ static PyObject* Reader_close(Reader *self, PyObject *args) { | |
| assert(self); | ||
| assert(!args); | ||
|
|
||
| if (!self->closed) { | ||
| self->closed = 1; | ||
| decr_ref_count(self); /* decrement initial reference (without incr) */ | ||
| } | ||
| Py_RETURN_NONE; | ||
| } | ||
|
|
||
| Expand Down Expand Up | @@ -501,7 +532,7 @@ static PyObject* Reader_next(Reader *self, PyObject *args) { | |
| return NULL; | ||
| } | ||
|
|
||
| INCR_REF_BEGIN_ALLOW_THREADS(self) | ||
| if (skip == 1LL) | ||
| r = sd_journal_next(self->j); | ||
| else if (skip == -1LL) | ||
| Expand All | @@ -512,7 +543,7 @@ static PyObject* Reader_next(Reader *self, PyObject *args) { | |
| r = sd_journal_previous_skip(self->j, -skip); | ||
| else | ||
| assert(!"should be here"); | ||
| DECR_REF_END_ALLOW_THREADS(self) | ||
|
|
||
| if (set_error(r, NULL, NULL) < 0) | ||
| return NULL; | ||
| Expand Down Expand Up | @@ -782,9 +813,9 @@ PyDoc_STRVAR(Reader_seek_head__doc__, | |
| "See :manpage:`sd_journal_seek_head(3)`."); | ||
| static PyObject* Reader_seek_head(Reader *self, PyObject *args) { | ||
| int r; | ||
| INCR_REF_BEGIN_ALLOW_THREADS(self) | ||
| r = sd_journal_seek_head(self->j); | ||
| DECR_REF_END_ALLOW_THREADS(self) | ||
|
|
||
| if (set_error(r, NULL, NULL) < 0) | ||
| return NULL; | ||
| Expand All | @@ -800,9 +831,9 @@ PyDoc_STRVAR(Reader_seek_tail__doc__, | |
| static PyObject* Reader_seek_tail(Reader *self, PyObject *args) { | ||
| int r; | ||
|
|
||
| INCR_REF_BEGIN_ALLOW_THREADS(self) | ||
| r = sd_journal_seek_tail(self->j); | ||
| DECR_REF_END_ALLOW_THREADS(self) | ||
|
|
||
| if (set_error(r, NULL, NULL) < 0) | ||
| return NULL; | ||
| Expand All | @@ -820,9 +851,9 @@ static PyObject* Reader_seek_realtime(Reader *self, PyObject *args) { | |
| if (!PyArg_ParseTuple(args, "K:seek_realtime", ×tamp)) | ||
| return NULL; | ||
|
|
||
| INCR_REF_BEGIN_ALLOW_THREADS(self) | ||
| r = sd_journal_seek_realtime_usec(self->j, timestamp); | ||
| DECR_REF_END_ALLOW_THREADS(self) | ||
|
|
||
| if (set_error(r, NULL, NULL) < 0) | ||
| return NULL; | ||
| Expand Down Expand Up | @@ -850,17 +881,17 @@ static PyObject* Reader_seek_monotonic(Reader *self, PyObject *args) { | |
| if (set_error(r, NULL, "Invalid bootid") < 0) | ||
| return NULL; | ||
| } else { | ||
| INCR_REF_BEGIN_ALLOW_THREADS(self) | ||
| r = sd_id128_get_boot(&id); | ||
| DECR_REF_END_ALLOW_THREADS(self) | ||
|
|
||
| if (set_error(r, NULL, NULL) < 0) | ||
| return NULL; | ||
| } | ||
|
|
||
| INCR_REF_BEGIN_ALLOW_THREADS(self) | ||
| r = sd_journal_seek_monotonic_usec(self->j, id, timestamp); | ||
| DECR_REF_END_ALLOW_THREADS(self) | ||
|
|
||
| if (set_error(r, NULL, NULL) < 0) | ||
| return NULL; | ||
| Expand Down Expand Up | @@ -924,9 +955,9 @@ static PyObject* Reader_process(Reader *self, PyObject *args) { | |
|
|
||
| assert(!args); | ||
|
|
||
| INCR_REF_BEGIN_ALLOW_THREADS(self) | ||
| r = sd_journal_process(self->j); | ||
| DECR_REF_END_ALLOW_THREADS(self) | ||
| if (set_error(r, NULL, NULL) < 0) | ||
| return NULL; | ||
|
|
||
| Expand All | @@ -950,9 +981,9 @@ static PyObject* Reader_wait(Reader *self, PyObject *args) { | |
| if (!PyArg_ParseTuple(args, "|L:wait", &timeout)) | ||
| return NULL; | ||
|
|
||
| INCR_REF_BEGIN_ALLOW_THREADS(self) | ||
| r = sd_journal_wait(self->j, timeout); | ||
| DECR_REF_END_ALLOW_THREADS(self) | ||
|
|
||
| if (set_error(r, NULL, NULL) < 0) | ||
| return NULL; | ||
| Expand All | @@ -970,9 +1001,9 @@ static PyObject* Reader_seek_cursor(Reader *self, PyObject *args) { | |
| if (!PyArg_ParseTuple(args, "s:seek_cursor", &cursor)) | ||
| return NULL; | ||
|
|
||
| INCR_REF_BEGIN_ALLOW_THREADS(self) | ||
| r = sd_journal_seek_cursor(self->j, cursor); | ||
| DECR_REF_END_ALLOW_THREADS(self) | ||
|
|
||
| if (set_error(r, NULL, "Invalid cursor") < 0) | ||
| return NULL; | ||
| Expand Down Expand Up | @@ -1035,9 +1066,9 @@ static PyObject* Reader_query_unique(Reader *self, PyObject *args) { | |
| if (!PyArg_ParseTuple(args, "s:query_unique", &query)) | ||
| return NULL; | ||
|
|
||
| INCR_REF_BEGIN_ALLOW_THREADS(self) | ||
| r = sd_journal_query_unique(self->j, query); | ||
| DECR_REF_END_ALLOW_THREADS(self) | ||
|
|
||
| if (set_error(r, NULL, "Invalid field name") < 0) | ||
| return NULL; | ||
| Expand Down Expand Up | @@ -1172,9 +1203,9 @@ static PyObject* Reader_get_catalog(Reader *self, PyObject *args) { | |
| assert(self); | ||
| assert(!args); | ||
|
|
||
| INCR_REF_BEGIN_ALLOW_THREADS(self) | ||
| r = sd_journal_get_catalog(self->j, &msg); | ||
| DECR_REF_END_ALLOW_THREADS(self) | ||
|
|
||
| if (r == -ENOENT) { | ||
| const void* mid; | ||
| Expand Down Expand Up | @@ -1264,7 +1295,7 @@ static int Reader_set_data_threshold(Reader *self, PyObject *value, void *closur | |
| PyDoc_STRVAR(closed__doc__, | ||
| "True iff journal is closed"); | ||
| static PyObject* Reader_get_closed(Reader *self, void *closure) { | ||
| return PyBool_FromLong(self->closed || !self->j); | ||
| } | ||
|
|
||
| static PyGetSetDef Reader_getsetters[] = { | ||
| Expand Down Expand Up | @@ -1330,7 +1361,7 @@ static PyTypeObject ReaderType = { | |
| .tp_methods = Reader_methods, | ||
| .tp_getset = Reader_getsetters, | ||
| .tp_init = (initproc) Reader_init, | ||
| .tp_new = Reader_new, | ||
| }; | ||
|
|
||
| static PyMethodDef methods[] = { | ||
| Expand Down | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityThat should probably be dropped as well, since it is EOL or in other words: with 3.7 dropped I see no reason not to drop 3.8
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityI dropped 3.7 only because of switch to ubuntu-latest, which doesn't have it.
I don't see any reason to drop 3.8. at least as long as it is compatible and running yet.
Anyway has nothing to do with the PR as is, I made the changes just in order to repair the GHA flow.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.