argstruct name clash

[nigoroll]

when two vmods implement the same class with optional arguments to the constructor, the argstruct symbols clash. I guess this would also happen with any other symbols by the same name.

Example:

import constant;
import taskvar;

from
https://code.uplex.de/uplex-varnish/varnish-objvar/blob/master/src/vmod_taskvar.rst
https://code.uplex.de/uplex-varnish/varnish-objvar/blob/master/src/vmod_constant.rst

Message from C-compiler:
vgc.c:15251:8: error: redefinition of 'vmod_acl__init_arg'
struct vmod_acl__init_arg {
       ^
vgc.c:14922:8: note: previous definition is here
struct vmod_acl__init_arg {
       ^
vgc.c:15275:8: error: redefinition of 'vmod_backend__init_arg'
struct vmod_backend__init_arg {
       ^
vgc.c:14938:8: note: previous definition is here

The workaround is to use different $Prefix, but we should clarify if this is really what $Prefix is intended for. Example: https://code.uplex.de/uplex-varnish/varnish-objvar/commit/6089fd5d1ba22ced780b69f19e3cfe4cc7d2d54d

We could also prefix the vmod name to just the argstruct, which would require changes to all vmods using optional arguments. But as of now, that's most likely not too many, so if we want to make such a breaking change, now is probably a good time.

[nigoroll]

FTR the argstruct-specific patch would look something like:

diff --git a/lib/libvcc/vmodtool.py b/lib/libvcc/vmodtool.py
index 7db51c668..e3f17e69c 100755
--- a/lib/libvcc/vmodtool.py
+++ b/lib/libvcc/vmodtool.py
@@ -479,7 +479,7 @@ class ProtoType(object):
         return "typedef " + self.proto(args, name=tn)
 
     def argstructname(self):
-        return "struct %s_arg" % self.cname(True)
+        return "struct %s_%s_arg" % (self.st.vcc.modname, self.cname(True))
 
     def argstructure(self):
         s = "\n" + self.argstructname() + " {\n"
diff --git a/lib/libvmod_debug/vmod_debug.c b/lib/libvmod_debug/vmod_debug.c
index a3a78975a..4e6a8ab89 100644
--- a/lib/libvmod_debug/vmod_debug.c
+++ b/lib/libvmod_debug/vmod_debug.c
@@ -195,7 +195,7 @@ xyzzy_rot52(VRT_CTX, VCL_HTTP hp)
 }
 
 VCL_STRING v_matchproto_(td_debug_argtest)
-xyzzy_argtest(VRT_CTX, struct xyzzy_argtest_arg *arg)
+xyzzy_argtest(VRT_CTX, struct debug_xyzzy_argtest_arg *arg)
 {
        char buf[100];
 

The vmod change is only included for vmod_debug here, if solved this way, vmod_directors would also need adjustment

[bsdphk]

Bugwash decision: Warn about missing $Prefix

[nigoroll]

bugwash conclusion: write up the $Prefix inconsistencies

[bsdphk]

How about something like this:

diff --git a/lib/libvcc/vmodtool.py b/lib/libvcc/vmodtool.py
index 7db51c668..359b04b9f 100755
--- a/lib/libvcc/vmodtool.py
+++ b/lib/libvcc/vmodtool.py
@@ -479,7 +479,7 @@ class ProtoType(object):
         return "typedef " + self.proto(args, name=tn)
 
     def argstructname(self):
-        return "struct %s_arg" % self.cname(True)
+        return "struct %s_%s_arg" % (self.st.vcc.modname, self.cname(True))
 
     def argstructure(self):
         s = "\n" + self.argstructname() + " {\n"
@@ -495,6 +495,9 @@ class ProtoType(object):
                 s += "\t"
             s += "\t" + i.nm2 + ";\n"
         s += "};\n"
+        s += "#undef %s_arg\n" % self.cname(True)
+        s += "#define %s_arg" % self.cname(True)
+        s += " %s\n" % self.argstructname()[7:]
         return s
 
     def cstuff(self, args, where):

I's pretty crude, but it gets the job done, and makes for consistent namespace as far as the vmod writer is concerned.

[nigoroll]

@phk lgtm
We might want to take this as an opportunity though to work on making the vmodtool-generated symbols' names more consistent. i had promised to write up the inconsistencies and apologize for having missed to get this done yet.

[nigoroll]

bugwash: I still want to do the writeup

[nigoroll]

Summary

Here's the overview of the various symbol name formats we are using in vmodtool. Details with examples from vmod_debug are given below.

The symbol names we are using can be summarized as:

1. ${prefix}_${name} / ${prefix}_${name}_${suffix}
2. ${prefix}_${vmod}_${name}
3. ${name}

1. is ok for symbols constrained to the vmod, and the issue seen here is caused by the fact that the argstruct falls under this category but is not.

@bsdphk 's patch moves the argstruct to category 2), so this would solve the problem.

One particular issue which I came across is that, for the example of vmod blob with $Object blob and $Prefix blob, we get a cat 2 manifestation as struct blob_blob_blob, which illustrates the fact that, where we got ${prefix}, we don't really need ${vmod}.

So my suggestion is to move all symbols to category 1) and default ${prefix} to ${vmod}.

Details

${prefix}_${name}

enum

extern VCL_ENUM xyzzy_enum_phk;

$Function/$Method C-function

VCL_STRING xyzzy_author(VRT_CTX, VCL_ENUM, VCL_ENUM);
VCL_STRING xyzzy_obj_string(VRT_CTX, struct xyzzy_debug_obj *);

${prefix}_${name}_${suffix}

$Object init/fini function

VCL_VOID xyzzy_obj__init(VRT_CTX, struct xyzzy_debug_obj **,
    const char *, VCL_STRING, VCL_ENUM);
VCL_VOID xyzzy_obj__fini(struct xyzzy_debug_obj **);

argstruct

struct xyzzy_argtest_arg;

${prefix}_${vmod}_${name}

$Object struct

struct xyzzy_debug_obj;

${name}

$Event function name

vmod_event_f event_function;

[nigoroll]

bugwash: @bsdphk wants to look into vmodtool.py: we might want backwards compat macros and will continue during a future bugwas

[bsdphk]

I believe this can be closed now.

[nigoroll]

as discussed on IRC, argstructs still clash

[bsdphk]

Fixed that.

Done now ?

[nigoroll]

yes, thank you