Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Willy Tarreau]

On Tue, May 17, 2016 at 11:34:24PM +0200, maxime de Roucy wrote:
> I can do it tomorow evening (France time).

Fine, thank you Maxime. No rush, don't worry. The most important is
that we know what to do and that everyone agrees.

Willy

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[maxime de Roucy]

I can do it tomorow evening (France time).
Le 17 mai 2016 11:18 PM, "Willy Tarreau"  a écrit :

> On Tue, May 17, 2016 at 11:12:35PM +0200, Nenad Merdanovic wrote:
> > Hello Willy,
> >
> > On 05/17/2016 09:41 PM, Willy Tarreau wrote:
> > > Nenad, you were the one reporting the sorting issue, what do you think
> > > about all this ?
> >
> > I don't have strong feelings about this -- the initial point I asked
> > about was the versionsort vs alphasort and then just pointing out that
> > we've made a change in sorting with not following the locale.
> >
> > We can resolve this in the documentation as well.
>
> OK so if everyone feels the same, I guess it's better to remove the
> setlocale() all and adjust the doc then. Who wants to do it ? Please
> mention the side effects Cyril reported in the commit message so that
> we're not tempted to reinvent it later without a clue.
>
> Thanks!
> Willy
>
>

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Willy Tarreau]

On Tue, May 17, 2016 at 11:12:35PM +0200, Nenad Merdanovic wrote:
> Hello Willy,
> 
> On 05/17/2016 09:41 PM, Willy Tarreau wrote:
> > Nenad, you were the one reporting the sorting issue, what do you think
> > about all this ?
> 
> I don't have strong feelings about this -- the initial point I asked
> about was the versionsort vs alphasort and then just pointing out that
> we've made a change in sorting with not following the locale.
> 
> We can resolve this in the documentation as well.

OK so if everyone feels the same, I guess it's better to remove the
setlocale() all and adjust the doc then. Who wants to do it ? Please
mention the side effects Cyril reported in the commit message so that
we're not tempted to reinvent it later without a clue.

Thanks!
Willy

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Nenad Merdanovic]

Hello Willy,

On 05/17/2016 09:41 PM, Willy Tarreau wrote:
> Nenad, you were the one reporting the sorting issue, what do you think
> about all this ?

I don't have strong feelings about this -- the initial point I asked
about was the versionsort vs alphasort and then just pointing out that
we've made a change in sorting with not following the locale.

We can resolve this in the documentation as well.

Cheers,
Nenad

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Maxime de Roucy]

Le mardi 17 mai 2016 à 21:28 +0200, Cyril Bonté a écrit :
> Well, I'd rather think we shouldn't bother with setlocale(), stay
> with the C locale, and document how the files are sorted. The
> behaviour is different than calling "haproxy -- *.cfg", where globing
> is made by the shell itself and its locale, but this looks to be
> consistent with how other programs behave (apache with the "Include"
> directive, nginx with the "include" one).

I agree. We should remove setlocale and document the fact that sorting
is done with LC_COLLATE=C.
-- 
Regards
Maxime

signature.asc
Description: This is a digitally signed message part

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Willy Tarreau]

On Tue, May 17, 2016 at 09:28:21PM +0200, Cyril Bonté wrote:
> Le 17/05/2016 20:01, Willy Tarreau a écrit :
> > At least we now need to find a way to fix this because we can't accept
> > to have such random breakage in a way that it all but auditable. When
> > you host 1000 customers and 50 of them start to report random breakage
> > after an upgrade, you can only downgrade with no way to spot what's
> > wrong.
> 
> Well, I'd rather think we shouldn't bother with setlocale(), stay with the C
> locale, and document how the files are sorted. The behaviour is different
> than calling "haproxy -- *.cfg", where globing is made by the shell itself
> and its locale, but this looks to be consistent with how other programs
> behave (apache with the "Include" directive, nginx with the "include" one).

OK, these are good arguments indeed.

Nenad, you were the one reporting the sorting issue, what do you think
about all this ?

Willy

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Cyril Bonté]

Le 17/05/2016 20:01, Willy Tarreau a écrit :

At least we now need to find a way to fix this because we can't accept
to have such random breakage in a way that it all but auditable. When
you host 1000 customers and 50 of them start to report random breakage
after an upgrade, you can only downgrade with no way to spot what's
wrong.


Well, I'd rather think we shouldn't bother with setlocale(), stay with 
the C locale, and document how the files are sorted. The behaviour is 
different than calling "haproxy -- *.cfg", where globing is made by the 
shell itself and its locale, but this looks to be consistent with how 
other programs behave (apache with the "Include" directive, nginx with 
the "include" one).


--
Cyril Bonté

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Willy Tarreau]

Hi Cyril,

On Tue, May 17, 2016 at 07:55:01PM +0200, Cyril Bonté wrote:
> Hi all,
> 
> Le 14/05/2016 00:13, Willy Tarreau a écrit :
> > Hi Maxime,
> > 
> > On Sat, May 14, 2016 at 12:09:07AM +0200, Maxime de Roucy wrote:
> > > Hello Willy,
> > > 
> > > Le vendredi 13 mai 2016 à 19:22 +0200, Willy Tarreau a écrit :
> > > > Maxime, could you please add a call to setlocale(LC_ALL, "") as
> > > > suggested by Nenad before starting to scan the directories ? If
> > > > something isn't 100% clear to you, do not hesitate to ask for more
> > > > details, you won't look stupid, I did before you :-)
> > > 
> > > It's clear.
> > > I didn't know either that I had to call setlocale to get the locale
> > > from the environment. I read the man strcoll and thought it would use
> > > LC_COLLATE (from the environment) without other call.
> > 
> > That's the benefit of posting patches to the list as you can see :-)
> > 
> > > I add the setlocale call in the main function because it doesn't set
> > > any haproxy variable. But "init" also sound like a good place.
> > > Let me know if you prefer to move setlocale from main.
> > 
> > Let's leave it there at least we can't miss it if we look for it. It's
> > easy to move if someone is unhappy with it.
> 
> Now we have setlocale(LC_ALL, ""), I was wondering what potential impact we
> could meet with existing configurations and if we should consider it normal.
> 
> For example, some regexes may have different behaviours depending on the
> locale (using the OS libc standard regex implementation, not PCRE).
> 
> Example :
> # locale.cfg content :
>   defaults
> mode http
> 
>   frontend test
> bind :9000
> mode http
> use_backend testbk if { hdr_reg(X-Test) ^\w+$ }
> 
>   backend testbk
> mode http
> server s 127.0.0.1:80
> 
> Launching haproxy with an UTF-8 locale :
> $ LANG=fr_FR.UTF-8 ./haproxy -f locale.cfg
> $ curl -i -H "X-Test: échec" localhost:9000
> HTTP/1.1 200 OK
> ...
> 
> But the same with the C locale :
> $ LANG=C ./haproxy -f locale.cfg
> curl -i -H "X-Test: échec" localhost:9000
> HTTP/1.0 503 Service Unavailable
> ...

Hmmm that's really really bad :-(  Welcome to the world of locales :-(

> Some LUA scripts may change their behaviour too. For more details :
> http://lua-users.org/wiki/LuaLocales
> 
> So, I wonder if we shouldn't limit the setlocale() to the
> cfgfiles_expand_directories() function only.
> - setlocale(LC_ALL, "") when entering the function
> - setlocale(LC_ALL, "C") when leaving the function

