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/ dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list