May 2011 - Devel - libvirt List Archives

[libvirt] [PATCH] maint: detect clang 2.9
by Eric Blake 04 May '11

04 May '11

In Fedora 15, with clang 2.8, 'scan-build env' shows: CCC_ANALYZER_ANALYSIS=-analyzer-check-objc-mem -analyzer-check-security-syntactic -analyzer-check-dead-stores -analyzer-check-objc-unused-ivars -analyzer-check-objc-methodsigs But in rawhide, with clang 2.9, the same variable is set but empty, implying the default set of analysis. We still want sa_assert defined in that case, to stop clang from hitting false positives. * configure.ac (STATIC_ANALYSIS): Detect clang even when the set of analyses is the default. --- I'm pushing this under the trivial rule. It has no effect on a real build, but makes it easier for me to repeat my clang testing on rawhide. configure.ac | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/configure.ac b/configure.ac index 7c68bca..314a594 100644 --- a/configure.ac +++ b/configure.ac @@ -2297,7 +2297,7 @@ cp -f COPYING.LIB COPYING # Detect when running under the clang static analyzer's scan-build driver # or Coverity-prevent's cov-build. Define STATIC_ANALYSIS accordingly. -test -n "$CCC_ANALYZER_ANALYSIS$COVERITY_BUILD_COMMAND" && t=1 || t=0 +test -n "${CCC_ANALYZER_ANALYSIS+set}$COVERITY_BUILD_COMMAND" && t=1 || t=0 AC_DEFINE_UNQUOTED([STATIC_ANALYSIS], [$t], [Define to 1 when performing static analysis.]) -- 1.7.4.4

1 0

[libvirt] [PATCH] storage: avoid null deref and leak on failure
by Eric Blake 04 May '11

04 May '11

Detected by clang. NULL deref added in commit 343a27a (Mar 11), but leak of voldef present since commit 2cd9b2d (Apr 09). * src/storage/storage_driver.c (storageVolumeCreateXML): Don't leak voldef or dereference null volobj. --- src/storage/storage_driver.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 1ea5d12..19c7d63 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1319,8 +1319,10 @@ storageVolumeCreateXML(virStoragePoolPtr obj, pool->volumes.objs[pool->volumes.count++] = voldef; volobj = virGetStorageVol(obj->conn, pool->def->name, voldef->name, voldef->key); + if (!volobj) + goto cleanup; - if (volobj && backend->buildVol) { + if (backend->buildVol) { int buildret; virStorageVolDefPtr buildvoldef = NULL; -- 1.7.4.4

2 5

[libvirt] qemu qxl video memory parameter - windows bsod with spice
by Emre Erenoglu 04 May '11

04 May '11