Probably. Another possibility would simply be to totally get rid of the
sorting function which requires the locale, implement the sort ourselves
and precisely document how it works.

At least we now need to find a way to fix this because we can't accept
to have such random breakage in a way that it all but auditable. When
you host 1000 customers and 50 of them start to report random breakage
after an upgrade, you can only downgrade with no way to spot what's
wrong.

Willy

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Cyril Bonté]

Hi all,

Le 14/05/2016 00:13, Willy Tarreau a écrit :

Hi Maxime,

On Sat, May 14, 2016 at 12:09:07AM +0200, Maxime de Roucy wrote:

Hello Willy,

Le vendredi 13 mai 2016 à 19:22 +0200, Willy Tarreau a écrit :

Maxime, could you please add a call to setlocale(LC_ALL, "") as
suggested by Nenad before starting to scan the directories ? If
something isn't 100% clear to you, do not hesitate to ask for more
details, you won't look stupid, I did before you :-)


It's clear.
I didn't know either that I had to call setlocale to get the locale
from the environment. I read the man strcoll and thought it would use
LC_COLLATE (from the environment) without other call.


That's the benefit of posting patches to the list as you can see :-)


I add the setlocale call in the main function because it doesn't set
any haproxy variable. But "init" also sound like a good place.
Let me know if you prefer to move setlocale from main.


Let's leave it there at least we can't miss it if we look for it. It's
easy to move if someone is unhappy with it.


Now we have setlocale(LC_ALL, ""), I was wondering what potential impact 
we could meet with existing configurations and if we should consider it 
normal.


For example, some regexes may have different behaviours depending on the 
locale (using the OS libc standard regex implementation, not PCRE).


Example :
# locale.cfg content :
  defaults
mode http

  frontend test
bind :9000
mode http
use_backend testbk if { hdr_reg(X-Test) ^\w+$ }

  backend testbk
mode http
server s 127.0.0.1:80

Launching haproxy with an UTF-8 locale :
$ LANG=fr_FR.UTF-8 ./haproxy -f locale.cfg
$ curl -i -H "X-Test: échec" localhost:9000
HTTP/1.1 200 OK
...

But the same with the C locale :
$ LANG=C ./haproxy -f locale.cfg
curl -i -H "X-Test: échec" localhost:9000
HTTP/1.0 503 Service Unavailable
...

Some LUA scripts may change their behaviour too. For more details :
http://lua-users.org/wiki/LuaLocales

So, I wonder if we shouldn't limit the setlocale() to the 
cfgfiles_expand_directories() function only.

- setlocale(LC_ALL, "") when entering the function
- setlocale(LC_ALL, "C") when leaving the function


--
Cyril Bonté

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Willy Tarreau]

Hi Maxime,

On Sat, May 14, 2016 at 12:09:07AM +0200, Maxime de Roucy wrote:
> Hello Willy,
> 
> Le vendredi 13 mai 2016 à 19:22 +0200, Willy Tarreau a écrit :
> > Maxime, could you please add a call to setlocale(LC_ALL, "") as
> > suggested by Nenad before starting to scan the directories ? If
> > something isn't 100% clear to you, do not hesitate to ask for more
> > details, you won't look stupid, I did before you :-)
> 
> It's clear.
> I didn't know either that I had to call setlocale to get the locale
> from the environment. I read the man strcoll and thought it would use
> LC_COLLATE (from the environment) without other call.

That's the benefit of posting patches to the list as you can see :-)

> I add the setlocale call in the main function because it doesn't set
> any haproxy variable. But "init" also sound like a good place.
> Let me know if you prefer to move setlocale from main.

Let's leave it there at least we can't miss it if we look for it. It's
easy to move if someone is unhappy with it.

Thanks!
Willy

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Maxime de Roucy]

Hello Willy,

Le vendredi 13 mai 2016 à 19:22 +0200, Willy Tarreau a écrit :
> Maxime, could you please add a call to setlocale(LC_ALL, "") as
> suggested by Nenad before starting to scan the directories ? If
> something isn't 100% clear to you, do not hesitate to ask for more
> details, you won't look stupid, I did before you :-)

It's clear.
I didn't know either that I had to call setlocale to get the locale
from the environment. I read the man strcoll and thought it would use
LC_COLLATE (from the environment) without other call.

I add the setlocale call in the main function because it doesn't set
any haproxy variable. But "init" also sound like a good place.
Let me know if you prefer to move setlocale from main.

$ LC_COLLATE='fr_FR.utf8' ls -l rootdir/aaa
total 0
-rw-r--r-- 1 max users 0 13 mai   22:17 a.cfg
-rw-r--r-- 1 max users 0 13 mai   22:18 A.cfg
-rw-r--r-- 1 max users 0 13 mai   22:17 à.cfg
-rw-r--r-- 1 max users 0 13 mai   22:18 À.cfg
-rw-r--r-- 1 max users 0 13 mai   22:17 â.cfg
-rw-r--r-- 1 max users 0 13 mai   22:18 Â.cfg
$ LC_COLLATE='fr_FR.utf8' ./haproxy -C rootdir -f aaa 
aaa/a.cfg
aaa/A.cfg
aaa/à.cfg
aaa/À.cfg
aaa/â.cfg
aaa/Â.cfg
$ LC_COLLATE=C ./haproxy -C rootdir -f aaa 
aaa/A.cfg
aaa/a.cfg
aaa/À.cfg
aaa/Â.cfg
aaa/à.cfg
aaa/â.cfg

-- 
Regards
Maxime de Roucy

signature.asc
Description: This is a digitally signed message part

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Willy Tarreau]

On Fri, May 13, 2016 at 07:15:11PM +0200, Nenad Merdanovic wrote:
> Hello Willy,
> 
> On 5/13/2016 7:04 PM, Willy Tarreau wrote:
> > I don't know, I'm not fond of setting things in the back of the user like
> > this. I don't even know if we have other parts that are currently sensitive
> > to the locale and which could be affected. Wouldn't it be better to simply
> > add this piece of information to the doc instead so that users know if they
> > need to specially tweak their locale ?
> 
> If we don't call 'setlocale', the tweaks will have no effect and we have
> a difference in behavior between this way and the '-- foo/*.cfg',
> because the latter will get expended by the shell in the ordered
> specified by LC_COLLATE while the new way will order the files as if
> LC_COLLATE was set to 'C'.

Ah OK that's what I did't know, that was not obvious from my reading of
the doc, thank you.

> Unless you are thinking of asking the user through the doc to adapt
> their filenames so that they are not affected by the locale differences.

No of course you're right, for me the purpose of the doc here was to let
the user know we respect the locale and that it's his problem to ensure
his sorting options are correct in his locale and match his shell for
instance. But of course if for this to work we have to do something we
must do it.

Maxime, could you please add a call to setlocale(LC_ALL, "") as suggested
by Nenad before starting to scan the directories ? If something isn't 100%
clear to you, do not hesitate to ask for more details, you won't look
stupid, I did before you :-)

Thanks,
Willy

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Nenad Merdanovic]

Hello Willy,

On 5/13/2016 7:04 PM, Willy Tarreau wrote:
> I don't know, I'm not fond of setting things in the back of the user like
> this. I don't even know if we have other parts that are currently sensitive
> to the locale and which could be affected. Wouldn't it be better to simply
> add this piece of information to the doc instead so that users know if they
> need to specially tweak their locale ?

If we don't call 'setlocale', the tweaks will have no effect and we have
a difference in behavior between this way and the '-- foo/*.cfg',
because the latter will get expended by the shell in the ordered
specified by LC_COLLATE while the new way will order the files as if
LC_COLLATE was set to 'C'.

