[libvirt] [PATCH] Remove invalid parsing of pty path from XML for PTY type chardevs
While running valgrind on the qemuhotplugtest, one of the many memory leaks detected are: ==3915== 12 bytes in 1 blocks are definitely lost in loss record 36 of 129 ==3915== at 0x4A0887C: malloc (vg_replace_malloc.c:270) ==3915== by 0x34268AF275: xmlStrndup (in /usr/lib64/libxml2.so.2.9.1) ==3915== by 0x4CCC3BB: virDomainChrSourceDefParseXML (domain_conf.c:6957) ==3915== by 0x4CD6A29: virDomainChrDefParseXML (domain_conf.c:7243) ==3915== by 0x4CEC0C9: virDomainDeviceDefParse (domain_conf.c:9616) ==3915== by 0x41D14F: testQemuHotplug (qemuhotplugtest.c:247) ==3915== by 0x41E421: virtTestRun (testutils.c:139) ==3915== by 0x41C6E3: mymain (qemuhotplugtest.c:428) ==3915== by 0x41EAB2: virtTestMain (testutils.c:600) ==3915== by 0x341F421A04: (below main) (libc-start.c:225) ==3915== The main reason behind this error is that for the PTY type chardevs, it is not valid to specify or parse a pty path from the XML. This is an output-only attribute, not under user control. Hence, the parsing code inside qemuMonitorJSONAttachCharDev() was wrong. Refer the discussion: https://www.redhat.com/archives/libvir-list/2013-November/msg01255.html tests/qemumonitorjsontest.c * Remove unwanted parsing code inside qemuMonitorJSONAttachCharDev src/qemu/qemu_monitor_json.c: * Remove erraneous testcase --- src/qemu/qemu_monitor_json.c | 20 -------------------- tests/qemumonitorjsontest.c | 16 ---------------- 2 files changed, 36 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index ec3b958..1bfeef0 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -5368,26 +5368,6 @@ qemuMonitorJSONAttachCharDev(qemuMonitorPtr mon, if (qemuMonitorJSONCheckError(cmd, reply) < 0) goto cleanup; - if (chr->type == VIR_DOMAIN_CHR_TYPE_PTY) { - virJSONValuePtr data; - const char *path; - - if (!(data = virJSONValueObjectGet(reply, "return"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("chardev-add reply was missing return data")); - goto cleanup; - } - - if (!(path = virJSONValueObjectGetString(data, "pty"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("chardev-add reply was missing pty path")); - goto cleanup; - } - - if (VIR_STRDUP(chr->data.file.path, path) < 0) - goto cleanup; - } - ret = 0; cleanup: diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 2152e4a..a3c38b6 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -708,22 +708,6 @@ testQemuMonitorJSONAttachChardev(const void *data) chr = (virDomainChrSourceDef) { .type =VIR_DOMAIN_CHR_TYPE_VC }; CHECK("chr_vc", "{\"return\": {}}"); -#define PTY_PATH "/dev/ttyS0" - chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_PTY }; - CHECK("chr_pty", "{\"return\": {\"pty\" : \"" PTY_PATH "\"}}"); - if (STRNEQ_NULLABLE(PTY_PATH, chr.data.file.path)) { - VIR_FREE(chr.data.file.path); - virReportError(VIR_ERR_INTERNAL_ERROR, - "expected PTY path: %s got: %s", - PTY_PATH, NULLSTR(chr.data.file.path)); - ret = -1; - } - VIR_FREE(chr.data.file.path); - - chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_PTY }; - CHECK_FAIL("chr_pty_fail", "{\"return\": {}}"); -#undef PTY_PATH - chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_FILE }; CHECK("chr_file", "{\"return\": {}}"); -- 1.8.1.4
On Thu, Nov 28, 2013 at 12:17:31AM +0530, Nehal J Wani wrote:
While running valgrind on the qemuhotplugtest, one of the many memory leaks detected are:
==3915== 12 bytes in 1 blocks are definitely lost in loss record 36 of 129 ==3915== at 0x4A0887C: malloc (vg_replace_malloc.c:270) ==3915== by 0x34268AF275: xmlStrndup (in /usr/lib64/libxml2.so.2.9.1) ==3915== by 0x4CCC3BB: virDomainChrSourceDefParseXML (domain_conf.c:6957) ==3915== by 0x4CD6A29: virDomainChrDefParseXML (domain_conf.c:7243) ==3915== by 0x4CEC0C9: virDomainDeviceDefParse (domain_conf.c:9616) ==3915== by 0x41D14F: testQemuHotplug (qemuhotplugtest.c:247) ==3915== by 0x41E421: virtTestRun (testutils.c:139) ==3915== by 0x41C6E3: mymain (qemuhotplugtest.c:428) ==3915== by 0x41EAB2: virtTestMain (testutils.c:600) ==3915== by 0x341F421A04: (below main) (libc-start.c:225) ==3915==
The main reason behind this error is that for the PTY type chardevs, it is not valid to specify or parse a pty path from the XML. This is an output-only attribute, not under user control. Hence, the parsing code inside qemuMonitorJSONAttachCharDev() was wrong.
Refer the discussion: https://www.redhat.com/archives/libvir-list/2013-November/msg01255.html
tests/qemumonitorjsontest.c * Remove unwanted parsing code inside qemuMonitorJSONAttachCharDev
src/qemu/qemu_monitor_json.c: * Remove erraneous testcase
--- src/qemu/qemu_monitor_json.c | 20 -------------------- tests/qemumonitorjsontest.c | 16 ---------------- 2 files changed, 36 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index ec3b958..1bfeef0 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -5368,26 +5368,6 @@ qemuMonitorJSONAttachCharDev(qemuMonitorPtr mon, if (qemuMonitorJSONCheckError(cmd, reply) < 0) goto cleanup;
- if (chr->type == VIR_DOMAIN_CHR_TYPE_PTY) { - virJSONValuePtr data; - const char *path; - - if (!(data = virJSONValueObjectGet(reply, "return"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("chardev-add reply was missing return data")); - goto cleanup; - } - - if (!(path = virJSONValueObjectGetString(data, "pty"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("chardev-add reply was missing pty path")); - goto cleanup; - } - - if (VIR_STRDUP(chr->data.file.path, path) < 0) - goto cleanup; - } - ret = 0;
NACK, this is completely wrong. I said the *XML parsing* was likely to be wrong. This is the QEMU monitor code you've deleted. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
NACK, this is completely wrong. I said the *XML parsing* was likely to be wrong. This is the QEMU monitor code you've deleted.
Oh! Apologies for misunderstanding. If I am not wrong (again :'() , is this the code you are talking about? (inside virDomainChrSourceDefParseXML()) switch ((enum virDomainChrType) def->type) { case VIR_DOMAIN_CHR_TYPE_PTY: case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: case VIR_DOMAIN_CHR_TYPE_PIPE: case VIR_DOMAIN_CHR_TYPE_UNIX: /* PTY path is only parsed from live xml. */ if (!path && (def->type != VIR_DOMAIN_CHR_TYPE_PTY || !(flags & VIR_DOMAIN_XML_INACTIVE))) path = virXMLPropString(cur, "path"); break;
participants (2)
-
Daniel P. Berrange -
Nehal J Wani