[PATCH wayland v3 3/4] scanner: enforce correct argument type for enums

[Bryce Harrington]

On Sat, Oct 24, 2015 at 12:07:49PM +0100, Auke Booij wrote:
> The scanner now checks whether arguments that have an associated
> <enum> have the right type.
> An argument with an enum attribute must be of type int or uint,
> and if the <enum> with that name has the bitfield attribute
> set to true, then the argument must be of type uint.
> 
> Signed-off-by: Auke Booij <auke at tulcod.com>

Reviewed-by: Bryce Harrington <bryce at osg.samsung.com>

A couple really minor nits below, not really worth doing unless you need
to do another rev of this patch for some other reason.

> ---
>  src/scanner.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
> 
> diff --git a/src/scanner.c b/src/scanner.c
> index f456aa5..9856475 100644
> --- a/src/scanner.c
> +++ b/src/scanner.c
> @@ -128,6 +128,7 @@ struct arg {
>  	char *interface_name;
>  	struct wl_list link;
>  	char *summary;
> +	char *enumeration_name;
>  };
>  
>  struct enumeration {
> @@ -136,6 +137,7 @@ struct enumeration {
>  	struct wl_list entry_list;
>  	struct wl_list link;
>  	struct description *description;
> +	int bitfield;

This appears to be used for tracking only a yes/no type value, so maybe
consider making it a boolean?

>  };
>  
>  struct entry {
> @@ -540,6 +542,8 @@ start_element(void *data, const char *element_name, const char **atts)
>  	const char *summary = NULL;
>  	const char *since = NULL;
>  	const char *allow_null = NULL;
> +	const char *enumeration_name = NULL;
> +	const char *bitfield = NULL;
>  	int i, version = 0;
>  
>  	ctx->loc.line_number = XML_GetCurrentLineNumber(ctx->parser);
> @@ -562,6 +566,10 @@ start_element(void *data, const char *element_name, const char **atts)
>  			since = atts[i + 1];
>  		if (strcmp(atts[i], "allow-null") == 0)
>  			allow_null = atts[i + 1];
> +		if (strcmp(atts[i], "enum") == 0)
> +			enumeration_name = atts[i + 1];
> +		if (strcmp(atts[i], "bitfield") == 0)
> +			bitfield = atts[i + 1];
>  	}
>  
>  	ctx->character_data_length = 0;
> @@ -655,6 +663,14 @@ start_element(void *data, const char *element_name, const char **atts)
>  				     "allow-null is only valid for objects, strings, and arrays");
>  		}
>  
> +		if (enumeration_name == NULL || strcmp(enumeration_name, "") == 0)
> +			arg->enumeration_name = NULL;
> +		else
> +			arg->enumeration_name = xstrdup(enumeration_name);
> +
> +		if (allow_null != NULL && !is_nullable_type(arg))
> +			fail(&ctx->loc, "allow-null is only valid for objects, strings, and arrays");
> +
>  		if (summary)
>  			arg->summary = xstrdup(summary);
>  
> @@ -665,6 +681,14 @@ start_element(void *data, const char *element_name, const char **atts)
>  			fail(&ctx->loc, "no enum name given");
>  
>  		enumeration = create_enumeration(name);
> +
> +		if (bitfield == NULL || strcmp(bitfield, "false") == 0)
> +			enumeration->bitfield = 0;
> +		else if (strcmp(bitfield, "true") == 0)
> +			enumeration->bitfield =1;

Space needed after the =

> +		else
> +			fail(&ctx->loc, "invalid value for bitfield attribute (%s)", bitfield);
> +
>  		wl_list_insert(ctx->interface->enumeration_list.prev,
>  			       &enumeration->link);
>  
> @@ -701,6 +725,46 @@ start_element(void *data, const char *element_name, const char **atts)
>  }
>  
>  static void
> +verify_arguments(struct parse_context *ctx, struct wl_list *messages, struct wl_list *enumerations)
> +{
> +	struct message *m;
> +	wl_list_for_each(m, messages, link) {
> +		struct arg *a;
> +		wl_list_for_each(a, &m->arg_list, link) {
> +			struct enumeration *e, *f;
> +
> +			if (!a->enumeration_name)
> +				continue;
> +
> +			f = NULL;
> +			wl_list_for_each(e, enumerations, link) {
> +				if(strcmp(e->name, a->enumeration_name) == 0)
> +					f = e;
> +			}
> +
> +			if (f == NULL)
> +				fail(&ctx->loc,
> +				     "could not find enumeration %s",
> +				     a->enumeration_name);
> +
> +			switch (a->type) {
> +			case INT:
> +				if (f->bitfield)
> +					fail(&ctx->loc,
> +					     "bitfield-style enum must be referenced by uint");

I think maybe you mean "must only be referenced"?

> +				break;
> +			case UNSIGNED:
> +				break;
> +			default:
> +				fail(&ctx->loc,
> +				     "enumeration-style argument has wrong type");
> +			}
> +		}
> +	}
> +
> +}
> +
> +static void
>  end_element(void *data, const XML_Char *name)
>  {
>  	struct parse_context *ctx = data;
> @@ -723,6 +787,12 @@ end_element(void *data, const XML_Char *name)
>  			     ctx->enumeration->name);
>  		}
>  		ctx->enumeration = NULL;
> +	} else if (strcmp(name, "interface") == 0) {
> +		struct interface *i = ctx->interface;
> +
> +		verify_arguments(ctx, &i->request_list, &i->enumeration_list);
> +		verify_arguments(ctx, &i->event_list, &i->enumeration_list);
> +
>  	}
>  }
>  
> -- 
> 2.6.1
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel