[PATCH 0/2] Remove static analysis cruft
Remove comments for silencing coverity and static assers which are no longer needed. CI pipelines: Yesterday's all-green: https://gitlab.com/pipo.sk/libvirt/-/pipelines/305953275 New run started when I've rebased this to current master: https://gitlab.com/pipo.sk/libvirt/-/pipelines/306458103 Peter Krempa (2): Drop magic comments for coverity Remove static analysis assertions src/conf/domain_conf.c | 13 ------------- src/conf/virdomainobjlist.c | 2 -- src/conf/virnwfilterbindingobjlist.c | 1 - src/cpu/cpu_x86.c | 1 - src/libxl/libxl_driver.c | 9 --------- src/qemu/qemu_command.c | 2 -- src/qemu/qemu_driver.c | 2 -- src/qemu/qemu_monitor.c | 3 --- src/qemu/qemu_process.c | 2 -- src/remote/remote_driver.c | 1 - src/rpc/virnetserver.c | 4 ---- src/storage/parthelper.c | 2 -- src/util/vircommand.c | 3 --- src/util/virconf.c | 6 ------ src/util/virhostmem.c | 2 -- src/util/virnetdev.c | 1 - src/util/virobject.c | 1 - src/util/virprocess.c | 1 - src/util/virutil.c | 3 --- tests/commandtest.c | 2 -- tests/virdrivermoduletest.c | 1 - tools/virsh.c | 1 - tools/virt-admin.c | 1 - tools/vsh.c | 1 - 24 files changed, 65 deletions(-) -- 2.31.1
They were added mostly randomly and we don't really want to keep working around of false positives. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 13 ------------- src/cpu/cpu_x86.c | 1 - src/qemu/qemu_command.c | 2 -- src/qemu/qemu_driver.c | 2 -- src/qemu/qemu_process.c | 1 - src/remote/remote_driver.c | 1 - src/storage/parthelper.c | 2 -- src/util/virnetdev.c | 1 - src/util/virprocess.c | 1 - tests/commandtest.c | 1 - tests/virdrivermoduletest.c | 1 - tools/virsh.c | 1 - tools/virt-admin.c | 1 - 13 files changed, 28 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e35c38caa3..d73c1df085 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -17403,7 +17403,6 @@ virDomainFeaturesDefParse(virDomainDef *def, break; } - /* coverity[dead_error_begin] */ case VIR_DOMAIN_FEATURE_LAST: break; } @@ -17491,7 +17490,6 @@ virDomainFeaturesDefParse(virDomainDef *def, } break; - /* coverity[dead_error_begin] */ case VIR_DOMAIN_HYPERV_LAST: break; } @@ -17546,7 +17544,6 @@ virDomainFeaturesDefParse(virDomainDef *def, def->kvm_features[feature] = value; break; - /* coverity[dead_error_begin] */ case VIR_DOMAIN_KVM_LAST: break; } @@ -17591,7 +17588,6 @@ virDomainFeaturesDefParse(virDomainDef *def, return -1; break; - /* coverity[dead_error_begin] */ case VIR_DOMAIN_XEN_LAST: break; } @@ -21523,7 +21519,6 @@ virDomainDefFeaturesCheckABIStability(virDomainDef *src, } break; - /* coverity[dead_error_begin] */ case VIR_DOMAIN_HYPERV_LAST: break; } @@ -21568,7 +21563,6 @@ virDomainDefFeaturesCheckABIStability(virDomainDef *src, } break; - /* coverity[dead_error_begin] */ case VIR_DOMAIN_XEN_LAST: break; } @@ -21594,7 +21588,6 @@ virDomainDefFeaturesCheckABIStability(virDomainDef *src, break; - /* coverity[dead_error_begin] */ case VIR_DOMAIN_KVM_LAST: break; } @@ -27541,7 +27534,6 @@ virDomainDefFormatFeatures(virBuffer *buf, def->hyperv_vendor_id); break; - /* coverity[dead_error_begin] */ case VIR_DOMAIN_HYPERV_LAST: break; } @@ -27568,7 +27560,6 @@ virDomainDefFormatFeatures(virBuffer *buf, def->kvm_features[j])); break; - /* coverity[dead_error_begin] */ case VIR_DOMAIN_KVM_LAST: break; } @@ -27610,7 +27601,6 @@ virDomainDefFormatFeatures(virBuffer *buf, } break; - /* coverity[dead_error_begin] */ case VIR_DOMAIN_XEN_LAST: break; } @@ -27704,7 +27694,6 @@ virDomainDefFormatFeatures(virBuffer *buf, virDomainIBSTypeToString(def->features[i])); break; - /* coverity[dead_error_begin] */ case VIR_DOMAIN_FEATURE_LAST: break; } @@ -29558,7 +29547,6 @@ virDomainObjGetMetadata(virDomainObj *vm, return NULL; break; - /* coverity[dead_error_begin] */ case VIR_DOMAIN_METADATA_LAST: break; } @@ -29640,7 +29628,6 @@ virDomainDefSetMetadata(virDomainDef *def, } break; - /* coverity[dead_error_begin] */ case VIR_DOMAIN_METADATA_LAST: break; } diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index d01be6f7ba..a4599499d0 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -1376,7 +1376,6 @@ x86ModelFromCPU(const virCPUDef *cpu, x86DataSubtract(&model->data, &feature->data); break; - /* coverity[dead_error_condition] */ case VIR_CPU_FEATURE_OPTIONAL: case VIR_CPU_FEATURE_LAST: break; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index dcc060bde9..bbe5e1633b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6575,7 +6575,6 @@ qemuBuildCpuCommandLine(virCommand *cmd, def->hyperv_vendor_id); break; - /* coverity[dead_error_begin] */ case VIR_DOMAIN_HYPERV_LAST: break; } @@ -6607,7 +6606,6 @@ qemuBuildCpuCommandLine(virCommand *cmd, virBufferAddLit(&buf, ",kvm-poll-control=on"); break; - /* coverity[dead_error_begin] */ case VIR_DOMAIN_KVM_LAST: break; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c90d52edc0..3af58503bd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8992,7 +8992,6 @@ qemuDomainGetNumaParameters(virDomainPtr dom, nodeset = NULL; break; - /* coverity[dead_error_begin] */ default: break; /* should not hit here */ @@ -10559,7 +10558,6 @@ qemuDomainGetInterfaceParameters(virDomainPtr dom, if (net->bandwidth && net->bandwidth->out) params[i].value.ui = net->bandwidth->out->burst; break; - /* coverity[dead_error_begin] */ default: break; /* should not hit here */ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index b69a9d1927..cf422656ff 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4239,7 +4239,6 @@ qemuProcessVerifyHypervFeatures(virDomainDef *def, virDomainHypervTypeToString(i)); return -1; - /* coverity[dead_error_begin] */ case VIR_DOMAIN_HYPERV_SPINLOCKS: case VIR_DOMAIN_HYPERV_VENDOR_ID: case VIR_DOMAIN_HYPERV_LAST: diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index eed99af127..0bacd0bc4b 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -5643,7 +5643,6 @@ remoteStreamEventAddCallback(virStreamPtr st, cleanup: remoteDriverUnlock(priv); - /* coverity[leaked_storage] - cbdata is not leaked */ return ret; } diff --git a/src/storage/parthelper.c b/src/storage/parthelper.c index 94d588615f..73ae28f718 100644 --- a/src/storage/parthelper.c +++ b/src/storage/parthelper.c @@ -131,7 +131,6 @@ int main(int argc, char **argv) content = "free"; else if (part->type & PED_PARTITION_METADATA) content = "metadata"; - /* coverity[dead_error_condition] - not true if defined */ else if (part->type & PED_PARTITION_PROTECTED) content = "protected"; else @@ -145,7 +144,6 @@ int main(int argc, char **argv) content = "free"; else if (part->type & PED_PARTITION_METADATA) content = "metadata"; - /* coverity[dead_error_condition] - not true if defined */ else if (part->type & PED_PARTITION_PROTECTED) content = "protected"; else diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 1870f3f875..407270271f 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -2750,7 +2750,6 @@ static int virNetDevParseMcast(char *buf, virNetDevMcastEntry *mcast) } break; - /* coverity[dead_error_begin] */ case VIR_MCAST_TYPE_LAST: break; } diff --git a/src/util/virprocess.c b/src/util/virprocess.c index a3facd8ac2..01d5d01d02 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -521,7 +521,6 @@ virProcessGetAffinity(pid_t pid) ret = virBitmapNew(ncpus); for (i = 0; i < ncpus; i++) { - /* coverity[overrun-local] */ if (CPU_ISSET_S(i, masklen, mask)) ignore_value(virBitmapSetBit(ret, i)); } diff --git a/tests/commandtest.c b/tests/commandtest.c index 15ea77b36a..57c7db67d8 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -1206,7 +1206,6 @@ mymain(void) virinitret = virInitialize(); /* Phase two of killing interfering fds; see above. */ - /* coverity[overwrite_var] - silence the obvious */ fd = 3; VIR_FORCE_CLOSE(fd); fd = 4; diff --git a/tests/virdrivermoduletest.c b/tests/virdrivermoduletest.c index 0d753cd0ee..e7930d3453 100644 --- a/tests/virdrivermoduletest.c +++ b/tests/virdrivermoduletest.c @@ -38,7 +38,6 @@ static int testDriverModule(const void *args) { const struct testDriverModuleData *data = args; - /* coverity[leaked_storage] */ if (virDriverLoadModule(data->module, data->regfunc, true) != 0) return -1; diff --git a/tools/virsh.c b/tools/virsh.c index 7d7109cfdf..fd32944c24 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -96,7 +96,6 @@ virshCatchDisconnect(virConnectPtr conn, case VIR_CONNECT_CLOSE_REASON_KEEPALIVE: str = N_("Disconnected from %s due to keepalive timeout"); break; - /* coverity[dead_error_condition] */ case VIR_CONNECT_CLOSE_REASON_CLIENT: case VIR_CONNECT_CLOSE_REASON_LAST: break; diff --git a/tools/virt-admin.c b/tools/virt-admin.c index 3a050ab9ab..dd17743b9d 100644 --- a/tools/virt-admin.c +++ b/tools/virt-admin.c @@ -92,7 +92,6 @@ vshAdmCatchDisconnect(virAdmConnectPtr conn G_GNUC_UNUSED, case VIR_CONNECT_CLOSE_REASON_KEEPALIVE: str = N_("Disconnected from %s due to keepalive timeout"); break; - /* coverity[dead_error_condition] */ case VIR_CONNECT_CLOSE_REASON_CLIENT: case VIR_CONNECT_CLOSE_REASON_LAST: break; -- 2.31.1
None of them are currently needed to pass our upstream CI, most were either for ancient clang versions or coverity for silencing false positives. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/virdomainobjlist.c | 2 -- src/conf/virnwfilterbindingobjlist.c | 1 - src/libxl/libxl_driver.c | 9 --------- src/qemu/qemu_monitor.c | 3 --- src/qemu/qemu_process.c | 1 - src/rpc/virnetserver.c | 4 ---- src/util/vircommand.c | 3 --- src/util/virconf.c | 6 ------ src/util/virhostmem.c | 2 -- src/util/virobject.c | 1 - src/util/virutil.c | 3 --- tests/commandtest.c | 1 - tools/vsh.c | 1 - 13 files changed, 37 deletions(-) diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index e1d455eefe..5f9fd9aabc 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -980,7 +980,6 @@ virDomainObjListCollect(virDomainObjList *domlist, struct virDomainListData data = { NULL, 0 }; virObjectRWLockRead(domlist); - sa_assert(domlist->objs); data.vms = g_new0(virDomainObj *, virHashSize(domlist->objs)); virHashForEach(domlist->objs, virDomainObjListCollectIterator, &data); @@ -1040,7 +1039,6 @@ virDomainObjListConvert(virDomainObjList *domlist, } virObjectRWUnlock(domlist); - sa_assert(*vms); virDomainObjListFilter(vms, nvms, conn, filter, flags); return 0; diff --git a/src/conf/virnwfilterbindingobjlist.c b/src/conf/virnwfilterbindingobjlist.c index fe97f2b3df..1f19e27eb0 100644 --- a/src/conf/virnwfilterbindingobjlist.c +++ b/src/conf/virnwfilterbindingobjlist.c @@ -432,7 +432,6 @@ virNWFilterBindingObjListCollect(virNWFilterBindingObjList *domlist, struct virNWFilterBindingListData data = { NULL, 0 }; virObjectRWLockRead(domlist); - sa_assert(domlist->objs); data.bindings = g_new0(virNWFilterBindingObj *, virHashSize(domlist->objs)); virHashForEach(domlist->objs, virNWFilterBindingObjListCollectIterator, &data); diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index d54cd41785..7b89e7c224 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1666,8 +1666,6 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, } if (flags & VIR_DOMAIN_MEM_CONFIG) { - /* Help clang 2.8 decipher the logic flow. */ - sa_assert(persistentDef); virDomainDefSetMemoryTotal(persistentDef, newmem); if (persistentDef->mem.cur_balloon > newmem) persistentDef->mem.cur_balloon = newmem; @@ -1702,7 +1700,6 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, } if (flags & VIR_DOMAIN_MEM_CONFIG) { - sa_assert(persistentDef); persistentDef->mem.cur_balloon = newmem; ret = virDomainDefSave(persistentDef, driver->xmlopt, cfg->configDir); goto endjob; @@ -2476,9 +2473,6 @@ libxlDomainPinVcpuFlags(virDomainPtr dom, unsigned int vcpu, if (flags & VIR_DOMAIN_AFFECT_LIVE) targetDef = vm->def; - /* Make sure coverity knows targetDef is valid at this point. */ - sa_assert(targetDef); - pcpumap = virBitmapNewData(cpumap, maplen); if (!pcpumap) goto endjob; @@ -2557,9 +2551,6 @@ libxlDomainGetVcpuPinInfo(virDomainPtr dom, int ncpumaps, if (flags & VIR_DOMAIN_AFFECT_LIVE) targetDef = vm->def; - /* Make sure coverity knows targetDef is valid at this point. */ - sa_assert(targetDef); - hostcpus = virBitmapNew(libxl_get_max_cpus(cfg->ctx)); virBitmapSetAll(hostcpus); diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 1e6f892e08..8f35b4240f 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -398,9 +398,6 @@ qemuMonitorIOWriteWithFD(qemuMonitor *mon, msg.msg_controllen = sizeof(control); cmsg = CMSG_FIRSTHDR(&msg); - /* Some static analyzers, like clang 2.6-0.6.pre2, fail to see - that our use of CMSG_FIRSTHDR will not return NULL. */ - sa_assert(cmsg); cmsg->cmsg_len = CMSG_LEN(sizeof(int)); cmsg->cmsg_level = SOL_SOCKET; cmsg->cmsg_type = SCM_RIGHTS; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index cf422656ff..a5f3114380 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2089,7 +2089,6 @@ qemuProcessReadLog(qemuDomainLogContext *logCtxt, *eol = '\n'; } } - filter_next = NULL; /* silence false coverity warning */ if (got > 0 && buf[got - 1] == '\n') { diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 42e06fcb59..cfb65f8b5f 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -937,10 +937,6 @@ virNetServerProcessClients(virNetServer *srv) reprocess: for (i = 0; i < srv->nclients; i++) { - /* Coverity 5.3.0 couldn't see that srv->clients is non-NULL - * if srv->nclients is non-zero. */ - sa_assert(srv->clients); - client = srv->clients[i]; virObjectLock(client); if (virNetServerClientWantCloseLocked(client)) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 7abb2e288f..6e01e4cb62 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -2263,9 +2263,6 @@ virCommandProcessIO(virCommand *cmd) buf = cmd->errbuf; len = &errlen; } - /* Silence a false positive from clang. */ - sa_assert(buf); - done = read(fds[i].fd, data, sizeof(data)); if (done < 0) { if (errno != EINTR && diff --git a/src/util/virconf.c b/src/util/virconf.c index 15a65ec394..07ecfc7b57 100644 --- a/src/util/virconf.c +++ b/src/util/virconf.c @@ -472,12 +472,6 @@ virConfParseValue(virConfParserCtxt *ctxt) } while ((ctxt->cur < ctxt->end) && (CUR != ']')) { - /* Tell Clang that when execution reaches this point - "lst" is guaranteed to be non-NULL. This stops it - from issuing an invalid NULL-dereference warning about - "prev = lst; while (prev->next..." below. */ - sa_assert(lst); - if (CUR != ',') { virConfError(ctxt, VIR_ERR_CONF_SYNTAX, _("expecting a separator in list")); diff --git a/src/util/virhostmem.c b/src/util/virhostmem.c index ae42978ed2..8aa675cb4f 100644 --- a/src/util/virhostmem.c +++ b/src/util/virhostmem.c @@ -313,7 +313,6 @@ virHostMemSetParameterValue(virTypedParameterPtr param) int rc = -1; char *field = strchr(param->field, '_'); - sa_assert(field); field++; path = g_strdup_printf("%s/%s", SYSFS_MEMORY_SHARED_PATH, field); @@ -338,7 +337,6 @@ virHostMemParametersAreAllSupported(virTypedParameterPtr params, virTypedParameterPtr param = ¶ms[i]; char *field = strchr(param->field, '_'); - sa_assert(field); field++; path = g_strdup_printf("%s/%s", SYSFS_MEMORY_SHARED_PATH, field); diff --git a/src/util/virobject.c b/src/util/virobject.c index 7173b9136f..3412985b79 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -182,7 +182,6 @@ virClassNew(virClass *parent, return NULL; } else if (objectSize <= parentSize || parentSize != (parent ? parent->objectSize : 0)) { - sa_assert(parent); virReportInvalidArg(objectSize, _("object size %zu of %s is not larger than parent class %zu"), objectSize, name, parent->objectSize); diff --git a/src/util/virutil.c b/src/util/virutil.c index 3f49a469e5..311cbbf93a 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -512,9 +512,6 @@ virGetHostnameImpl(bool quiet) return g_strdup(hostname); } - /* Tell static analyzers about getaddrinfo semantics. */ - sa_assert(info); - if (info->ai_canonname == NULL || STRPREFIX(info->ai_canonname, "localhost")) /* in this case, we tried to canonicalize and we ended up back with diff --git a/tests/commandtest.c b/tests/commandtest.c index 57c7db67d8..5267e1bf1c 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -611,7 +611,6 @@ static int test17(const void *unused G_GNUC_UNUSED) goto cleanup; } - sa_assert(outbuf); if (*outbuf) { puts("output buffer is not an allocated empty string"); goto cleanup; diff --git a/tools/vsh.c b/tools/vsh.c index ebaf17f326..123284c636 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -449,7 +449,6 @@ vshCmddefGetOption(vshControl *ctl, const vshCmdDef *cmd, const char *name, opt->help = "string": straight replacement of name opt->help = "string=value": treat boolean flag as alias of option and its default value */ - sa_assert(!alias); alias = g_strdup(opt->help); name = alias; if ((value = strchr(name, '='))) { -- 2.31.1
On Thu, May 20, 2021 at 12:26:22PM +0200, Peter Krempa wrote:
Remove comments for silencing coverity and static assers which are no longer needed.
CI pipelines:
Yesterday's all-green:
https://gitlab.com/pipo.sk/libvirt/-/pipelines/305953275
New run started when I've rebased this to current master:
https://gitlab.com/pipo.sk/libvirt/-/pipelines/306458103
Peter Krempa (2): Drop magic comments for coverity Remove static analysis assertions
I have no objection to this patches but I would way if others have different opinion. Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
On 5/20/21 12:26 PM, Peter Krempa wrote:
Remove comments for silencing coverity and static assers which are no longer needed.
CI pipelines:
Yesterday's all-green:
https://gitlab.com/pipo.sk/libvirt/-/pipelines/305953275
New run started when I've rebased this to current master:
https://gitlab.com/pipo.sk/libvirt/-/pipelines/306458103
Peter Krempa (2): Drop magic comments for coverity Remove static analysis assertions
src/conf/domain_conf.c | 13 ------------- src/conf/virdomainobjlist.c | 2 -- src/conf/virnwfilterbindingobjlist.c | 1 - src/cpu/cpu_x86.c | 1 - src/libxl/libxl_driver.c | 9 --------- src/qemu/qemu_command.c | 2 -- src/qemu/qemu_driver.c | 2 -- src/qemu/qemu_monitor.c | 3 --- src/qemu/qemu_process.c | 2 -- src/remote/remote_driver.c | 1 - src/rpc/virnetserver.c | 4 ---- src/storage/parthelper.c | 2 -- src/util/vircommand.c | 3 --- src/util/virconf.c | 6 ------ src/util/virhostmem.c | 2 -- src/util/virnetdev.c | 1 - src/util/virobject.c | 1 - src/util/virprocess.c | 1 - src/util/virutil.c | 3 --- tests/commandtest.c | 2 -- tests/virdrivermoduletest.c | 1 - tools/virsh.c | 1 - tools/virt-admin.c | 1 - tools/vsh.c | 1 - 24 files changed, 65 deletions(-)
So how are we supposed to deal with Coverity false positives? I know that we are not happy to change our code esp. to silence a false positive on some "random" tool. Anyway, changes make sense: Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (3)
-
Michal Prívozník -
Pavel Hrdina -
Peter Krempa