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

[Auke Booij]

On 26 October 2015 at 18:07, Bryce Harrington <bryce at osg.samsung.com> wrote:
> 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

I was actually somewhat planning to fix these things (for both
bitfield and allow-null) in after this patch is merged: I didn't want
to include unrelated fixes into this series, but also wanted to write
code in a consistent style. So that's why it is still like this.

(except for the space after = of course, which I missed)

But now I noticed that my patch introduces a useless check regarding
allow_null on line 672 (this is already checked earlier, and is there
because I rebased incorrectly at some point) - this should NOT be in
here!

I will fix all of this. Should I send in a new series, or can I just
update this one patch?

(Is there a systematic way to retract patches?)