Mailing List Archive

Jakob Hirsch wrote:

> I kind of overloaded the acl set command so people can use it just as
> the numbered acl vars. I did the same with the spool file format, so you
> now have "-aclm m_bla 1234" in your spoolfile, so changes to externals
> tools should be minimal (or zero, ideally).

You shouldn't use -aclm or -aclc, the problem is that prior versions of
exim use this code to check for a valid entry:

--------------------- SNIP -----------------------------------
else if (Ustrncmp(big_buffer, "-aclc ", 6) == 0)
{
int index, count;
if (sscanf(CS big_buffer + 6, "%d %d", &index, &count) != 2)
goto SPOOL_FORMAT_ERROR;
--------------------- SNIP -----------------------------------

So, if someone uses the patch and the new acl-variables, he won't be
able to switch back to an old version, unless he modifies the -H files
manually.

We had the some problem some years ago, when the acl-storage was
introduced. Back then, Philip was a bit lazy and allowed to spread the
acl-variables over multiple lines, something that prior versions of exim
couldn't handle, because they just ignored the -acl line and tried the
next one, which didn't start with a dash ;)

Guess what happened, some people needed to switch back to their old
version (not sure, i think there was a bug in the new release), and
found out that their old exim couldn't handle the data anymore ;)

.... just had a look at the current code (damn, it's a long time ago
since i had a look at the exim code *G*) it seems like the recent(?)
change to use -aclm and -aclc in favor of -acl didn't care for this
problem either, so maybe a wrote a lot of useless text *G*.

> I'm not sure if using store_release if a variable is redefined is
> correct, but it's easy to remove that. The existing aclvar code does not

It would be better to check the size of your current value and maybe
overwrite it if it fits, store_release is only able to release store if
it is on top of the stack.

> To find a variable, I use a simple linear search, which is not the most
> efficient method (euphemism alert!), but as most people only use a few
> variables there's is really no need for a bloated hash function with
> collision handling. And many people do just the same with files...

Exim already includes code for balanced trees, check tree.c.
And you don't seem to know the exim community very well, they will use
new features until something breaks *G*

> Comments welcome, especially "this is soo cool" style. :)

This is sooooo cool! :)
But IMHO needs some rework. :)

Nico

--
## List details at http://www.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##

Quoting Nico Erfurth:

> .... just had a look at the current code (damn, it's a long time ago
> since i had a look at the exim code *G*) it seems like the recent(?)
> change to use -aclm and -aclc in favor of -acl didn't care for this
> problem either, so maybe a wrote a lot of useless text *G*.

I think you have a point, but changing the keyword will not help. As you
wrote, old versions will not detect the new keyword, skip the line, but
bail out on the next line. One way to prevent that would be to put the
value in the same line (and encode line feeds). But then people wouldn't
notice that some variables are suddenly lost, which they probably rely
on. With exim complaining about broken queue files, they can at least
fix them by replacing the name with the number they used before (not
only the spool files, but also in the config and all the filter files).

>> I'm not sure if using store_release if a variable is redefined is
>> correct, but it's easy to remove that. The existing aclvar code does not
> It would be better to check the size of your current value and maybe
> overwrite it if it fits, store_release is only able to release store if
> it is on top of the stack.

I've seen no function in store.c which tells me the size. I could put it
in the acl_named_var struct, but I have the feeling that this is not
really necessary (and was we all know, "premature optimization is the
root of all evil").

>> To find a variable, I use a simple linear search, which is not the most
>> efficient method (euphemism alert!), but as most people only use a few
>> variables there's is really no need for a bloated hash function with
>> collision handling. And many people do just the same with files...
> Exim already includes code for balanced trees, check tree.c.

I thought there must be something, but wanted to keep it simple and
therefore not use some bloated API, but this looks nice. Again, I think
it's not really needed, but if it helps to get this into Exim, I will
happily do it. This would even simplify my own code. :)

> And you don't seem to know the exim community very well, they will use
> new features until something breaks *G*

Nothing will break, of course. :)



--
## List details at http://www.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##

On Mon, 7 Aug 2006, Nico Erfurth wrote:

> > Comments welcome, especially "this is soo cool" style. :)
>
> This is sooooo cool! :)

I have to admit that my gut feeling doesn't like this because it is
adding more code and complication, just to avoid using macros for
variable names. But then I come from the "if it isn't written in
assembler and uses every last bit, it's too expensive" school of
thought, having been writing code for way too long. So I'll wait to see
what other reactions there on this list.