Unless you are thinking of asking the user through the doc to adapt
their filenames so that they are not affected by the locale differences.

> Do not hesitate to tell me if I'm
> wrong on something, I always try to stay away from locale manipulation
> because I know how painful everything becomes once you start to break the
> fragile equilibrium!

Agree there 100%, but all this is to follow up that we might be changing
the order the files are loaded, so we either address this in the code or
in the docs.

Thanks,
Nenad

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Willy Tarreau]

On Fri, May 13, 2016 at 06:38:06PM +0200, Nenad Merdanovic wrote:
> Hello Willy,
> 
> On 5/13/2016 12:54 PM, Willy Tarreau wrote:
> > Wait a minute, what do you mean by "different lower/upper case sorting" ?
> > Do you mean that alphasort() ignores the case ? I'm seeing no mention about
> > it in the man page, so I'm confused. If this is the case, it can be annoying
> > for the same reason.
> 
> Sorry for my vagueness, was writing a response in a hurry so didn't
> explain it well. The thing is, we don't set the locale anywhere within
> HAproxy, as far as I am aware at least. The shell expansion will honor
> LC_COLLATE, while the ordering of scandir's alphasort will default to C.

OK, thanks for clarifying.

> Maybe it would be worth adding 'setlocale(LC_ALL, "")' before, so we
> pick up the user's environment?

I don't know, I'm not fond of setting things in the back of the user like
this. I don't even know if we have other parts that are currently sensitive
to the locale and which could be affected. Wouldn't it be better to simply
add this piece of information to the doc instead so that users know if they
need to specially tweak their locale ? Do not hesitate to tell me if I'm
wrong on something, I always try to stay away from locale manipulation
because I know how painful everything becomes once you start to break the
fragile equilibrium!

Thanks,
Willy

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Nenad Merdanovic]

Hello Willy,

On 5/13/2016 12:54 PM, Willy Tarreau wrote:
> Wait a minute, what do you mean by "different lower/upper case sorting" ?
> Do you mean that alphasort() ignores the case ? I'm seeing no mention about
> it in the man page, so I'm confused. If this is the case, it can be annoying
> for the same reason.

Sorry for my vagueness, was writing a response in a hurry so didn't
explain it well. The thing is, we don't set the locale anywhere within
HAproxy, as far as I am aware at least. The shell expansion will honor
LC_COLLATE, while the ordering of scandir's alphasort will default to C.

Maybe it would be worth adding 'setlocale(LC_ALL, "")' before, so we
pick up the user's environment?

Regards,
Nenad

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Willy Tarreau]

On Fri, May 13, 2016 at 11:18:23AM +0200, Nenad Merdanovic wrote:
> >> If the .cnf files contain rules that are sensitive to ordering, this
> >> could be problematic. I am not saying we should change it, just to
> >> consider it :)
> > 
> > I've thought about this as well but figured that we'd rather not change
> > the ordering of what people used to get with "-- dir/*.cfg". Changing
> > the sort algorithm would make a difference here and it might not encourage
> > people to use directories. I could be wrong of course, but that's my 
> > feeling.
> > 
> 
> I figured that since we are changing behavior already with different
> lowercase/uppercase sorting, this might be less of an issue. But you are
> right, the less behavior changes we do, the better.

Wait a minute, what do you mean by "different lower/upper case sorting" ?
Do you mean that alphasort() ignores the case ? I'm seeing no mention about
it in the man page, so I'm confused. If this is the case, it can be annoying
for the same reason.

Thanks,
Willy

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Cyril Bonté]

Hi all,

Le 13/05/2016 08:07, Willy Tarreau a écrit :

Hi Maxime,

On Fri, May 13, 2016 at 12:48:43AM +0200, Maxime de Roucy wrote:

If -f argument is a directory add all the files (and only files) it
containes to the config files list.
These files are added in lexical order (man alphasort).
Only files with ".cfg" extension are added.
Only non hidden files (not prefixed with ".") are added.
Symlink are followed.
The -f order is still respected:

$ tree -a rootdir
rootdir
? dir1
???   ? 1.cfg
???   ? 2
???   ? 3.cfg
???   ? 4.cfg -> 1.cfg
???   ? 5 -> 1.cfg
???   ? .6.cfg
???   ? 7.cfg -> .
???   ? dir4
???   ? 8.cfg
? dir2
???   ? 10.cfg
???   ? 9.cfg
? dir3
???   ? 11.cfg
? link -> dir3/
? root1
? root2
? root3

$ ./haproxy -C rootdir -f root2 -f dir2 -f root3 -f dir1 \
   -f link -f root1
root2
dir2/10.cfg
dir2/9.cfg
root3
dir1/1.cfg
dir1/3.cfg
dir1/4.cfg
link/11.cfg
root1

This can be useful on systemd where you can't change the haproxy
commande line options on service reload.


I think I'm fine with this one (I'll still wait a bit to let others respond).


From the commit message, it looks OK for me too, thanks ;-)


--
Cyril Bonté

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Nenad Merdanovic]

Hello Willy,

On 5/13/2016 11:04 AM, Willy Tarreau wrote:
> Hi Nenad,
> 
> On Fri, May 13, 2016 at 11:02:01AM +0200, Nenad Merdanovic wrote:
>> Hello,
>>
>> On 5/13/2016 8:07 AM, Willy Tarreau wrote:
>>> I think I'm fine with this one (I'll still wait a bit to let others 
>>> respond).
>>> I just have one small request, please move the addition of 
>>> list_append_word()
>>> to its own patch : if later we use it to fix a bug which needs to be
>>> backported, we'll be able to simply pick it for the backport as well.
>>
>> I'd just like to ask for consideration between alphasort and
>> versionsort. I've seen people have a (bad) habit of naming files like:
>> 1-first.cnf
>> 2-second.cnf
>> ...
>> 10-tenth.cnf
>>
>> If the .cnf files contain rules that are sensitive to ordering, this
>> could be problematic. I am not saying we should change it, just to
>> consider it :)
> 
> I've thought about this as well but figured that we'd rather not change
> the ordering of what people used to get with "-- dir/*.cfg". Changing
> the sort algorithm would make a difference here and it might not encourage
> people to use directories. I could be wrong of course, but that's my feeling.
> 

I figured that since we are changing behavior already with different
lowercase/uppercase sorting, this might be less of an issue. But you are
right, the less behavior changes we do, the better.

Cheers,
Nenad

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Willy Tarreau]

Hi Nenad,

On Fri, May 13, 2016 at 11:02:01AM +0200, Nenad Merdanovic wrote:
> Hello,
> 
> On 5/13/2016 8:07 AM, Willy Tarreau wrote:
> > I think I'm fine with this one (I'll still wait a bit to let others 
> > respond).
> > I just have one small request, please move the addition of 
> > list_append_word()
> > to its own patch : if later we use it to fix a bug which needs to be
> > backported, we'll be able to simply pick it for the backport as well.
> 
> I'd just like to ask for consideration between alphasort and
> versionsort. I've seen people have a (bad) habit of naming files like:
> 1-first.cnf
> 2-second.cnf
> ...
> 10-tenth.cnf
> 
> If the .cnf files contain rules that are sensitive to ordering, this
> could be problematic. I am not saying we should change it, just to
> consider it :)

I've thought about this as well but figured that we'd rather not change
the ordering of what people used to get with "-- dir/*.cfg". Changing
the sort algorithm would make a difference here and it might not encourage
people to use directories. I could be wrong of course, but that's my feeling.

Willy

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Nenad Merdanovic]

Hello,

On 5/13/2016 8:07 AM, Willy Tarreau wrote:
> I think I'm fine with this one (I'll still wait a bit to let others respond).
> I just have one small request, please move the addition of list_append_word()
> to its own patch : if later we use it to fix a bug which needs to be
> backported, we'll be able to simply pick it for the backport as well.

I'd just like to ask for consideration between alphasort and
versionsort. I've seen people have a (bad) habit of naming files like:
1-first.cnf
2-second.cnf
...
10-tenth.cnf

If the .cnf files contain rules that are sensitive to ordering, this
could be problematic. I am not saying we should change it, just to
consider it :)

Cheers,
Nenad

> 
> Thanks,
> Willy
> 
> 

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Willy Tarreau]

Hi Maxime,

On Fri, May 13, 2016 at 12:48:43AM +0200, Maxime de Roucy wrote:
> If -f argument is a directory add all the files (and only files) it
> containes to the config files list.
> These files are added in lexical order (man alphasort).
> Only files with ".cfg" extension are added.
> Only non hidden files (not prefixed with ".") are added.
> Symlink are followed.
> The -f order is still respected:
> 
>   $ tree -a rootdir
>   rootdir
>   ? dir1
>   ???   ? 1.cfg
>   ???   ? 2
>   ???   ? 3.cfg
>   ???   ? 4.cfg -> 1.cfg
>   ???   ? 5 -> 1.cfg
>   ???   ? .6.cfg
>   ???   ? 7.cfg -> .
>   ???   ? dir4
>   ???   ? 8.cfg
>   ? dir2
>   ???   ? 10.cfg
>   ???   ? 9.cfg
>   ? dir3
>   ???   ? 11.cfg
>   ? link -> dir3/
>   ? root1
>   ? root2
>   ? root3
> 
>   $ ./haproxy -C rootdir -f root2 -f dir2 -f root3 -f dir1 \
>  -f link -f root1
>   root2
>   dir2/10.cfg
>   dir2/9.cfg
>   root3
>   dir1/1.cfg
>   dir1/3.cfg
>   dir1/4.cfg
>   link/11.cfg
>   root1
> 
> This can be useful on systemd where you can't change the haproxy
> commande line options on service reload.

I think I'm fine with this one (I'll still wait a bit to let others respond).
I just have one small request, please move the addition of list_append_word()
to its own patch : if later we use it to fix a bug which needs to be
backported, we'll be able to simply pick it for the backport as well.

Thanks,
Willy

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Maxime de Roucy]

Hi Willy,

> > I don't receive all the mails from haproxy@formilux.org.
> > For exemple I didn't received :
> > http://article.gmane.org/gmane.comp.web.haproxy/27795
> 
> Well, this one was sent to you directly by Cyril, so you should have
> received it.

Indeed, it was this one:
 http://article.gmane.org/gmane.comp.web.haproxy/27802

I will check my anti-spam.

Anyway :
> I've subscribed you by hand now.

I now receive all the mailling mail.
Thanks a lot.

> > > If tmp_str fails and wlt succeeds, wlt is not freed.
> > If tmp_str fails and wlt succeeds we still got the Alert and
> > everything it freed on exit.
> 
> Yes I know but as I said, if/when such code later moves to its own
> function, this function might initially decide to exit then to let
> the caller take the decision and one day all of this will be used
> dynamically or from the CLI and then people discover a memory leak.
> And there are the valgrind users who send patches very often to fix
> such warnings that annoy them. I mean we spent a lot of time killing
> some such old issues that were not bugs initially and that became
> bugs later, so we try to be careful. We don't want to be the next
> openssl if you see what I mean :-)

Yes I see :-)

For the record I now always check my code with "valgrind  --leak-
check=full".

> > I create the function "void cfgfiles_expand_directories(void)", but
> > not the "load_config_file" one.
> > I am not accustomed to using goto and it's hard for me to use it
> > here as I actually don't see the point of it (in
> > cfgfiles_expand_directories).
> 
> That's the best way to deal with error unrolling. I'm sad that
> teachers at school teach students not to use it because :
>   1) it's what the compiler implements anyway for all other
> constructs
>   2) it's the only safe way to perform unrolling which resists to
> code additions.
> 
> We used to have some leaks in the past because we were not using it.
> When you have some session initialization code like this :
> 
> s = malloc(sizeof(*s));
> if (!s)
> return;
> 
> s->req = malloc(sizeof(*s->req));
> if (!s->req)) {
>    free(s);
>    return;
> }
> 
> s->res = malloc(sizeof(*s->res));
> if (!s->res)) {
>    free(s->req);
>    free(s);
>    return;
> }
> 
> s->txn = malloc(sizeof(*s->txn));
> if (!s->txn)) {
>    free(s->res);
>    free(s->req);
>    free(s);
>    return;
> }
> 
> s->log = malloc(sizeof(*s->log));
> if (!s->log)) {
>    free(s->txn);
>    free(s->res);
>    free(s->req);
>    free(s);
>    return;
> }
> 
> s->req_capture = malloc(sizeof(*s->req_capture));
> if (!s->req_capture)) {
>    free(s->log);
>    free(s->txn);
>    free(s->res);
>    free(s->req);
>    free(s);
>    return;
> }
> 
> s->res_capture = malloc(sizeof(*s->res_capture));
> if (!s->res_capture)) {
>    free(s->req_capture);
>    free(s->log);
>    free(s->txn);
>    free(s->res);
>    free(s->req);
>    free(s);
>    return;
> }
> 
> ... and so on for 10 entries ...
> 
> Then you may already have bugs above due to the inevitable copy-
> paste, and once you insert a new field in the middle (eg: s->vars)
> you're pretty sure that you will miss it in one of the next "if"
> blocks because they are never as clear as above but themselves
> enclosed within other "if" blocks. And when you need to switch
> allocation order, that's even worse. But the horrible thing above can
> be safely turned into this :
> 
> s = malloc(sizeof(*s));
> if (!s)
> goto fail_s;
> 
> s->req = malloc(sizeof(*s->req));
> if (!s->req))
> goto fail_req;
> 
> s->res = malloc(sizeof(*s->res));
> if (!s->res))
> goto fail_res;
> 
> s->txn = malloc(sizeof(*s->txn));
> if (!s->txn))
> goto fail_txn;
> 
> s->log = malloc(sizeof(*s->log));
> if (!s->log))
> goto fail_log;
> 
> s->req_capture = malloc(sizeof(*s->req_capture));
> if (!s->req_capture))
>    goto fail_req_cap;
> 
> s->res_capture = malloc(sizeof(*s->res_capture));
> if (!s->res_capture))
>    goto fail_res_cap;
> 
> return s;
> 
>  fail_res_cap:
> free(s->req_capture);
>  fail_req_cap:
> free(s->log);
>  fail_log:
> free(s->txn);
>  fail_txn:
> free(s->res);
>  fail_res:
> free(s->req);
>  fail_req:
> free(s);
>  fail_s:
> return NULL;
> 
> And a nice side effect is that when you look at the assembly code,
> it's much smaller and much more efficient.
> 
> > It doesn't reduce the number of lines and, as after every alert we
> > call exit, there is no need to clean anything.
> 
> As I explained above I agree on this but code correctness and code
> cleanness are two different things. We try to have a bit of
> modularity because we know that code moves and that it's better if it
> can move safely. For example I recently 

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Willy Tarreau]

