9:28 a.m.
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 :|