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