cyr_expire: add noexpire_until annotation #4533
Conversation
rsto
force-pushed
the
cyr_expire_noexpire
branch
from
f63918e
to
f053bd9
Compare
|
CI shows this failing these two tests: |
There was a problem hiding this comment.
Two failing tests under CI, though they might just be stale tests following these changes.
Of greater concern to me is the syntax incompatibility within cyr_expire between durations as received from the command line vs durations read from annotations. This needs to be carefully documented, or perhaps homogenised... and then carefully documented. Details within. :)
imap/cyr_expire.c
Outdated
| time_t until; | ||
| if (get_time_annotation(mbname_intname(mbname), | ||
| IMAP_ANNOT_NS "noexpire_until", &until, false)) | ||
| ret = !until || progtime < until; |
There was a problem hiding this comment.
time_t is signed, so there's an odd discontinuity in the value range here like:
| [valid negative times] [0=infinity] [valid positive times] |.
I'm not sure if that's likely to be a problem here, but it would be easier to reason about if the ret = ... line was unfolded into separate checks for each possibility.
There was a problem hiding this comment.
These conditionals should be fine. Quoting GNU libc documentation:
On POSIX-conformant systems, time_t is an integer type and its values represent the number of seconds elapsed since the epoch, which is 00:00:00 on January 1, 1970, Coordinated Universal Time.
Also, the annotation value type in annotate.c is defined be an unsigned integer, so any negative value will be rejected on write.
I'm not sure if there's a need to break up the lines, still?
There was a problem hiding this comment.
In that case it might be worth putting a comment there, so the next reader doesn't need to wrack their brain wondering whether there's a bad edge case for pre-1970 dates (negative time_t)
There was a problem hiding this comment.
| The first three mailbox annotations specify the age of messages in the | ||
| given mailbox that should be expired/archived/deleted. The | ||
| age is specified as a duration, the default unit are days. |
There was a problem hiding this comment.
Need to describe the valid annotation durations here (or refer them to imapd.conf(5), which already describes them), and also need to be explicit that these are different from the durations supported in the command line arguments (which are described under the heading for -D, --delete-duration).
The command line durations are parsed by parse_duration(), a static function in cyr_expire.c, which allows and understands decimal places (e.g. 1.5d) for whatever reason, though it reduces to integer seconds in the end. config_parseduration() doesn't accept decimals because we didn't think it necessary -- the same thing can be expressed as 36h or 1d12h, for example (the latter of which parse_duration() can't handle). So as presently implemented, valid command line durations and valid annotation/config durations are superficially similar, but aren't strictly interchangeable.
An approach here might be to just get rid of parse_duration(), and switch to using config_parseduration() for the command-line options too. We'd need to document the change loudly when we release 3.10, in case anyone had been relying on the decimal parsing, but at least then all our "durations" will be consistent (unless there's others hiding somewhere...). If we decide decimals are important enough to keep after all, we could probably extend config_parseduration() to support them. (Though that could get messy... what should 1.5h12m do?)
The description in imapd.conf(5) comes from here: https://github.com/cyrusimap/cyrus-imapd/blob/master/lib/imapoptions#L87-L94. But you can't just copy and paste that into an RST document, it's in the wrong format (for silly historical reasons). If you want to copy and paste this text, first make man so that docsrc/imap/reference/manpages/configs/imapd.conf.rst gets generated, then copy and paste from that.
The RST syntax for linking to one of our man pages is:
:cyrusman:`imapd.conf(5)`
There was a problem hiding this comment.
Argh, thanks for this. I was totally not aware of this.
There was a problem hiding this comment.
cyr_expire is a mess and it feels everything we try to clean up in it shakes out more chaos 😅
There was a problem hiding this comment.
I now replaced custom cyr_expire durations with config_parseduration: cf10431
cyr_expire already supports reading durations such as '1h' but only numeric durations defaulting to day units could be set via IMAP. Signed-off-by: Robert Stepanek <[email protected]>
cyr_expire annotations are either set on a mailbox or any of its parents. But looking up annotations on the parent mailboxes got broken in 2018 during mailbox name to mailbox id migration. Signed-off-by: Robert Stepanek <[email protected]>
It's just noisy and in its current form does not provide enough context for debugging anyway. Signed-off-by: Robert Stepanek <[email protected]>
rsto
force-pushed
the
cyr_expire_noexpire
branch
from
f053bd9
to
cf10431
Compare
There was a problem hiding this comment.
That looks good, thanks.
The two Metadata tests are still failing in CI. I haven't looked closely at them; I assume they're probably just out of date wrt these changes and need to be updated, but they might be complaining about something real. They failed for me locally last time I reviewed, and I assume they would do the same again, but I haven't actually pulled the branch and built/tested it this time.
Signed-off-by: Robert Stepanek <[email protected]>
Signed-off-by: Robert Stepanek <[email protected]>
rsto
force-pushed
the
cyr_expire_noexpire
branch
from
cf10431
to
f8e55e7
Compare
Fixed now. |
This PR includes fixes and enhancements for cyr_expire:
expire/delete/archivemailbox annotations were only read for a given mailbox, rather than looking its value up in all the parent mailboxes.expire/delete/archiveannotations by use of the Cyrus-config supported duration format. cyr_expire already had included support to read such values, but they could only be set as command line flags.noexpire_untilannotation that allows to block expiring and deleting until a given future date, or indefinitely.This PR best is read commit by commit.