Re: [Qemu-devel] [PATCH 00/19] RFC: Reporting QEMU binary capabilities

[Luiz Capitulino <lcapitulino@redhat.com>]

On Mon, 07 Jun 2010 11:07:14 -0500
Anthony Liguori <anthony@codemonkey.ws> wrote:

> Hi Daniel,
> 
> On 06/07/2010 09:42 AM, Daniel P. Berrange wrote:
> > As everyone here agrees, having management apps parse -help output
> > to determine the QEMU capabilities is not at all nice, because it
> > is an ill-defined&  fragile data format. Looking more broadly these
> > same issues apply to all the other command line options that accept
> > a '?' flag for querying capabilities.
> >
> > We have a very nice structured data format we could be using for
> > this: JSON. What's lacking is code to output all this information
> > in the JSON format. This patch series can thus be summarized as
> > 'JSON for everything'
> >    
> 
> For the most part, this series looks pretty nice.

 For me too, some comments though:

  - All protocol visible changes must be documented in qemu-monitor.hx and
    as there's a lot of stuff being exposed, would be good to get more ACKs
    on them (maybe from Avi, Markus etc)

  - This series overlaps a bit with self-description support, specially if
    you're willing to work on a really useful version of query-commands. It's
    worth reading the following thread:

       http://lists.gnu.org/archive/html/qemu-devel/2010-01/msg00852.html

  - As the series is going to change, I've skipped some implementation details
    while reviewing. Will do a deeper review in the non-rfc version
