Regression: VMOD ENUMs cannot default to NULL

slimhazard commented 6 years ago

Expected Behavior

Existing VMODs that set NULL (literal 0) as the default value for an ENUM should not be broken.

Since VCL_ENUM is typedef const char *, NULL is a valid value, and may be the best choice as a default, depending on the requirements of the VMOD -- by default "none of the above". (Of course the VMOD must be implemented to handle NULL correctly, but that's the VMOD's problem.)

Until the change mentioned below, it was possible to set NULL as the default with this syntax in the VCC declaration:

... foo(ENUM {VAL1, VAL2} bar=0)

Examples of VMODs that do this: https://code.uplex.de/uplex-varnish/libvmod-re2 https://code.uplex.de/uplex-varnish/libvmod-pcre2 https://code.uplex.de/uplex-varnish/libvmod-gcrypt

Current Behavior

Since the change in c91a6ce, the syntax shown above is rejected by the vmodtool as an error. This is because the values listed in {...} are appended to a .spec field in the vmodtool (a list of strings), and the default value is now checked if it appears in the list. If it doesn't, the VCC declaration is rejected as an error.

Possible Solution

Recognize the =0 case and allow it, setting NULL as the default; or
(Maybe better) Permit a distinguished value like __NONE__ as the default, and set NULL as the default in that case. That would preclude VMODs from using the __NONE__ literal (or whatever it turns out to be), but a choice like that with the double underscore, or some other "magic" spelling, is unlikely to break anything.

Steps to Reproduce (for bugs)

Build any of the VMODs mentioned above.

Context

Trying to keep VMODs compatible with master (and get a head start on compatibility with the March 2018 release), without having to chase down ... that many breaking changes ... (?)

Your Environment

Version used: master since c91a6ce (any platform)

nigoroll commented 6 years ago

The obvious simple solution is to designate one ENUM (e.g. undef) for this purpose, but this would duplicate the logic for each vmod and I do agree that having a standard value for undef semantics would make sense. I'd even propose to widen the scope and define undef for as many VCL types as possible:

NULL for all the pointers (VCL_ACL, VCL_BACKEND, VCL_BLOB etc.)
USHRT_MAX for BOOL
NaN for all the doubles (VCL_DURATION, VCL_REAL, VCL_TIME)
LLONG_MIN for VCL_BYTES
LONG_MIN for VCL_INT The latter two would need to be documented as reserved for this purpose (and we could have VCL_BYTES_MIN = LLONG_MIN + 1 and VCL_INT_MIN = LONG_MIN + 1) We could define *_UNDEF in vrt.h and make vmodtool.py + vcc generate the right function call if undef is used as an argument default.

bsdphk commented 6 years ago

I'm not keen on the _UNDEF idea, in particular not for numerical types.

A more palatable idea would be to pass a bitmap as a separate argument which tells which args are default values. (Or maybe that bitmap should live in VRT_CTX).

nigoroll commented 6 years ago

The bitmap idea also came to my mind last night on the bicycle ride home. Yes, we should put it into CTX to not waste arguments for this uncommon use case. I'll write a patch.

nigoroll commented 6 years ago

@bsdphk but anyway, do you have an opinion about @slimhazard s initial point to bring back 0/NULL for ENUMs?

slimhazard commented 6 years ago

Currently if no default is used for a parameter in the VCC declaration, that establishes the parameter as required, and then it's a compile-time error if no value is set for it.

Depending on what the VMOD wants to do, that can be just the right thing for many purposes. I for one put some thought into it, and document the status of params as required or not.

If we pass in a bitmap saying which params are set or unset, would it still be possible to establish that some of them are required, and emit a compile-time error if they were left out? If not, I would think of that as a regression as well.

nigoroll commented 6 years ago

@slimhazard IMHO, we should 1) bring back 0/NULL as default for ENUMs 2) add the bitmask which specifies where defaults were used.

If @bsdphk disagreed on 1), the fall back would be an explicit ENUM value dedicated as a default value

dridi commented 6 years ago

If @bsdphk disagreed on 1), the fall back would be an explicit ENUM value dedicated as a default value

