On Wed, 24 Feb 2010 18:55:12 +0100 Markus Armbruster <arm...@redhat.com> 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. This looks reasonable to me. When we started discussing possible solutions, we considered moving the description table to its own module and supporting error message override. The solution in this series is a bit simpler, but it does the job and adding message override support later shouldn't be difficult, if needed. > 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't think of anything better, at least not for the short term. However, I'm wondering if exposing a monster command like device_add is the right thing to do for QMP. We didn't expose 'info', for example, because having a command (or 'remote procedure') returning all sorts of possible data is no good. We mapped each 'info foo' command to a 'query' command instead. This is also natural for info commands, as they implemented separately. So, I'm wondering if the same argument applies for device_add, as we're going to have a large number of devices, each of them with their own properties. The other possible solution is to introduce sets of commands to add specific devices, but this might not be doable at all as we're moving away from this approach with qdev... We do need a way to add devices in the system through QMP ASAP, so we'll have to use device_add, anyway. Two questions: 1. Don't we need a 'query-devices' command? 2. How are we going to document all the accepted parameters? Note that as we're exposing them through QMP, they obviusly must not change (although this probably a requirement for the CLI as well)
