On Thu, Sep 07, 2023 at 10:45:58AM -0700, Lokesh Jindal wrote:
> Resending the patch with the right email subject. Previos email
> discussion - here
> <https://www.mail-archive.com/haproxy@formilux.org/msg43973.html>.
I saw your previous mail, just didn't have time yet to review it.
> PFA the patch file.
Thanks!
> I have two questions in general:
> 1. Can you explain what this method does and why is it needed? (I used it
> in my patch following the pattern in the converter "sub")
> *smp_set_owner(&smp_arg0, smp->px, smp->sess, smp->strm, smp->opt);**
It just makes sure that the newly created sample properly references
the context from which it was created. I remember that *some* functions
retrieve some elements there, maybe ACLs or Lua actions that need to
yield, etc.
> 2. In my patch, *sample_conv_bytes* returns 0 in case of an invalid arg
> e.g. negative offset. Can you explain when a converter should return 0 V 1?
> I am not sure how that impacts the haproxy behavior.
Normally when evaluating an expression, 0 is returned to indicate
"nothing for now" but this doesn't preclude that it will not be possible
to have more info later (depending on extra flags added such as
SMP_F_MAY_CHANGE). 1 normally indicates that we have a result and that
the sample is correctly filled. If a zero is returned without any flag,
it will usually abort the evaluation of the expression (e.g. it cannot
be converted thus processed). ACL evaluation will not go further with
that sample for example. If 1 is returned but the sample type is not
filled, implicitly the caller will see that SMP_T_ANY is returned and
not compatible with the next expected type so it will stop there as well.
Thus I think that returning 0 or 1 in case of failure to process args will
achieve the same result but for slightly different reasons. 0 is probably
cleaner though.
> + if (smp_arg1.data.u.sint < 0 || (smp_arg1.data.u.sint >
> (smp->data.u.str.data - start_idx))) {
> + // empty output if the arg2 value is negative or bigger
> than the remaining length
> + smp->data.u.str.data = 0;
> + return 0;
> + }
Here I'm wondering what we should do if the user asks for more bytes than
available. I think we have 3 options:
- return 0 so that the sample's flags are consulted by the caller (e.g.
SMP_F_MAY_CHANGE will allow the caller to come back later trying to
get more bytes, or fail if not enough)
- truncate to what is available
- maybe a combination of both, i.e. return 0 as long as the caller does
not pass SMP_OPT_FINAL, or return a truncated response.
Truncation can be useful but I could understand that when extracting bytes
one would want this exact number of bytes to continue the processing. I
just wanted to mention this to make sure this is what you had in mind as
well.
> + length = smp_arg1.data.u.sint;
> + } else {
> + length = smp->data.u.str.data - start_idx;
> + }
>
> - if ((arg_p[1].type == ARGT_SINT) && (arg_p[1].data.sint <
> smp->data.u.str.data))
> - smp->data.u.str.data = arg_p[1].data.sint;
It seems to me that previously we would truncate if less bytes were
available, so maybe we'll need to preserve this or do a mix of the
two as I mentioned above.
> + // update the output using the start_idx and length
> + smp->data.u.str.area += start_idx;
> + smp->data.u.str.data = length;
>
> return 1;
> }
> @@ -3120,7 +3144,7 @@ static int check_operator(struct arg *args, struct
> sample_conv *conv,
> long long int i;
>
> /* Try to decode a variable. */
> - if (vars_check_arg(&args[0], NULL))
> + if (vars_check_arg(&args[0], err))
> return 1;
I can't say if this change was intentional or not, nor if the NULL had a
particular reason or not, but at least this is unrelated to your patch
and would need to be in its own patch, so that in case of regression we
can decide whether to fix or revert it. And if it really fixes an issue
maybe we'll want to backport it.
> /* Try to convert an integer */
> @@ -4905,7 +4929,34 @@ static int smp_fetch_conn_timers(const struct arg
> *args, struct sample *smp, con
> return 0;
> }
>
> +static int sample_conv_bytes_check(struct arg *args, struct sample_conv
> *conv,
> + const char *file, int line, char **err)
> +{
> + // arg0 is not optional, must be >= 0
> + if (!check_operator(&args[0], conv, file, line, err)) {
> + return 0;
> + }
> + if (args[0].type != ARGT_VAR) {
> + if (args[0].type != ARGT_SINT || args[0].data.sint < 0) {
> + memprintf(err, "expects a non-negative integer");
> + return 0;
> + }
> + }
You're having indentation issues in the "if" block above, which only
uses 4-spaces instead of a tab.
> + // arg1 is optional, must be > 0
> + if (args[1].type != ARGT_STOP) {
> + if (!check_operator(&args[1], conv, file, line, err)) {
> + return 0;
> + }
And here the "return" misses an indent level.
> + if (args[1].type != ARGT_VAR) {
> + if (args[1].type != ARGT_SINT || args[1].data.sint <=
> 0) {
> + memprintf(err, "expects a positive integer");
> + return 0;
> + }
> + }
> + }
>
> + return 1;
> +}
(...)
OK overall that looks pretty good. Just one trivial patch to split, a
need to decide what to do on too small inputs, and trivial cleanups. Ah
also, please make sure to wrap your text at 80-cols in the doc (having
the examples slightly exceed it is not critical though as some users
prefer to easily copy-paste them).
I predict the next one will be mergeable as-is :-)
Thanks!
Willy
