From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=57356 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OLerF-0007mA-TW for qemu-devel@nongnu.org; Mon, 07 Jun 2010 12:07:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OLerA-0004G1-Cv for qemu-devel@nongnu.org; Mon, 07 Jun 2010 12:07:25 -0400 Received: from mail-iw0-f173.google.com ([209.85.214.173]:49725) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OLerA-0004Fs-87 for qemu-devel@nongnu.org; Mon, 07 Jun 2010 12:07:20 -0400 Received: by iwn41 with SMTP id 41so3658152iwn.4 for ; Mon, 07 Jun 2010 09:07:19 -0700 (PDT) Message-ID: <4C0D1932.6030103@codemonkey.ws> Date: Mon, 07 Jun 2010 11:07:14 -0500 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 00/19] RFC: Reporting QEMU binary capabilities References: <1275921752-29420-1-git-send-email-berrange@redhat.com> In-Reply-To: <1275921752-29420-1-git-send-email-berrange@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" Cc: qemu-devel@nongnu.org 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. 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 > 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(-) > > > >