--
Philip Hazel University of Cambridge Computing Service
Get the Exim 4 book: http://www.uit.co.uk/exim-book

--
## List details at http://www.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##

On Sun, Aug 06, 2006 at 04:37:24PM +0200, Jakob Hirsch wrote:
> with the attached patch, you can use acl_m_something and acl_c_anything.
> Memory is allocated dynamically, so there is no compile time limit.

I very much like that idea. From the distributions point of view, this
eases our work since we now can use ACL variables in our default
configuration without risking to break user setups.

Greetings
Marc

--
-----------------------------------------------------------------------------
Marc Haber | "I don't trust Computers. They | Mailadresse im Header
Mannheim, Germany | lose things." Winona Ryder | Fon: *49 621 72739834
Nordisch by Nature | How to make an American Quilt | Fax: *49 621 72739835

--
## List details at http://www.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##

On Mon, 7 Aug 2006, Philip Hazel wrote:

> I have to admit that my gut feeling doesn't like this because it is
> adding more code and complication, just to avoid using macros for
> variable names. But then I come from the "if it isn't written in

As Marc said, from the viewpoint of a distributor it's great, as you can
create your own "namespace" like $acl_m_debian_* without limiting the
user or worse, having hard to trace problems because of the user using
your variables.

And for "more code and complication", I don't think that this really is a
problem here, exim contains far more code that is user by a VERY small
userbase. But with the adding of more and more variables and options to
exim, one should think about refactoring the current code a bit, so it
gets easier to maintain.

Nico

--
## List details at http://www.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##

Quoting Philip Hazel:

>>> Comments welcome, especially "this is soo cool" style. :)
>> This is sooooo cool! :)
> I have to admit that my gut feeling doesn't like this because it is
> adding more code and complication, just to avoid using macros for
> variable names.

Using macros to map names to numbers seemed always a little cumbersome
to me. And it does not work in filter files. We discussed this a while
ago (when you said you don't like named variables because they are
inefficient :).

As for code complexity: Sure, the new code makes changes in central
places, which is always dangerous, but I think it is pretty
straightforward, easy to read and to validate.

To be honest, I even thought about moving the existing numbered
variables to the same storage as the named ones (which would reduce code
complexity :), but then it seemed too revolutionary for now...
I doubt anybody would notice a performance hit, though, especially when
we'd use a binary tree.

> But then I come from the "if it isn't written in
> assembler and uses every last bit, it's too expensive" school of
> thought, having been writing code for way too long. So I'll wait to see
> what other reactions there on this list.

Quite fair.


--
## List details at http://www.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##

On Mon, 7 Aug 2006, Jakob Hirsch wrote:
>
> To be honest, I even thought about moving the existing numbered
> variables to the same storage as the named ones (which would reduce code
> complexity :)

Seems like a sensible idea to me.

Tony.
--
<fanf@exim.org> <dot@dotat.at> http://dotat.at/ ${sg{\N${sg{\
N\}{([^N]*)(.)(.)(.*)}{\$1\$3\$2\$1\$3\n\$2\$3\$4\$3\n\$3\$2\$4}}\
\N}{([^N]*)(.)(.)(.*)}{\$1\$3\$2\$1\$3\n\$2\$3\$4\$3\n\$3\$2\$4}}

--
## List details at http://www.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##

Nico Erfurth wrote:
> And for "more code and complication", I don't think that this really
> is a problem here, exim contains far more code that is user by a
> VERY small userbase.

You aren't perhaps arguing one may worsen things because they are quite
bad allready, are you? :o)

Personally, I like these named variables. This "acl_?\d" stuff is just
not memorable and thus provokes misconfiguration. Using macros to name
variables is in turn quite a "hack". (With "hack" being "a dirty one".)

lg,
daniel

--
## List details at http://www.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##

On Mon, 7 Aug 2006, Tony Finch wrote:

> On Mon, 7 Aug 2006, Jakob Hirsch wrote:
>>
>> To be honest, I even thought about moving the existing numbered
>> variables to the same storage as the named ones (which would reduce code
>> complexity :)
>
> Seems like a sensible idea to me.

Seems like the best idea to remove code duplication. There really is no
need to handle them seperatly and it wouldn't hurt performance THAT much.

Most complex configurations use database-lookups anyway, these are more of
a performance hog than a little btree search that would help to keep the
code clean and the configuration more flexible.

Nico



--
## List details at http://www.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##

On Mon, 7 Aug 2006, Jakob Hirsch wrote:

