diff mbox series

[RFC] string-output-visitor: show structs as "<omitted>"

Message ID 20231211195734.425040-1-stefanha@redhat.com
State New
Headers show
Series [RFC] string-output-visitor: show structs as "<omitted>" | expand

Commit Message

Stefan Hajnoczi Dec. 11, 2023, 7:57 p.m. UTC
StringOutputVisitor crashes when it visits a struct because
->start_struct() is NULL.

Show "<omitted>" instead of crashing. This is necessary because the
virtio-blk-pci iothread-vq-mapping parameter that I'd like to introduce
soon is a list of IOThreadMapping structs.

Cc: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
Can we do better?

I am unfamiliar with StringOutputVisitor, so I wasn't sure how to
proceed. Is the format or at least the intended use of
StringOutputVisitor's output defined somewhere? Does it need to be a
single line or can the output be multiple lines?

Or maybe I shouldn't introduce a qdev property with IOThreadMappingList
as its type in
https://lore.kernel.org/qemu-devel/ZUoPiFxIIwFq5wMg@redhat.com/?
---
 include/qapi/string-output-visitor.h |  6 +++---
 qapi/string-output-visitor.c         | 14 ++++++++++++++
 2 files changed, 17 insertions(+), 3 deletions(-)

Comments

Markus Armbruster Dec. 12, 2023, 6:12 a.m. UTC | #1
Stefan Hajnoczi <stefanha@redhat.com> writes:

> StringOutputVisitor crashes when it visits a struct because
> ->start_struct() is NULL.
>
> Show "<omitted>" instead of crashing. This is necessary because the
> virtio-blk-pci iothread-vq-mapping parameter that I'd like to introduce
> soon is a list of IOThreadMapping structs.
>
> Cc: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> Can we do better?
>
> I am unfamiliar with StringOutputVisitor, so I wasn't sure how to
> proceed. Is the format or at least the intended use of
> StringOutputVisitor's output defined somewhere?

Bwuahahahaha!

SCNR

>                                                 Does it need to be a
> single line or can the output be multiple lines?

I'm afraid we need to review its users to be sure.

> Or maybe I shouldn't introduce a qdev property with IOThreadMappingList
> as its type in
> https://lore.kernel.org/qemu-devel/ZUoPiFxIIwFq5wMg@redhat.com/?

QOM initially supported only scalar properties.

I think maintaining this restriction will lead to awkward interfaces.
Lists are clearly useful.  And then we'll likely want list of struct,
not multiple lists.

Lifting the restriction will take some work.

On the string visitors, see also my musings in

    Subject: Re: [PATCH v2 1/2] qdev: add IOThreadVirtQueueMappingList property type 
    Date: Mon, 11 Dec 2023 16:32:06 +0100
    Message-ID: <87msugah6x.fsf@pond.sub.org>

> ---
>  include/qapi/string-output-visitor.h |  6 +++---
>  qapi/string-output-visitor.c         | 14 ++++++++++++++
>  2 files changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/include/qapi/string-output-visitor.h b/include/qapi/string-output-visitor.h
> index 268dfe9986..762fe3f705 100644
> --- a/include/qapi/string-output-visitor.h
> +++ b/include/qapi/string-output-visitor.h
> @@ -26,9 +26,9 @@ typedef struct StringOutputVisitor StringOutputVisitor;
>   * If everything else succeeds, pass @result to visit_complete() to
>   * collect the result of the visit.
>   *
> - * The string output visitor does not implement support for visiting
> - * QAPI structs, alternates, null, or arbitrary QTypes.  It also
> - * requires a non-null list argument to visit_start_list().
> + * The string output visitor does not implement support for alternates, null,
> + * or arbitrary QTypes.  It also requires a non-null list argument to
> + * visit_start_list().

Mention output for structs is information-free?

>   */
>  Visitor *string_output_visitor_new(bool human, char **result);
>  
> diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
> index c0cb72dbe4..363dac00fe 100644
> --- a/qapi/string-output-visitor.c
> +++ b/qapi/string-output-visitor.c
> @@ -292,6 +292,18 @@ static bool print_type_null(Visitor *v, const char *name, QNull **obj,
>      return true;
>  }
>  
> +static bool start_struct(Visitor *v, const char *name, void **obj,
> +                         size_t size, Error **errp)
> +{
> +    return true;
> +}
> +
> +static void end_struct(Visitor *v, void **obj)
> +{
> +    StringOutputVisitor *sov = to_sov(v);
> +    string_output_set(sov, g_strdup("<omitted>"));

TODO comment?

> +}
> +
>  static bool
>  start_list(Visitor *v, const char *name, GenericList **list, size_t size,
>             Error **errp)
> @@ -379,6 +391,8 @@ Visitor *string_output_visitor_new(bool human, char **result)
>      v->visitor.type_str = print_type_str;
>      v->visitor.type_number = print_type_number;
>      v->visitor.type_null = print_type_null;
> +    v->visitor.start_struct = start_struct;
> +    v->visitor.end_struct = end_struct;
>      v->visitor.start_list = start_list;
>      v->visitor.next_list = next_list;
>      v->visitor.end_list = end_list;
diff mbox series

Patch

diff --git a/include/qapi/string-output-visitor.h b/include/qapi/string-output-visitor.h
index 268dfe9986..762fe3f705 100644
--- a/include/qapi/string-output-visitor.h
+++ b/include/qapi/string-output-visitor.h
@@ -26,9 +26,9 @@  typedef struct StringOutputVisitor StringOutputVisitor;
  * If everything else succeeds, pass @result to visit_complete() to
  * collect the result of the visit.
  *
- * The string output visitor does not implement support for visiting
- * QAPI structs, alternates, null, or arbitrary QTypes.  It also
- * requires a non-null list argument to visit_start_list().
+ * The string output visitor does not implement support for alternates, null,
+ * or arbitrary QTypes.  It also requires a non-null list argument to
+ * visit_start_list().
  */
 Visitor *string_output_visitor_new(bool human, char **result);
 
diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
index c0cb72dbe4..363dac00fe 100644
--- a/qapi/string-output-visitor.c
+++ b/qapi/string-output-visitor.c
@@ -292,6 +292,18 @@  static bool print_type_null(Visitor *v, const char *name, QNull **obj,
     return true;
 }
 
+static bool start_struct(Visitor *v, const char *name, void **obj,
+                         size_t size, Error **errp)
+{
+    return true;
+}
+
+static void end_struct(Visitor *v, void **obj)
+{
+    StringOutputVisitor *sov = to_sov(v);
+    string_output_set(sov, g_strdup("<omitted>"));
+}
+
 static bool
 start_list(Visitor *v, const char *name, GenericList **list, size_t size,
            Error **errp)
@@ -379,6 +391,8 @@  Visitor *string_output_visitor_new(bool human, char **result)
     v->visitor.type_str = print_type_str;
     v->visitor.type_number = print_type_number;
     v->visitor.type_null = print_type_null;
+    v->visitor.start_struct = start_struct;
+    v->visitor.end_struct = end_struct;
     v->visitor.start_list = start_list;
     v->visitor.next_list = next_list;
     v->visitor.end_list = end_list;