Re: [PATCH] string-output-visitor: Fix (pseudo) struct handling

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5935 bytes --]

On Tue, Jan 09, 2024 at 07:17:17PM +0100, Kevin Wolf wrote:
> Commit ff32bb53 tried to get minimal struct support into the string
> output visitor by just making it return "<omitted>". Unfortunately, it
> forgot that the caller will still make more visitor calls for the
> content of the struct.
> 
> If the struct is contained in a list, such as IOThreadVirtQueueMapping,
> in the better case its fields show up as separate list entries. In the
> worse case, it contains another list, and the string output visitor
> doesn't support nested lists and asserts that this doesn't happen. So as
> soon as the optional "vqs" field in IOThreadVirtQueueMapping is
> specified, we get a crash.
> 
> This can be reproduced with the following command line:
> 
>   echo "info qtree" | ./qemu-system-x86_64 \
>     -object iothread,id=t0 \
>     -blockdev null-co,node-name=disk \
>     -device '{"driver": "virtio-blk-pci", "drive": "disk",
>               "iothread-vq-mapping": [{"iothread": "t0", "vqs": [0]}]}' \
>     -monitor stdio
> 
> Fix the problem by counting the nesting level of structs and ignoring
> any visitor calls for values (apart from start/end_struct) while we're
> not on the top level.
> 
> Fixes: ff32bb53476539d352653f4ed56372dced73a388
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2069
> Reported-by: Aihua Liang <aliang@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qapi/string-output-visitor.c | 46 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)

Thanks for getting to this before I could:

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

> diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
> index f0c1dea89e..5115536b15 100644
> --- a/qapi/string-output-visitor.c
> +++ b/qapi/string-output-visitor.c
> @@ -65,6 +65,7 @@ struct StringOutputVisitor
>      } range_start, range_end;
>      GList *ranges;
>      void *list; /* Only needed for sanity checking the caller */
> +    unsigned int struct_nesting;
>  };
>  
>  static StringOutputVisitor *to_sov(Visitor *v)
> @@ -144,6 +145,10 @@ static bool print_type_int64(Visitor *v, const char *name, int64_t *obj,
>      StringOutputVisitor *sov = to_sov(v);
>      GList *l;
>  
> +    if (sov->struct_nesting) {
> +        return true;
> +    }
> +
>      switch (sov->list_mode) {
>      case LM_NONE:
>          string_output_append(sov, *obj);
> @@ -231,6 +236,10 @@ static bool print_type_size(Visitor *v, const char *name, uint64_t *obj,
>      uint64_t val;
>      char *out, *psize;
>  
> +    if (sov->struct_nesting) {
> +        return true;
> +    }
> +
>      if (!sov->human) {
>          out = g_strdup_printf("%"PRIu64, *obj);
>          string_output_set(sov, out);
> @@ -250,6 +259,11 @@ static bool print_type_bool(Visitor *v, const char *name, bool *obj,
>                              Error **errp)
>  {
>      StringOutputVisitor *sov = to_sov(v);
> +
> +    if (sov->struct_nesting) {
> +        return true;
> +    }
> +
>      string_output_set(sov, g_strdup(*obj ? "true" : "false"));
>      return true;
>  }
> @@ -260,6 +274,10 @@ static bool print_type_str(Visitor *v, const char *name, char **obj,
>      StringOutputVisitor *sov = to_sov(v);
>      char *out;
>  
> +    if (sov->struct_nesting) {
> +        return true;
> +    }
> +
>      if (sov->human) {
>          out = *obj ? g_strdup_printf("\"%s\"", *obj) : g_strdup("<null>");
>      } else {
> @@ -273,6 +291,11 @@ static bool print_type_number(Visitor *v, const char *name, double *obj,
>                                Error **errp)
>  {
>      StringOutputVisitor *sov = to_sov(v);
> +
> +    if (sov->struct_nesting) {
> +        return true;
> +    }
> +
>      string_output_set(sov, g_strdup_printf("%.17g", *obj));
>      return true;
>  }
> @@ -283,6 +306,10 @@ static bool print_type_null(Visitor *v, const char *name, QNull **obj,
>      StringOutputVisitor *sov = to_sov(v);
>      char *out;
>  
> +    if (sov->struct_nesting) {
> +        return true;
> +    }
> +
>      if (sov->human) {
>          out = g_strdup("<null>");
>      } else {
> @@ -295,6 +322,9 @@ static bool print_type_null(Visitor *v, const char *name, QNull **obj,
>  static bool start_struct(Visitor *v, const char *name, void **obj,
>                           size_t size, Error **errp)
>  {
> +    StringOutputVisitor *sov = to_sov(v);
> +
> +    sov->struct_nesting++;
>      return true;
>  }
>  
> @@ -302,6 +332,10 @@ static void end_struct(Visitor *v, void **obj)
>  {
>      StringOutputVisitor *sov = to_sov(v);
>  
> +    if (--sov->struct_nesting) {
> +        return;
> +    }
> +
>      /* TODO actually print struct fields */
>      string_output_set(sov, g_strdup("<omitted>"));
>  }
> @@ -312,6 +346,10 @@ start_list(Visitor *v, const char *name, GenericList **list, size_t size,
>  {
>      StringOutputVisitor *sov = to_sov(v);
>  
> +    if (sov->struct_nesting) {
> +        return true;
> +    }
> +
>      /* we can't traverse a list in a list */
>      assert(sov->list_mode == LM_NONE);
>      /* We don't support visits without a list */
> @@ -329,6 +367,10 @@ static GenericList *next_list(Visitor *v, GenericList *tail, size_t size)
>      StringOutputVisitor *sov = to_sov(v);
>      GenericList *ret = tail->next;
>  
> +    if (sov->struct_nesting) {
> +        return ret;
> +    }
> +
>      if (ret && !ret->next) {
>          sov->list_mode = LM_END;
>      }
> @@ -339,6 +381,10 @@ static void end_list(Visitor *v, void **obj)
>  {
>      StringOutputVisitor *sov = to_sov(v);
>  
> +    if (sov->struct_nesting) {
> +        return;
> +    }
> +
>      assert(sov->list == obj);
>      assert(sov->list_mode == LM_STARTED ||
>             sov->list_mode == LM_END ||
> -- 
> 2.43.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]