Skip to content

Commit

Permalink
Tighten what is allowed inside the varnish mod
Browse files Browse the repository at this point in the history
I think it is a bad practice to allow structs and enums inside the varnish `mod` item - because it conflates implementation detail and the public interface, making it less clear what will get exported. Instead, I propose everything except `impl` blocks and functions to be dis-allowed.
  • Loading branch information
nyurik authored and gquintard committed Nov 5, 2024
1 parent 24c0d59 commit b32d57f
Show file tree
Hide file tree
Showing 9 changed files with 124 additions and 53 deletions.
5 changes: 5 additions & 0 deletions examples/vmod_be/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@ import be from "path/to/libbe.so";

### Object `parrot`

parrot is our VCL object, which just holds a rust Backend,
it only needs two functions:
- new(), so that the VCL can instantiate it
- backend(), so that we can produce a C pointer for varnish to use

```vcl
// Create a new instance of the object in your VCL init function
sub vcl_init {
Expand Down
14 changes: 7 additions & 7 deletions examples/vmod_be/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,24 +1,24 @@
use varnish::vcl::{Ctx, Serve, Transfer, VclError};
use varnish::vcl::{Backend, Ctx, Serve, Transfer, VclError};

varnish::run_vtc_tests!("tests/*.vtc");

#[allow(non_camel_case_types)]
struct parrot {
be: Backend<Sentence, Body>,
}

/// a simple STRING dictionary in your VCL
#[varnish::vmod(docs = "README.md")]
mod be {
use varnish::ffi::VCL_BACKEND;
use varnish::vcl::{Backend, Ctx, VclError};

use super::{Body, Sentence};
use super::{parrot, Sentence};

/// parrot is our VCL object, which just holds a rust Backend,
/// it only needs two functions:
/// - new(), so that the VCL can instantiate it
/// - backend(), so that we can produce a C pointer for varnish to use
#[allow(non_camel_case_types)]
pub struct parrot {
be: Backend<Sentence, Body>,
}

impl parrot {
pub fn new(
ctx: &mut Ctx,
Expand Down
15 changes: 8 additions & 7 deletions examples/vmod_event/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,18 +1,19 @@
use std::sync::atomic::AtomicI64;

varnish::run_vtc_tests!("tests/*.vtc");

/// Number of times `on_event()` was called with `Event::Load`.
/// * `on_event()` will increment it and store it in the `PRIV_VCL`
/// * `loaded()` will access it and return to the VCL
static EVENT_LOADED_COUNT: AtomicI64 = AtomicI64::new(0);

/// Listen to VCL event
#[varnish::vmod(docs = "README.md")]
mod event {
use std::sync::atomic::AtomicI64;
use std::sync::atomic::Ordering::Relaxed;

use varnish::vcl::{Ctx, Event, LogTag};

/// number of times on_event() was called with `Event::Load`
/// `on_event()` will increment it and store it in the `PRIV_VCL`
/// loaded() will access it and return to the VCL
static EVENT_LOADED_COUNT: AtomicI64 = AtomicI64::new(0);

/// Return the number of VCL loads stored during when the event function ran.
pub fn loaded(#[shared_per_vcl] shared: Option<&i64>) -> i64 {
shared.copied().unwrap_or(0)
Expand All @@ -34,7 +35,7 @@ mod event {
// we only care about load events, which is why we don't use `match`
if matches!(event, Event::Load) {
// increment the count in a thread-safe way
let last_count = EVENT_LOADED_COUNT.fetch_add(1, Relaxed);
let last_count = super::EVENT_LOADED_COUNT.fetch_add(1, Relaxed);
if last_count == 1 {
// Demo that we can fail on the second `load` event
return Err("second load always fail");
Expand Down
15 changes: 9 additions & 6 deletions examples/vmod_object/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,16 +1,19 @@
use dashmap::DashMap;

varnish::run_vtc_tests!("tests/*.vtc");

/// kv only contains one element: a String->String hashmap that can be used in parallel
#[allow(non_camel_case_types)]
pub struct kv {
storage: DashMap<String, String>,
}

/// A simple string dictionary in your VCL
#[varnish::vmod(docs = "README.md")]
mod object {
use super::kv;
use dashmap::DashMap;

/// kv only contains one element: a String->String hashmap that can be used in parallel
#[allow(non_camel_case_types)]
pub struct kv {
storage: DashMap<String, String>,
}

// implementation needs the same methods as defined in the vcc, plus "new()"
// corresponding to the constructor, which requires the context (_ctx) , and the
// name of the object in VLC (_vcl_name)
Expand Down
4 changes: 1 addition & 3 deletions varnish-macros/snapshots/[email protected]
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ mod types {
};
const JSON: &CStr = c"VMOD_JSON_SPEC\u{2}\n[\n [\n \"$VMOD\",\n \"1.0\",\n \"types\",\n \"Vmod_vmod_types_Func\",\n \"84444b833e86e8a67f59228fa2634183f81f58c50e684ec4152fd478ab9875fe\",\n \"Varnish (version) (hash)\",\n \"0\",\n \"0\"\n ],\n [\n \"$CPROTO\",\n \"\\nstruct vmod_types_DocStruct;\\n\\ntypedef VCL_VOID td_vmod_types_with_docs(\\n VRT_CTX\\n);\\n\\ntypedef VCL_VOID td_vmod_types_no_docs(\\n VRT_CTX\\n);\\n\\ntypedef VCL_VOID td_vmod_types_doctest(\\n VRT_CTX,\\n VCL_INT,\\n VCL_INT\\n);\\n\\ntypedef VCL_VOID td_vmod_types_arg_only(\\n VRT_CTX,\\n VCL_INT\\n);\\n\\nstruct arg_vmod_types_DocStruct__init {\\n char valid_cap;\\n VCL_INT cap;\\n};\\n\\ntypedef VCL_VOID td_vmod_types_DocStruct__init(\\n VRT_CTX,\\n struct vmod_types_DocStruct **,\\n const char *,\\n struct arg_vmod_types_DocStruct__init *\\n);\\n\\ntypedef VCL_VOID td_vmod_types_DocStruct__fini(\\n struct vmod_types_DocStruct **\\n);\\n\\ntypedef VCL_VOID td_vmod_types_DocStruct_function(\\n VRT_CTX,\\n struct vmod_types_DocStruct *,\\n VCL_STRING\\n);\\n\\nstruct Vmod_vmod_types_Func {\\n td_vmod_types_with_docs *f_with_docs;\\n td_vmod_types_no_docs *f_no_docs;\\n td_vmod_types_doctest *f_doctest;\\n td_vmod_types_arg_only *f_arg_only;\\n td_vmod_types_DocStruct__init *f_DocStruct__init;\\n td_vmod_types_DocStruct__fini *f_DocStruct__fini;\\n td_vmod_types_DocStruct_function *f_DocStruct_function;\\n};\\n\\nstatic struct Vmod_vmod_types_Func Vmod_vmod_types_Func;\"\n ],\n [\n \"$FUNC\",\n \"with_docs\",\n [\n [\n \"VOID\"\n ],\n \"Vmod_vmod_types_Func.f_with_docs\",\n \"\"\n ]\n ],\n [\n \"$FUNC\",\n \"no_docs\",\n [\n [\n \"VOID\"\n ],\n \"Vmod_vmod_types_Func.f_no_docs\",\n \"\"\n ]\n ],\n [\n \"$FUNC\",\n \"doctest\",\n [\n [\n \"VOID\"\n ],\n \"Vmod_vmod_types_Func.f_doctest\",\n \"\",\n [\n \"INT\",\n \"_no_docs\"\n ],\n [\n \"INT\",\n \"_v\"\n ]\n ]\n ],\n [\n \"$FUNC\",\n \"arg_only\",\n [\n [\n \"VOID\"\n ],\n \"Vmod_vmod_types_Func.f_arg_only\",\n \"\",\n [\n \"INT\",\n \"_v\"\n ]\n ]\n ],\n [\n \"$OBJ\",\n \"DocStruct\",\n {\n \"NULL_OK\": false\n },\n \"struct vmod_types_DocStruct\",\n [\n \"$INIT\",\n [\n [\n \"VOID\"\n ],\n \"Vmod_vmod_types_Func.f_DocStruct__init\",\n \"struct arg_vmod_types_DocStruct__init\",\n [\n \"INT\",\n \"cap\",\n null,\n null,\n true\n ]\n ]\n ],\n [\n \"$FINI\",\n [\n [\n \"VOID\"\n ],\n \"Vmod_vmod_types_Func.f_DocStruct__fini\",\n \"\"\n ]\n ],\n [\n \"$METHOD\",\n \"function\",\n [\n [\n \"VOID\"\n ],\n \"Vmod_vmod_types_Func.f_DocStruct_function\",\n \"\",\n [\n \"STRING\",\n \"key\"\n ]\n ]\n ]\n ]\n]\n\u{3}";
}
use super::DocStruct;
/// doctest on a function
/// with multiple lines
/// # Big header
Expand All @@ -163,9 +164,6 @@ mod types {
/// doctest on a function
pub fn doctest(_no_docs: i64, _v: i64) {}
pub fn arg_only(_v: i64) {}
/// doctest for `DocStruct`.
/// This comment is ignored because we do not handle structs, only impl blocks.
pub struct DocStruct;
/// doctest for `DocStruct` implementation
impl DocStruct {
/// doctest for `new`
Expand Down
4 changes: 1 addition & 3 deletions varnish-macros/snapshots/[email protected]
Original file line number Diff line number Diff line change
Expand Up @@ -303,8 +303,8 @@ mod obj {
};
const JSON: &CStr = c"VMOD_JSON_SPEC\u{2}\n[\n [\n \"$VMOD\",\n \"1.0\",\n \"obj\",\n \"Vmod_vmod_obj_Func\",\n \"e4dde1367a8785e0bb9e3b32a6a53880156eb4050d20f22573df4b5ea5f44461\",\n \"Varnish (version) (hash)\",\n \"0\",\n \"0\"\n ],\n [\n \"$CPROTO\",\n \"\\nstruct vmod_obj_kv1;\\n\\nstruct vmod_obj_kv2;\\n\\nstruct vmod_obj_kv3;\\n\\nstruct arg_vmod_obj_kv1__init {\\n char valid_cap;\\n VCL_INT cap;\\n};\\n\\ntypedef VCL_VOID td_vmod_obj_kv1__init(\\n VRT_CTX,\\n struct vmod_obj_kv1 **,\\n const char *,\\n struct arg_vmod_obj_kv1__init *\\n);\\n\\ntypedef VCL_VOID td_vmod_obj_kv1__fini(\\n struct vmod_obj_kv1 **\\n);\\n\\ntypedef VCL_VOID td_vmod_obj_kv1_set(\\n VRT_CTX,\\n struct vmod_obj_kv1 *,\\n VCL_STRING,\\n VCL_STRING\\n);\\n\\ntypedef VCL_STRING td_vmod_obj_kv1_get(\\n VRT_CTX,\\n struct vmod_obj_kv1 *,\\n VCL_STRING\\n);\\n\\nstruct arg_vmod_obj_kv2__init {\\n char valid_cap;\\n VCL_INT cap;\\n};\\n\\ntypedef VCL_VOID td_vmod_obj_kv2__init(\\n VRT_CTX,\\n struct vmod_obj_kv2 **,\\n const char *,\\n struct arg_vmod_obj_kv2__init *\\n);\\n\\ntypedef VCL_VOID td_vmod_obj_kv2__fini(\\n struct vmod_obj_kv2 **\\n);\\n\\nstruct arg_vmod_obj_kv2_set {\\n VCL_STRING key;\\n char valid_value;\\n VCL_STRING value;\\n};\\n\\ntypedef VCL_VOID td_vmod_obj_kv2_set(\\n VRT_CTX,\\n struct vmod_obj_kv2 *,\\n struct arg_vmod_obj_kv2_set *\\n);\\n\\nstruct arg_vmod_obj_kv3__init {\\n char valid_cap;\\n VCL_INT cap;\\n};\\n\\ntypedef VCL_VOID td_vmod_obj_kv3__init(\\n VRT_CTX,\\n struct vmod_obj_kv3 **,\\n const char *,\\n struct arg_vmod_obj_kv3__init *\\n);\\n\\ntypedef VCL_VOID td_vmod_obj_kv3__fini(\\n struct vmod_obj_kv3 **\\n);\\n\\nstruct arg_vmod_obj_kv3_set {\\n VCL_STRING key;\\n char valid_value;\\n VCL_STRING value;\\n};\\n\\ntypedef VCL_VOID td_vmod_obj_kv3_set(\\n VRT_CTX,\\n struct vmod_obj_kv3 *,\\n struct arg_vmod_obj_kv3_set *\\n);\\n\\nstruct Vmod_vmod_obj_Func {\\n td_vmod_obj_kv1__init *f_kv1__init;\\n td_vmod_obj_kv1__fini *f_kv1__fini;\\n td_vmod_obj_kv1_set *f_kv1_set;\\n td_vmod_obj_kv1_get *f_kv1_get;\\n td_vmod_obj_kv2__init *f_kv2__init;\\n td_vmod_obj_kv2__fini *f_kv2__fini;\\n td_vmod_obj_kv2_set *f_kv2_set;\\n td_vmod_obj_kv3__init *f_kv3__init;\\n td_vmod_obj_kv3__fini *f_kv3__fini;\\n td_vmod_obj_kv3_set *f_kv3_set;\\n};\\n\\nstatic struct Vmod_vmod_obj_Func Vmod_vmod_obj_Func;\"\n ],\n [\n \"$OBJ\",\n \"kv1\",\n {\n \"NULL_OK\": false\n },\n \"struct vmod_obj_kv1\",\n [\n \"$INIT\",\n [\n [\n \"VOID\"\n ],\n \"Vmod_vmod_obj_Func.f_kv1__init\",\n \"struct arg_vmod_obj_kv1__init\",\n [\n \"INT\",\n \"cap\",\n null,\n null,\n true\n ]\n ]\n ],\n [\n \"$FINI\",\n [\n [\n \"VOID\"\n ],\n \"Vmod_vmod_obj_Func.f_kv1__fini\",\n \"\"\n ]\n ],\n [\n \"$METHOD\",\n \"set\",\n [\n [\n \"VOID\"\n ],\n \"Vmod_vmod_obj_Func.f_kv1_set\",\n \"\",\n [\n \"STRING\",\n \"key\"\n ],\n [\n \"STRING\",\n \"value\"\n ]\n ]\n ],\n [\n \"$METHOD\",\n \"get\",\n [\n [\n \"STRING\"\n ],\n \"Vmod_vmod_obj_Func.f_kv1_get\",\n \"\",\n [\n \"STRING\",\n \"key\"\n ]\n ]\n ]\n ],\n [\n \"$OBJ\",\n \"kv2\",\n {\n \"NULL_OK\": false\n },\n \"struct vmod_obj_kv2\",\n [\n \"$INIT\",\n [\n [\n \"VOID\"\n ],\n \"Vmod_vmod_obj_Func.f_kv2__init\",\n \"struct arg_vmod_obj_kv2__init\",\n [\n \"INT\",\n \"cap\",\n null,\n null,\n true\n ]\n ]\n ],\n [\n \"$FINI\",\n [\n [\n \"VOID\"\n ],\n \"Vmod_vmod_obj_Func.f_kv2__fini\",\n \"\"\n ]\n ],\n [\n \"$METHOD\",\n \"set\",\n [\n [\n \"VOID\"\n ],\n \"Vmod_vmod_obj_Func.f_kv2_set\",\n \"struct arg_vmod_obj_kv2_set\",\n [\n \"STRING\",\n \"key\"\n ],\n [\n \"STRING\",\n \"value\",\n null,\n null,\n true\n ]\n ]\n ]\n ],\n [\n \"$OBJ\",\n \"kv3\",\n {\n \"NULL_OK\": false\n },\n \"struct vmod_obj_kv3\",\n [\n \"$INIT\",\n [\n [\n \"VOID\"\n ],\n \"Vmod_vmod_obj_Func.f_kv3__init\",\n \"struct arg_vmod_obj_kv3__init\",\n [\n \"INT\",\n \"cap\",\n null,\n null,\n true\n ]\n ]\n ],\n [\n \"$FINI\",\n [\n [\n \"VOID\"\n ],\n \"Vmod_vmod_obj_Func.f_kv3__fini\",\n \"\"\n ]\n ],\n [\n \"$METHOD\",\n \"set\",\n [\n [\n \"VOID\"\n ],\n \"Vmod_vmod_obj_Func.f_kv3_set\",\n \"struct arg_vmod_obj_kv3_set\",\n [\n \"STRING\",\n \"key\"\n ],\n [\n \"STRING\",\n \"value\",\n null,\n null,\n true\n ]\n ]\n ]\n ]\n]\n\u{3}";
}
use super::*;
use varnish::vcl::Ctx;
pub struct kv1;
impl kv1 {
pub fn new(cap: Option<i64>) -> Self {
kv1
Expand All @@ -314,14 +314,12 @@ mod obj {
String::default()
}
}
pub struct kv2;
impl kv2 {
pub fn new(cap: Option<i64>, name: &str) -> Self {
kv2
}
pub fn set(&self, key: &str, value: Option<&str>) {}
}
pub struct kv3;
impl kv3 {
pub fn new(ctx: &mut Ctx, cap: Option<i64>, name: &str) -> Self {
kv3
Expand Down
102 changes: 82 additions & 20 deletions varnish-macros/src/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ pub fn tokens_to_model(args: TokenStream, item_mod: &mut ItemMod) -> ProcResult<

impl VmodInfo {
/// Parse the `mod` item and generate the model of everything
#[expect(clippy::too_many_lines)]
fn parse(params: VmodParams, item: &mut ItemMod) -> ProcResult<Self> {
let mut errors = Errors::new();
let mut funcs = Vec::<FuncInfo>::new();
Expand All @@ -32,29 +33,86 @@ impl VmodInfo {
let mut has_event = false;
if let Some((_, content)) = &mut item.content {
for item in content {
if let Item::Fn(fn_item) = item {
// a function or an event handler
let func = FuncInfo::parse(
&mut shared_types,
&mut fn_item.sig,
&fn_item.vis,
&mut fn_item.attrs,
false,
);
if let Some(func) = errors.on_err(func) {
if matches!(func.func_type, FuncType::Event) {
if has_event {
errors.add(&fn_item.sig.ident, "Only one event handler is allowed");
continue;
match item {
Item::Fn(fn_item) => {
// a function or an event handler
let func = FuncInfo::parse(
&mut shared_types,
&mut fn_item.sig,
&fn_item.vis,
&mut fn_item.attrs,
false,
);
if let Some(func) = errors.on_err(func) {
if matches!(func.func_type, FuncType::Event) {
if has_event {
errors.add(
&fn_item.sig.ident,
"Only one event handler is allowed",
);
continue;
}
has_event = true;
}
has_event = true;
funcs.push(func);
}
funcs.push(func);
}
} else if let Item::Impl(impl_item) = item {
// an object
if let Some(obj) = errors.on_err(ObjInfo::parse(impl_item, &mut shared_types)) {
objects.push(obj);
Item::Impl(impl_item) => {
// an object
if let Some(obj) =
errors.on_err(ObjInfo::parse(impl_item, &mut shared_types))
{
objects.push(obj);
}
}
Item::Use(_) => { /* ignore */ }
Item::Struct { .. } => {
errors.add(item, &err_msg_item_not_allowed("Structs"));
}
Item::Enum { .. } => {
errors.add(item, &err_msg_item_not_allowed("Enums"));
}
Item::Const(_) => {
errors.add(
item,
"Constants are not allowed in a `mod` tagged with `#[varnish::vmod]",
);
}
Item::Macro(_) => {
errors.add(
item,
"Macros are not allowed in a `mod` tagged with `#[varnish::vmod]",
);
}
Item::Mod(_) => {
errors.add(item, "Nested modules are not allowed in a `mod` tagged with `#[varnish::vmod]");
}
Item::Static(_) => {
errors.add(item, "Static variables are not allowed in a `mod` tagged with `#[varnish::vmod]");
}
Item::Trait(_) => {
errors.add(
item,
"Traits are not allowed in a `mod` tagged with `#[varnish::vmod]",
);
}
Item::TraitAlias(_) => {
errors.add(item, "Trait aliases are not allowed in a `mod` tagged with `#[varnish::vmod]");
}
Item::Type(_) => {
errors.add(
item,
"Type aliases are not allowed in a `mod` tagged with `#[varnish::vmod]",
);
}
Item::Union(_) => {
errors.add(
item,
"Unions are not allowed in a `mod` tagged with `#[varnish::vmod]",
);
}
_ => {
errors.add(item, "Only functions and impl blocks are allowed inside a `mod` tagged with `#[varnish::vmod]`");
}
}
}
Expand All @@ -75,6 +133,10 @@ impl VmodInfo {
}
}

