Re: [PATCH]: Check for duplicated conflicting proxies

On Sat, Oct 20, 2007 at 02:22:16PM +0200, Krzysztof Oledzki wrote:
> >After some thinking, the listen should not share its name with anything,
> >because it also provides the "ruleset" capability. BTW, while rulesets
> >can be declared, they are currently not really used. The goal was to be
> >able to share complex rulesets between various listeners instead of
> >having to rewrite them each time.
>
> Indeed, I was really surprised when I found this in the code as there is
> nothing about this in the documentation. Now I know why. ;)

:-)

In fact, while working on it, I realized that it would become a nightmare for the admin to understand what rules were evaluated at what moment (frontend, rulesets, backend, ...). Later this will probably evolve, but not immediately.

> >+ if (!strcmp(curproxy->id, args[1]) &&
> >+ (curproxy->cap & rc & (PR_CAP_LISTEN))) {
> >+ Alert("parsing %s: duplicated proxy %s with
> >conflicting capabilities: %X/%X!\n",
> >+ file, args[1], curproxy->cap, rc);
> >+ return -1;
> >+ }
>
>
> I'm not sure about this as all frontend, backend and listen get the
> PR_CAP_RS attribute:
>
> if (!strcmp(args[0], "listen"))
> rc = PR_CAP_LISTEN;
> else if (!strcmp(args[0], "frontend"))
> rc = PR_CAP_FE | PR_CAP_RS;
> else if (!strcmp(args[0], "backend"))
> rc = PR_CAP_BE | PR_CAP_RS;
> else if (!strcmp(args[0], "ruleset"))
> rc = PR_CAP_RS;
> else
> rc = PR_CAP_NONE;

Ah yes you're right.

> If I get the code rigth, PR_CAP_LISTEN simply means "match all":
>
> #define PR_CAP_NONE 0x0000
> #define PR_CAP_FE 0x0001
> #define PR_CAP_BE 0x0002
> #define PR_CAP_RS 0x0004
> #define PR_CAP_LISTEN (PR_CAP_FE|PR_CAP_BE|PR_CAP_RS)

Not "all", but "all of BE, FE, RS", which is somewhat different if the code is meant to evolve. Indeed, it's easier to look for PR_CAP_* everywhere than to find where it may be implicit.

> Additionally IMHO, there is no gain using PR_CAP_LISTEN in the "if".
>
> So, to do it what you described we would rather need something like this:
>
> if (x==y || ((x & y) & (PR_CAP_FE | PR_CAP_BE)) || x==PR_CAP_LISTEN
> || y==PR_CAP_LISTEN)
>
> listen backend frontend ruleset
> listen - - - -
> backend - - OK OK
> frontend - OK - OK
> ruleset - OK OK -
>
>
> The question remains if we should allow to use ruleset also with backend
> and frontend. Reading your above explanation I think no, so as soon you
> confirm this, I will send a fixed patch with:
>
> if (((x | y) != PR_CAP_LISTEN) || x==PR_CAP_LISTEN ||
> y==PR_CAP_LISTEN)
>
> listen backend frontend ruleset
> listen - - - -
> backend - - OK -
> frontend - OK - -
> ruleset - - - -

I agree with your table, but the "if" becomes hard to understand IMHO. I wouldn't like to read it at 3am. I'd rather use something like this :

   if (!((x == PR_CAP_FE && y == PR_CAP_BE) ||

         (x == PR_CAP_BE && y == PR_CAP_FE)))

Basically, it explicitly catches all cases where the two entries are not exactly one FE and one BE, with both being different. Also, since we've been discussing it, it would be good to add a good comment around the test.

Regards,
Willy Received on 2007/10/20 15:10