On 06/02/13 00:55, Zbigniew Jędrzejewski-Szmek wrote:
On Tue, Feb 05, 2013 at 11:45:10PM +0000, Steven Hiscocks wrote:
On 05/02/13 23:00, Zbigniew Jędrzejewski-Szmek wrote:
On Tue, Feb 05, 2013 at 09:22:46PM +0000, Steven Hiscocks wrote:
On 05/02/13 02:49, Zbigniew Jędrzejewski-Szmek wrote:
Hi,
On Mon, Feb 04, 2013 at 10:42:02PM +0000, Steven Hiscocks wrote:
I've made the suggested changes and pushed it to github. Feedback
welcomed :)
Thanks!
Some more thoughts on the API below. Some of those are probably
stupid, but I want to throw them out in the open, for your feedback.
SD_MESSAGE_* are string constants. Shouldn't they be int constants
like in C? The conversion both ways is pretty simple, but if the
constants were used outside of journal matches it would be nicer
to have them as ints. The downside would be that the user
would have to printf the int to use it in a match. But... see next
point.
It would be nice to expose the rest of sd-id128 API:
sd_id128_to_string(3), sd_id128_randomize(3),
sd_id128_get_machine(3). They would probably go in a separate module
(systemd.id128), since they are useful in writing journal entries too.
Okay. Sounds like they should be dropped from the current code, as
in the future the SD_MESSAGE_* constants will be accessed via python
module systemd.id128?
Yes.
I think that once pyjournalctl is part of the systemd tree, the constants
should be generated from sd-messages.h by a script. Otherwise, we'll
be constantly forgetting to update those.
journal.seek_monotonic(int(monotonic.total_seconds()*1E6), bootid)
Python interfaces usually use floating point numbers to mean
seconds. A double has ~16 significant numbers, so the accuracy should
be enough, so I believe the detail that this is microseconds should
be hidden.
Makes sense to me. Done.
It would be better to replace PyRun_String with normal C methods,
but that can be done later.
Yeah... I cheated a bit here ;)
sd_journal_open_directory is not wrapped, but that can be added
later.
Good point, easy enough to add. Done.
What about renaming Journalctl to Journal? It doesn't really control
anything :)
Yeah. I wasn't too sure on the name when I got started. I was
concious of not clashing with the present systemd.journal. What is
the overall planned structure for the python modules, and where
would this fit in?
Good question. Once the SD_MESSAGE constants are moved, pyjournalctl
will only export Journalctl and a few constants. If think that could
go straight into the systemd.journal module. _journal.so already
links against libsystemd-journal.so.0, so I don't think that the
additional code for Journalctl will make any different.
Specifically: rename pyjournalctl.c to src/python-systemd/_reader.c
(unless somebody comes up with a better name), and Journalctl to Journal.
In journal.py import Journal and the constants from _reader.
SD_JOURNAL_LOCAL_ONLY should probably be renamed to LOCAL_ONLY
(SD_JOURNAL_RUNTIME_ONLY, SYSTEM_ONLY likewise). Here namespaceing
will be provided by the module, so there's no need for the long name.
Good point. Done.
Second argument to .seek(), a documentation only change: it would be
nice to use io.SEEK_SET, io.SEEK_CUR, io.SEEK_END in the description.
I had this in mind when developing, but was just a bit lazy and
stuck the number in :-p . Done.
Should .query_unique() return a set instead? This would make checking
if an field is present faster, and also underline the fact that those
are non-repeating entries.
Of course! Done.
Your module will be great for creating a test suite for journal. At the
same time it will also serve as a test suite for the module.
Zbyszek
Thanks again for the feedback. Latest changes pushed to github.
Thank you for your work.
Let me know what you think about the proposed integration scheme.
Zbyszek
Okay. Sounds good.
You'll have to pardon my ignorance :), but my experience of git is
limited to use of github...
What's the best way to go about achieving this? Should I fork the
systemd-repo from freedesktop, putting pyjournalctl.c in as
src/python-systemd/_reader.c (and make other changes mentioned) and
use `git format-patch` to submit via email?
I'll do it. I need to throughly check if everything compiles anyway.
Zbyszek
Sure thing :)
Just also wanted to give you a heads up that I've also got a pull
request on fail2ban for having it utilise the journal
(https://github.com/fail2ban/fail2ban/pull/82)
--
Steven Hiscocks
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
