December 2013 - CIM - libvirt List Archives

[PATCH] VSMS: fv_vssd_to_domain() resolve Coverity error
by John Ferlan 22 Jan '14

22 Jan '14

Coverity discovered that the free(domain->os_info.fv.arch) and then usage later on during get_default_machine() and get_default_emulator() calls could result in using free()'d memory. If the 'cu_get_str_prop() failed or capsinfo == NULL, then the fv.arch wouldn't necessarily be strdup()'d. Passing a NULL os_info.fv_arch into the get*() API's is fine since they'll call findDomainInfo() which can handle a NULL arch value. Also added an initialization of val just to be safe. I don't think it's necessary though. --- NOTE: I found this during a Coverity run applying the endianness patches. For some reason Coverity "woke up" and saw this even though it hasn't found this issue in a couple months of runs since the changes to this module were made. See commit id '117dabb9'. src/Virt_VirtualSystemManagementService.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Virt_VirtualSystemManagementService.c b/src/Virt_VirtualSystemManagementService.c index d51f230..9f8b5b9 100644 --- a/src/Virt_VirtualSystemManagementService.c +++ b/src/Virt_VirtualSystemManagementService.c @@ -464,7 +464,7 @@ static int fv_vssd_to_domain(CMPIInstance *inst, { int ret = 1; int retr; - const char *val; + const char *val = NULL; const char *domtype = NULL; const char *ostype = "hvm"; struct capabilities *capsinfo = NULL; @@ -494,6 +494,7 @@ static int fv_vssd_to_domain(CMPIInstance *inst, } free(domain->os_info.fv.arch); + domain->os_info.fv.arch = NULL; retr = cu_get_str_prop(inst, "Arch", &val); if (retr != CMPI_RC_OK) { if (capsinfo != NULL) { /* set default */ -- 1.8.3.1

2 2

[PATCH] PanicCheckABIStability: Need to check for existence
by John Ferlan 20 Dec '13

20 Dec '13

