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 _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
