Hello Willy Thanks for the detailed explanations and the feedback. PFA the two patches based on your feedback. My apologies for the delay.
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
Yes, this is what I was thinking.
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.
Sounds good. The revised patch uses option 3 above.
> > /* 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.
This was intentional. I thought it was a "miss" in the existing code. This
change is now part of the second patch.
I have also fixed indentation/tabs, column width, etc. based on your
feedback.
I have a Q for you. Pasting code from the revised patch.
+ // arg1 value is greater than the remaining length
+ if (smp->opt & SMP_OPT_FINAL) {
+ // truncate to remaining length
+ length = smp->data.u.str.data - start_idx;
+ } else {
+ smp->data.u.str.data = 0;
+ return 0;
+ }
I tried to write a test in the vtc file for the "else" clause above, but
could not figure out when we will hit that condition. Here is what I tried.
Added following the config section.
+ http-response set-header hdr_bytes_0_7
"%[res.hdr(input),bytes(0,7)]"
+ http-response set-header hdr_bytes_0_7_concat
"%[res.hdr(input),bytes(0,7),concat()]"
Added following in the client section
+ # since "bytes" is the non-final operator, response would be empty
+ expect resp.http.hdr_bytes_0_7_concat == ""
But the test failed:
---- c2 EXPECT resp.http.hdr_bytes_0_7_concat (012345) == "" failed
Anyway, I can add this such a test in another patch if needed and time
permits. Let me know what you think.
Thanks
Lokesh
On Fri, Sep 8, 2023 at 10:05 AM Willy Tarreau <[email protected]> wrote:
> 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/[email protected]/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
>
0002-MEDIUM-sample-Small-fix-in-function-check_operator-f.patch
Description: Binary data
0001-MEDIUM-sample-Enhances-converter-bytes-to-take-varia.patch
Description: Binary data
