From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NkcQE-00007a-Lg for qemu-devel@nongnu.org; Thu, 25 Feb 2010 07:02:26 -0500 Received: from [199.232.76.173] (port=38151 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NkcQD-00007P-Ma for qemu-devel@nongnu.org; Thu, 25 Feb 2010 07:02:25 -0500 Received: from Debian-exim by monty-python.gnu.org with spam-scanned (Exim 4.60) (envelope-from ) id 1NkcQB-0001MJ-J8 for qemu-devel@nongnu.org; Thu, 25 Feb 2010 07:02:25 -0500 Received: from mx1.redhat.com ([209.132.183.28]:30268) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NkcQB-0001MF-66 for qemu-devel@nongnu.org; Thu, 25 Feb 2010 07:02:23 -0500 Received: from int-mx08.intmail.prod.int.phx2.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.21]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o1PC2MAq030019 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 25 Feb 2010 07:02:22 -0500 Date: Thu, 25 Feb 2010 13:59:09 +0200 From: "Michael S. Tsirkin" Message-ID: <20100225115909.GE9116@redhat.com> References: <1267034160-3517-1-git-send-email-armbru@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1267034160-3517-1-git-send-email-armbru@redhat.com> Subject: [Qemu-devel] Re: [PATCH RFC 00/48] Convert device_add to QObject / QError List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org On Wed, Feb 24, 2010 at 06:55:12PM +0100, Markus Armbruster wrote: > Why this is such a big job? There are two issues with a naive > conversion: > > * Error message degradation > > The error messages are worded for -device. They aren't so hot to > begin with: we typically have many -device options, and to which one > a message applies is often not obvious. > > Now, QMP wants relatively generic errors. For instance, "-device: > no driver specified" becomes "Parameter 'driver' is missing". > Emitting such an error with our lengthy command lines is just too > mean to users. > > However, if you know *where* the parameter is missing, the generic > message is perfectly adequate: "-device a=b: Parameter 'driver' is > missing". In fact, it's even superior to our current message. > > So the first part of the patch series is about error locations. I > feel it's very useful all by itself. I can split it off into its > own patch series. But then the rest of this series depends on it, > so I'm not sure splitting is all that useful. > > We may still encounter cases where a generic message is not adequate > even with precise location information. Let's solve that problem > when we actually encounter it. > > * String argument with option syntax, i.e. NAME=VALUE,... > > QMP uses JSON to encode collections of name/value pairs. Adding a > second encoding for the same thing would be a mistake, in my > opinion. > > Note that we already have two competing encodings in our code: QDict > and QemuOpts. But we should not permit that to leak into an > external interface like QMP. > > QemuOpts originated in the command line and spread from there into a > few monitor commands, including device_add, and a few internal > interfaces. > > QDict originated in the monitor. It sits right at the interface > between monitor core and command handlers. > > My proposed solution is modest and pragmatic: > > * Lift the parsing of arguments into QemuOpts from monitor handlers > up into the human monitor core. This removes QemuOpts from the > handler interface, and thus avoids leaking it into QMP. It's > exactly what we did for other argument types with syntax > inappropriate for QMP, such as arguments of migrate_set_speed and > migrate_set_downtime (commits 9da92c49..b0fbf7d3). > > * Monitor handlers that need to pass their arguments in > QemuOpts-form to internal interfaces use a converter function to > translate from QDict to QemuOpts. > > This is what the last part of the patch series is about. If you'd > prefer a different solution, let's talk. > > I can split this part off into its own patch series if that helps. > However, the patches before it aren't all that useful without it, so > I'm not sure splitting buys us much. > > A possible alternative is to add the concept of optional named > arguments to the monitor. Instead of encoding multiple optional > named arguments in a single positional argument, we encode them as > multiple named arguments. For instance, "device_add > ide-drive,drive=hda,bus=ide.0,unit=0" becomes "device_add ide-drive > drive=hda bus=ide.0 unit=0". > > Of course, if you think that adding a second encoding for > collections of name/value pairs to QMP is fine, then this last part > can be dropped. > > So, the series starts with error locations (part I), and ends with > keeping QemuOpts out of QMP (part III). Wedged in between is the > conversion of device_add to QError (part II). In more detail: > > Part I: Error Locations > > [01-07] Preliminary cleanup & fixes > [08] Separate "default monitor" and "current monitor" cleanly > [09-16] More cleanup > [17-21] Error Locations > > Part II: Convert device_add to QError > > [22-25] Preliminary qdev cleanup & fixes > [26-42] Convert device_add to QError > > Part III > > [43] Conversions between QDict and QemuOpts > [44-46] New monitor argument type O > [47-48] Convert device_add to QObject > > I cut a few corners clearly marked in commit messages and code. I'll > fix them up for the non-RFC submit. I only read the patches in parts I and II, these look very good. There are some FIXME's there, but you know that yourself. -- MST