VSB_tofile() not public in libvarnishapi.so

[slimhazard]

VSB_tofile() has been around since cf14a0f, but:

$ nm libvarnishapi.so.2.0.0 | grep tofile
000000000000a790 t VSB_tofile

The lowercase t meaning not global, so it can't be used from a VMOD or utility that just links to libvarnishapi.

Is that intentional? I suspect that adding it to libvarnishapi.map may have been forgotten.

[bsdphk]

As far as I know, it is not intentional.

[nigoroll]

bugwash:

• start LIBVARNISHAPI_2.4
• review if anything else is missing
• add missing symbols

[Dridi]

I didn't find anything missing, but I think I found a spurious entry:

VCLI_WriteResult doesn't look like something a libvarnishapi client would have any business calling. For this reason, I'm not worried about breaking the ABI since this should be a false-positive, similar to the VRT vs VPI situation. Correct me if I'm wrong, but I have a patch ready that adds VSB_tofile and removes VCLI_WriteResult. All I need is a confirmation for the latter.

[slimhazard]

Agree that VCLI_WriteResult doesn't make sense in a client API.

[Dridi]

@slimhazard I'm surprised though that you can't use it in a VMOD. It's available via libvarnish, so if you link against varnishd and varnishd uses VSB_tofile (and it does) then your VMOD should be able to link it as well.

[slimhazard]

@Dridi sure, if it's a VMOD, and once you get Varnish to load the VMOD. So I may have been wrong about VMODs, but I had tried it with a client app.

I'm not sure if a VMOD build can get past the ld step if it depends on a symbol in libvarnish.so. Unless you put libvarnish.so into the linker path, and then you're into source-dependent builds.

At any rate, I think making $ABI vrt and libvarnishapi.so as friendly as possible to VMOD authors is in the best interest of the project.

This is an area where I think there's a need for clarity. It confuses me when I see something in the Varnish /usr/include or /usr/local/include headers, but the symbols aren't public -- those are known as "public include paths", after all. If it's in there, I assume that my client is allowed to use it, but that doesn't seem to guide the actual thinking of the project.

[Dridi]

I definitely agree that public includes could better be curated, and it has been incrementally improved release after release.

[Dridi]

Reopening because I have a question before VSB_tofile() becomes part of the public API: why is it the only VSB_*() function that doesn't take a VSB as its first argument?

[slimhazard]

Just to comment that although I opened the ticket, it's probably @bsdphk who can answer the question.

If I had to guess, I'd say it's following the style of a lib call like write(2) that has an int fd as the first arg and a *buf as the next arg.

[nigoroll]

@Dridi I do not know the answer to why but I think the different signature makes sense: The other VSB function do something to the VSB, this one does something with the VSB. The exception to this rule are VSB_error(), VSB_data() and VSB_len(), but they only take one const argument.

[bsdphk]

You both got a point.

As a general rule (with many exceptions!) in C-land one should start out with the function arguments which change state.

On the other hand, in OO-ish-C, the first argument should consistently be the 'self/this' argument.

Given how VSB tries hard to isolate the internals, we should probably lean the OO direction and make vsb the first argument.

[Dridi]

Nobody really argued one way or the other. I asked, and Zejerman formulated hypotheses.

If we need to make a decision, it needs to happen "fast", and fwiw I would lean towards the OO direction.

[slimhazard]

+-0 for either way, flip a coin or go with OO, since some of us are leaning that way. #bikeshed