I keep writing lengthy emails about how we can use this as an opportunity to reduce redundancy and improve consistency, but I should probably ping you on #systemd IRC to hash it out. I can't think of anything elegant that doesn't involve altering the existing journal.py or _reader.c code.
What's your handle? Mine's davidstrauss. As a starter, I want to enumerate the places where __REALTIME_TIMESTAMP has special handling: (1) Existing code: A C-based member function in _reader.c. (2) Existing code: A native Python conversion function in journal.py. (3) Existing code: An entry in DEFAULT_CONVERTERS. (4) Added in your patch: A key setting in get_next(). (5) Added in your patch: A Python-based member function in journal.py that overrides (1). (6) Added in your patch: A condition in get() that invokes (1) when the specific field is requested. While I want to fix this bug, I also don't want six scattered special cases for __REALTIME_TIMESTAMP and similar fields. On Sat, Apr 13, 2013 at 4:18 PM, Steven Hiscocks <[email protected]> wrote: > On 13/04/13 23:47, Steven Hiscocks wrote: >> >> On 13/04/13 23:00, David Strauss wrote: >>> >>> If seems like we should put the conditional special handling for >>> __REALTIME_TIMESTAMP and __MONOTONIC_TIMESTAMP in either _reader.c or >>> right in get(). >>> >>> Here's why: >>> * With the code above, calling Reader.get('__REALTIME_TIMESTAMP') >>> results in the wrong type (if anything, I'd need to check), but >>> Reader.get_realtime() gives the right thing. >>> * The type of Reader.get('__REALTIME_TIMESTAMP') is different from >>> Reader.get_next()['__REALTIME_TIMESTAMP'], and it should be >>> equivalent. >>> >> Good point. Currently, `get` for realtime, monotonic or cursor do not >> return anything, but raise an exception (invalid field). Given the plan >> to keep things in `_Reader` close to the C API, I'd suggest adding the >> handling within the `Reader.get`. Suggestion merged below: >> >> ``` >> ``` >>> >>> On Fri, Apr 12, 2013 at 10:13 AM, Steven Hiscocks >>> <[email protected]> wrote: >>>> >>>> Hi, >>>> >>>> In the python journal Reader, the splitting out of monotonic and >>>> realtime >>>> stamps, has affected `get_next` function as timestamp values are no >>>> longer >>>> present in the dictionary returned. Also the new `get_monotonic` and >>>> `get_realtime` functions are not run through the converters. Equally, >>>> the >>>> 'get' method added is not run through the converters. I also noted the >>>> additional `next` method doesn't work on python2, as it clashes with the >>>> iter `next` method (python3 not affected as it changes iter method to >>>> `__next__`) >>>> >>>> My suggestion with the python Reader `get_next` method is that the >>>> realtime >>>> and monotonic timestamps remain part of it, as these are key parts of >>>> a log >>>> entry, and two more lines in everyone's code to get them seems >>>> cumbersome. >>>> Equally also the cursor value. This also makes the output fields the >>>> same as >>>> the journalctl json format. (I agree it makes sense the _Reader object >>>> element to remain separate so close to actual C API). >>>> >>>> I'm not sure what the best approach to the `next` method issue is… >>>> >>>> Proposed changes below. I've add `get_cursor` to go through >>>> converters for >>>> consistency, even if not required currently. >>>> >>>> ``` >>>> ``` >>>> >>>> Thanks >>>> -- >>>> Steven Hiscocks >>>> _______________________________________________ >>>> systemd-devel mailing list >>>> [email protected] >>>> http://lists.freedesktop.org/mailman/listinfo/systemd-devel >>> >>> >>> >>> >> >> > Just noticed an unintended effect if _Reader.get_next returns empty > dictionary. Fixed: > > ``` > diff --git a/src/python-systemd/journal.py b/src/python-systemd/journal.py > index 48f57ac..281d7c3 100644 > > --- a/src/python-systemd/journal.py > +++ b/src/python-systemd/journal.py > @@ -189,6 +189,28 @@ class Reader(_Reader): > for arg in args: > super(Reader, self).add_match(arg) > > + def get(self, key): > + if key == "__REALTIME_TIMESTAMP": > + return self.get_realtime() > + elif key == "__MONOTONIC_TIMESTAMP": > + return self.get_monotonic() > + elif key == "__CURSOR": > + return self.get_cursor() > + else: > + return self._convert_field(key, super(Reader, self).get(key)) > + > + def get_realtime(self): > + return self._convert_field( > + '__REALTIME_TIMESTAMP', super(Reader, self).get_realtime()) > + > + def get_monotonic(self): > + return self._convert_field( > + '__MONOTONIC_TIMESTAMP', super(Reader, self).get_monotonic()) > + > + def get_cursor(self): > + return self._convert_field( > + '__CURSOR', super(Reader, self).get_cursor()) > + > def get_next(self, skip=1): > """Return the next log entry as a dictionary of fields. > > @@ -197,8 +219,14 @@ class Reader(_Reader): > > Entries will be processed with converters specified during > Reader creation. > """ > - return self._convert_entry( > + entry = self._convert_entry( > super(Reader, self).get_next(skip)) > + if entry: > > + entry['__REALTIME_TIMESTAMP'] = self.get_realtime() > + entry['__MONOTONIC_TIMESTAMP'] = self.get_monotonic() > + entry['__CURSOR'] = self.get_cursor() > + > + return entry > > def query_unique(self, field): > """Return unique values appearing in the journal for given `field`. > ``` > > -- > Steven Hiscocks -- David Strauss | [email protected] | +1 512 577 5827 [mobile] _______________________________________________ systemd-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/systemd-devel