> I think you have a point, but changing the keyword will not help. As you
> wrote, old versions will not detect the new keyword, skip the line, but
> bail out on the next line. One way to prevent that would be to put the
> value in the same line (and encode line feeds). But then people wouldn't
> notice that some variables are suddenly lost, which they probably rely
> on. With exim complaining about broken queue files, they can at least
> fix them by replacing the name with the number they used before (not
> only the spool files, but also in the config and all the filter files).

They would notice, because exim will complain about an unknown variable.
;)

> I've seen no function in store.c which tells me the size. I could put it

There is none, the exim memory manager is very simple, but effective, if
used correctly.

> in the acl_named_var struct, but I have the feeling that this is not
> really necessary (and was we all know, "premature optimization is the
> root of all evil").

I don't think it's necessary either, but memory can pile up in some
configurations.
Just think of something like

set acl_c_myvar = ${eval: $acl_c_myvar + 1}

(I didn't use the expansion language for some time now, so i hope the
example is correct)

People tend to do the strangest things with exim. You don't wanna know
what kind of questions I get these days, even after I wasn't active in the
exim community for a loooooong time now.

> I thought there must be something, but wanted to keep it simple and
> therefore not use some bloated API, but this looks nice. Again, I think
> it's not really needed, but if it helps to get this into Exim, I will
> happily do it. This would even simplify my own code. :)

It's used in some places in the code (and maybe could be used in more
....).

> Nothing will break, of course. :)

Very selfconfident the young jedi is. ;)

Nico

--
## List details at http://www.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##

Quoting Nico Erfurth:

>> I think you have a point, but changing the keyword will not help. As you
>> wrote, old versions will not detect the new keyword, skip the line, but
>> bail out on the next line. One way to prevent that would be to put the
>> value in the same line (and encode line feeds). But then people wouldn't
>> notice that some variables are suddenly lost, which they probably rely
>> on. With exim complaining about broken queue files, they can at least
>> fix them by replacing the name with the number they used before (not
>> only the spool files, but also in the config and all the filter files).
> They would notice, because exim will complain about an unknown variable.
> ;)

Um, no, the current code skips unknown keywords in spool files. So, if
the whole variable's information is in a single line, an old version
will simply skip it and the variable is lost.

>> in the acl_named_var struct, but I have the feeling that this is not
>> really necessary (and was we all know, "premature optimization is the
>> root of all evil").
> I don't think it's necessary either, but memory can pile up in some
> configurations.
> Just think of something like
> set acl_c_myvar = ${eval: $acl_c_myvar + 1}

Yes, that was also my concern. But the code for numbered variables
simply allocates new storage space. I'm hesitant to change the
established behaviour if it never raised problems.

>> I thought there must be something, but wanted to keep it simple and
>> therefore not use some bloated API, but this looks nice. Again, I think
>> it's not really needed, but if it helps to get this into Exim, I will
>> happily do it. This would even simplify my own code. :)
> It's used in some places in the code (and maybe could be used in more
> ....).

Ok, there seems to be some consensus to consolidate named and numbered
vars, so I'll rework the code to do that and use btrees. Let's see what
Philip will say about it.

--
## List details at http://www.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##

On Mon, 7 Aug 2006, Jakob Hirsch wrote:

> Um, no, the current code skips unknown keywords in spool files. So, if
> the whole variable's information is in a single line, an old version
> will simply skip it and the variable is lost.

The code in spool_in.c will skip it, but exim will complain if the user
tries to use a variable that is unknown.

Hmmm, while thinking about it, how do you handle undefined variables now?
Empty string or error?

Nico

--
## List details at http://www.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##

> I very much like that idea. From the distributions point of view, this
> eases our work since we now can use ACL variables in our default
> configuration without risking to break user setups.

Good point. I had overlooked that.

> Ok, there seems to be some consensus to consolidate named and numbered
> vars, so I'll rework the code to do that and use btrees. Let's see what
> Philip will say about it.

Everyone (who is posting) is in favour, so it must be a good idea. :-)
If the job is done, it should be done properly. Allow names to consist
entirely of digits, then the numbered ones can be handled like the named
ones. I would suggest using the existing tree functions; sooner or later
somebody is going to have 1000 variables and linear search will be slow.

> Hmmm, while thinking about it, how do you handle undefined variables now?
> Empty string or error?

There are currently no undefined variables, only ones that you have not
set. They are empty. With names, it would probably be better to generate
an error so that a typo doesn't cause weird effects that are hard to
diagnose. And perhaps we could make an incompatible change and fault the
use of undefined numerical ones too.

