[libvirt] clang vs. libvirt
I've been running the clang static analyzer against libvirt regularly, but it's usually been that of F12 or F13. Yesterday I used rawhide's version of clang: scan-build -o clang ./autogen.sh --enable-compile-warnings=error && scan-build -o clang make -j6 and it spotted a few new problems. Most are false positives, but a few were not. Here are the latest additions. I have an additional handful of patches that address the remaining false positives, and will post those later today.
From: Jim Meyering <meyering@redhat.com> * tools/virsh.c (vshCommandRun): Test only the initial value of ctl->timing, so that static analyzers don't have to consider that it might be changed by cmd->def->handler. --- tools/virsh.c | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index d5fe6c4..b2a1538 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -9411,16 +9411,17 @@ vshCommandRun(vshControl *ctl, const vshCmd *cmd) while (cmd) { struct timeval before, after; + bool enable_timing = ctl->timing; if ((ctl->conn == NULL) || (disconnected != 0)) vshReconnect(ctl); - if (ctl->timing) + if (enable_timing) GETTIMEOFDAY(&before); ret = cmd->def->handler(ctl, cmd); - if (ctl->timing) + if (enable_timing) GETTIMEOFDAY(&after); if (ret == FALSE) @@ -9440,7 +9441,7 @@ vshCommandRun(vshControl *ctl, const vshCmd *cmd) if (STREQ(cmd->def->name, "quit")) /* hack ... */ return ret; - if (ctl->timing) + if (enable_timing) vshPrint(ctl, _("\n(Time: %.3f ms)\n\n"), DIFF_MSEC(&after, &before)); else -- 1.7.1.rc1.248.gcefbb
On 04/14/2010 02:46 AM, Jim Meyering wrote:
From: Jim Meyering <meyering@redhat.com>
* tools/virsh.c (vshCommandRun): Test only the initial value of ctl->timing, so that static analyzers don't have to consider that it might be changed by cmd->def->handler. --- tools/virsh.c | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index d5fe6c4..b2a1538 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -9411,16 +9411,17 @@ vshCommandRun(vshControl *ctl, const vshCmd *cmd)
while (cmd) { struct timeval before, after; + bool enable_timing = ctl->timing;
if ((ctl->conn == NULL) || (disconnected != 0)) vshReconnect(ctl);
- if (ctl->timing) + if (enable_timing) GETTIMEOFDAY(&before);
ret = cmd->def->handler(ctl, cmd);
- if (ctl->timing) + if (enable_timing)
ACK. No semantic change unless we had a weird handler(), in which case the new semantics are better. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
From: Jim Meyering <meyering@redhat.com> * src/openvz/openvz_driver.c (openvzGetProcessInfo): Reorganize so that unexpected /proc/vz/vestat content cannot make us use uninitialized variables. Without this change, an input line with a matching "readvps", but fewer than 4 numbers would result in our using at least "systime" uninitialized. --- src/openvz/openvz_driver.c | 30 +++++++++++++++--------------- 1 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index 95c4236..47004d6 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -1384,14 +1384,15 @@ static int openvzGetProcessInfo(unsigned long long *cpuTime, int vpsid) { int fd; char line[1024] ; unsigned long long usertime, systime, nicetime; - int readvps = 0, ret; + int readvps = vpsid + 1; /* ensure readvps is initially different */ + int ret; /* read statistic from /proc/vz/vestat. sample: Version: 2.2 - VEID user nice system uptime idle other.. - 33 78 0 1330 59454597 142650441835148 other.. - 55 178 0 5340 59424597 542650441835148 other.. + VEID user nice system uptime idle other.. + 33 78 0 1330 59454597 142650441835148 other.. + 55 178 0 5340 59424597 542650441835148 other.. */ if ((fd = open("/proc/vz/vestat", O_RDONLY)) == -1) @@ -1400,15 +1401,18 @@ Version: 2.2 /*search line with VEID=vpsid*/ while(1) { ret = openvz_readline(fd, line, sizeof(line)); - if(ret <= 0) + if (ret <= 0) break; - if (sscanf(line, "%d %llu %llu %llu", - &readvps, &usertime, &nicetime, &systime) != 4) - continue; - - if (readvps == vpsid) - break; /*found vpsid*/ + if (sscanf (line, "%d %llu %llu %llu", + &readvps, &usertime, &nicetime, &systime) == 4 + && readvps == vpsid) { /*found vpsid*/ + /* convert jiffies to nanoseconds */ + *cpuTime = (1000ull * 1000ull * 1000ull + * (usertime + nicetime + systime) + / (unsigned long long)sysconf(_SC_CLK_TCK)); + break; + } } close(fd); @@ -1418,10 +1422,6 @@ Version: 2.2 if (readvps != vpsid) /*not found*/ return -1; - /* convert jiffies to nanoseconds */ - *cpuTime = 1000ull * 1000ull * 1000ull * (usertime + nicetime + systime) - / (unsigned long long)sysconf(_SC_CLK_TCK); - return 0; } -- 1.7.1.rc1.248.gcefbb
On 04/14/2010 02:46 AM, Jim Meyering wrote:
From: Jim Meyering <meyering@redhat.com>
* src/openvz/openvz_driver.c (openvzGetProcessInfo): Reorganize so that unexpected /proc/vz/vestat content cannot make us use uninitialized variables. Without this change, an input line with a matching "readvps", but fewer than 4 numbers would result in our using at least "systime" uninitialized. --- src/openvz/openvz_driver.c | 30 +++++++++++++++--------------- 1 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index 95c4236..47004d6 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -1384,14 +1384,15 @@ static int openvzGetProcessInfo(unsigned long long *cpuTime, int vpsid) { int fd; char line[1024] ; unsigned long long usertime, systime, nicetime; - int readvps = 0, ret; + int readvps = vpsid + 1; /* ensure readvps is initially different */ + int ret;
- if (sscanf(line, "%d %llu %llu %llu", - &readvps, &usertime, &nicetime, &systime) != 4) - continue; - - if (readvps == vpsid) - break; /*found vpsid*/ + if (sscanf (line, "%d %llu %llu %llu", + &readvps, &usertime, &nicetime, &systime) == 4 + && readvps == vpsid) { /*found vpsid*/ + /* convert jiffies to nanoseconds */ + *cpuTime = (1000ull * 1000ull * 1000ull + * (usertime + nicetime + systime) + / (unsigned long long)sysconf(_SC_CLK_TCK)); + break; + }
ACK that the rewrite fixes the problem. However, there's still the issue that we're using sscanf in the first place, instead of virStrToLong_ull; do you want to prepare a followup patch, or shall I? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake wrote:
On 04/14/2010 02:46 AM, Jim Meyering wrote:
From: Jim Meyering <meyering@redhat.com>
* src/openvz/openvz_driver.c (openvzGetProcessInfo): Reorganize so that unexpected /proc/vz/vestat content cannot make us use uninitialized variables. Without this change, an input line with a matching "readvps", but fewer than 4 numbers would result in our using at least "systime" uninitialized. --- src/openvz/openvz_driver.c | 30 +++++++++++++++--------------- 1 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index 95c4236..47004d6 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -1384,14 +1384,15 @@ static int openvzGetProcessInfo(unsigned long long *cpuTime, int vpsid) { int fd; char line[1024] ; unsigned long long usertime, systime, nicetime; - int readvps = 0, ret; + int readvps = vpsid + 1; /* ensure readvps is initially different */ + int ret;
- if (sscanf(line, "%d %llu %llu %llu", - &readvps, &usertime, &nicetime, &systime) != 4) - continue; - - if (readvps == vpsid) - break; /*found vpsid*/ + if (sscanf (line, "%d %llu %llu %llu", + &readvps, &usertime, &nicetime, &systime) == 4 + && readvps == vpsid) { /*found vpsid*/ + /* convert jiffies to nanoseconds */ + *cpuTime = (1000ull * 1000ull * 1000ull + * (usertime + nicetime + systime) + / (unsigned long long)sysconf(_SC_CLK_TCK)); + break; + }
ACK that the rewrite fixes the problem. However, there's still the
Thanks for the review.
issue that we're using sscanf in the first place, instead of virStrToLong_ull; do you want to prepare a followup patch, or shall I?
I am in no big hurry on that front, at least not for this particular case, where the risk of bogus input is probably negligible.
From: Jim Meyering <meyering@redhat.com> * src/storage/storage_backend_fs.c (virStorageBackendFileSystemMount): Clang was not smart enough, and mistakenly reported that "options" could be used uninitialized. Initialize it. --- src/storage/storage_backend_fs.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index a83fc01..0b81c6c 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -287,7 +287,7 @@ virStorageBackendFileSystemIsMounted(virStoragePoolObjPtr pool) { static int virStorageBackendFileSystemMount(virStoragePoolObjPtr pool) { char *src; - char *options; + char *options = NULL; const char **mntargv; /* 'mount -t auto' doesn't seem to auto determine nfs (or cifs), -- 1.7.1.rc1.248.gcefbb
On 04/14/2010 02:46 AM, Jim Meyering wrote:
From: Jim Meyering <meyering@redhat.com>
* src/storage/storage_backend_fs.c (virStorageBackendFileSystemMount): Clang was not smart enough, and mistakenly reported that "options" could be used uninitialized. Initialize it.
I had to convince myself of your claim that it cannot be used uninitialized: int glusterfs = (pool->def->type == VIR_STORAGE_POOL_NETFS && pool->def->source.format == VIR_STORAGE_POOL_NETFS_GLUSTERFS); ... if (pool->def->type == VIR_STORAGE_POOL_NETFS) { if (pool->def->source.format == VIR_STORAGE_POOL_NETFS_GLUSTERFS) { if (virAsprintf(&options, "direct-io-mode=1") == -1) { ... if (glusterfs) { mntargv[option_index] = options; } Given 110 lines in between the declaration and the use, I agree with your patch to make our assertion obvious, even if we now have a dead store of NULL. ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake wrote:
On 04/14/2010 02:46 AM, Jim Meyering wrote:
From: Jim Meyering <meyering@redhat.com>
* src/storage/storage_backend_fs.c (virStorageBackendFileSystemMount): Clang was not smart enough, and mistakenly reported that "options" could be used uninitialized. Initialize it.
I had to convince myself of your claim that it cannot be used uninitialized:
int glusterfs = (pool->def->type == VIR_STORAGE_POOL_NETFS && pool->def->source.format == VIR_STORAGE_POOL_NETFS_GLUSTERFS); ... if (pool->def->type == VIR_STORAGE_POOL_NETFS) { if (pool->def->source.format == VIR_STORAGE_POOL_NETFS_GLUSTERFS) { if (virAsprintf(&options, "direct-io-mode=1") == -1) { ... if (glusterfs) { mntargv[option_index] = options; }
Given 110 lines in between the declaration and the use, I agree with your patch to make our assertion obvious, even if we now have a dead store of NULL. ACK.
Thanks. All pushed.
From: Jim Meyering <meyering@redhat.com> * src/storage/storage_backend_fs.c (virStorageBackendFileSystemMount): Use virAsprintf only when needed. In this case, strdup works fine. --- src/storage/storage_backend_fs.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 0b81c6c..c96c4f3 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -376,7 +376,7 @@ virStorageBackendFileSystemMount(virStoragePoolObjPtr pool) { if (pool->def->type == VIR_STORAGE_POOL_NETFS) { if (pool->def->source.format == VIR_STORAGE_POOL_NETFS_GLUSTERFS) { - if (virAsprintf(&options, "direct-io-mode=1") == -1) { + if ((options = strdup("direct-io-mode=1")) == NULL) { virReportOOMError(); return -1; } -- 1.7.1.rc1.248.gcefbb
On 04/14/2010 02:46 AM, Jim Meyering wrote:
From: Jim Meyering <meyering@redhat.com>
* src/storage/storage_backend_fs.c (virStorageBackendFileSystemMount): Use virAsprintf only when needed. In this case, strdup works fine. --- src/storage/storage_backend_fs.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 0b81c6c..c96c4f3 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -376,7 +376,7 @@ virStorageBackendFileSystemMount(virStoragePoolObjPtr pool) {
if (pool->def->type == VIR_STORAGE_POOL_NETFS) { if (pool->def->source.format == VIR_STORAGE_POOL_NETFS_GLUSTERFS) { - if (virAsprintf(&options, "direct-io-mode=1") == -1) { + if ((options = strdup("direct-io-mode=1")) == NULL) {
ACK - much lighter-weight. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
From: Jim Meyering <meyering@redhat.com> * src/esx/esx_vmx.c (esxVMX_GatherSCSIControllers): Do not dereference a NULL disk->driverName. We already detect this condition in another case. Check for it here, too. --- src/esx/esx_vmx.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/esx/esx_vmx.c b/src/esx/esx_vmx.c index 647e720..aed5cc1 100644 --- a/src/esx/esx_vmx.c +++ b/src/esx/esx_vmx.c @@ -570,11 +570,12 @@ esxVMX_GatherSCSIControllers(virDomainDefPtr def, char *virtualDev[4], if (virtualDev[controller] == NULL) { virtualDev[controller] = disk->driverName; - } else if (STRCASENEQ(virtualDev[controller], disk->driverName)) { + } else if (disk->driverName == NULL + || STRCASENEQ(virtualDev[controller], disk->driverName)) { ESX_ERROR(VIR_ERR_INTERNAL_ERROR, _("Inconsistent driver usage ('%s' is not '%s') on SCSI " "controller index %d"), virtualDev[controller], - disk->driverName, controller); + disk->driverName ? disk->driverName : "?", controller); return -1; } } -- 1.7.1.rc1.248.gcefbb
2010/4/14 Jim Meyering <jim@meyering.net>:
From: Jim Meyering <meyering@redhat.com>
* src/esx/esx_vmx.c (esxVMX_GatherSCSIControllers): Do not dereference a NULL disk->driverName. We already detect this condition in another case. Check for it here, too. --- src/esx/esx_vmx.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/esx/esx_vmx.c b/src/esx/esx_vmx.c index 647e720..aed5cc1 100644 --- a/src/esx/esx_vmx.c +++ b/src/esx/esx_vmx.c @@ -570,11 +570,12 @@ esxVMX_GatherSCSIControllers(virDomainDefPtr def, char *virtualDev[4],
if (virtualDev[controller] == NULL) { virtualDev[controller] = disk->driverName; - } else if (STRCASENEQ(virtualDev[controller], disk->driverName)) { + } else if (disk->driverName == NULL + || STRCASENEQ(virtualDev[controller], disk->driverName)) {
Style nitpick: I'd like to have the || at the end of the previous line, like in the rest of this file.
ESX_ERROR(VIR_ERR_INTERNAL_ERROR, _("Inconsistent driver usage ('%s' is not '%s') on SCSI " "controller index %d"), virtualDev[controller], - disk->driverName, controller); + disk->driverName ? disk->driverName : "?", controller); return -1; } } -- 1.7.1.rc1.248.gcefbb
ACK. Matthias
Matthias Bolte wrote:
2010/4/14 Jim Meyering <jim@meyering.net>:
From: Jim Meyering <meyering@redhat.com>
* src/esx/esx_vmx.c (esxVMX_GatherSCSIControllers): Do not dereference a NULL disk->driverName. We already detect this condition in another case. Check for it here, too. --- src/esx/esx_vmx.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/esx/esx_vmx.c b/src/esx/esx_vmx.c index 647e720..aed5cc1 100644 --- a/src/esx/esx_vmx.c +++ b/src/esx/esx_vmx.c @@ -570,11 +570,12 @@ esxVMX_GatherSCSIControllers(virDomainDefPtr def, char *virtualDev[4],
if (virtualDev[controller] == NULL) { virtualDev[controller] = disk->driverName; - } else if (STRCASENEQ(virtualDev[controller], disk->driverName)) { + } else if (disk->driverName == NULL + || STRCASENEQ(virtualDev[controller], disk->driverName)) {
Style nitpick: I'd like to have the || at the end of the previous line, like in the rest of this file.
Ok. Adjusted. However, note that there is a good (human-factors-related) reason to put operators at the beginning of a line rather than at the end like that: it makes them more apparent. Sure, some of the time you sacrifice a little on the alignment front, but that is outweighed by the improved readability. It's similar to the 80-column max rule: things at the end of a line (especially when wrapped) are far easier to miss.
ESX_ERROR(VIR_ERR_INTERNAL_ERROR, _("Inconsistent driver usage ('%s' is not '%s') on SCSI " "controller index %d"), virtualDev[controller], - disk->driverName, controller); + disk->driverName ? disk->driverName : "?", controller); return -1; } } -- 1.7.1.rc1.248.gcefbb
ACK.
Thanks. Pushed.
participants (3)
-
Eric Blake -
Jim Meyering -
Matthias Bolte