I don't think we should allow shrödingenums since it's trivial to indicate a default value in the VCC descriptor. And bringing back the null ENUM would mean that you can legally get a value that is not part of the enum. If anything I think this was a desirable side effect.

Now regarding the bitmap idea, this would mean that a VCL user passing the exact list of default parameters could result in a different behavior than omitting default parameters. Unless we have some kind of syntax to indicate the lack of default value in the VCC descriptor.

$Function VOID defaultee(STRING other = NULL, INT value = $Undef)

This way there wouldn't be any ambiguity, you'd be forced to check the bitmap because you could legally not pass a meaningful value.

What happens to the bitmap if in my VCL I write this?

sub something {
    strawman.defaultee(C{}C);
    strawman.defaultee(C{"def", 0}C);
}

slimhazard commented 6 years ago

Yes, it's possible to replace this:

ENUM {VAL1, VAL2} e=0

with this:

ENUM {VAL1, VAL2, DEFAULT} e=DEFAULT

Note that if we stay with this, then NULL is from now on precluded as a possible value to pass in for an ENUM.

FTR I disagree that this is a desirable side effect. In some cases, the DEFAULT addition is fairly intuitive (for the pcre2 VMOD, for example, it means "fall back to whatever the pcre2 build-time option was"). But for others it just adds conceptual clutter to the VMOD interface, and requires more words in the documentation to explain how it works.

If the decision is that this is how it works now and I'll have to go change the VMODs, then I can go change the VMODs and close the issue.

Or we can keep it open for the next bugwash, if we want to discuss a way to express "param with default was not set by the user" -- I know that @nigoroll has a use for that with a BOOL param in the shard director.

nigoroll commented 6 years ago

@Dridi I think your last comment creates a circular argument: if we had some magic $Undef, we would not need the bitmask (but that was busted by phk) and even with the bitmask, we would need some thing to pass to the function at c level for $Undef.

Now regarding the bitmap idea, this would mean that a VCL user passing the exact list of default parameters could result in a different behavior than omitting default parameters

This is exactly what I intend.

I've created a PR for the bitmask.

dridi commented 6 years ago

@Dridi I think your last comment creates a circular argument: if we had some magic $Undef, we would not need the bitmask (but that was busted by phk) and even with the bitmask, we would need some thing to pass to the function at c level for $Undef.

No, what I mean is that in the presence of an explicit $Undef it would be possible to get no meaningful value from the client. For a given argument you could know from the bitmap that it hadn't been passed.

Otherwise there's no way to know from the stack only whether a parameter was passed by VCL or added by VCC.

This is exactly what I intend.

In the absence of a theoretical $Undef, getting a different result depending on whether you omit arguments or use their default value explicitly would be quite astonishing.

nigoroll commented 6 years ago

@Dridi ,

Otherwise there's no way to know from the stack only whether a parameter was passed by VCL or added by VCC.

Can you have a look at #2524 please?

In the absence of a theoretical $Undef, getting a different result depending on whether you omit arguments or use their default value explicitly would be quite astonishing.

Regarding the $Undef, based on the discussed phk and myself had in this ticket, what would be the value of $Undef being passed to the C implementation for non-pointer types? I see your point regarding consistency in behavior, but does it matter from a user perspective? The first use case I have is "only change the passed parameters of an object", which appears pretty straight forward to me.

nigoroll commented 6 years ago

@Dridi and myself continued the discussion 1:1 via IRC, my summary:

@Dridi proposes a vcc file $Undef default value which would still make vcc pass some argument to the vmod C function, but we won't define it. The contract would be that the vmod needs to look at the ARGMASK before looking at the argument. The main advantage would be clarity of the declaration, IMHO the main disadvantage would be that users would need to read more documentation to understand what the behavior for omitted arguments with undefined default values is. @Dridi also proposes that ARGMASK would not be an explicitly defined vcc file parameter, but rather gets added implicitly IFF $Undef is used for any argument.

I'm fine with his suggestions, but don't see that much value to them. One obvious compromise would be to allow $Undef if an ARGMASK vcc file definition argument is present, but not enforce it.

dridi commented 6 years ago

We've discussed this on IRC, here is a summary:

If a VMOD author wants an optional parameter but doesn't want to specify a default value, they should use a hypothetical name=$Undef argument in the VCC descriptor.

What do we get on the stack? We don't know: by using $Undef you're supposed to check the ARGMASK first to know whether you have something usable on the stack. ARGMASK contains bits only for $Undef parameters.

$Function strawman(INT mandatory, STRING optional=$Undef, REAL defaulted=0.0)

This would give only one bit to check. ARGMASK wouldn't appear in the VCC file, only in the generated vcc_*.[ch].

@nigoroll isn't completely convinced but doesn't object since it doesn't break his use cases. My main concern is to avoid ambiguity where a VCL could supply arguments with the same values as the default ones declared, but a VMOD could nevertheless behave differently because of the ARGMASK.

edit: too slow :)

hermunn commented 6 years ago

If a VMOD author wants an optional parameter but doesn't want to specify a default value,

Why would we let him? If you are a VMOD author and you want an optional parameter, then you have to have some idea on what the default value is. It is natural (to me) to require the VMOD author specify this explicitly and let that information be automatically a part of the VMOD's documentation.

Some languages allow several functions with the same name and different argument lists, but these can be sources of misunderstandings and bugs, so I do not propose to introduce them here. Instead, if one can not provide a function with an optional parameter (with an explicit default), one should use different function names and let the C code call its general functions behind the scenes.

dridi commented 6 years ago

Why would we let him? If you are a VMOD author and you want an optional parameter, then you have to have some idea on what the default value is.

I agree, but see https://github.com/varnishcache/varnish-cache/issues/2515#issuecomment-351769289

I think this is mostly about non-pointer parameters: you don't have the equivalent of NULL for INT and other types, so unless you have a magic value (like 0 for no timeout) you can't make it optional.

rezan commented 6 years ago

I think the proper way to allow for a default and then detect a user value is to just make 2 functions:

INT init_a(INT a);
INT init_a_b(INT a, INT b);

Then behind the scenes:

int init_a(int a) {
  return init_a_b_proper(a, 0, false);
}

int init_a_b(int a, int b) {
  return init_a_b_proper(a, b, true);
}

int init_a_b_proper(int a, int b, int b_init) {
  // do some logic on a and b and use b_init to detect non initialization
}

Having multiple functions should work. You could even split it into OO style:

VOID init();
VOID set_a();
VOID set_b();
INT do_something(); // do something with a and b

nigoroll commented 6 years ago

@rezan I disagree passionately. Solving this issue by clobbering the user interface might look like an easy way out for toy examples, but not in reality, see https://github.com/varnishcache/varnish-cache/pull/2525/commits/1c0baf41df436912492a5b4a5d5c5e6211cca25e#diff-3d218abe997d14ef6c4b6d4d0d7bfc46R424 : We want one method returning a result and arguments are not independent.

rezan commented 6 years ago

First, I guess im confused why a default value will not work?

Not really a fan of having a bit field which then loosely maps to argument order. Apologies if I got this wrong, I saw xyzzy_args() and it didnt show how args map to bits other than having to count the positions and code in the number. If this becomes reality, might be seen as a negative for VMOD writers as this isnt very C like or nice to look at, and possibly a bit dangerous. Right now we actually have something pretty nice with the way arguments can be optional and then C code gets them mapped directly with default values. If this is infact a problem that cannot be solved with the current patterns, then I would just ask for something a bit cleaner? Maybe this only works for pointer types which can have a NULL value?

I guess if the argument here is that VCL UX is better served with this, I would have to better understand the examples. I looked at the diff, hard to understand why defaults didnt work? Can you provide an quick example showcasing this?

dridi commented 6 years ago

@rezan overall I agree with you, the current mechanism for default values is enough for me, and I consider forcing a valid value for $Enum desirable. However if we go the bitmap way, I already shared my concerns and views with @nigoroll.

I agree with @nigoroll that having a way to safely omit the presence of arguments for non-pointer types could be useful because not all scalars arguments could be mapped to magic values to denote their absence (like a timeout could and, already does).

