[systemd-devel] python - reading the journal

[Steven Hiscocks]

On 08/02/13 20:51, Zbigniew Jędrzejewski-Szmek wrote:
> On Fri, Feb 08, 2013 at 07:51:48PM +0000, Steven Hiscocks wrote:
>> 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
>>>
>>
>> Out of interest, I had a quick go myself :)
>> https://github.com/kwirk/systemd/commit/7207a5547924684bc54eaad0fdff706eec2402a5
> Looks nice. But I want to clean up those Py_RunString and add full
> error handling before we merge it.
>
> I prepared a patch for the id128 stuff (https://github.com/systemd/systemd/pull/1).
> David Strauss rightly suggested converting the constants to uuid.UUID.
>
> Looking at your module, I'm starting to think that it would be good
> to split out call_dict/default_func stuff into a separate class in
> pure Python. It'll have to be either pure Python or quite a bit of C
> code, which I'm pretty sure nobody wants to write.
>
> Zbyszek
>

I thought the easiest way was to just inherit the Journal (now 
_Journal), replacing __new__ with all the python RunString stuff. 
https://github.com/kwirk/systemd/commit/9003fdbc69577840ab6a1501a1c49f7377b6124d

-- 
Steven Hiscocks