qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Kevin Wolf <kwolf@redhat.com>
To: Markus Armbruster <armbru@redhat.com>
Cc: lvivier@redhat.com, thuth@redhat.com,
	Peter Krempa <pkrempa@redhat.com>,
	ehabkost@redhat.com, qemu-block@nongnu.org,
	libvir-list@redhat.com, jasowang@redhat.com,
	qemu-devel@nongnu.org, mreitz@redhat.com, kraxel@redhat.com,
	Paolo Bonzini <pbonzini@redhat.com>,
	dgilbert@redhat.com
Subject: Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add
Date: Mon, 15 Mar 2021 16:52:06 +0100	[thread overview]
Message-ID: <YE+CppsqCGAdwrf4@merkur.fritz.box> (raw)
In-Reply-To: <87im5se8v4.fsf@dusky.pond.sub.org>

Am 15.03.2021 um 16:26 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 13.03.2021 um 14:40 hat Markus Armbruster geschrieben:
> >> Markus Armbruster <armbru@redhat.com> writes:
> >> 
> >> > Paolo Bonzini <pbonzini@redhat.com> writes:
> >> >
> >> >> On 11/03/21 15:08, Markus Armbruster wrote:
> >> >>>> I would rather keep the OptsVisitor here.  Do the same check for JSON
> >> >>>> syntax that you have in qobject_input_visitor_new_str, and whenever
> >> >>>> you need to walk all -object arguments, use something like this:
> >> >>>>
> >> >>>>      typedef struct ObjectArgument {
> >> >>>>          const char *id;
> >> >>>>          QDict *json;    /* or NULL for QemuOpts */
> >> >>>>          QSIMPLEQ_ENTRY(ObjectArgument) next;
> >> >>>>      }
> >> >>>>
> >> >>>> I already had patches in my queue to store -object in a GSList of
> >> >>>> dictionaries, changing it to use the above is easy enough.
> >> >>> 
> >> >>> I think I'd prefer following -display's precedence.  See my reply to
> >> >>> Kevin for details.
> >> >>
> >> >> Yeah, I got independently to the same conclusion and posted patches
> >> >> for that.  I was scared that visit_type_ObjectOptions was too much for 
> >> >> OptsVisitor but it seems to work...
> >> >
> >> > We have reason to be scared.  I'll try to cover this in my review.
> >> 
> >> The opts visitor has serious limitations.  From its header:
> >> 
> >>  * The Opts input visitor does not implement support for visiting QAPI
> >>  * alternates, numbers (other than integers), null, or arbitrary
> >>  * QTypes.  It also requires a non-null list argument to
> >>  * visit_start_list().
> >> 
> >> This is retro-documentation for hairy code.  I don't trust it.  Commit
> >> eb7ee2cbeb "qapi: introduce OptsVisitor" hints at additional
> >> restrictions:
> >> 
> >>     The type tree in the schema, corresponding to an option with a
> >>     discriminator, must have the following structure:
> >>     
> >>       struct
> >>         scalar member for non-discriminated optarg 1 [*]
> >>         list for repeating non-discriminated optarg 2 [*]
> >>           wrapper struct
> >>             single scalar member
> >>         union
> >>           struct for discriminator case 1
> >>             scalar member for optarg 3 [*]
> >>             list for repeating optarg 4 [*]
> >>               wrapper struct
> >>                 single scalar member
> >>             scalar member for optarg 5 [*]
> >>           struct for discriminator case 2
> >>             ...
> >
> > Is this a long-winded way of saying that it has to be flat, except that
> > it allows lists, i.e. there must be no nested objects on the "wire"?
> 
> I think so.
> 
> > The difference between structs and unions, and different branches inside
> > the union isn't visible for the visitor anyway.
> 
> Yes, only the code using the visitor deals with that.
> 
> >>     The "type" optarg name is fixed for the discriminator role. Its schema
> >>     representation is "union of structures", and each discriminator value must
> >>     correspond to a member name in the union.
> >>     
> >>     If the option takes no "type" descriminator, then the type subtree rooted
> >>     at the union must be absent from the schema (including the union itself).
> >>     
> >>     Optarg values can be of scalar types str / bool / integers / size.
> >> 
> >> Unsupported visits are treated as programming error.  Which is a nice
> >> way to say "they crash".
> >
> > The OptsVisitor never seems to crash explicitly by calling something
> > like abort().
> >
> > It may crash because of missing callbacks that are called without a NULL
> > check, like v->type_null.
> 
> Correct.
> 
> >                           This case should probably be fixed in
> > qapi/qapi-visit-core.c to do the check and simply return an error.
> 
> I retro-documented what I inherited: qapi-visit-core.c code expects the
> visitors to implement the full visitor-impl.h interface, but some
> visitors don't.  So I documented "method must be set to visit FOOs" in
> visitor-impl.h, and for the visitors that don't, I documented "can't
> visit FOOs".
> 
> If the crashing behavior we've always had gets in the way, there are two
> ways to change it:
> 
> 1. Complicate qapi-visit-core.c slightly to cope with incomplete visitor
>    implementations.
> 
> 2. Complete the visitor implementations: add dummy callbacks that fail.
> 
> I prefer 2., because I feel it keeps the visitor-impl.h interface
> simpler, and puts the extra complications where they belong.