That leaves us with INT, REAL, TIME and DURATION where it could make sense (if I remember the type system correctly) and I'm not counting BOOL on purpose. ;)

nigoroll commented 6 years ago

First, I guess im confused why a default value will not work?

See previous comments. Simply put: "argument not used" may be a different case than "default value used". For the shard director, the default can come from another object.

Not really a fan of having a bit field which then loosely maps to argument order.

In which way would it map loosely? In my PR, it maps exactly. 1 == first argument, 1<<1 == second argument, 1<<2 third argument etc.

Apologies if I got this wrong, I saw xyzzy_args() and it didnt show how args map to bits other than having to count the positions and code in the number.

Yes, good point, we could make this more easy to use by having vmodtool generate some useful macros for named arguments. Something like if (vcc_arg_given(args, arg_timeout)) { ...}

I volunteer to implement this once we have reached agreement on the basics.

Right now we actually have something pretty nice with the way arguments can be optional and then C code gets them mapped directly with default values.

My suggestion was to not change the default value behaviour even with the argument mask. @Dridi had valid points to introduce $Undef, and if we followed his suggestion, adding $Undef to the vcc file declaration would be a conscious process and we could help C devs with proper examples.

I looked at the diff, hard to understand why defaults didnt work? Can you provide an quick example showcasing this?

Does this help as an example? https://github.com/varnishcache/varnish-cache/pull/2525/files#diff-e8cdd3e2fc7d32ecbfd7694cf38f6cd3R464

Again, the use case is to only change some internal change IFF an argument is given.

rezan commented 6 years ago

Simply put: "argument not used" may be a different case than "default value used".

So wouldn't "argument not used" be the default value here? Is the user setting the "default value"? Then this is just a value.

nigoroll commented 6 years ago

So wouldn't "argument not used" be the default value here? Is the user setting the "default value"? Then this is just a value.

Please read previous comments in this ticket: How would you determine on the C side of things whether a value being equal to the default was given or just the default value because the argument was left out? If this is still unclear, please come to #varnish-hacking or let's clarify 1:1 on irc.

gquintard commented 6 years ago

Sorry to barge in, but would the idea of using a struct instead of the value directly be preposterous?

Something like { bool set; (double|int|char) value}. It breaks the api, but only for optional arguments, you get a strong coupling between the "explicitly-set" boolean and the argument, and the inline C is harder to mess up.

-- Guillaume Quintard

On Thu, Dec 21, 2017 at 4:43 PM, Nils Goroll notifications@github.com wrote:

So wouldn't "argument not used" be the default value here? Is the user setting the "default value"? Then this is just a value.

Please read previous comments in this ticket: How would you determine on the C side of things whether a value being equal to the default was given or just the default value because the argument was left out? If this is still unclear, please come to #varnish-hacking or let's clarify 1:1 on irc.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/varnishcache/varnish-cache/issues/2515#issuecomment-353383103, or mute the thread https://github.com/notifications/unsubscribe-auth/ADmgKfd_Q15xK5wYxdfeSNTnEOsHGkslks5tCnyfgaJpZM4Q9liI .

dridi commented 6 years ago

Sorry to barge in, but would the idea of using a struct instead of the value directly be preposterous?

SGTM

nigoroll commented 6 years ago

@gquintard @Dridi struct-wrapping all arguments opens up a new can of worms:

vcc needs to generate the packing code
memory is needed for packing, so either we need to waste ws or teach vcc to get stack space.

On the other hand, bitmask is simpler to implement and more efficient.

dridi commented 6 years ago

Why not declaring a struct on the stack in the generated code? We can also get away with a single struct definition with an union for the value. Either way I don't really mind.

nigoroll commented 6 years ago

FYI next take on it: #2526

nigoroll commented 6 years ago

I think the issue discussed here is solved by @bsdphk s solution along the lines of #2526 leaving to @slimhazard to close

fgsch commented 6 years ago

What's the status of this?

nigoroll commented 6 years ago

@slimhazard please reopen if you disagree

varnishcache / varnish-cache