Re: QEMU API cleanup initiative - Let's chat during the KVM call

[Daniel P . Berrangé]

On Tue, Oct 06, 2020 at 11:30:20AM +0200, Paolo Bonzini wrote:
> On 05/10/20 16:52, John Snow wrote:
> > - Markus considers the platonic ideal of a CLI one in which each flag is
> > a configuration directive, and each directive that references another
> > directive must appear after the directive in which it references.
> > 
> > - I tend to consider the ideal configuration to be a static object that
> > has no inherent order from one key to the next, e.g. a JSON object or
> > static flat file that can be used to configure the sysemu.
> > 
> > They're not compatible visions; and they have implications for error
> > ordering and messages and so on.
> 
> I think they aren't incompatible.  Even your idea would probably forbid
> cycles, so it only takes a topological sort to go from an unordered
> configuration to an ordered one.  The only difference is whether it's
> the user or the program that does it.
> 
> > For the meantime, Markus's vision is closer to what QEMU already does,
> > so it's likely the winning answer for now and if a conversion to the
> > other idea is required at a time later, we'll have to tackle it then. (I
> > think.)
> > 
> > It's a good subject of discussion. (Also relevant: if parsing is to
> > occur in more than the CLI context, the existing contextual CLI parser
> > error system needs to be reworked to avoid monitor-unsafe error calls.
> > It's not trivial, I think.)
> 
> I think parsing should occur in CLI context only, but errors can occur
> elsewhere too.
> 
> I think the idea for this kind of refactoring is always to first make
> the code behave the way you want, and only then change the
> implementation to look the way you want.
> 
> Currently we have:
> 
>     switch (...) {
>         case QEMU_OPT_...:
>             /* something has side effects, something is just parsing */
>     }
> 
>     init1();
>     qemu_opts_foreach(something_opts, configure_something);
>     init2();
>     qemu_opts_foreach(some_more_opts, configure_some_more);
>     init3();
> 
>     enter_preconfig();
> 
> We should first of all change it to
> 
>     parse_command_line() {
>         apply_simple_options()l
>         qemu_opts_foreach(something_opts, configure_something);
>         qemu_opts_foreach(some_more_opts, configure_some_more);
>     }
> 
>     switch (...) {
>         case QEMU_OPT_...:
>         /* no side effects on the initN() calls below */
>     }
> 
>     init1();
>     init2();
>     init3();
> 
>     parse_command_line()
> 
>     enter_preconfig();
> 
>     more_init_that_needs_side_effects();
> 
> This way, the parse_command_line() and its qemu_opts_foreach callbacks
> can become changed into a series of qmp_*() commands.  The commands can
> be called within the appropriate loc_push() though.
> 
> Problem is, it's 1000 lines of initialization interspersed with
> qemu_opts_foreach calls.  But it's doable.

I feel that both of these approaches are equally broken, as they don't
honour the order in which arguments are passed by the caller when
creating resources.

This leads to the crazy hacks we have with -object where we have to
create certain objects at certain stages.

To make QEMU CLI parsing sane we need to be able to create objects as
we parse them.

   while (n++ < argc) {
        switch (argv[n]) {
           case QEMU_OPT_foo:
             ...parse argv[n]...
             ...create argv[n]...
        }
   }

IOW, all usage of 'qemu_opts_foreach' should be targetted for complete
elimination.

I'm not convinced that your proposed change takes us in direction, if
anything it is encoding the current split of parsing vs creation even
more strongly.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|