Commit id '4313fead' added a call to virDomainPanicCheckABIStability() which did not check whether the panic device existed before making a call to virDomainDeviceInfoCheckABIStability() which ended up segfaulting: Thread 1 (Thread 0x7f5332837700 (LWP 10964)): (src=<optimized out>, dst=<optimized out>) at conf/domain_conf.c:13007 (dst=<optimized out>, src=<optimized out>) at conf/domain_conf.c:13712 (src=<optimized out>, dst=<optimized out>) at conf/domain_conf.c:14056 (domain=domain@entry=0x7f53000057c0, vm=vm@entry=0x7f53000036d0, defptr=defptr@entry=0x7f5332836978, snap=snap@entry=0x7f5332836970, update_current=update_current@entry=0x7f5332836962, flags=flags@entry=1) at conf/snapshot_conf.c:1230 (domain=0x7f53000057c0, xmlDesc=<optimized out>, flags=1) at qemu/qemu_driver.c:12719 (domain=domain@entry=0x7f53000057c0, xmlDesc=0x7f53000081d0 "<domainsnapshot>\n <name>snap2</name>\n <description>new-desc</description>\n <state>running</state>\n <parent>\n <name>snap1</name>\n </parent>\n <creationTime>1387487268</creationTime>\n <memory s"..., flags=1) at libvirt.c:19695 ... (gdb) up 3 (gdb) print *other->def->dom $2 = {virtType = 2, id = -1, .. ... rng = 0x0, panic = 0x0, namespaceData = 0x0,... ... (gdb) print *def->dom $3 = {virtType = 2, id = -1, ... ... rng = 0x0, panic = 0x0, namespaceData = 0x0,... ... (gdb) Signed-off-by: John Ferlan <jferlan(a)redhat.com> --- NOTE: Optionally the call could be changed to: "if (src->panic && !virDomainPanicCheckABIStability(...)" I went with what I did following the RNGDefCheckABIStability. src/conf/domain_conf.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0079234..c86af9a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13709,6 +13709,9 @@ static bool virDomainPanicCheckABIStability(virDomainPanicDefPtr src, virDomainPanicDefPtr dst) { + if (!src && !dst) + return true; + return virDomainDeviceInfoCheckABIStability(&src->info, &dst->info); } -- 1.8.3.1

1 1

[PATCH 00/20] REWORK/PARIAL: Changes to solve unsupported tag issue
by John Ferlan 13 Dec '13

13 Dec '13

This is a *partial rework* of Xu Wang's patches sent last month: https://www.redhat.com/archives/libvirt-cim/2013-October/msg00081.html Although not the complete set of changes - it's a good stopping point insomuch as it handles the "others" parsing. If this looks good, I can push it, then work through the changes to write the xml. I have run all the changes through cimtest - even with the patches on the list from Viktor. No new issues are found. Changes to the original patches 1. I rebased to top of tree as of today (11/14/13) 2. I reworked each of the patches to only add the 'others' to the particular *_device structure. This was done to be able to show the progression and to ensure I didn't forget something 2a. This found that there needed to be a cleanup_vcpu_device() and cleanup_mem_device() since both added "others" to their structure but were never cleaned up during cleanup_virt_device() 2b. I did not see a need for "others" in "vnc_device" and "sdl_device" 2c. I added a cleanup_others() call in _get_dominfo() 3. I added fetch_device_address_from_others() to replace parse_device_address(). This bridges the "gap" bewteen the parsing of the <address> tag that was recently added and the 'others' parsing code which didn't handle it. All I did was traverse the others list looking for parent name and id match for TYPE_PROP entries. Essentially anything inside of <address...> us then copied in to name/value structure managed by devaddr. Once the last caller of parse_device_address() was removed, that function went away 4. I changed the type for unknown from CIM_RES_TYPE_UNKNOWN to CIM_RES_TYPE_UNKDEV. Turns out the former is used in other contexts and if/when cleanup_virt_device() was called from one of those contexts the result was a core in cimprovagt. This was seen in the cimtest for VirtualSystemManagementService 16_removeresource.py. 4. Various "bug fixes" based on a coverity run and what I saw in code review from Boris. - Original code had a "ddev->disk_type == DISK_PHY;" - There was a "if (node->name == NULL)" check in parse_graphics_device after "else if (STREQC(gdev->type, "pty")) {" which coverity flagged as unnecessary since earlier code assumed node->name != NULL - In parse_os() there was a "STREQC(dominfo->os_info.pv.type, "linux")" which needed a "dominfo->os_info.pv.type &&" prior to it since the value could have been NULL according to a check earlier in the routine - I added the "free(dev->name);" to cleanup_unknown_device() - Checks for others->{name|parent_name|value} were removed. Since the structures are calloc()'d and free(NULL) does nothing, this is OK. Xu Wang (20): Add others member for saving unsupported tag and unknown device Add others and unknown_device clean up Add basic operations for reading data from xml node Fix xml parsing algorithm for parse_fs_device() Fix xml parsing algorithm for parse_block_device() Fix xml parsing algorithm for parse_vsi_device() Fix xml parsing algorithm for parse_net_device() Fix xml parsing algorithm for parse_vcpu_device() Fix xml parsing algorithm for parse_emu_device() Fix xml parsing algorithm for parse_mem_device() Fix xml parsing algorithm for parse_console_device() Fix xml parsing algorithm for parse_graphics_device() Fix xml parsing algorithm for parse_input_device() Add parse_unknown_device() Add parse_devices() for unknown type in get_dominfo_from_xml() Fix xml parsing algorithm in parse_domain() Fix xml parsing algorithm in parse_os() Fix xml parsing algorithm in parse_features() Add dup function for device copy Add type CIM_RES_TYPE_DELETED and modify type as it after resource_del libxkutil/device_parsing.c | 2055 ++++++++++++++++++++++++----- libxkutil/device_parsing.h | 58 + src/Virt_VirtualSystemManagementService.c | 2 +- src/svpc_types.h | 2 + 4 files changed, 1774 insertions(+), 343 deletions(-) -- 1.8.3.1

5 36