On Mon, 07 Jun 2010 11:07:14 -0500
Anthony Liguori <anth...@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.igles...@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(-)
> >
> >
> >
> >
>
>
