Hi Daniel,

On Thu, May 30, 2019 at 12:57:10AM -0400, Daniel Corbett wrote:
> Hello,
> 
> 
> When communicating over SPOP the AGENT-HELLO, AGENT-DISCONNECT,
> and ACK frames must have the FIN flag set.
> 
> This patch also upgrades the SPOP_VERSION to 2.0 and adds the
> "ip_score" transaction variable to the python & lua scripts.
> 
> Thanks to Christopher for giving me the answer on how to solve this issue.

Are all these changes related ?  It seems to me these are distinct
features, or maybe even a bug fix plus an extra feature, in which
case these should be separate patches.

I'm having a few quick questions below.

> From e0e7992b3b2af3ac0e898d48d6ce5f7e9416b568 Mon Sep 17 00:00:00 2001
> From: Daniel Corbett <dcorb...@haproxy.com>
> Date: Wed, 29 May 2019 17:44:05 -0400
> Subject: [PATCH] BUG/MEDIUM: contrib/spoa_server: Set FIN flag on agent frames
> 
> When communicating over SPOP the AGENT-HELLO, AGENT-DISCONNECT,
> and ACK frames must have the FIN flag set.
> 
> This patch also upgrades the SPOP_VERSION to 2.0 and adds the
> "ip_score" transaction variable to the python & lua scripts.
(...)
> diff --git a/contrib/spoa_server/ps_lua.lua b/contrib/spoa_server/ps_lua.lua
> index 26620459..aaf373c6 100644
> --- a/contrib/spoa_server/ps_lua.lua
> +++ b/contrib/spoa_server/ps_lua.lua
> @@ -14,4 +14,5 @@ spoa.register_message("check-client-ip", function(args)
>       spoa.set_var_ipv6("ipv6", spoa.scope.txn, "1::f")
>       spoa.set_var_str("str", spoa.scope.txn, "1::f")
>       spoa.set_var_bin("bin", spoa.scope.txn, "1::f")
> +        spoa.set_var_int32("ip_score", spoa.scope.txn, 77)

Be careful, your editor seems to have mixed spaces and tabs here, and
a few other times in the same patch.

> diff --git a/contrib/spoa_server/spoa-server.conf 
> b/contrib/spoa_server/spoa-server.conf
> index 13bd126a..87bbb721 100644
> --- a/contrib/spoa_server/spoa-server.conf
> +++ b/contrib/spoa_server/spoa-server.conf
> @@ -1,5 +1,6 @@
>  global
>       debug
> +     nbthread 1

What is the reason for forcing nbthread to 1 here ? Did you face a
particular bug revealed by threads ? Given that many people use example
configs as starting point for theirs, I'd rather avoid doing this, or
this will last 10 years just like "option httpclose" or even the funny
"ssl-engine rdrand" which in addition to being wrong was so much copied
that google now auto-completes it when searching "ssl-engine haproxy"!
 
> diff --git a/contrib/spoa_server/spoa.c b/contrib/spoa_server/spoa.c
> index a958f222..8cb40c54 100644
> --- a/contrib/spoa_server/spoa.c
> +++ b/contrib/spoa_server/spoa.c
> @@ -679,13 +679,16 @@ error:
>   * the number of written bytes otherwise. */
>  static void prepare_agentack(struct worker *w)
>  {
> +     unsigned int flags = SPOE_FRM_FL_FIN;
> +
>       w->ack_len = 0;
>  
>       /* Frame type */
>       w->ack[w->ack_len++] = SPOE_FRM_T_AGENT_ACK;
>  
> -     /* No flags for now */
> -     memset(w->ack + w->ack_len, 0, 4); /* No flags */
> +     /* Set flags */
> +     flags = htonl(flags);

In general it's not a good idea to have the same variable hold two
different endian versions of a same value. You can be certain that
at one point something will be added above or below and will use the
wrong one. Better simply do "flags = htonl(SPOE_FRM_FL_FIN)" or even
better, preset flags to zero at the beginning of the function, and
then add this flag this way : "flags |= htonl(SPOE_FRM_FL_FIN)".

> +     memcpy(w->ack + w->ack_len, (char *)&flags, 4);

You don't need the "(char*)" cast here, "&flags" is correct.

The same applies to the two or three other locations. Thanks!
Willy

Reply via email to