Hi Maxime,

On Wed, May 11, 2016 at 12:37:36AM +0200, Maxime de Roucy wrote:
> Hello Willy,
> 
> I don't receive all the mails from haproxy@formilux.org.
> For exemple I didn't received :
> http://article.gmane.org/gmane.comp.web.haproxy/27795

Well, this one was sent to you directly by Cyril, so you should have
received it.

> However I am sure I sent a blank mail to haproxy+subscr...@formilux.org
>  (I rechecked).
> Can you check on the server side ?

Indeed you're not subscribed here. You might have some anti-spam filtering
which blocked the validation e-mail. Some people have already had trouble
with gmail's spam filtering. It seems to vary in time, but we have a bit
more than 300 subscribers using gmail here so I guess overall it works.
Ah and there's this well-known rant from Linus about a huge amount of
false positives, lots of people are experiencing two-digit percents of
false positive rate :

   https://plus.google.com/+LinusTorvalds/posts/DiG9qANf5PA

I've subscribed you by hand now.

> > > I forgot to free the memory allocated at 'filename = calloc' (why
> > > valgrind
> > > didn't warn...). Forget this patch. I will send another one
> > > tomorow.
> > 
> > Yes I noticed, and there's this one as well :
> > 
> > > > +   wlt = calloc(1, sizeof(*wlt));
> > > > +   tmp_str = strdup(filename);
> > > > +   if (!wlt || !tmp_str) {
> > > > +   Alert("Cannot load
> > > > configuration
> > > > files %s : out of memory.\n",
> > > > + filename);
> > > > +   exit(1);
> > > > +   }
> > 
> > If tmp_str fails and wlt succeeds, wlt is not freed.
> 
> If tmp_str fails and wlt succeeds we still got the Alert and everything
> it freed on exit.

Yes I know but as I said, if/when such code later moves to its own function,
this function might initially decide to exit then to let the caller take the
decision and one day all of this will be used dynamically or from the CLI and
then people discover a memory leak. And there are the valgrind users who send
patches very often to fix such warnings that annoy them. I mean we spent a
lot of time killing some such old issues that were not bugs initially and
that became bugs later, so we try to be careful. We don't want to be the
next openssl if you see what I mean :-)

> Anyway the problem isn't here anymore as I get ride of strdup.
> See the end of this mail.

OK.

> I create the function "void cfgfiles_expand_directories(void)", but not
> the "load_config_file" one.
> I am not accustomed to using goto and it's hard for me to use it here
> as I actually don't see the point of it (in
> cfgfiles_expand_directories).

That's the best way to deal with error unrolling. I'm sad that teachers at
school teach students not to use it because :
  1) it's what the compiler implements anyway for all other constructs
  2) it's the only safe way to perform unrolling which resists to code
 additions.

We used to have some leaks in the past because we were not using it. When
you have some session initialization code like this :

s = malloc(sizeof(*s));
if (!s)
return;

s->req = malloc(sizeof(*s->req));
if (!s->req)) {
   free(s);
   return;
}

s->res = malloc(sizeof(*s->res));
if (!s->res)) {
   free(s->req);
   free(s);
   return;
}

s->txn = malloc(sizeof(*s->txn));
if (!s->txn)) {
   free(s->res);
   free(s->req);
   free(s);
   return;
}

s->log = malloc(sizeof(*s->log));
if (!s->log)) {
   free(s->txn);
   free(s->res);
   free(s->req);
   free(s);
   return;
}

s->req_capture = malloc(sizeof(*s->req_capture));
if (!s->req_capture)) {
   free(s->log);
   free(s->txn);
   free(s->res);
   free(s->req);
   free(s);
   return;
}

s->res_capture = malloc(sizeof(*s->res_capture));
if (!s->res_capture)) {
   free(s->req_capture);
   free(s->log);
   free(s->txn);
   free(s->res);
   free(s->req);
   free(s);
   return;
}

... and so on for 10 entries ...

Then you may already have bugs above due to the inevitable copy-paste,
and once you insert a new field in the middle (eg: s->vars) you're
pretty sure that you will miss it in one of the next "if" blocks because
they are never as clear as above but themselves enclosed within other
"if" blocks. And when you need to switch allocation order, that's even
worse. But the horrible thing above can be safely turned into this :

s = malloc(sizeof(*s));
if (!s)
goto fail_s;

s->req = malloc(sizeof(*s->req));
if (!s->req))
goto fail_req;

s->res = malloc(sizeof(*s->res));
if (!s->res))
goto fail_res;

s->txn = malloc(sizeof(*s->txn));
if (!s->txn))
goto fail_txn;

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Maxime de Roucy]

Hello Willy,

I don't receive all the mails from haproxy@formilux.org.
For exemple I didn't received :
http://article.gmane.org/gmane.comp.web.haproxy/27795

However I am sure I sent a blank mail to haproxy+subscr...@formilux.org
 (I rechecked).
Can you check on the server side ?

> > I forgot to free the memory allocated at 'filename = calloc' (why
> > valgrind
> > didn't warn...). Forget this patch. I will send another one
> > tomorow.
> 
> Yes I noticed, and there's this one as well :
> 
> > > +   wlt = calloc(1, sizeof(*wlt));
> > > +   tmp_str = strdup(filename);
> > > +   if (!wlt || !tmp_str) {
> > > +   Alert("Cannot load
> > > configuration
> > > files %s : out of memory.\n",
> > > + filename);
> > > +   exit(1);
> > > +   }
> 
> If tmp_str fails and wlt succeeds, wlt is not freed.

If tmp_str fails and wlt succeeds we still got the Alert and everything
it freed on exit.
Anyway the problem isn't here anymore as I get ride of strdup.
See the end of this mail.

> Overall I still think that writing this into a dedicated function
> will make the controls and unrolling easier. I'd suggest something
> like this which is much more conventional, auditable and maintainable
> :
> 
> /* loads config file/dir , returns 0 on failure with errmsg
>  * filled with an error message.
>  */
> int load_config_file(const char *arg, char **errmsg)
> {
> ...
> wlt = calloc(1, sizeof(*wlt));
> if (!wlt) {
>  memprintf(errmsg, "out of memory");
>  goto fail_wlt;
> }
> 
> tmp_str = strdup(filename);
> if (!tmp_str) {
>  memprintf(errmsg, "out of memory");
>  goto fail_tmp_str;
> }
> ...
> return 1;
> ...
> fail_smp_str:
> free(wlt);
> fail_wlt:
> ...
> return 0;
> }
> 
> Then in init() :
> 
> if (!load_config_from_file(argv, )) {
> Alert("Cannot load configuration files %s : %s.\n", filename,
> errmsg);
> exit(1);
> }

I create the function "void cfgfiles_expand_directories(void)", but not
the "load_config_file" one.
I am not accustomed to using goto and it's hard for me to use it here
as I actually don't see the point of it (in
cfgfiles_expand_directories).

It doesn't reduce the number of lines and, as after every alert we call
exit, there is no need to clean anything.

> Also, please don't use sprintf() but snprintf() as I mentionned in
> the earlier mail.

I used sprintf at first because the man says it's not C89 compatible :
  fprintf(), printf(), sprintf(), vprintf(), vfprintf(), vsprintf(): 
POSIX.1-2001, POSIX.1-2008, C89, C99.
  snprintf(), vsnprintf(): POSIX.1-2001, POSIX.1-2008, C99.

