Hi Bron,
On Mon, Jan 12, 2015, at 12:48 AM, Bron Gondwana wrote:
> Hi James,
>
> Sorry I haven't replied to anything earlier. I was on holiday in Tasmania
> for the past couple of weeks, and totally out of contract (rafting on a
> remote river that doesn't even have internet access).
>
Sounds like fun!
> I'll also be away for most of the next couple of weeks, at a choir festival.
> All my holidays are coming at once. And after that I'll be at the CalConnect
> conference in San Jose and then Fosdem in Brussels.
>
It is a busy time of the year, I know.
> In between this, I would love to read and review your code. I'm just hoping
> I can find the time! We also have to get 2.5 ready to go, because I've
> promised a zillion people that it will be out by Fosdem at the latest.
>
Yeah, I don't expect to include the variables extension in 2.5,
but possibly in 2.5.1 or later, depending on how much testing
it gets and how long it takes to fix any bugs.
> So I will try to get at least something for you before I leave on Wednesday :)
>
Sounds great!
> Regards,
>
> Bron.
>
V/r,
James Cassell
> On Mon, Jan 12, 2015, at 04:19 PM, James Cassell wrote:
> > Hello,
> >
> > I believe I have a more or less complete implementation of RFC 5229,
> > Sieve: Variables Extension.
> >
> > tl;dr: I'd like help making sure that the sanity checks currently
> > performed at script compile time are also performed at runtime for
> > the actions (or tests) that require it.
> >
> > At this point, I have cleaned up the patches, and I plan to drop the
> > ones that haven't been cleaned up since they were mostly for
> > debugging. I'm open to any feedback if anyone would like the commits
> > reworked in a particular way.
> >
> > I plan to add some more CUnit tests to get some ongoing automated
> > testing to make sure nothing breaks going forward, though I'm (as
> > always) short on time.
> >
> > One problem that I have thought of is that checks performed in the
> > sieve parser (sieve/sieve.y) will possibly need to be performed at
> > runtime if the variables extension is enabled. These checks are in
> > verify_utf8, verify_regex, verify_envelope, verify_addrheader,
> > verify_header, verify_mailbox, and verify_address, in addition to any
> > one-off checks in any individual rules. It appears that this will
> > apply mainly to actions, and possibly to the regex match-type. It
> > might also apply to what happens if an address or envelope test is
> > applied to a non-address containing header, or if a test becomes
> > malformed. In such a case, should we have the test return false, or
> > should we abort the script and fallback to the implicit keep? The
> > thing I hate about runtime errors is that there is no notification to
> > the script author that they happened.
> >
> >
> > Updates from last time:
> >
> > On Tue, Nov 11, 2014, at 11:12 AM, James Cassell wrote:
> > [...]
> > > What works:
> > > - setting variables using the SET action
> > > - match variables are set when the :matches match-type is used (${0},
> > > ${1}, etc)
> > > - did this by overloading the comprock, which had been null for
> > > :matches test
> > > - should I do this differently?
> > > - most of the places that you would normally use a string, you can also
> > > add variables to your strings
> > >
> >
> > This all still works.
> >
> > > What doesn't work:
> > > - modifiers to the set action
> >
> > Done
> >
> > > - the STRING test
> >
> > Done
> >
> > > - :regex match-type
> > > - Ken, I might need your help on this one
> > >
> >
> > I don't think I'll get to this one and I don't consider it crucial
> > since its not an official RFC.
> >
> > > What I don't know how to do:
> > > - evaluate for UTF-8 support
> >
> > I still have no idea what I should be looking for here.
> >
> > [...]
> > > What I'd like reviewed:
> > > - the code
> > > - the above stuff that I don't know
> > > - security issues due to VARIABLES interaction with sieve actions
> > > - anything else you can think of
> > >
> >
> > Still applies
> >
> > > Also, I believe the first five patches are acceptable for master. (I
> > > won't push them, though, until I've given people a chance to look them
> > > over if they want.)
> > >
> >
> > At this point, I'll likely just push them when merging variables to
> > master.
> >
> > I look forward to any feedback!
> >
> > Thanks!
> >
> >
> > V/r,
> > James Cassell
> >
> >
> >
> > The following changes since commit e1ff81794b435586d640fa41e16a285f8bccdeb9:
> >
> > [doc] clarify how to write CUnit tests (2015-01-02 21:21:53 -0500)
> >
> > are available in the git repository at:
> >
> > git://git.cyrusimap.org/cyrus-imapd.git
> > dev/cassell/for-review-sieve-variables-rev1
> >
> > for you to fetch changes up to 099c7abf2bf98ea8ce9fa44c6f93585587e6734d:
> >
> > [sieve] grammar.c: check attempted use of namespaced variables
> > (2015-01-09 17:32:04 -0500)
> >
> > ----------------------------------------------------------------
> > James Cassell (50):
> > [sieve] bc_eval.c: allow {add,set,remove}flag actions with reject
> > [sieve] change imap4flags variable names to support VARIABLES
> > extension
> > [sieve] use central variable_list_t to hold action flag lists
> > [sieve] bc_eval.c: do all imap4flags processing in bc_eval.c
> > [sieve] remove do_{{add,set,remove}flag,{un,}mark}() functions
> > [sieve] sieve.y: define tokens for VARIABLES extension
> > [sieve] sieve-lex.l: teach lexer VARIABLES tokens
> > [sieve] add VARIABLES reference
> > [sieve] enable awareness of the variables extension
> > [sieve] add parser support for string test
> > [sieve] add parser support for SET action
> > [sieve] bytecode.h: add codepoints required by variables base spec
> > [sieve] bc_generate.c: variables base spec support
> > [sieve] bc_dump.c: variables base spec support
> > [sieve] bc_emit.c: variables base spec support
> > sieve test script
> > [sieve] sieved.c: variables base spec support
> > [sieve] store require'd extensions in the bytecode
> > [sieve] implement parse_string() to resolve variables in strings
> > [sieve] special variables for storing match vars and parsed strings
> > [sieve] implement variables SET action
> > debug printf
> > [sieve] bc_eval.c: pass variables to eval_bc_test()
> > [sieve] add support for match variables to :matches match-type
> > [sieve] comparator.c: *_matches(): only use given void *rock if
> > non-NULL
> > [sieve] grammar.c: parse_string(): Allow parsing of match variables
> > [sieve] bc_eval.c: parse strings for variables when reading bytecode
> > [sieve] bc_eval.c: attempt proper variables support in imap4flags
> > break line to avoid horiz scrolling
> > BC_STRING maps to BC_HASFLAG
> > test script changes
> > temp
> > [sieve] bc_eval.c: implement variables STRING test
> > update for debugging
> > [sieve] sieve.y: support variables in HASFLAG test
> > [sieve] bc_eval.c: support non-existent variable in hasflag test
> > [sieve] variables.[ch]: template for SET action modifiers
> > [sieve] variables.c: allocate buffer for resultant modified string
> > [sieve] bc_eval.c: body test should not set match variables
> > [sieve] variables.c: implement and test set action modifiers
> > [cunit] sieve: :quotewildcard with :encodeurl in variable_modifiers
> > [sieve] bc_eval.c: apply specified variable modifiers in set action
> > [sieve] support variables extension in setflag action
> > [sieve] parser support for {add,remove}flag actions with variables
> > [sieve] support variables extension in {add,remove}flag actions
> > [sieve] bc_eval.c: plan support for :count with variables in tests
> > [sieve] support :count with variables in hasflag and string tests
> > [sieve] bc_eval.c: break from loop after calculating :count
> > [sieve] flags.[ch]: properly use EXPORTED
> > [sieve] grammar.c: check attempted use of namespaced variables
> >
> > Makefile.am | 4 +
> > cunit/sieve.testc | 79 +++-
> > doc/specs.html | 2 +
> > lib/imapoptions | 2 +-
> > sieve/README | 3 +
> > sieve/bc_dump.c | 59 ++-
> > sieve/bc_emit.c | 84 +++-
> > sieve/bc_eval.c | 517
> > ++++++++++++++++++---
> > sieve/bc_generate.c | 105 ++++-
> > sieve/bytecode.h | 34 +-
> > sieve/comparator.c | 96 +++-
> > sieve/flags.c | 4 +
> > sieve/flags.h | 2 +-
> > sieve/grammar.c | 167 +++++++
> > sieve/grammar.h | 18 +
> > sieve/message.c | 84 ----
> > sieve/script.c | 67 +--
> > sieve/script.h | 1 +
> > sieve/sieve-lex.l | 16 +-
> > sieve/sieve.y | 309 ++++++++++--
> > sieve/sieve_interface.h | 1 +
> > sieve/sieved.c | 63 ++-
> > .../actionExtensions/uberExtensionActionScript.s | 76 +--
> > .../tests/testExtension/uberExtensionTestScript.s | 59 ++-
> > sieve/tree.c | 9 +-
> > sieve/tree.h | 12 +-
> > sieve/variables.c | 197 ++++++++
> > sieve/variables.h | 13 +
> > sieve/varlist.h | 2 +
> > 29 files changed, 1724 insertions(+), 361 deletions(-)
> > create mode 100644 sieve/grammar.c
> > create mode 100644 sieve/grammar.h
> > create mode 100644 sieve/variables.c
> > create mode 100644 sieve/variables.h
>
>
> --
> Bron Gondwana
> br...@fastmail.fm
