Re: [PATCH] MINOR: sample: add json_string
Willy,
On 4/16/21 8:24 AM, Willy Tarreau wrote:
- tok = mjson_find(smp->data.u.str.area, smp->data.u.str.data,
args[0].data.str.area, , );
+ enum mjson_tok token_type = mjson_find(smp->data.u.str.area,
smp->data.u.str.data, args[0].data.str.area, , _size);
As much as possible, please try to avoid assignments in declarations,
especially when using functions. There are several reasons that make
this annoying:
- sometimes just changing one arg requires to reorder the declarations
- debugging with them is painful
- trying to comment some of them out is painful as well
- it discourages from adding finer control (e.g. if you need to do
this or that using and if/else block, you also have to significantly
modify them).
- quite a few times I've seen code like this been deleted because when
the compiler says "unused variable foo", it makes one think it's now
OK to delete it, except that the function possibly had a desired side
effect
=> thus better declare the enum with the variables
Same goes for one or two other ones in the switch/case statements.
I forgot to do in this patch, but I prefer directly assigning, because I
can mark the variables `const` that way. It also increases verbosity for
these single-assignment variables which IMO reduces readability.
Also I really dislike the (old) C style of declaring all variables at
the very top of the scope. Most notably not being allowed to `for
(size_t i = 0; ...; ...)` is bothering me quite a bit.
But who am I to complain? Updated patches attached. I left the 'trash'
one as-is, because that one really is a common pattern. I hope I did not
miss anything else :-)
Best regards
Tim Düsterhus
>From d37a4788b6be31a7dd53a0724baaaeb4e160b85d Mon Sep 17 00:00:00 2001
From: Tim Duesterhus
Date: Thu, 15 Apr 2021 18:08:48 +0200
Subject: [PATCH v2 1/3] CLEANUP: sample: Improve local variables in
sample_conv_json_query
To: haproxy@formilux.org
Cc: w...@1wt.eu
This improves the use of local variables in sample_conv_json_query:
- Use the enum type for the return value of `mjson_find`.
- Do not use single letter variables.
- Reduce the scope of variables that are only needed in a single branch.
- Add missing newlines after variable declaration.
---
src/sample.c | 26 --
1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/src/sample.c b/src/sample.c
index 728c7c76d..c2d9beda3 100644
--- a/src/sample.c
+++ b/src/sample.c
@@ -3723,16 +3723,17 @@ static int sample_check_json_query(struct arg *arg, struct sample_conv *conv,
static int sample_conv_json_query(const struct arg *args, struct sample *smp, void *private)
{
struct buffer *trash = get_trash_chunk();
- const char *p; /* holds the temporary string from mjson_find */
- int tok, n;/* holds the token enum and the length of the value */
- int rc;/* holds the return code from mjson_get_string */
+ const char *token; /* holds the temporary string from mjson_find */
+ int token_size;/* holds the length of */
- tok = mjson_find(smp->data.u.str.area, smp->data.u.str.data, args[0].data.str.area, , );
+ enum mjson_tok token_type;
- switch(tok) {
+ token_type = mjson_find(smp->data.u.str.area, smp->data.u.str.data, args[0].data.str.area, , _size);
+
+ switch (token_type) {
case MJSON_TOK_NUMBER:
if (args[1].type == ARGT_SINT) {
-smp->data.u.sint = atoll(p);
+smp->data.u.sint = atoll(token);
if (smp->data.u.sint < JSON_INT_MIN || smp->data.u.sint > JSON_INT_MAX)
return 0;
@@ -3740,6 +3741,7 @@ static int sample_conv_json_query(const struct arg *args, struct sample *smp, vo
smp->data.type = SMP_T_SINT;
} else {
double double_val;
+
if (mjson_get_number(smp->data.u.str.area, smp->data.u.str.data, args[0].data.str.area, _val) == 0) {
return 0;
} else {
@@ -3757,17 +3759,21 @@ static int sample_conv_json_query(const struct arg *args, struct sample *smp, vo
smp->data.type = SMP_T_BOOL;
smp->data.u.sint = 0;
break;
- case MJSON_TOK_STRING:
- rc = mjson_get_string(smp->data.u.str.area, smp->data.u.str.data, args[0].data.str.area, trash->area, trash->size);
- if (rc == -1) {
+ case MJSON_TOK_STRING: {
+ int len;
+
+ len = mjson_get_string(smp->data.u.str.area, smp->data.u.str.data, args[0].data.str.area, trash->area, trash->size);
+
+ if (len == -1) {
/* invalid string */
return 0;
} else {
-trash->data = rc;
+trash->data = len;
smp->data.u.str = *trash;
smp->data.type = SMP_T_STR;
}
break;
+ }
default:
/* no valid token found */
return 0;
--
2.31.1
>From ecc5b3f91d8fc02f1cd5ed84949cb0aef02df075 Mon Sep 17 00:00:00 2001
From: Tim Duesterhus
Date: Thu, 15 Apr 2021 18:14:32 +0200
Subject: [PATCH v2 2/3] CLEANUP: sample: Explicitly handle all possible enum
values from mjson
To: haproxy@formilux.org
Cc: w...@1wt.eu
This makes it easier to find bugs, because -Wswitch can help us.
---
Re: [PATCH v2 0/8] URI normalization / Issue #714
Willy, Christopher, On 4/16/21 6:56 PM, Willy Tarreau wrote: Thanks Tim, The series looks good to me. Except if Willy has any comments, I'll merge it soon. No need for me to double-check, I'll trust you (and you know I like to complain afterwards :-)) I'll probably complain myself afterwards as well :-) As a heads up I'll definitely add a few more converters (most notably the percent decoder is missing) and maybe I'll rename the existing ones in the configuration for consistency once I finish this up. So: It should definitely considered *EXPERIMENTAL* for 2.4. Best regards Tim Düsterhus
Re: [PATCH v2 0/8] URI normalization / Issue #714
On Fri, Apr 16, 2021 at 06:51:41PM +0200, Christopher Faulet wrote: > Le 15/04/2021 à 21:45, Tim Duesterhus a écrit : > > Christopher, > > > > again: Thank you for the very helpful review. In this series you can find > > v2 of > > my URI normalization patches. I hope I did not forget to incorporate any of > > your feedback. > > > > Some general notes: > > > > - I completely cleaned up the commit history to group similar patches (e.g. > > the > >two patches for dotdot) and to avoid later commits completely refactoring > >earlier commits (e.g. the error handling). > > - As suggested I am now returning the error code and taking a `struct ist > > *dst`. > > - The values of `enum uri_normalizer_err` are cleaned up. > > - I cleaned up the error handling in `http_action_normalize_uri`. > > - I am now using `istdiff()`. > > - Dynamic allocation is no more. > > - I fixed some parts of the code style (`struct ist* foo` -> `struct ist > > *foo`). > > - I const'ified as much as possible. > > > > Thanks Tim, > > The series looks good to me. Except if Willy has any comments, I'll merge it > soon. No need for me to double-check, I'll trust you (and you know I like to complain afterwards :-)) Thanks! Willy
Re: [PATCH v2 0/8] URI normalization / Issue #714
Le 15/04/2021 à 21:45, Tim Duesterhus a écrit : Christopher, again: Thank you for the very helpful review. In this series you can find v2 of my URI normalization patches. I hope I did not forget to incorporate any of your feedback. Some general notes: - I completely cleaned up the commit history to group similar patches (e.g. the two patches for dotdot) and to avoid later commits completely refactoring earlier commits (e.g. the error handling). - As suggested I am now returning the error code and taking a `struct ist *dst`. - The values of `enum uri_normalizer_err` are cleaned up. - I cleaned up the error handling in `http_action_normalize_uri`. - I am now using `istdiff()`. - Dynamic allocation is no more. - I fixed some parts of the code style (`struct ist* foo` -> `struct ist *foo`). - I const'ified as much as possible. Thanks Tim, The series looks good to me. Except if Willy has any comments, I'll merge it soon. -- Christopher Faulet
Re: changed IP messages overrunning /var/log ?
Root cause - haproxy intentionally double logs :
https://github.com/haproxy/haproxy/blob/master/src/server.c
srv_update_addr(...) { ... /* generates a log line and a warning on
stderr */ ... }
On Thu, Apr 15, 2021 at 11:06 PM Jim Freeman wrote:
...
> The duplication of logging the new(?) 'changed its IP' messages to daemon.log
> (when only local* facilities are configured) is still getting root-cause
> analysis.
...
Bandwidth limitation in HAProxy
Hi. How difficult will it be to add a bandwidth limitation into HAProxy similar to the nginx feature? https://nginx.org/en/docs/http/ngx_http_core_module.html#limit_rate Regards Aleks
Re: [PATCH] MINOR: sample: add json_string
On Thu, Apr 15, 2021 at 06:47:29PM +0200, Tim Düsterhus wrote:
> You know that I'm obsessed with the correct use of data types, so please
> find three CLEANUP patches attached. Feel free to drop the ones you don't
> agree with :-)
I don't disagree with them, but am just having a few comments on style
in turn :-)
> static int sample_conv_json_query(const struct arg *args, struct sample
> *smp, void *private)
> {
> struct buffer *trash = get_trash_chunk();
> - const char *p; /* holds the temporary string from mjson_find */
> - int tok, n;/* holds the token enum and the length of the value */
> - int rc;/* holds the return code from mjson_get_string */
> + const char *token; /* holds the temporary string from mjson_find */
> + int token_size;/* holds the length of */
>
> - tok = mjson_find(smp->data.u.str.area, smp->data.u.str.data,
> args[0].data.str.area, , );
> + enum mjson_tok token_type = mjson_find(smp->data.u.str.area,
> smp->data.u.str.data, args[0].data.str.area, , _size);
As much as possible, please try to avoid assignments in declarations,
especially when using functions. There are several reasons that make
this annoying:
- sometimes just changing one arg requires to reorder the declarations
- debugging with them is painful
- trying to comment some of them out is painful as well
- it discourages from adding finer control (e.g. if you need to do
this or that using and if/else block, you also have to significantly
modify them).
- quite a few times I've seen code like this been deleted because when
the compiler says "unused variable foo", it makes one think it's now
OK to delete it, except that the function possibly had a desired side
effect
=> thus better declare the enum with the variables
Same goes for one or two other ones in the switch/case statements.
I'm otherwise OK.
Thanks!
Willy
Re: [PATCH] MINOR: sample: add json_string
On Thu, Apr 15, 2021 at 05:44:44PM +0200, Aleksandar Lazic wrote: > Now the Statement what I wanted to say ;-) > > HAProxy have now at least 4 possibilities to route traffic and > catch some data. > > HTTP fields > GRPC fields > FIX fields > JSON fields > > Have I missed something? You can extract various data from TCP payloads or even HTTP bodies, so it's just a matter of how simple you want things to be done. For example we've long supported RDP cookies, and I even deployed a somewhat limited but effective Corba analyser a long time ago :-) Willy