and the CONTRIBUTING file says :
  It is important to keep in mind that certain facilities offered by
  recent versions must not be used in the code :
    - …
    - in general, anything which requires C99 (such as declaring
      variables in "for" statements)

I changed it to snprintf, yet I didn't check the output as it shouldn't
be able to fail (the string size == copied size == calloc size).

Here is a new version of the patch. Only for the src/haproxy.c file.
I will send a complete patch when everything will be settled.
I also free head memory from the "wlt = calloc(1, sizeof(*wlt));" line,
in the deinit function.
I don't think it's necessary as deinit is followed by exit but I
thought it would be cleaner as we also free cfg_cfgfiles elements.

Thank you very much for all your comments !
Maxime de Roucy

#

diff --git a/src/haproxy.c b/src/haproxy.c
index 0c223e5..3fae428 100644
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -33,10 +33,12 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -423,7 +425,7 @@ void usage(char *name)
 {
display_version();
fprintf(stderr,
-   "Usage : %s [-f ]* [ -vdV"
+   "Usage : %s [-f ]* [ -vdV"
"D ] [ -n  ] [ -N  ]\n"
"[ -p  ] [ -m  ] [ -C  ] [-- 
*]\n"
"-v displays version ; -vv shows known build options.\n"
@@ -551,6 +553,95 @@ void dump(struct sig_handler *sh)
pool_gc2();
 }
 
+/* This function check if cfg_cfgfiles containes directories.
+ * If it find one, it add all the files (and only files) it containes
+ * in cfg_cfgfiles in place of the directory (and remove the directory).
+ * It add the files in lexical order.
+ */
+void cfgfiles_expand_directories(void)
+{
+   struct wordlist *wl, *wlb;
+
+   list_for_each_entry_safe(wl, wlb, _cfgfiles, list) {
+   struct stat file_stat;
+   struct dirent **dir_entries;
+

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Willy Tarreau]

Hi Maxime,

On Tue, May 10, 2016 at 12:07:15AM +0200, Maxime de Roucy wrote:
> > > I'm not sure to like this feature in its current implementation.
> > > I fear it will also create some new issues depending on how people
> > > will use it.
> 
> Indeed it should be use with care.
> But for me it's as dangerous as the '--' option as '--' had clearly
> been implemented to be used with bash globling.
> However with '--' the sysadmin can filter himself the files list ; it
> can't with my patch.

That's exactly the point.

(...)
> > > Other use cases I immediately see :
> > > - some configurations provide the crt files in the same directory,
> > > which will break things
> > > - some others will store map files in the same directory also
> > > - what about configurations with a README or similar in the
> > > directory ? or swap files because someone else is editing a file at
> > > the same time ?
> 
> The last point is a very good one I think, swap files can easily be
> forgotten.
> The previous ones are more controversial for me as with my patch the
> directory itself become a configuration "file". If sysadmin put "trash"
> in it, it's the same as if he put "trash" in a configuration file??? it's
> not haproxy's fault if it fail.
>
> However, I agree that we should implement some safeguards.

User error can be addressed with good documentation, and stupidity with a
baseball bat. But we need to protect the user against issues he's not aware
of (swap files, backup files, core files if the editor dies, etc).

> > I think that before going further we should start by trying to
> > define what we want to see and what we don't want.
> >  Maybe a solution could be to at least impose a file name extension
> > (eg: .cfg), and I'm not sure it's enough. For example what should we
> > do with symlinks, some will prefer to follow them, others not to.
> > Most likely we should skip all dot files, etc.
> > I think the discussion should go on before we go further with the
> > code.
> 
> OK.
> 
> For me we should add a filter on file name ; keeping only ending with
> ".cfg" and not starting with ".".

I do think that it could be sufficient but I have not thought about it
for a long time, so I could be missing some cases.

> I vote for following symlinks, as the implementation currently does.
> It means less work for me :-) ??? Joking aside, I don't see any point
> against it and sysadmins will assume it does except if it's explicitly
> said in the docs (docs which I should complet regarding this point).

Yes I think you're right. Also it matches the output of "grep" when admins
search for something (eg: a domain name). And it allows to share some
elements between multiple directories if needed (eg: defaults sections).

Regards,
Willy

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Willy Tarreau]

On Tue, May 10, 2016 at 12:54:40AM +0200, maxime de Roucy wrote:
> I forgot to free the memory allocated at 'filename = calloc' (why valgrind
> didn't warn...). Forget this patch. I will send another one tomorow.

Yes I noticed, and there's this one as well :

> > +   wlt = calloc(1, sizeof(*wlt));
> > +   tmp_str = strdup(filename);
> > +   if (!wlt || !tmp_str) {
> > +   Alert("Cannot load configuration
> > files %s : out of memory.\n",
> > + filename);
> > +   exit(1);
> > +   }

If tmp_str fails and wlt succeeds, wlt is not freed. Overall I still think
that writing this into a dedicated function will make the controls and
unrolling easier. I'd suggest something like this which is much more
conventional, auditable and maintainable :

/* loads config file/dir , returns 0 on failure with errmsg
 * filled with an error message.
 */
int load_config_file(const char *arg, char **errmsg)
{
...
wlt = calloc(1, sizeof(*wlt));
if (!wlt) {
 memprintf(errmsg, "out of memory");
 goto fail_wlt;
}

tmp_str = strdup(filename);
if (!tmp_str) {
 memprintf(errmsg, "out of memory");
 goto fail_tmp_str;
}
...
return 1;
...
fail_smp_str:
free(wlt);
fail_wlt:
...
return 0;
}

Then in init() :

if (!load_config_from_file(argv, )) {
Alert("Cannot load configuration files %s : %s.\n", filename, errmsg);
exit(1);
}

Also, please don't use sprintf() but snprintf() as I mentionned in the
earlier mail. Some platforms will systematically emit a warning when
sprintf() is used, for the same reason as strcpy() and strcat(), and
we managed to get rid of the last one already :

> > +   struct dirent *dir_entry = dir_entries[dir_entries_it];
> > +   char *filename;
> > +   int filename_size = strlen(wl->s)
> > +   + 1 /* for '/' */
> > +   + strlen(dir_entry->d_name);
> > +
> > +   filename = calloc(filename_size + 1 /* for \0 */, 
> > sizeof(*filename));
(...)
> > +   sprintf(filename, "%s/%s", wl->s, dir_entry->d_name);

At first I thought there was a bug in the calloc call but it's OK. It's not
much common to use it with anything but "1" in the element size so it confused
me. You don't need to justify that +1 is for \0 since that's usual in every
string allocation.

Thanks,
Willy

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[maxime de Roucy]

I forgot to free the memory allocated at 'filename = calloc' (why valgrind
didn't warn...). Forget this patch. I will send another one tomorow.

Sorry
Le 9 mai 2016 11:28 PM, "Maxime de Roucy"  a
écrit :

> If -f argument is a directory add all the files (and only files) it
> containes to the config files list.
> These files are added in lexical order (man alphasort).
> The -f order is still respected:
>
> $ tree rootdir
> rootdir
> ├── root1
> ├── root2
> ├── root3
> ├── superdir1
> │   ├── aaa1
> │   ├── aaa2
> │   ├── aaa3
> │   ├── bbb1 -> aaa1
> │   └── wrong
> │   └── wrongfile
> └── superdir2
> ├── ccc1
> └── wronglink -> .
>
> $ ./haproxy -C rootdir -f root2 -f superdir2 \
>-f root3 -f superdir1 -f root1
> root2
> superdir2/ccc1
> root3
> superdir1/aaa1
> superdir1/aaa2
> superdir1/aaa3
> superdir1/bbb1
> root1
>
> This can be useful on systemd where you can't change the haproxy
> commande line options on service reload.
> ---
>  doc/haproxy.1  |   8 +++--
>  doc/management.txt |  23 ++--
>  src/haproxy.c  | 103
> ++---
>  3 files changed, 116 insertions(+), 18 deletions(-)
>
> diff --git a/doc/haproxy.1 b/doc/haproxy.1
> index a836d5d..6017efa 100644
> --- a/doc/haproxy.1
> +++ b/doc/haproxy.1
> @@ -6,7 +6,7 @@ HAProxy \- fast and reliable http reverse proxy and load
> balancer
>
>  .SH SYNOPSIS
>
> -haproxy \-f  [\-L\ ] [\-n\ maxconn] [\-N\
> maxconn] [\-C\ ] [\-v|\-vv] [\-d] [\-D] [\-q] [\-V] [\-c] [\-p\
> ] [\-dk] [\-ds] [\-de] [\-dp] [\-db] [\-dM[]] [\-m\ ]
> [{\-sf|\-st}\ pidlist...]
> +haproxy \-f  [\-L\ ] [\-n\ maxconn] [\-N\
> maxconn] [\-C\ ] [\-v|\-vv] [\-d] [\-D] [\-q] [\-V] [\-c] [\-p\
> ] [\-dk] [\-ds] [\-de] [\-dp] [\-db] [\-dM[]] [\-m\ ]
> [{\-sf|\-st}\ pidlist...]
>
>  .SH DESCRIPTION
>
> @@ -33,8 +33,10 @@ instances without risking the system's stability.
>  .SH OPTIONS
>
>  .TP
> -\fB\-f \fP
> -Specify configuration file path.
> +\fB\-f \fP
> +Specify configuration file or directory path. If the argument is a
> directory
> +the files (and only files) it containes are added in lexical order (man
> +alphasort).
>
>  .TP
>  \fB\-L \fP
> diff --git a/doc/management.txt b/doc/management.txt
> index e0469aa..3e51d49 100644
> --- a/doc/management.txt
> +++ b/doc/management.txt
> @@ -134,16 +134,19 @@ list of options is :
>  one of "global", "defaults", "peers", "listen", "frontend",
> "backend", and
>  so on. A file cannot contain just a server list for example.
>
> -  -f  : adds  to the list of configuration files to be
> -loaded. Configuration files are loaded and processed in their
> declaration
> -order. This option may be specified multiple times to load multiple
> files.
> -See also "--". The difference between "--" and "-f" is that one "-f"
> must
> -be placed before each file name, while a single "--" is needed before
> all
> -file names. Both options can be used together, the command line
> ordering
> -still applies. When more than one file is specified, each file must
> start
> -on a section boundary, so the first keyword of each file must be one
> of
> -"global", "defaults", "peers", "listen", "frontend", "backend", and so
> -on. A file cannot contain just a server list for example.
> +  -f  : adds  to the list of configuration files
> to be
> +loaded. If  is a directory, all the files (and only files) it
> +containes are added in lexical order (man alphasort) to the list of
> +configuration files to be loaded. Configuration files are loaded and
> +processed in their declaration order. This option may be specified
> multiple
> +times to load multiple files. See also "--". The difference between
> "--"
> +and "-f" is that one "-f" must be placed before each file name, while
> a
> +single "--" is needed before all file names. Both options can be used
> +together, the command line ordering still applies. When more than one
> file
> +is specified, each file must start on a section boundary, so the first
> +keyword of each file must be one of "global", "defaults", "peers",
> +"listen", "frontend", "backend", and so on. A file cannot contain
> just a
> +server list for example.
>
>-C  : changes to directory  before loading configuration
>  files. This is useful when using relative paths. Warning when using
> diff --git a/src/haproxy.c b/src/haproxy.c
> index 0c223e5..9824b7c 100644
> --- a/src/haproxy.c
> +++ b/src/haproxy.c
> @@ -33,10 +33,12 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> 

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Pavlos Parissis]

On 10/05/2016 12:07 πμ, Maxime de Roucy wrote:
> Hi Willy and Cyril,
> 
> I just send a new version of the patch.
> I made some changes following the remarks of Willy.
> 
>>> I'm not sure to like this feature in its current implementation.
>>> I fear it will also create some new issues depending on how people
>>> will use it.
> 
> Indeed it should be use with care.
> But for me it's as dangerous as the '--' option as '--' had clearly
> been implemented to be used with bash globling.
> However with '--' the sysadmin can filter himself the files list ; it
> can't with my patch.
> 
>>> For example, I know lots of sysadmin who have the (bad) habit to
>>> make backup of the configuration files in the same directory,
>>> without cleaning it up. We may see some directories like this :
>>>
>>> service1.cfg
>>> service2.cfg
>>> service2.cfg~
>>> service3.cfg.20160509
>>> service4.cfg-19980101
>>> service5.cfg
>>> service5.cfg.old
>>> service6.cfg.disabled
>>>
>>> ...and so on.
>>>
>>> When several sysadmins share the same haproxy instance, it can
>>> quickly
>>> become annoying.
>>>
>>> Other use cases I immediately see :
>>> - some configurations provide the crt files in the same directory,
>>> which will break things
>>> - some others will store map files in the same directory also
>>> - what about configurations with a README or similar in the
>>> directory ? or swap files because someone else is editing a file at
>>> the same time ?
> 
> The last point is a very good one I think, swap files can easily be
> forgotten.
> The previous ones are more controversial for me as with my patch the
> directory itself become a configuration "file". If sysadmin put "trash"
> in it, it's the same as if he put "trash" in a configuration file… it's
> not haproxy's fault if it fail.
> 

IMHO: If as operator configure haproxy to source all files from a
directory and at the same time I leave broken configuration in that
directory then I blame myself and I don't expect from HAProxy to be
smart enough to do right job for me.

Some times having craftsmanship as one of the main competence of your
coworkers is the right solution, rather having technical solutions to
compensate the absence of craftsmanship.

My 2 cents,
Pavlos



signature.asc
Description: OpenPGP digital signature

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Maxime de Roucy]

Hi Willy,


> > Instead I create Alert_exit which embedded the exit call inside
> > Alert.
> > I just sent the patchs.
> > 
> > Is this solution good for you ?
> 
> To be honnest, no, I really don't like it. There are very few such
> locations that benefit from it and 2/3 of them even had the code
> ordering changed to be able to use the function (unrolling done
> before the message instead of after), which basically means that if
> any error or warning is printed in any of these functions (or if any
> crashes) the error output will be confusing or missing.

Indeed… let's forget about it.
-- 
Maxime de Roucy

signature.asc
Description: This is a digitally signed message part

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Maxime de Roucy]

Hi Willy and Cyril,

I just send a new version of the patch.
I made some changes following the remarks of Willy.

> > I'm not sure to like this feature in its current implementation.
> > I fear it will also create some new issues depending on how people
> > will use it.

Indeed it should be use with care.
But for me it's as dangerous as the '--' option as '--' had clearly
been implemented to be used with bash globling.
However with '--' the sysadmin can filter himself the files list ; it
can't with my patch.

