[libvirt] [PATCH 11/14] Support for JSON mode monitor

[Daniel P. Berrange]

On Thu, Dec 03, 2009 at 04:05:54PM +0100, Daniel Veillard wrote:
> On Thu, Nov 26, 2009 at 06:27:29PM +0000, Daniel P. Berrange wrote:
> > Initial support for the new QEMU monitor protocol  using JSON
> > as the data encoding format instead of plain text
> [...]
> > --- a/src/Makefile.am
> > +++ b/src/Makefile.am
> > @@ -188,6 +188,8 @@ QEMU_DRIVER_SOURCES =						\
> >  		qemu/qemu_monitor.c qemu/qemu_monitor.h		\
> >  		qemu/qemu_monitor_text.c			\
> >  		qemu/qemu_monitor_text.h			\
> > +		qemu/qemu_monitor_json.c			\
> > +		qemu/qemu_monitor_json.h			\
> >  		qemu/qemu_driver.c qemu/qemu_driver.h        	\
> >  		qemu/qemu_bridge_filter.c 			\
> 
>   Hum could you take the opportunity to cleanup the tab/space mix
> in the 2 following lines, above ?
> 
> [...]
> > @@ -283,9 +285,14 @@ qemuMonitorIOProcess(qemuMonitorPtr mon)
> >          msg = mon->msg;
> >  
> >      VIR_DEBUG("Process %d", (int)mon->bufferOffset);
> > -    len = qemuMonitorTextIOProcess(mon,
> > -                                   mon->buffer, mon->bufferOffset,
> > -                                   msg);
> > +    if (mon->json)
> > +        len = qemuMonitorJSONIOProcess(mon,
> > +                                       mon->buffer, mon->bufferOffset,
> > +                                       msg);
> > +    else
> > +        len = qemuMonitorTextIOProcess(mon,
> > +                                       mon->buffer, mon->bufferOffset,
> > +                                       msg);
> 
>   I have just one doubt here, assuming we have a json handle, is the
> text monitor still available. In which case should we try to fallback
> assuming the json interface get into troubles ? I assume if one fail
> then the other should fail too but ...

No, QEMU works in an either/or  mode - you can't have both active. 

> > +#define LINE_ENDING "\r\n"
> > +
> > +static int
> > +qemuMonitorJSONIOProcessLine(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
> > +                             const char *line,
> > +                             qemuMonitorMessagePtr msg)
> > +{
> > +    virJSONValuePtr obj = NULL;
> > +    int ret = -1;
> > +
> > +    VIR_DEBUG("Line [%s]", line);
> > +
> > +    if (!(obj = virJSONValueFromString(line))) {
> > +        VIR_DEBUG0("Parsing JSON string failed");
> > +        errno = EINVAL;
> > +        goto cleanup;
> > +    }
> > +
> > +    if (obj->type != VIR_JSON_TYPE_OBJECT) {
> > +        VIR_DEBUG0("Parsed JSON string isn't an object");
> > +        errno = EINVAL;
> > +    }
> > +
> > +    if (virJSONValueObjectHasKey(obj, "QMP") == 1) {
> > +        VIR_DEBUG0("Got QMP capabilities data");
> > +        ret = 0;
> > +        goto cleanup;
> > +    }
> > +
> > +    if (virJSONValueObjectHasKey(obj, "event") == 1) {
> > +        VIR_DEBUG0("Got an event");
> > +        ret = 0;
> > +        goto cleanup;
> > +    }
> > +
> > +    if (msg) {
> > +        msg->rxBuffer = strdup(line);
> > +        msg->rxLength = strlen(line);
> > +        msg->finished = 1;
> > +    } else {
> > +        VIR_DEBUG("Ignoring unexpected JSON message [%s]", line);
> > +    }
> > +
> > +    ret = 0;
> > +
> > +cleanup:
> > +    virJSONValueFree(obj);
> > +    return ret;
> > +}
> 
>   I must be missing something in that routine, we parse the json blob,
> get an object, check some of the object content and discard it, saving
> the raw text .... seems to me we dropped the actual parsed content
> instead of handling it, no ?

Yes this is a little bit of a wierd scenario, mostly an artifact of
the way we have code sharing between the text & json modes. The shared
I/O handling code simply works on char * buffers, and so we can't 
easily pass back the parsed JSON object at this point. So the object
here is only used for detecting events - it is reparsed later on in
the place which handles method replies/errors. A little inefficient
perhaps, but these messages are very small so its not really too bad

> > +    while ((key = va_arg(args, char *)) != NULL) {
> > +        int ret;
> > +        char type;
> > +
> > +        if (strlen(key) < 3) {
> > +            qemudReportError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> > +                             _("argument key '%s' is too short, missing type prefix"),
> > +                             key);
> > +            goto error;
> > +        }
> > +
> > +        /* Keys look like   s:name  the first letter is a type code */
> > +        type = key[0];
> > +        key += 2;
> 
>   Hum, we add a type info on top using prefixing ... weird but why not ...

I originally had 3 values per param in the callers, eg

   qemuMonitorJSONMakeCommand("eject",
                              "device, VIR_JSON_VALUE_STRING, "hda",
                              NULL)

but I thought it was getting far too verbose, so I switched to this
simple type format character

   qemuMonitorJSONMakeCommand("eject",
       	       	       	      "s:device, "hda",
                              NULL)


Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|