Re: [Qemu-devel] [RFC 1/6] OptsVisitor: introduce basic list modes

[Paolo Bonzini <pbonzini@redhat.com>]

Il 18/07/2013 15:59, Laszlo Ersek ha scritto:
> We're going to need more state while processing a list of repeated
> options. This change eliminates "repeated_opts_first" and adds a new state
> variable:
> 
>   list_mode       repeated_opts  repeated_opts_first
>   --------------  -------------  -------------------
>   LM_NONE         NULL           false
>   LM_STARTED      non-NULL       true
>   LM_IN_PROGRESS  non-NULL       false
> 
> Additionally, it is documented that lookup_scalar() and processed(), both
> called by opts_type_XXX(), are invalid in LM_STARTED -- generated qapi
> code calls opts_next_list() to allocate the very first link before trying
> to parse a scalar into it. List mode restrictions are expressed in
> positive / inclusive form.
> 
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  qapi/opts-visitor.c |   43 ++++++++++++++++++++++++++++++++++---------
>  1 files changed, 34 insertions(+), 9 deletions(-)
> 
> diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
> index 174bd8b..ab9a602 100644
> --- a/qapi/opts-visitor.c
> +++ b/qapi/opts-visitor.c
> @@ -18,6 +18,15 @@
>  #include "qapi/visitor-impl.h"
>  
>  
> +enum ListMode
> +{
> +    LM_NONE,             /* not traversing a list of repeated options */
> +    LM_STARTED,          /* opts_start_list() succeeded */
> +    LM_IN_PROGRESS       /* opts_next_list() has been called */
> +};
> +
> +typedef enum ListMode ListMode;
> +
>  struct OptsVisitor
>  {
>      Visitor visitor;
> @@ -35,8 +44,8 @@ struct OptsVisitor
>      /* The list currently being traversed with opts_start_list() /
>       * opts_next_list(). The list must have a struct element type in the
>       * schema, with a single mandatory scalar member. */
> +    ListMode list_mode;
>      GQueue *repeated_opts;
> -    bool repeated_opts_first;
>  
>      /* If "opts_root->id" is set, reinstantiate it as a fake QemuOpt for
>       * uniformity. Only its "name" and "str" fields are set. "fake_id_opt" does
> @@ -156,9 +165,11 @@ opts_start_list(Visitor *v, const char *name, Error **errp)
>      OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
>  
>      /* we can't traverse a list in a list */
> -    assert(ov->repeated_opts == NULL);
> +    assert(ov->list_mode == LM_NONE);
>      ov->repeated_opts = lookup_distinct(ov, name, errp);
> -    ov->repeated_opts_first = (ov->repeated_opts != NULL);
> +    if (ov->repeated_opts != NULL) {
> +        ov->list_mode = LM_STARTED;
> +    }
>  }
>  
>  
> @@ -168,10 +179,13 @@ opts_next_list(Visitor *v, GenericList **list, Error **errp)
>      OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
>      GenericList **link;
>  
> -    if (ov->repeated_opts_first) {
> -        ov->repeated_opts_first = false;
> +    switch (ov->list_mode) {
> +    case LM_STARTED:
> +        ov->list_mode = LM_IN_PROGRESS;
>          link = list;
> -    } else {
> +        break;
> +
> +    case LM_IN_PROGRESS: {
>          const QemuOpt *opt;
>  
>          opt = g_queue_pop_head(ov->repeated_opts);
> @@ -180,6 +194,11 @@ opts_next_list(Visitor *v, GenericList **list, Error **errp)
>              return NULL;
>          }
>          link = &(*list)->next;
> +        break;
> +    }
> +
> +    default:
> +        assert(0);

"abort()" to avoid excessively punishing folks who use -DNDEBUG (and
also to ensure they do not get extra compiler warnings).

Otherwise looks good!

Paolo

>      }
>  
>      *link = g_malloc0(sizeof **link);
> @@ -192,14 +211,16 @@ opts_end_list(Visitor *v, Error **errp)
>  {
>      OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
>  
> +    assert(ov->list_mode == LM_STARTED || ov->list_mode == LM_IN_PROGRESS);
>      ov->repeated_opts = NULL;
> +    ov->list_mode = LM_NONE;
>  }
>  
>  
>  static const QemuOpt *
>  lookup_scalar(const OptsVisitor *ov, const char *name, Error **errp)
>  {
> -    if (ov->repeated_opts == NULL) {
> +    if (ov->list_mode == LM_NONE) {
>          GQueue *list;
>  
>          /* the last occurrence of any QemuOpt takes effect when queried by name
> @@ -207,6 +228,7 @@ lookup_scalar(const OptsVisitor *ov, const char *name, Error **errp)
>          list = lookup_distinct(ov, name, errp);
>          return list ? g_queue_peek_tail(list) : NULL;
>      }
> +    assert(ov->list_mode == LM_IN_PROGRESS);
>      return g_queue_peek_head(ov->repeated_opts);
>  }
>  
> @@ -214,9 +236,12 @@ lookup_scalar(const OptsVisitor *ov, const char *name, Error **errp)
>  static void
>  processed(OptsVisitor *ov, const char *name)
>  {
> -    if (ov->repeated_opts == NULL) {
> +    if (ov->list_mode == LM_NONE) {
>          g_hash_table_remove(ov->unprocessed_opts, name);
> +        return;
>      }
> +    assert(ov->list_mode == LM_IN_PROGRESS);
> +    /* do nothing */
>  }
>  
>  
> @@ -365,7 +390,7 @@ opts_start_optional(Visitor *v, bool *present, const char *name,
>      OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
>  
>      /* we only support a single mandatory scalar field in a list node */
> -    assert(ov->repeated_opts == NULL);
> +    assert(ov->list_mode == LM_NONE);
>      *present = (lookup_distinct(ov, name, NULL) != NULL);
>  }
>  
>