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

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)