Hi, Please see the below discussion with the spice community. libvirt is adding a parameter about video ram to qemu process and this parameter might be responsible of a crash in the qxl at higher resolutions. Spice people tell me that their default ram should be 64 Mbytes. I'm using libvirt 0.9.0 and qemu 0.14, virt-manager 0.8.7. When configuring a guest in virt-manager, cirrus driver is added as default, then you can change it to qxl, but the 9 MB videoram parameter can't be adjusted and it does not automatically change to 64 MB. If you add a 2nd qxl video card, then it has 64 MB memory. The parameter that qemu gets with one qxl device is: -vga qxl -global qxl-vga.vram_size=9437184 The parameter that qemu gets with a second qxl device is: -vga qxl -global qxl-vga.vram_size=9437184 -device qxl,id=video1,vram_size=67108864,bus=pci.0,addr=0x8 Is this a bug in libvirt or other related tools? Thanks, Br, Emre ---------- Forwarded message ---------- From: Emre Erenoglu <erenoglu(a)gmail.com> Date: Wed, May 4, 2011 at 7:42 PM Subject: Re: [Spice-devel] windows bsod with spice 0.8.1 To: Emre Erenoglu <erenoglu(a)gmail.com>, spice-devel(a)freedesktop.org On Wed, May 4, 2011 at 6:00 PM, Alon Levy <alevy(a)redhat.com> wrote: > On Wed, May 04, 2011 at 05:10:31PM +0400, Emre Erenoglu wrote: > > On Wed, May 4, 2011 at 2:12 PM, Alon Levy <alevy(a)redhat.com> wrote: > > > > > On Wed, May 04, 2011 at 01:45:39PM +0400, Emre Erenoglu wrote: > > > > On Wed, Apr 27, 2011 at 1:16 AM, Emre Erenoglu <erenoglu(a)gmail.com> > > > wrote: > > > > > > > > > Hi, > > > > > > > > > > I started to see frequent crashes with my guests running windows xp > and > > > > > windows server 2003 x86. The bsod appears on qxldd.dll. I can give > > > further > > > > > info if needed. > > > > > > > > > > The host is Linux x86_64, qemu 0.14, spice 0.8.1. Guest has qxl > 0.6.1 > > > > > drivers loaded. I may revert back to 0.8.0 since it was not > crashing. > > > > > Nothing changed in the guests when I upgraded spice from 0.8.0 to > > > 0.8.1. > > > > > > > > > > > > > > Some updates on the original issue. I checked a lot and I found out > that > > > the > > > > issue continued with any version of spice and guest qxl drivers. > > > > > > > > I finally digged down to the point that, the qemu comment line which > > > states > > > > " -global qxl-vga.vram_size=9xxxxxx " parameter is the problem. > When > > > the > > > > guest is started with 9 MB of video memory given as comment line > > > parameter > > > > to qemu, the guest qxldd driver crashes with BSOD. > > > > > > Why was it started with 9MB? anyway, good catch, we should add a check > for > > > this > > > in the driver (and probably just refuse to load). Could you open a bug > on > > > this > > > in bugzilla.freedesktop.org? > > > > > > > This is added by libvirt (virt-manager) when launching qemu. Do we need > to > > file it against libvirt or against spice? I will post this to the > libvirt > > list also. > > > > Btw, rather than refusing to load, I think we should just limit the > possible > > resolutions. I'm not sure if the resolutions are the issue though. The > crash > > was not happening immediately and it was working OK with that 9 MB > memory. > > When I launched outlook, or IE8, or just in the middle of an application, > it > > could crash and bsod. > > But that number doesn't make any sense - the spice default is 64MB, we > never tested > with less then that. It is probably fixable like you say, but until that's > fixed why > not stick to the spice default? Well, this parameter is not configurable from the virt-manager UI and it configures 9 MB to the command line of qemu by libvirt (even latest 0.9.0) version. When you add a 2nd VGA adapter, it adds 64 MB to that adapter correctly (-device qxl,id=video1,vram_size=67108864,bus=pci.0,addr=0x8) I guess i need to file a bug report to libvirt too. Emre

4 5

[libvirt] [PATCH] esx: avoid null dereference on error
by Eric Blake 04 May '11

04 May '11

Detected by clang. * src/esx/esx_driver.c (esxDomainGetInfo): Fail early on error. --- src/esx/esx_driver.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 1f8f90b..e929208 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -2372,8 +2372,9 @@ esxDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info) if (perfEntityMetric == NULL) { VIR_ERROR(_("QueryPerf returned object with unexpected type '%s'"), esxVI_Type_ToString(perfEntityMetricBase->_type)); + goto cleanup; } perfMetricIntSeries = esxVI_PerfMetricIntSeries_DynamicCast(perfEntityMetric->value); -- 1.7.4.4

4 4

[libvirt] [PATCH] remote: avoid null dereference on error
by Eric Blake 04 May '11

04 May '11