You can take your time polishing the patch. I am unlikely to work on the
Exim code or seriously look at patches before September (other stuff to
do this week and next, and then 2 weeks' vacation).

--
Philip Hazel University of Cambridge Computing Service
Get the Exim 4 book: http://www.uit.co.uk/exim-book

--
## List details at http://www.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##

Quoting Philip Hazel:

> Everyone (who is posting) is in favour, so it must be a good idea. :-)
> If the job is done, it should be done properly. Allow names to consist
> entirely of digits,

This is already done, as named variables are identified by an underscore
after the c/m (e.g. acl_m_name1 in contrast to acl_m5). In the
consolidated code, this is not necessary any more.

>> Hmmm, while thinking about it, how do you handle undefined variables now?
>> Empty string or error?
> There are currently no undefined variables, only ones that you have not
> set. They are empty. With names, it would probably be better to generate
> an error so that a typo doesn't cause weird effects that are hard to
> diagnose. And perhaps we could make an incompatible change and fault the
> use of undefined numerical ones too.

Oh, are you sure about that? I think many people (including me) rely on
variables being empty by default. It's surely no big deal to set to the
empty string them at the beginning of the connect and mail acl. This
should probably be configurable (strict_var_names, like "use strict" in
perl).

> You can take your time polishing the patch. I am unlikely to work on the

I'll see when I can finish it. Would be good if others could also test it.

Have a nice vacation!



--
## List details at http://www.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##

On Mon, 7 Aug 2006, Jakob Hirsch wrote:

> Oh, are you sure about that? I think many people (including me) rely on
> variables being empty by default. It's surely no big deal to set to the
> empty string them at the beginning of the connect and mail acl.

It is if you don't know what their names are. :-)

But I don't really mind on this one. Whatever people want...


--
Philip Hazel University of Cambridge Computing Service
Get the Exim 4 book: http://www.uit.co.uk/exim-book

--
## List details at http://www.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##

Quoting Philip Hazel:

>> Oh, are you sure about that? I think many people (including me) rely on
>> variables being empty by default. It's surely no big deal to set to the
>> empty string them at the beginning of the connect and mail acl.
> It is if you don't know what their names are. :-)

I meant in the config file. Whoever edits it should know which variables
he uses.

> But I don't really mind on this one. Whatever people want...

I'd prefer to make that configurable, if you don't mind another config
option.


But now there's a different problem: I rewrote the code to use the btree
functions. Easy to handle while they are in memory, but what to do when
they go to disk? Dumping is easy, but when reading in, tree_insertnode
has to be called for every variable, which is expensive with all its
rebalancing etc. As far as I can see, there are three possible solutions:

1. Ignore it, it's a non-problem. The 1000-variables-person will have
already have this problem already when setting the variables, so he
should have a proper machine, anyway.

2. Write information about the tree into the spool file (like the
existing tree_write does). This an incompatible change, once again, but
it's easy to write a small script to rewrite the spool files into the
old format, should somebody have a need for that.

3. Use a hash function for lookups instead of the btree. There won't be
millions of records, so we can take something light-weight.


Being lazy, I'd prefer the first solution (works this way right now). :)
Comments, anyone?


--
## List details at http://www.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##

Jakob Hirsch wrote:

> But now there's a different problem: I rewrote the code to use the btree
> functions. Easy to handle while they are in memory, but what to do when
> they go to disk? Dumping is easy, but when reading in, tree_insertnode
> has to be called for every variable, which is expensive with all its
> rebalancing etc. As far as I can see, there are three possible solutions:
.......
> 2. Write information about the tree into the spool file (like the
> existing tree_write does). This an incompatible change, once again,
> but it's easy to write a small script to rewrite the spool files into
> the old format, should somebody have a need for that.

As the format will be changing anyway, I don't see a problem here.

Nico

--
## List details at http://www.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##

Quoting Nico Erfurth:

> > 2. Write information about the tree into the spool file (like the
> > existing tree_write does). This an incompatible change, once again,
> > but it's easy to write a small script to rewrite the spool files into
> > the old format, should somebody have a need for that.
> As the format will be changing anyway, I don't see a problem here.

In the current state, the format will not change. As long as you only
use old style numbered acl vars, the spool file will look exactly the
same as before.


--
## List details at http://www.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##

Hi List,

here's the version with binary trees (no changes to the spool file
format). I've added a strict_acl_vars config option (default is false),
which will let an expansion fail if an unset variable is used, so admins
can change their config (to be more safe against name typos) and then
activate it.

