Skip to content

Commit

Permalink
Introduce $ABI [strict|vrt] for VMOD descriptors
Browse files Browse the repository at this point in the history
When versioning appeared in the VRT API, the goal was to allow loose
ABI compliance on loaded VMODs based on the major/minor revision against
which it was built. Strict checking was performed if Varnish was built
from the master branch in the VCC code, but omitted by the child.

This however has two flaws:

1) Release management might go wrong like it happened in 5.1.2 that got
   released from the master branch.

2) This doesn't solve the original problem that some VMODs might rely
   on supported symbols encompassed by the VRT major/minor while others
   may choose to integrate deeper with Varnish and lose guarantees.

This patch retires the `VCS_Branch` macro that is no longer needed and
provides a new `$ABI` stanza that defaults to strict when omitted. To
help discovery, in-tree modules advertise a strict match.

To indicate that a VMOD needs the exact Varnish build to be loaded,
the VMOD's metadata contains 0.0 for the VRT major/minor revision.

In addition, both the VCC and child now perform the full ABI compliance
check, picking the strict or vrt option depending on the VMOD's metadata.

Closes varnishcache#2330
  • Loading branch information
dridi authored and dmatetelki committed Mar 14, 2019
1 parent 9b7eaac commit 2ce17c1
Show file tree
Hide file tree
Showing 8 changed files with 48 additions and 25 deletions.
17 changes: 13 additions & 4 deletions bin/varnishd/cache/cache_vrt_vmod.c
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
#include <stdlib.h>

#include "vcli_serve.h"
#include "vmod_abi.h"
#include "vrt.h"

/*--------------------------------------------------------------------
Expand Down Expand Up @@ -65,6 +66,16 @@ struct vmod {

static VTAILQ_HEAD(,vmod) vmods = VTAILQ_HEAD_INITIALIZER(vmods);

static unsigned
vmod_abi_mismatch(const struct vmod_data *d)
{

if (d->vrt_major == 0 && d->vrt_minor == 0)
return (d->abi == NULL || strcmp(d->abi, VMOD_ABI_Version));

return (d->vrt_major != VRT_MAJOR_VERSION ||
d->vrt_minor > VRT_MINOR_VERSION);
}

int
VRT_Vmod_Init(VRT_CTX, struct vmod **hdl, void *ptr, int len, const char *nm,
Expand Down Expand Up @@ -112,15 +123,13 @@ VRT_Vmod_Init(VRT_CTX, struct vmod **hdl, void *ptr, int len, const char *nm,
FREE_OBJ(v);
return (1);
}
if (d->vrt_major != VRT_MAJOR_VERSION ||
d->vrt_minor > VRT_MINOR_VERSION ||
if (vmod_abi_mismatch(d) ||
d->name == NULL ||
strcmp(d->name, nm) ||
d->func == NULL ||
d->func_len <= 0 ||
d->proto == NULL ||
d->spec == NULL ||
d->abi == NULL) {
d->spec == NULL) {
VSB_printf(ctx->msg,
"Loading VMOD %s from %s:\n", nm, path);
VSB_printf(ctx->msg, "VMOD data is mangled.\n");
Expand Down
28 changes: 13 additions & 15 deletions config.phk
Original file line number Diff line number Diff line change
Expand Up @@ -107,25 +107,23 @@ fi

VCSF=include/vcs_version.h
VMAV=include/vmod_abi.h
V=NOGIT

if [ -d ./.git ] ; then
V=`git show -s --pretty=format:%h`
B=`git rev-parse --abbrev-ref HEAD`
else
V="NOGIT"
B="NOGIT"
fi
(
echo "/* $V */"
echo "/*"
echo " * NB: This file is machine generated, DO NOT EDIT!"
echo " *"
echo " * make(1) updates this when necessary"
echo " *"
echo " */"
echo "#define VCS_Version \"$V\""
echo "#define VCS_Branch \"$B\""
) > ${VCSF}_

cat > ${VCSF}_ <<EOF
/* $V */
/*
* NB: This file is machine generated, DO NOT EDIT!
*
* make(1) updates this when necessary
*
*/
#define VCS_Version "$V"
EOF

if [ ! -f ${VCSF} ] ; then
mv ${VCSF}_ ${VCSF}
rm -f ${VMAV}
Expand Down
1 change: 0 additions & 1 deletion lib/libvcc/generate.py
Original file line number Diff line number Diff line change
Expand Up @@ -1417,7 +1417,6 @@ def rst_where(fo, h, l):
fo = open(vcsfn, "w")
file_header(fo)
fo.write('#define VCS_Version "%s"\n' % v)
fo.write('#define VCS_Branch "%s"\n' % b)
fo.close()

