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

[Daniel Veillard]

On Thu, Dec 03, 2009 at 03:28:02PM +0000, Daniel P. Berrange wrote:
> 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. 

  okay

> > > +#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

  Okay, double parsing but fine, I just wanted to make sure :-)

> > > +    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)

  Yup that's nicer :-)

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/