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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list