fn err_msg_item_not_allowed(typ: &str) -> String {
format!("{typ} are not allowed inside a `mod` tagged with `#[varnish::vmod]`. Move it to an outer scope and keep just the `impl` block. More than one `impl` blocks are allowed.")
}

impl ObjInfo {
/// Parse an `impl` block and treat all public functions as object methods
fn parse(item_impl: &mut ItemImpl, shared_types: &mut SharedTypes) -> ProcResult<Self> {
Expand Down
10 changes: 6 additions & 4 deletions varnish-macros/tests/pass/docs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@ use varnish::vmod;

fn main() {}

/// doctest for `DocStruct`.
/// This comment is ignored because we do not handle structs, only impl blocks.
pub struct DocStruct;

/// main docs
/// # Big header
/// ## sub header
Expand All @@ -16,6 +20,8 @@ mod types {
* The end
*/

use super::DocStruct;

/// doctest on a function
/// with multiple lines
/// # Big header
Expand Down Expand Up @@ -47,10 +53,6 @@ mod types {
) {
}

/// doctest for `DocStruct`.
/// This comment is ignored because we do not handle structs, only impl blocks.
pub struct DocStruct;

/// doctest for `DocStruct` implementation
impl DocStruct {
/// doctest for `new`
Expand Down
Loading

0 comments on commit b32d57f

Please sign in to comment.