> > For example, I know lots of sysadmin who have the (bad) habit to
> > make backup of the configuration files in the same directory,
> > without cleaning it up. We may see some directories like this :
> > 
> > service1.cfg
> > service2.cfg
> > service2.cfg~
> > service3.cfg.20160509
> > service4.cfg-19980101
> > service5.cfg
> > service5.cfg.old
> > service6.cfg.disabled
> > 
> > ...and so on.
> > 
> > When several sysadmins share the same haproxy instance, it can
> > quickly
> > become annoying.
> > 
> > Other use cases I immediately see :
> > - some configurations provide the crt files in the same directory,
> > which will break things
> > - some others will store map files in the same directory also
> > - what about configurations with a README or similar in the
> > directory ? or swap files because someone else is editing a file at
> > the same time ?

The last point is a very good one I think, swap files can easily be
forgotten.
The previous ones are more controversial for me as with my patch the
directory itself become a configuration "file". If sysadmin put "trash"
in it, it's the same as if he put "trash" in a configuration file… it's
not haproxy's fault if it fail.

However, I agree that we should implement some safeguards.

> I think that before going further we should start by trying to
> define what we want to see and what we don't want.
>  Maybe a solution could be to at least impose a file name extension
> (eg: .cfg), and I'm not sure it's enough. For example what should we
> do with symlinks, some will prefer to follow them, others not to.
> Most likely we should skip all dot files, etc.
> I think the discussion should go on before we go further with the
> code.

OK.

For me we should add a filter on file name ; keeping only ending with
".cfg" and not starting with ".".
I vote for following symlinks, as the implementation currently does.
It means less work for me :-) … Joking aside, I don't see any point
against it and sysadmins will assume it does except if it's explicitly
said in the docs (docs which I should complet regarding this point).
-- 
Regards
Maxime de Roucy

signature.asc
Description: This is a digitally signed message part

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Cyril Bonté]

Hi Maxime and Willy,

Le 09/05/2016 11:17, Maxime de Roucy a écrit :

Hello,

Thanks for your remarks !


I think this is a nice addition, it completes well the ability to
load an arbitrary file list.


Good to hear that :)


I'm not sure to like this feature in its current implementation.
I fear it will also create some new issues depending on how people will 
use it.


For example, I know lots of sysadmin who have the (bad) habit to make 
backup of the configuration files in the same directory, without 
cleaning it up. We may see some directories like this :


service1.cfg
service2.cfg
service2.cfg~
service3.cfg.20160509
service4.cfg-19980101
service5.cfg
service5.cfg.old
service6.cfg.disabled

...and so on.

When several sysadmins share the same haproxy instance, it can quickly 
become annoying.


Other use cases I immediately see :
- some configurations provide the crt files in the same directory, which 
will break things

- some others will store map files in the same directory also
- what about configurations with a README or similar in the directory ? 
or swap files because someone else is editing a file at the same time ?




--
Cyril Bonté

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Willy Tarreau]

On Mon, May 09, 2016 at 01:30:51PM +0200, Willy Tarreau wrote:
> Hello Maxime,
> 
> On Mon, May 09, 2016 at 11:17:25AM +0200, Maxime de Roucy wrote:
> > > I have another comment here, please try to factor the error messages
> > > using a goto, this block appears at least 3 times :
> > 
> > I tried to use goto but there is always a slight difference in the
> > Alert(...) calls, so there is quite as many label as Alert call.
> 
> I had the impression that the error messages were the same, maybe I
> have not seen well.

I've rechecked and indeed there are some tiny differences, and the only two
identical ones will be merged once you switch to strdup(), so that's fine.

Regards,
Willy

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Willy Tarreau]

Hello Maxime,

On Mon, May 09, 2016 at 11:17:25AM +0200, Maxime de Roucy wrote:
> > I have another comment here, please try to factor the error messages
> > using a goto, this block appears at least 3 times :
> 
> I tried to use goto but there is always a slight difference in the
> Alert(...) calls, so there is quite as many label as Alert call.

I had the impression that the error messages were the same, maybe I
have not seen well.

> After some lines it seams error-prone and not very readable.

I understand.

> Instead I create Alert_exit which embedded the exit call inside Alert.
> I just sent the patchs.
> 
> Is this solution good for you ?

To be honnest, no, I really don't like it. There are very few such locations
that benefit from it and 2/3 of them even had the code ordering changed to
be able to use the function (unrolling done before the message instead of
after), which basically means that if any error or warning is printed in
any of these functions (or if any crashes) the error output will be confusing
or missing.

Thanks,
Willy

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Maxime de Roucy]

Hello,

Thanks for your remarks !

> I think this is a nice addition, it completes well the ability to
> load an arbitrary file list.

Good to hear that :)

> However I cannot merge it as is, there is a huge buffer overflow
> …

Ok, I see. I will try to patch that this evening.

> I have another comment here, please try to factor the error messages
> using a goto, this block appears at least 3 times :

I tried to use goto but there is always a slight difference in the
Alert(...) calls, so there is quite as many label as Alert call.
After some lines it seams error-prone and not very readable.

Instead I create Alert_exit which embedded the exit call inside Alert.
I just sent the patchs.

Is this solution good for you ?
-- 
Thanks
Maxime

signature.asc
Description: This is a digitally signed message part

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Willy Tarreau]

Hi,

On Sun, May 08, 2016 at 11:41:17AM +0200, Maxime de Roucy wrote:
> If -f argument is a directory add all the files (and only files) it
> containes to the config files list.

I think this is a nice addition, it completes well the ability to load
an arbitrary file list.

However I cannot merge it as is, there is a huge buffer overflow here :

> + char filename[MAX_CLI_DIRFILE_NAME];
> + struct dirent *dir_entry = dir_entries[dir_entries_it];
> +
> + strcpy(filename, wl->s);
> + strcat(filename, "/");
> + strcat(filename, dir_entry->d_name);

You'd rather use snprintf("%s/%s") and check the result.

As a hint, you should always consider that strcpy() with a variable on
input is almost always a bug, and that strcat() is always a bug.


I have another comment here, please try to factor the error messages
using a goto, this block appears at least 3 times :

> +
> + if (stat(filename, _stat)) {
> + Alert("Cannot open configuration file %s : 
> %s\n",
> +   filename,
> +   strerror(errno));
> + exit(1);
> + }

I think it would be easier to move all this patch into a dedicated function.

Here instead of calloc+strcpy, you can simply use strdup() :

> + wlt->s = calloc(strlen(filename) + 1, 
> sizeof(*wlt->s));
> + if (!wlt->s) {
> + Alert("Cannot load configuration files 
> %s : out of memory.\n",
> +   filename);
> + exit(1);
> + }
> + strcpy(wlt->s, filename);

Otherwise it looks good.

Thanks,
Willy

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Maxime de Roucy]

Hello,

The second version of the patch fix the typo already mentioned and
change :
- wlt->s = calloc(MAX_CLI_DIRFILE_NAME, 1);
+ wlt->s = calloc(strlen(filename) + 1, sizeof(*wlt->s));
-- 
Regards
Maxime de Roucy

signature.asc
Description: This is a digitally signed message part

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Maxime de Roucy]

Hello,

I just realize I include a typo when editing my commit message to
respect the 75 char limit :

>   $ ./haproxy -C rootdir -f root2 -f superdir2 \
>      -f root -f superdir1 -f root1

is in fact :

>   $ ./haproxy -C rootdir -f root2 -f superdir2 \
>      -f root3 -f superdir1 -f root1

Should I re-send my patch or is it OK ?
-- 
Regards
Maxime de Roucy

signature.asc
Description: This is a digitally signed message part