Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SUB arguments use-before-define #3555

Closed
nigoroll opened this issue Mar 18, 2021 · 4 comments · Fixed by #3613
Closed

SUB arguments use-before-define #3555

nigoroll opened this issue Mar 18, 2021 · 4 comments · Fixed by #3613

Comments

@nigoroll
Copy link
Member

SUB arguments currently can only be used after being defined. E.g. this works:

sub foo { }
sub caller {
  some.vmod(foo);
}

but this does not:

sub caller {
  some.vmod(foo);
}
sub foo { }
@dridi
Copy link
Member

dridi commented Mar 18, 2021

Isn't this the case with other symbols too? (backend, acl, anything named)

@nigoroll
Copy link
Member Author

exactly. But call sub instantiates sub, so I wonder if use as an argument should do the same

@dridi
Copy link
Member

dridi commented May 12, 2021

It's actually not true that anything named can be defined after use. It's only true for a subset of the VCL parsing like the call action or the IP ~ operator.

With this test case snippet:

varnish v1 -vcl {
        sub vcl_recv {
                set req.backend_hint = b;
        }
        backend b { .host = "${localhost}"; }
}

I get this:

**** v1    CLI RX|Message from VCC-compiler:
**** v1    CLI RX|Symbol not found: 'b'
**** v1    CLI RX|At: ('<vcl.inline>' Line 4 Pos 40) -- (Pos 40)
**** v1    CLI RX|                set req.backend_hint = b;
**** v1    CLI RX|---------------------------------------#-
**** v1    CLI RX|
**** v1    CLI RX|Running VCC-compiler failed, exited with 2
**** v1    CLI RX|VCL compilation failed
---- v1    VCL compilation failed expected success

I think we could generalize "defined after use" for all types that need to be referenced when used via a symbol:

  • ACL
  • BACKEND
  • PROBE
  • SUB

If we agree on doing that, I would implement this special case with a new field in struct type.

dridi added a commit to dridi/varnish-cache that referenced this issue May 12, 2021
This is as far declaring symbols goes, because we can still find
hard-coded prefixes in other places. Having them in the types could
actually help centralize them for good, but this is a minimal change
for the purpose of referencing global symbols before they are defined.

Refs varnishcache#3555
dridi added a commit to dridi/varnish-cache that referenced this issue May 12, 2021
A side effect is slightly more accurate error messages.

Closes varnishcache#3555
@dridi
Copy link
Member

dridi commented May 12, 2021

#3613

dridi added a commit to dridi/varnish-cache that referenced this issue May 25, 2021
This is as far declaring symbols goes, because we can still find
hard-coded prefixes in other places. Having them in the types could
actually help centralize them for good, but this is a minimal change
for the purpose of referencing global symbols before they are defined.

Refs varnishcache#3555
dridi added a commit to dridi/varnish-cache that referenced this issue May 25, 2021
A side effect is slightly more accurate error messages.

Closes varnishcache#3555
dridi added a commit to dridi/varnish-cache that referenced this issue May 27, 2021
This is as far declaring symbols goes, because we can still find
hard-coded prefixes in other places. Having them in the types could
actually help centralize them for good, but this is a minimal change
for the purpose of referencing global symbols before they are defined.

Refs varnishcache#3555
dridi added a commit to dridi/varnish-cache that referenced this issue May 27, 2021
A side effect is slightly more accurate error messages.

Closes varnishcache#3555
dridi added a commit that referenced this issue Jun 1, 2021
This is as far declaring symbols goes, because we can still find
hard-coded prefixes in other places. Having them in the types could
actually help centralize them for good, but this is a minimal change
for the purpose of referencing global symbols before they are defined.

Refs #3555
dridi added a commit that referenced this issue Jun 1, 2021
A side effect is slightly more accurate error messages.

Closes #3555
nigoroll added a commit to nigoroll/varnish-cache that referenced this issue Jun 2, 2021
to the extent that the match operator ~ will actually accept vmod
function/method calls returning it.

Because VMODs also need an invalid ACL value to signal error, we stop
panicking for a NULL ACL and trigger a VCL failure instead.

The use case is not (yet) to generate dynamic ACLs, but rather to store
and recall global ACL symbols.

This implementation was particularly simple thanks to Dridis work
on varnishcache#3555 and VCL_REGEX.
nigoroll added a commit that referenced this issue Jun 4, 2021
to the extent that the match operator ~ will actually accept vmod
function/method calls returning it.

Because VMODs also need an invalid ACL value to signal error, we stop
panicking for a NULL ACL and trigger a VCL failure instead.

The use case is not (yet) to generate dynamic ACLs, but rather to store
and recall global ACL symbols.

This implementation was particularly simple thanks to Dridis work
on #3555 and VCL_REGEX.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants