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 ...
[...]
> +#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 ?
> +int qemuMonitorJSONIOProcess(qemuMonitorPtr mon,
> + const char *data,
> + size_t len,
> + qemuMonitorMessagePtr msg)
> +{
> + int used = 0;
> + /*VIR_DEBUG("Data %d bytes [%s]", len, data);*/
> +
> + while (used < len) {
> + char *nl = strstr(data + used, LINE_ENDING);
> +
> + if (nl) {
> + int got = nl - (data + used);
> + char *line = strndup(data + used, got);
> + used += got + strlen(LINE_ENDING);
> + line[got] = '\0'; /* kill \n */
> + if (qemuMonitorJSONIOProcessLine(mon, line, msg) < 0) {
> + VIR_FREE(line);
> + return -1;
> + }
> +
> + VIR_FREE(line);
> + } else {
> + break;
> + }
> + }
Okay, I understand the problem of progressive parsing now... if
somehow they extend the json blob with some (error) string containing
LINE_ENDING we're stuck I'm afraid.
> + VIR_DEBUG("Total used %d bytes out of %d available in buffer", used, len);
> + return used;
> +}
> +static virJSONValuePtr
> +qemuMonitorJSONMakeCommand(const char *cmdname,
> + ...)
Hum, let's add a SENTINEL attribute !
> +{
> + virJSONValuePtr obj;
> + virJSONValuePtr jargs = NULL;
> + va_list args;
> + char *key;
> +
> + va_start(args, cmdname);
> +
> + if (!(obj = virJSONValueNewObject()))
> + goto no_memory;
> +
> + if (virJSONValueObjectAppendString(obj, "execute", cmdname) < 0)
> + goto no_memory;
> +
> + if (qemuMonitorJSONCommandAddTimestamp(obj) < 0)
> + goto error;
> +
> + 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 ...
> + if (!jargs &&
> + !(jargs = virJSONValueNewObject()))
> + goto no_memory;
> +
> + /* This doesn't supports maps/arrays. This hasn't
> + * proved to be a problem..... yet :-) */
> + switch (type) {
> + case 's': {
> + char *val = va_arg(args, char *);
> + ret = virJSONValueObjectAppendString(jargs, key, val);
> + } break;
> + case 'i': {
> + int val = va_arg(args, int);
> + ret = virJSONValueObjectAppendNumberInt(jargs, key, val);
> + } break;
> + case 'u': {
> + unsigned int val = va_arg(args, unsigned int);
> + ret = virJSONValueObjectAppendNumberUint(jargs, key, val);
> + } break;
> + case 'I': {
> + long long val = va_arg(args, long long);
> + ret = virJSONValueObjectAppendNumberLong(jargs, key, val);
> + } break;
> + case 'U': {
> + unsigned long long val = va_arg(args, unsigned long long);
> + ret = virJSONValueObjectAppendNumberUlong(jargs, key, val);
> + } break;
> + case 'd': {
> + double val = va_arg(args, double);
> + ret = virJSONValueObjectAppendNumberDouble(jargs, key, val);
> + } break;
> + case 'b': {
> + int val = va_arg(args, int);
> + ret = virJSONValueObjectAppendBoolean(jargs, key, val);
> + } break;
> + case 'n': {
> + ret = virJSONValueObjectAppendNull(jargs, key);
> + } break;
> + default:
> + qemudReportError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> + _("unsupported data type '%c' for arg '%s'"), type, key - 2);
> + goto error;
> + }
> + if (ret < 0)
> + goto no_memory;
> + }
> +
> + if (jargs &&
> + virJSONValueObjectAppend(obj, "arguments", jargs) < 0)
> + goto no_memory;
> +
> + va_end(args);
> +
> + return obj;
> +
> +no_memory:
> + virReportOOMError(NULL);
> +error:
> + virJSONValueFree(obj);
> + virJSONValueFree(jargs);
> + va_end(args);
> + return NULL;
> +}
[...]
> +{
> + int ret;
> + virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("balloon",
> + "U:value", (unsigned long long)newmem,
I would rather drop alognment of args and fit into 80 cols.
[...]
good to see a more formal interface, now they just have to make it XML
and that would be perfect ! >:->
> diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
> new file mode 100644
> index 0000000..62a88c0
> --- /dev/null
> +++ b/src/qemu/qemu_monitor_json.h
> @@ -0,0 +1,156 @@
[...]
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index 677c5b4..c39cbbc 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -60,7 +60,7 @@ static int testCompareXMLToArgvFiles(const char *xml,
> goto fail;
>
> if (qemudBuildCommandLine(NULL, &driver,
> - vmdef, &monitor_chr, flags,
> + vmdef, &monitor_chr, 0, flags,
> &argv, &qenv,
> NULL, NULL, migrateFrom) < 0)
> goto fail;
As pointed earlier by Matthias it would be good to have some kind
of regression testing for JSON code, maybe not down to QEmu actual
format but at least parsing and serializing the common stuff.
ACK
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list