Mailing List Archive: VSB_quote has gotten messy

I finally got around to look at VSB_QUOTE_GLOB feature Guillaume committed
by accident some time ago, and it doesn't work correctly as far as I
can tell, for instance, the difference between inputs:
[]
and
["]
makes no sense to me.

However, I can hardly blame Guillaume, because it is not very
consistent or clear how VSB_QUOTE is supposed to work in the first
place, I just spent 4 hours trying to find out, because we sort of
made it up as we went.

I propose that b0d1a40f326f... gets backed out before it has any
use in the tree, and put an errata on the 6.4 release page to the
effect of "do not use VSB_QUOTE_GLOB".

I also propose that we should deprecate VSB_quote*() in its current
form, ie: leave around for the benefit of VMODers for 7.x, remove
in 8.x.

Finally, I propose a new and more well thought, and better documented
replacement, VSB_encode(), to be added shortly.

Comments ?

--
Poul-Henning Kamp | UNIX since Zilog Zeus 3.20
phk@FreeBSD.ORG | TCP/IP since RFC 956
FreeBSD committer | BSD since 4.3-tahoe
Never attribute to malice what can adequately be explained by incompetence.
_______________________________________________
varnish-dev mailing list
varnish-dev@varnish-cache.org
https://www.varnish-cache.org/lists/mailman/listinfo/varnish-dev

On Mon, Apr 20, 2020 at 9:45 AM Poul-Henning Kamp <phk@phk.freebsd.dk> wrote:
>
> I finally got around to look at VSB_QUOTE_GLOB feature Guillaume committed
> by accident some time ago, and it doesn't work correctly as far as I
> can tell, for instance, the difference between inputs:
> []
> and
> ["]
> makes no sense to me.
>
> However, I can hardly blame Guillaume, because it is not very
> consistent or clear how VSB_QUOTE is supposed to work in the first
> place, I just spent 4 hours trying to find out, because we sort of
> made it up as we went.

As discussed previously, it was also on my review queue, and still is.

> I propose that b0d1a40f326f... gets backed out before it has any
> use in the tree, and put an errata on the 6.4 release page to the
> effect of "do not use VSB_QUOTE_GLOB".

Agreed.

> I also propose that we should deprecate VSB_quote*() in its current
> form, ie: leave around for the benefit of VMODers for 7.x, remove
> in 8.x.

No opinion, but if we replace the old API with a new one, maybe we can
manage to translate old API calls to new ones?

> Finally, I propose a new and more well thought, and better documented
> replacement, VSB_encode(), to be added shortly.
>
> Comments ?

One annoying limitation with the current quote logic is that calls
must be self-contained. If I want to quote a string that will come in
multiple parts, I have to assemble it (eg. with a VSB!) prior to
feeding it to VSB_quote().

If we land a new API, it would be nice to be able to delimit begin and
end steps, either with flags or more functions:

AZ(VSB_encode_begin(vsb, VSB_ENCODE_JSON)); /* adds the leading quote */
/* encode a VCL_STRANDS in a loop */
AZ(VSB_encode_end(vsb); /* adds the trailing quote */

If we move struct strands to libvarnish, we can also have
VSB_encode_strands(), but being able to encode multiple string
components independently of struct strands would be appreciated.

Dridi
_______________________________________________
varnish-dev mailing list
varnish-dev@varnish-cache.org
https://www.varnish-cache.org/lists/mailman/listinfo/varnish-dev