> 
> I think my only real objection is the query-argv bits.  The enums are a 
> bit awkward to define but I understand the value of it and I can't think 
> of a better way to do it.
> 
> Regards,
> 
> Anthony Liguori
> 
> > For reference, here is the full list of information libvirt currently
> > queries from QEMU:
> >
> >    * Detection of command line flags (-help parsing)
> >
> >      -no-kqemu, -no-kvm, -enable-kvm, -no-reboot
> >      -name, -uuid, -xen-domid, -domid, -drive
> >      -vga, -std-vga, -pcidevice, -mem-path
> >      -chardev, -balloon, -device, -rtc, -rtc-td-hack
> >      -no-hpet, -no-kvm-pit-reinjection, -tdf
> >      -fsdev -sdl
> >
> >    * Detection of parameters (-help parsing)
> >
> >       -drive format=
> >       -drive boot=
> >       -drive serial=
> >       -drive cache=
> >       -cpu cores=, threads=, sockets=
> >       -netdev vhost=
> >
> >    * Detection of parameter values (-help parsing)
> >
> >       -drive cache=writethrough|writeback|none
> >         vs
> >       -drive cache=default|on|off
> >
> >    * Version number  (-help parsing)
> >
> >      -netdev  + 0.13.0
> >
> >      0.9.0  for VNC syntax
> >
> >      0.10.0 for vnet hdr
> >
> >    * Parse -M ?  output to get list of machine types + aliases
> >
> >    * Parse -device pci-assign,? to check for 'configfd' parameter
> >
> >    * Parse -cpu ?  to get list of named CPU types
> >
> >    * Parse binary name (qemu-system-XXXX) to guess arch of target
> >
> >
> > This isn't an 100% exhaustive list of things that we would like
> > to be able to get access to though. It can likely cover all of
> > the following:
> >
> >   * Version
> >
> >        QEMU major, minor, micro numbers. KVM version. Package
> >        version
> >
> >   * Devices
> >
> >        List of device names. Parameter names. Parameter value
> >        data types. Allowed strings for enums
> >
> >   * Arguments
> >
> >        List of command line arguments. Parameter names. Parameter
> >        value data types. Allowed strings for enums
> >
> >   * Machine types
> >
> >        List of names + aliases
> >        List of default devices in machine
> >
> >   * CPU types
> >
> >        List of names, associated feature flags, all allowed
> >        feature flags
> >
> >   * Target info
> >
> >        Arch name, wordsize
> >
> >   * Monitor commands
> >
> >        List of all monitor commands. Parameter names. Parameter
> >        value data types. Allowed strings for enums
> >
> >   * Block device backends
> >
> >        List of all block device backends. Parameter names.
> >        Parameter value data types. Allowed strings for enums
> >
> >   * Netdev device backends
> >
> >        List of all netdev device backends. Parameter names.
> >        Parameter value data types. Allowed strings for enums
> >
> >   * Chardev device backends
> >
> >        List of all chardev device backends. Parameter names.
> >        Parameter value data types. Allowed strings for enums
> >
> >
> > This patch series attempts to satisfy as much of this as is
> > feasible with the current QEMU codebase structure. The series
> > comprises four stages
> >
> >   * Some basic infrastructure. Support for JSON pretty printing.
> >     Introduction of standardized helpers for converting enums
> >     to/from strings. Introduction of an 'enum' data type for
> >     QemuOpt to expose a formal list of valid values&  simplify
> >     config handling / parsing. Compile time assertion checking
> >
> >   * Convert driver, netdev&  rtc config options to use the new
> >     enum data type for all appropriate args
> >
> >   * Add new QMP monitor commands exposing data for machine
> >     types, devices, cpu types, arch target, argv, config params,
> >     and netdev backends.
> >
> >   * Add a new '-capabilities' command line arg as syntactic
> >     sugar for the monitor commands that just report on static
> >     info about the QEMU binary.
> >
> > The main problem encountered with this patch series is the
> > split between argv and config parameters. The qemu-config.c
> > file provides the information is a good data format, allowing
> > programatic access to the list of parameters for each config
> > option (eg, the 'cache', 'file', 'aio', etc bits for -drive).
> > There are some issues with it though
> >
> >   - It lacks a huge amount of coverage wrt to the full argv
> >     available, even simple things like the '-m' argument
> >     don't exist.
> >
> >   - It is inconsistent in handling parameters. eg it  hands
> >     off validation of netdev backends to other code, to allow
> >     for handling of different parameters for tap vs user vs
> >     socket backends.  For chardev backends though, it directly
> >     includes a union of all possible parameters.
> >
> > Ideally the 'query-argv' command would not be required, but
> > unless those problems with the qemu-config are resolved, it
> > is the only way to access alot of the information. This is
> > sub-optimal because although it lets apps easily determine
> > whether an arg like '-smp' is present, it still leaves them
> > parsing that args' help string to discover parameters.
> >
> > This series is lacking a 'query-blockdev' and 'query-chardev'
> > commands to report on availability of backends for -blockdev
> > and '-chardev' commands.
> >
> > Finally the existing 'query-commands' output is lacking any
> > information about the parameters associated with each command.
> > This needs to be addressed somehow.
> >
> > In summary though, IMHO, this patch series provides a good
> > base for providing information about static QEMU binary
> > capabilities to applications. It should ensure apps never
> > again need to go anywhere near -help, -M ?, -cpu ?, -device ?,
> > -soundhw ? and other such ill defined output formats.
> >
> > NB, I know this patch series will probably clash horribly
> > with other patch series recently posted for review, in
> > particular Jes' cleanup of vl.c   I will rebase as required
> > if any of those get merged.
> >
> > This series is currently based on GIT master as of:
> >
> >    commit fd1dc858370d9a9ac7ea2512812c3a152ee6484b
> >    Author: Edgar E. Iglesias<edgar.iglesias@gmail.com>
> >    Date:   Mon Jun 7 11:54:27 2010 +0200
> >
> > Regards,
> > Daniel
> >
> >   Makefile.objs                |    2
> >   block.c                      |   32 +-
> >   block.h                      |   55 ++++
> >   cpus.c                       |   78 ++++++
> >   cpus.h                       |    1
> >   hw/boards.h                  |    2
> >   hw/mc146818rtc.c             |    8
> >   hw/qdev.c                    |  148 +++++++++++++
> >   hw/qdev.h                    |    2
> >   monitor.c                    |  167 ++++++++++++++
> >   monitor.h                    |    5
> >   net.c                        |  173 ++++++++++-----
> >   net.h                        |    2
> >   qemu-config.c                |  228 +++++++++++++++++++-
> >   qemu-config.h                |    2
> >   qemu-enum.c                  |   44 +++
> >   qemu-enum.h                  |   45 ++++
> >   qemu-option.c                |   35 +++
> >   qemu-option.h                |   14 +
> >   qemu-options.hx              |    9
> >   qjson.c                      |   55 ++++
> >   qjson.h                      |    1
> >   sysemu.h                     |   43 +++
> >   target-arm/cpu.h             |    7
> >   target-arm/helper.c          |   21 +
> >   target-cris/cpu.h            |    7
> >   target-cris/translate.c      |   22 +
> >   target-i386/cpu.h            |    7
> >   target-i386/cpuid.c          |   79 +++++++
> >   target-m68k/cpu.h            |    9
> >   target-m68k/helper.c         |   23 ++
> >   target-mips/cpu.h            |    7
> >   target-mips/translate.c      |    4
> >   target-mips/translate_init.c |   19 +
> >   target-ppc/cpu.h             |    7
> >   target-ppc/translate.c       |    4
> >   target-ppc/translate_init.c  |   17 +
> >   target-sh4/cpu.h             |    8
> >   target-sh4/translate.c       |   23 ++
> >   target-sparc/cpu.h           |    9
> >   target-sparc/helper.c        |   60 +++++
> >   verify.h                     |  164 ++++++++++++++
> >   vl.c                         |  482 +++++++++++++++++++++++++++++--------------
> >   43 files changed, 1869 insertions(+), 261 deletions(-)
> >
> >
> >
> >    
> 
>