I suggested making the callbacks optional because I expected that there
might be more than one visitor that doesn't support a callback and I
wouldn't like duplicating dummy callbacks in multiple places. But if
it's only the OptsVisitor, then we wouldn't get any duplication either
way and it becomes a matter of taste.

Kevin



      reply	other threads:[~2021-03-15 15:54 UTC|newest]

Thread overview: 78+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-08 16:54 [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add Kevin Wolf
2021-03-08 16:54 ` [PATCH v3 01/30] qapi/qom: Drop deprecated 'props' from object-add Kevin Wolf
2021-03-08 16:54 ` [PATCH v3 02/30] qapi/qom: Add ObjectOptions for iothread Kevin Wolf
2021-03-08 16:54 ` [PATCH v3 03/30] qapi/qom: Add ObjectOptions for authz-* Kevin Wolf
2021-03-09  9:17   ` Daniel P. Berrangé
2021-03-08 16:54 ` [PATCH v3 04/30] qapi/qom: Add ObjectOptions for cryptodev-* Kevin Wolf
2021-03-08 19:23   ` Eric Blake
2021-03-08 16:54 ` [PATCH v3 05/30] qapi/qom: Add ObjectOptions for dbus-vmstate Kevin Wolf
2021-03-08 16:54 ` [PATCH v3 06/30] qapi/qom: Add ObjectOptions for memory-backend-* Kevin Wolf
2021-03-08 19:25   ` Eric Blake
2021-03-08 16:54 ` [PATCH v3 07/30] qapi/qom: Add ObjectOptions for rng-*, deprecate 'opened' Kevin Wolf
2021-03-08 16:54 ` [PATCH v3 08/30] qapi/qom: Add ObjectOptions for throttle-group Kevin Wolf
2021-03-08 16:54 ` [PATCH v3 09/30] qapi/qom: Add ObjectOptions for secret*, deprecate 'loaded' Kevin Wolf
2021-03-09  9:21   ` Daniel P. Berrangé
2021-03-08 16:54 ` [PATCH v3 10/30] qapi/qom: Add ObjectOptions for tls-*, " Kevin Wolf
2021-03-09  9:23   ` Daniel P. Berrangé
2021-03-08 16:54 ` [PATCH v3 11/30] qapi/qom: Add ObjectOptions for can-* Kevin Wolf
2021-03-08 16:54 ` [PATCH v3 12/30] qapi/qom: Add ObjectOptions for colo-compare Kevin Wolf
2021-03-08 16:54 ` [PATCH v3 13/30] qapi/qom: Add ObjectOptions for filter-* Kevin Wolf
2021-03-08 16:54 ` [PATCH v3 14/30] qapi/qom: Add ObjectOptions for pr-manager-helper Kevin Wolf
2021-03-08 16:54 ` [PATCH v3 15/30] qapi/qom: Add ObjectOptions for confidential-guest-support Kevin Wolf
2021-03-08 16:54 ` [PATCH v3 16/30] qapi/qom: Add ObjectOptions for input-* Kevin Wolf
2021-03-08 16:54 ` [PATCH v3 17/30] qapi/qom: Add ObjectOptions for x-remote-object Kevin Wolf
2021-03-08 16:54 ` [PATCH v3 18/30] qapi/qom: QAPIfy object-add Kevin Wolf
2021-03-08 16:54 ` [PATCH v3 19/30] qom: Make "object" QemuOptsList optional Kevin Wolf
2021-03-08 16:54 ` [PATCH v3 20/30] qemu-storage-daemon: Implement --object with qmp_object_add() Kevin Wolf
2021-03-08 16:54 ` [PATCH v3 21/30] qom: Remove user_creatable_add_dict() Kevin Wolf
2021-03-08 16:54 ` [PATCH v3 22/30] qom: Factor out user_creatable_process_cmdline() Kevin Wolf
2021-03-13  8:41   ` Markus Armbruster
2021-03-13  9:28     ` Paolo Bonzini
2021-03-15 11:48     ` Kevin Wolf
2021-03-08 16:54 ` [PATCH v3 23/30] qemu-io: Use user_creatable_process_cmdline() for --object Kevin Wolf
2021-03-08 16:54 ` [PATCH v3 24/30] qemu-nbd: " Kevin Wolf
2021-03-08 16:54 ` [PATCH v3 25/30] qom: Add user_creatable_add_from_str() Kevin Wolf
2021-03-08 16:54 ` [PATCH v3 26/30] qemu-img: Use user_creatable_process_cmdline() for --object Kevin Wolf
2021-03-08 19:32   ` Eric Blake
2021-03-13  7:40   ` Markus Armbruster
2021-03-13  7:47     ` Paolo Bonzini
2021-03-13 12:30       ` Markus Armbruster
2021-03-15 11:38         ` Kevin Wolf
2021-03-15 14:15           ` Markus Armbruster
2021-03-15 14:43             ` Kevin Wolf
2021-03-08 16:54 ` [PATCH v3 27/30] hmp: QAPIfy object_add Kevin Wolf
2021-03-13 13:28   ` Markus Armbruster
2021-03-13 14:11     ` Paolo Bonzini
2021-03-15  9:39       ` Markus Armbruster
2021-03-15 11:09         ` Kevin Wolf
2021-03-15 11:38           ` Dr. David Alan Gilbert
2021-03-15 11:58             ` Paolo Bonzini
2021-03-08 16:54 ` [PATCH v3 28/30] qom: Add user_creatable_parse_str() Kevin Wolf
2021-03-08 16:54 ` [PATCH v3 29/30] vl: QAPIfy -object Kevin Wolf
2021-03-08 19:34   ` Eric Blake
2021-03-08 16:54 ` [PATCH v3 30/30] qom: Drop QemuOpts based interfaces Kevin Wolf
2021-03-10 14:22 ` [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add Peter Krempa
2021-03-10 14:31   ` Paolo Bonzini
2021-03-10 14:48     ` Peter Krempa
2021-03-10 17:30     ` Kevin Wolf
2021-03-11  7:47       ` Peter Krempa
2021-03-11  8:16         ` Paolo Bonzini
2021-03-11  8:37         ` Kevin Wolf
2021-03-11 11:24           ` Peter Krempa
2021-03-11 11:41             ` Kevin Wolf
2021-03-11 12:29               ` Peter Krempa
2021-03-11 14:01               ` Markus Armbruster
2021-03-11  8:14       ` Paolo Bonzini
2021-03-11  8:45         ` Kevin Wolf
2021-03-11  8:49           ` Paolo Bonzini
2021-03-11 10:38       ` Markus Armbruster
2021-03-11 11:00         ` Paolo Bonzini
2021-03-11 14:08           ` Markus Armbruster
2021-03-11 17:50             ` Paolo Bonzini
2021-03-12  8:14               ` Markus Armbruster
2021-03-12  8:46                 ` Paolo Bonzini
2021-03-12  8:52                   ` Peter Krempa
2021-03-13 13:40                 ` Markus Armbruster
2021-03-15 11:36                   ` Kevin Wolf
2021-03-15 15:26                     ` Markus Armbruster
2021-03-15 15:52                       ` Kevin Wolf [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YE+CppsqCGAdwrf4@merkur.fritz.box \
    --to=kwolf@redhat.com \
    --cc=armbru@redhat.com \
    --cc=dgilbert@redhat.com \
    --cc=ehabkost@redhat.com \
    --cc=jasowang@redhat.com \
    --cc=kraxel@redhat.com \
    --cc=libvir-list@redhat.com \
    --cc=lvivier@redhat.com \
    --cc=mreitz@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=pkrempa@redhat.com \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=thuth@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).