Willy,
On 4/15/21 5:09 PM, Willy Tarreau wrote:
On Thu, Apr 15, 2021 at 04:49:00PM +0200, Aleksandar Lazic wrote:
#define JSON_INT_MAX ((1ULL << 53) - 1)
^
Sorry I was not clear, please drop that 'U' here.
I'm also sorry, I was in a tunnel :-/
Attached now the next patches.
Thank you! Now applied. I just fixed this remaining double indent issue
and that was all:
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 :-)
Aleks: Thanks for quickly addressing my feedback, even if you might
think I was overly pedantic. The final version looks pretty good to me,
my CLEANUP really is meant to be a final polishing!
Best regards
Tim Düsterhus
>From 1af53a6e73e75f48793720017fe07d0f57e8c74a Mon Sep 17 00:00:00 2001
From: Tim Duesterhus <t...@bastelstu.be>
Date: Thu, 15 Apr 2021 18:08:48 +0200
Subject: [PATCH 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 | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/src/sample.c b/src/sample.c
index 728c7c76d..a56d59245 100644
--- a/src/sample.c
+++ b/src/sample.c
@@ -3723,16 +3723,15 @@ 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 <token> */
- tok = mjson_find(smp->data.u.str.area, smp->data.u.str.data, args[0].data.str.area, &p, &n);
+ enum mjson_tok token_type = mjson_find(smp->data.u.str.area, smp->data.u.str.data, args[0].data.str.area, &token, &token_size);
- switch(tok) {
+ 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 +3739,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, &double_val) == 0) {
return 0;
} else {
@@ -3757,17 +3757,19 @@ 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 = 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 166ceb7c9a82f87f35580e3c379103d4d213fb18 Mon Sep 17 00:00:00 2001
From: Tim Duesterhus <t...@bastelstu.be>
Date: Thu, 15 Apr 2021 18:14:32 +0200
Subject: [PATCH 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.
---
src/sample.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/src/sample.c b/src/sample.c
index a56d59245..3cc571560 100644
--- a/src/sample.c
+++ b/src/sample.c
@@ -3770,8 +3770,19 @@ static int sample_conv_json_query(const struct arg *args, struct sample *smp, vo
}
break;
}
- default:
- /* no valid token found */
+ case MJSON_TOK_NULL:
+ case MJSON_TOK_ARRAY:
+ case MJSON_TOK_OBJECT:
+ /* We cannot handle these. */
+ return 0;
+ case MJSON_TOK_INVALID:
+ /* Nothing matches the query. */
+ return 0;
+ case MJSON_TOK_KEY:
+ /* This is not a valid return value according to the
+ * mjson documentation, but we handle it to benefit
+ * from '-Wswitch'.
+ */
return 0;
}
return 1;
--
2.31.1
>From 722b41673c0d31fa99583e75947451e47f506855 Mon Sep 17 00:00:00 2001
From: Tim Duesterhus <t...@bastelstu.be>
Date: Thu, 15 Apr 2021 18:40:06 +0200
Subject: [PATCH 3/3] CLEANUP: sample: Use explicit return for successful
`json_query`s
To: haproxy@formilux.org
Cc: w...@1wt.eu
Move the `return 1` into each of the cases, instead of relying on the single
`return 1` at the bottom of the function.
---
src/sample.c | 37 ++++++++++++++++++++++---------------
1 file changed, 22 insertions(+), 15 deletions(-)
diff --git a/src/sample.c b/src/sample.c
index 3cc571560..de32f43b6 100644
--- a/src/sample.c
+++ b/src/sample.c
@@ -3737,38 +3737,43 @@ static int sample_conv_json_query(const struct arg *args, struct sample *smp, vo
return 0;
smp->data.type = SMP_T_SINT;
+
+ return 1;
} else {
double double_val;
- if (mjson_get_number(smp->data.u.str.area, smp->data.u.str.data, args[0].data.str.area, &double_val) == 0) {
+ if (mjson_get_number(smp->data.u.str.area, smp->data.u.str.data, args[0].data.str.area, &double_val) == 0)
return 0;
- } else {
- trash->data = snprintf(trash->area,trash->size,"%g",double_val);
- smp->data.u.str = *trash;
- smp->data.type = SMP_T_STR;
- }
+
+ trash->data = snprintf(trash->area,trash->size,"%g",double_val);
+ smp->data.u.str = *trash;
+ smp->data.type = SMP_T_STR;
+
+ return 1;
}
- break;
case MJSON_TOK_TRUE:
smp->data.type = SMP_T_BOOL;
smp->data.u.sint = 1;
- break;
+
+ return 1;
case MJSON_TOK_FALSE:
smp->data.type = SMP_T_BOOL;
smp->data.u.sint = 0;
- break;
+
+ return 1;
case MJSON_TOK_STRING: {
int 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 = len;
- smp->data.u.str = *trash;
- smp->data.type = SMP_T_STR;
}
- break;
+
+ trash->data = len;
+ smp->data.u.str = *trash;
+ smp->data.type = SMP_T_STR;
+
+ return 1;
}
case MJSON_TOK_NULL:
case MJSON_TOK_ARRAY:
@@ -3785,7 +3790,9 @@ static int sample_conv_json_query(const struct arg *args, struct sample *smp, vo
*/
return 0;
}
- return 1;
+
+ my_unreachable();
+ return 0;
}
--
2.31.1