http://plonk.de/sw/exim/exim-4.63.named-acl-vars.patch



--
## List details at http://www.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##

On Mon, 14 Aug 2006, Jakob Hirsch wrote:

> here's the version with binary trees (no changes to the spool file
> format).

Noted, but it will be some weeks before I can look at it.

--
Philip Hazel University of Cambridge Computing Service
Get the Exim 4 book: http://www.uit.co.uk/exim-book

--
## List details at http://www.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##

On Mon, 14 Aug 2006, Jakob Hirsch wrote:

> here's the version with binary trees (no changes to the spool file
> format). I've added a strict_acl_vars config option (default is false),
> which will let an expansion fail if an unset variable is used, so admins
> can change their config (to be more safe against name typos) and then
> activate it.

I have applied this patch and committed it to CVS, so it will be
available in tonight's snapshot. Documentation in doc/NewStuff.

Nice patch, Jakob! I found only a few things that I felt needed to be
changed. For the record:

1. There was one bug. :-( But only one. :-) This line:

return ((node == NULL && !strict_acl_vars) ? US"": node->data.ptr);

fails when NODE==NULL and strict_acl_vars is true.

2. Most of the changes I made were to update comments in the area of the
patched code. I reworded some error messages.

3. You obviously didn't try to compile eximon with the patch on, because
it failed (eximon reads the spool -H file using some of the main Exim
modules). Eximon is a horrible hack as it is; I hacked it some more.
It was also easy to avoid the use of string_cat() in spool_in.c
(because this isn't currently included in eximon).

4. I removed ACL_CVARS and ACL_MVARS from src/config.h.defaults because
they are no longer used (and from doc/Optionlists.txt).

5. I changed the call to snprintf(), which is not in the C standard, to
use Exim's own string_format() function (which does the same thing).

That's all. Some testing of this new feature would be useful. Since
several people were in favour of this feature, I hope you'll try it
out.

Philip

--
Philip Hazel, University of Cambridge Computing Service.

--
## List details at http://www.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##

On Tue, Sep 19, 2006 at 12:40:11PM +0100, Philip Hazel wrote:
> That's all. Some testing of this new feature would be useful. Since
> several people were in favour of this feature, I hope you'll try it
> out.

Do you want me to package a snapshot for Debian experimental?

Greetings
Marc

--
-----------------------------------------------------------------------------
Marc Haber | "I don't trust Computers. They | Mailadresse im Header
Mannheim, Germany | lose things." Winona Ryder | Fon: *49 621 72739834
Nordisch by Nature | How to make an American Quilt | Fax: *49 621 72739835

--
## List details at http://www.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##

Quoting Philip Hazel:

> Nice patch, Jakob! I found only a few things that I felt needed to be

Thanks.

> 1. There was one bug. :-( But only one. :-) This line:
>
> return ((node == NULL && !strict_acl_vars) ? US"": node->data.ptr);
>
> fails when NODE==NULL and strict_acl_vars is true.

ah, stupid braino... thought of node->data.ptr being NULL when nothing
is found, but node itself is NULL then, of course.

> 2. Most of the changes I made were to update comments in the area of the

Your comments made me think... mabye we should limit the names to
acl_[cm][_0-9]..., to keep the variable namespace poisoning low.
Blocking variables with names like $acl_check_fancy or
$acl_multiple_fails is drastic (even though they are only names).
acl_msomething and acl_canythin aren't nice to read, anyway, and more
error prone. The scope identifier (c/m) should be cleary separated from
the name.

> 3. You obviously didn't try to compile eximon with the patch on, because
> it failed (eximon reads the spool -H file using some of the main Exim

You got me... I have no X on my servers, so I never used it...

> That's all. Some testing of this new feature would be useful. Since
> several people were in favour of this feature, I hope you'll try it
> out.

Sure I will :)


--
## List details at http://www.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##

On Tue, 19 Sep 2006, Marc Haber wrote:

> On Tue, Sep 19, 2006 at 12:40:11PM +0100, Philip Hazel wrote:
> > That's all. Some testing of this new feature would be useful. Since
> > several people were in favour of this feature, I hope you'll try it
> > out.
>
> Do you want me to package a snapshot for Debian experimental?

Sure, if you think that will get it used. But maybe wait until the
question Jakob has just asked is resolved.


--
Philip Hazel University of Cambridge Computing Service
Get the Exim 4 book: http://www.uit.co.uk/exim-book

--
## List details at http://www.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##