for i in open(os.path.join(buildroot, "Makefile")):
Expand Down
6 changes: 3 additions & 3 deletions lib/libvcc/vcc_vmod.c
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ vcc_ParseImport(struct vcc *tl)
vcc_ErrWhere(tl, mod);
return;
}
if (strcmp(VCS_Branch, "master") == 0 &&
if (vmd->vrt_major == 0 && vmd->vrt_minor == 0 &&
strcmp(vmd->abi, VMOD_ABI_Version) != 0) {
VSB_printf(tl->sb, "Incompatible VMOD %.*s\n", PF(mod));
VSB_printf(tl->sb, "\tFile name: %s\n", fnp);
Expand All @@ -164,8 +164,8 @@ vcc_ParseImport(struct vcc *tl)
vcc_ErrWhere(tl, mod);
return;
}
if (vmd->vrt_major != VRT_MAJOR_VERSION ||
vmd->vrt_minor > VRT_MINOR_VERSION) {
if (vmd->vrt_major != 0 && (vmd->vrt_major != VRT_MAJOR_VERSION ||
vmd->vrt_minor > VRT_MINOR_VERSION)) {
VSB_printf(tl->sb, "Incompatible VMOD %.*s\n", PF(mod));
VSB_printf(tl->sb, "\tFile name: %s\n", fnp);
VSB_printf(tl->sb, "\tVMOD version %u.%u\n",
Expand Down
18 changes: 16 additions & 2 deletions lib/libvcc/vmodtool.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import random

rstfmt = False
strict_abi = True

ctypes = {
'ACL': "VCL_ACL",
Expand Down Expand Up @@ -434,6 +435,13 @@ def rsttail(self, fo, man):
fo.write("* :ref:`%s`\n" % i[1])
fo.write("\n")

class s_abi(stanza):
def parse(self):
if self.line[1] not in ('strict', 'vrt'):
err("Valid ABI types are 'strict' or 'vrt', got '%s'\n" %
self.line[1])
strict_abi = self.line[1] == 'strict'

class s_event(stanza):
def parse(self):
self.event_func = self.line[1]
Expand Down Expand Up @@ -651,6 +659,8 @@ def parse(self):
err("$Module must be first stanze")
if c[0] == "Module":
s_module(c, b[1:], self)
elif c[0] == "ABI":
s_abi(c, b[1:], self)
elif c[0] == "Event":
s_event(c, b[1:], self)
elif c[0] == "Function":
Expand Down Expand Up @@ -734,8 +744,12 @@ def api(self, fo):
fo.write("\n/*lint -esym(%d, Vmod_%s_Data) */\n" % (i, self.modname))
fo.write("const struct vmod_data Vmod_%s_Data = {\n" %
self.modname)
fo.write("\t.vrt_major =\tVRT_MAJOR_VERSION,\n")
fo.write("\t.vrt_minor =\tVRT_MINOR_VERSION,\n")
if strict_abi:
fo.write("\t.vrt_major =\t0,\n")
fo.write("\t.vrt_minor =\t0,\n")
else:
fo.write("\t.vrt_major =\tVRT_MAJOR_VERSION,\n")
fo.write("\t.vrt_minor =\tVRT_MINOR_VERSION,\n")
fo.write('\t.name =\t\t"%s",\n' % self.modname)
fo.write('\t.func =\t\t&Vmod_Func,\n')
fo.write('\t.func_len =\tsizeof(Vmod_Func),\n')
Expand Down
1 change: 1 addition & 0 deletions lib/libvmod_debug/vmod.vcc
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
# SUCH DAMAGE.

$Module debug 3 Development, test and debug
$ABI strict

DESCRIPTION
===========
Expand Down
1 change: 1 addition & 0 deletions lib/libvmod_directors/vmod.vcc
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
# SUCH DAMAGE.

$Module directors 3 Varnish Directors Module
$ABI strict

DESCRIPTION
===========
Expand Down
1 change: 1 addition & 0 deletions lib/libvmod_std/vmod.vcc
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
# SUCH DAMAGE.

$Module std 3 Varnish Standard Module
$ABI strict

DESCRIPTION
===========
Expand Down

0 comments on commit 2ce17c1

Please sign in to comment.