Clang found three instances of uninitialized use of nparams in the cleanup path. Unfortunately, one is a false positive: clang couldn't see that ret->params.params_val is guaranteed to be NULL unless allocated within a function, and that nparams is guaranteed to be assigned prior to the allocation; hoisting the assignment to nparams to be earlier in the function shuts up that false positive. But two of the reports also happened to highlight a real bug - the error path can dereference NULL. * daemon/remote.c (remoteDispatchDomainGetMemoryParameters) (remoteDispatchDomainGetBlkioParameters): Don't clear fields if array was not allocated. (remoteDispatchDomainGetSchedulerParameters): Initialize nparams earlier. --- daemon/remote.c | 29 ++++++++++++++++------------- 1 files changed, 16 insertions(+), 13 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index eedbc77..214f7a4 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -886,7 +886,8 @@ remoteDispatchDomainGetSchedulerParameters(struct qemud_server *server ATTRIBUTE { virDomainPtr dom = NULL; virSchedParameterPtr params = NULL; - int i, nparams; + int i; + int nparams = args->nparams; int rv = -1; if (!conn) { @@ -894,8 +895,6 @@ remoteDispatchDomainGetSchedulerParameters(struct qemud_server *server ATTRIBUTE goto cleanup; } - nparams = args->nparams; - if (nparams > REMOTE_DOMAIN_SCHEDULER_PARAMETERS_MAX) { virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); goto cleanup; @@ -2970,7 +2969,8 @@ remoteDispatchDomainGetMemoryParameters(struct qemud_server *server { virDomainPtr dom = NULL; virMemoryParameterPtr params = NULL; - int i, nparams; + int i; + int nparams = args->nparams; unsigned int flags; int rv = -1; @@ -2979,7 +2979,6 @@ remoteDispatchDomainGetMemoryParameters(struct qemud_server *server goto cleanup; } - nparams = args->nparams; flags = args->flags; if (nparams > REMOTE_DOMAIN_MEMORY_PARAMETERS_MAX) { @@ -3060,9 +3059,11 @@ success: cleanup: if (rv < 0) { remoteDispatchError(rerr); - for (i = 0; i < nparams; i++) - VIR_FREE(ret->params.params_val[i].field); - VIR_FREE(ret->params.params_val); + if (ret->params.params_val) { + for (i = 0; i < nparams; i++) + VIR_FREE(ret->params.params_val[i].field); + VIR_FREE(ret->params.params_val); + } } if (dom) virDomainFree(dom); @@ -3186,7 +3187,8 @@ remoteDispatchDomainGetBlkioParameters(struct qemud_server *server { virDomainPtr dom = NULL; virBlkioParameterPtr params = NULL; - int i, nparams; + int i; + int nparams = args->nparams; unsigned int flags; int rv = -1; @@ -3195,7 +3197,6 @@ remoteDispatchDomainGetBlkioParameters(struct qemud_server *server goto cleanup; } - nparams = args->nparams; flags = args->flags; if (nparams > REMOTE_DOMAIN_BLKIO_PARAMETERS_MAX) { @@ -3276,9 +3277,11 @@ success: cleanup: if (rv < 0) { remoteDispatchError(rerr); - for (i = 0; i < nparams; i++) - VIR_FREE(ret->params.params_val[i].field); - VIR_FREE(ret->params.params_val); + if (ret->params.params_val) { + for (i = 0; i < nparams; i++) + VIR_FREE(ret->params.params_val[i].field); + VIR_FREE(ret->params.params_val); + } } VIR_FREE(params); if (dom) -- 1.7.4.4

2 3

[libvirt] [PATCH] esx: remove dead store
by Eric Blake 04 May '11

04 May '11

Detected by clang. * src/esx/esx_util.c (esxUtil_ParseDatastorePath): No need to increment. --- src/esx/esx_util.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/esx/esx_util.c b/src/esx/esx_util.c index 9ef947c..640e984 100644 --- a/src/esx/esx_util.c +++ b/src/esx/esx_util.c @@ -2,7 +2,7 @@ /* * esx_util.c: utility functions for the VMware ESX driver * - * Copyright (C) 2010 Red Hat, Inc. + * Copyright (C) 2010-2011 Red Hat, Inc. * Copyright (C) 2009 Matthias Bolte <matthias.bolte(a)googlemail.com> * Copyright (C) 2009 Maximilian Wilhelm <max(a)rfc2324.org> * @@ -332,7 +332,7 @@ esxUtil_ParseDatastorePath(const char *datastorePath, char **datastoreName, preliminaryFileName = strrchr(preliminaryDirectoryAndFileName, '/'); if (preliminaryFileName != NULL) { - *preliminaryFileName++ = '\0'; + *preliminaryFileName = '\0'; } if (esxVI_String_DeepCopyValue(directoryName, -- 1.7.4.4

3 4

[libvirt] [PATCH] cgroup: avoid leaking a file
by Eric Blake 04 May '11

04 May '11

Clang detected a dead store to rc. It turns out that in fixing this, I also found a FILE* leak. * src/util/cgroup.c (virCgroupKillInternal): Abort rather than resuming loop on fscanf failure, and cleanup file on error. --- As a bonus, this also fixes a decl-after-statement for fp. src/util/cgroup.c | 9 ++++++--- 1 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/util/cgroup.c b/src/util/cgroup.c index afe8731..62b371d 100644 --- a/src/util/cgroup.c +++ b/src/util/cgroup.c @@ -1351,7 +1351,9 @@ static int virCgroupKillInternal(virCgroupPtr group, int signum, virHashTablePtr int killedAny = 0; char *keypath = NULL; bool done = false; - VIR_DEBUG("group=%p path=%s signum=%d pids=%p", group, group->path, signum, pids); + FILE *fp = NULL; + VIR_DEBUG("group=%p path=%s signum=%d pids=%p", + group, group->path, signum, pids); rc = virCgroupPathOfController(group, -1, "tasks", &keypath); if (rc != 0) { @@ -1364,7 +1366,6 @@ static int virCgroupKillInternal(virCgroupPtr group, int signum, virHashTablePtr */ while (!done) { done = true; - FILE *fp; if (!(fp = fopen(keypath, "r"))) { rc = -errno; VIR_DEBUG("Failed to read %s: %m\n", keypath); @@ -1376,7 +1377,8 @@ static int virCgroupKillInternal(virCgroupPtr group, int signum, virHashTablePtr if (feof(fp)) break; rc = -errno; - break; + VIR_DEBUG("Failed to read %s: %m\n", keypath); + goto cleanup; } if (virHashLookup(pids, (void*)pid)) continue; @@ -1403,6 +1405,7 @@ static int virCgroupKillInternal(virCgroupPtr group, int signum, virHashTablePtr cleanup: VIR_FREE(keypath); + VIR_FORCE_FCLOSE(fp); return rc; } -- 1.7.4.4

3 5

[libvirt] [PATCH] qemu: update qemuCgroupControllerActive signature
by Eric Blake 04 May '11

04 May '11

Clang warned about a dead assignment. In the process, I noticed that we are only using the function for a bool value. I audited all other callers in qemu_{migration,cgroup,driver,hotplug), and all were making the call in a bool context. * src/qemu/qemu_cgroup.c (qemuSetupCgroup): Delete dead assignment. (qemuCgroupControllerActive): Change return type to bool. * src/qemu/qemu_cgroup.h (qemuCgroupControllerActive): Likewise. --- src/qemu/qemu_cgroup.c | 14 +++++++------- src/qemu/qemu_cgroup.h | 4 ++-- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 7e88a67..ac1c016 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -43,16 +43,16 @@ static const char *const defaultDeviceACL[] = { #define DEVICE_PTY_MAJOR 136 #define DEVICE_SND_MAJOR 116 -int qemuCgroupControllerActive(struct qemud_driver *driver, - int controller) +bool qemuCgroupControllerActive(struct qemud_driver *driver, + int controller) { if (driver->cgroup == NULL) - return 0; + return false; if (!virCgroupMounted(driver->cgroup, controller)) - return 0; + return false; if (driver->cgroupControllers & (1 << controller)) - return 1; - return 0; + return true; + return false; } static int @@ -312,7 +312,7 @@ int qemuSetupCgroup(struct qemud_driver *driver, if (vm->def->mem.hard_limit != 0 || vm->def->mem.soft_limit != 0 || vm->def->mem.swap_hard_limit != 0) { - if ((rc = qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_MEMORY))) { + if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_MEMORY)) { if (vm->def->mem.hard_limit != 0) { rc = virCgroupSetMemoryHardLimit(cgroup, vm->def->mem.hard_limit); if (rc != 0) { diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h index 299bd2d..e8abfb4 100644 --- a/src/qemu/qemu_cgroup.h +++ b/src/qemu/qemu_cgroup.h @@ -34,8 +34,8 @@ struct _qemuCgroupData { }; typedef struct _qemuCgroupData qemuCgroupData; -int qemuCgroupControllerActive(struct qemud_driver *driver, - int controller); +bool qemuCgroupControllerActive(struct qemud_driver *driver, + int controller); int qemuSetupDiskCgroup(struct qemud_driver *driver, virDomainObjPtr vm, virCgroupPtr cgroup, -- 1.7.4.4

2 2

[libvirt] [PATCH] lxc: report correct error
by Eric Blake 04 May '11

04 May '11

Clang noticed a dead assignment, which turned out to be the use of the wrong variable. rc starts life as -1, and is only ever assigned to 0 just before a successful cleanup. * src/lxc/lxc_driver.c (lxcSetupInterfaces): Don't call virReportSystemError(-1). --- src/lxc/lxc_driver.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index e905302..b94941d 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1100,10 +1100,9 @@ static int lxcSetupInterfaces(virConnectPtr conn, } if ((ret = brAddInterface(brctl, bridge, parentVeth)) != 0) { - virReportSystemError(rc, + virReportSystemError(ret, _("Failed to add %s device to %s"), parentVeth, bridge); - rc = -1; goto error_exit; } -- 1.7.4.4

2 2

[libvirt] [PATCH] util: remove dead assignment
by Eric Blake 04 May '11

04 May '11

Clang complained about this, and it was easy enough to fix. * src/util/util.c (virFileOpenAs): Drop dead assignment. --- src/util/util.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) diff --git a/src/util/util.c b/src/util/util.c index 37472bb..9041ab6 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -1523,8 +1523,7 @@ virFileOpenAs(const char *path, int openflags, mode_t mode, if (ret < 0 && errno != EACCES) { ret = -errno; VIR_FORCE_CLOSE(pair[0]); - while ((waitret = waitpid(pid, NULL, 0) == -1) - && (errno == EINTR)); + while (waitpid(pid, NULL, 0) == -1 && errno == EINTR); goto parenterror; } else { fd = ret; -- 1.7.4.4

2 2