[PATCH 0/3] batch: don't require checking retvalue of some bitmap ops

Most of bitmap setBit/clearBit/getBit users know that the bitmap index is not out of bound and thus don't check the return value. More precisely the stats is next: Method all check =================================== virBitmapSetBit 85 14 virBitmapClearBit 15 3 virBitmapGetBit 15 6 where 'all' is the number of all occurences of the method and 'check' is the number of occurences with 'if (method' pattern. Thus keeping the retvalue checking requirement produces more noise then helps. I guess we even can make these function return void as users can simply compare the index with the bitmap size. The removing of ignore_value was done by sed together with several manual editings where methods calls were splitted across two lines. FILES=`git grep -l 'ignore_value(virBitmapGetBit('` sed -ibak -re 's/ignore_value\(virBitmapGetBit\((.*)\)\);/virBitmapGetBit(\1\);/' "$FILES" Nikolay Shirokovskiy (3): batch: don't require checking retvalue for virBitmapSetBit batch: don't require checking retvalue for virBitmapClearBit batch: don't require checking retvalue for virBitmapGetBit src/conf/capabilities.c | 2 +- src/conf/checkpoint_conf.c | 2 +- src/conf/domain_addr.c | 8 ++++---- src/conf/domain_conf.c | 9 ++++----- src/conf/node_device_conf.c | 2 +- src/conf/snapshot_conf.c | 2 +- src/conf/storage_conf.c | 2 +- src/libxl/libxl_capabilities.c | 2 +- src/network/bridge_driver.c | 6 +++--- src/qemu/qemu_capabilities.c | 4 ++-- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_domain_address.c | 4 ++-- src/qemu/qemu_driver.c | 8 ++++---- src/qemu/qemu_hotplug.c | 6 +++--- src/qemu/qemu_migration_cookie.c | 8 ++++---- src/qemu/qemu_migration_params.c | 28 +++++++++++++--------------- src/qemu/qemu_monitor.c | 2 +- src/qemu/qemu_slirp.c | 2 +- src/test/test_driver.c | 2 +- src/util/virbitmap.h | 6 +++--- src/util/vircommand.c | 2 +- src/util/virdnsmasq.c | 2 +- src/util/virhostcpu.c | 2 +- src/util/virjson.c | 2 +- src/util/virnetdev.c | 10 +++++----- src/util/virnuma.c | 4 ++-- src/util/virprocess.c | 4 ++-- src/util/virresctrl.c | 2 +- src/util/virstoragefile.c | 2 +- src/vmx/vmx.c | 2 +- tests/qemumonitorjsontest.c | 2 +- tests/testutils.c | 2 +- tests/virbitmaptest.c | 28 ++++++++++++++-------------- tools/virsh-domain.c | 4 ++-- tools/virt-host-validate-common.c | 2 +- 35 files changed, 87 insertions(+), 90 deletions(-) -- 1.8.3.1

Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/conf/capabilities.c | 2 +- src/conf/checkpoint_conf.c | 2 +- src/conf/domain_addr.c | 6 +++--- src/conf/domain_conf.c | 4 ++-- src/conf/node_device_conf.c | 2 +- src/conf/snapshot_conf.c | 2 +- src/conf/storage_conf.c | 2 +- src/libxl/libxl_capabilities.c | 2 +- src/network/bridge_driver.c | 2 +- src/qemu/qemu_capabilities.c | 2 +- src/qemu/qemu_domain_address.c | 4 ++-- src/qemu/qemu_driver.c | 6 +++--- src/qemu/qemu_hotplug.c | 4 ++-- src/qemu/qemu_migration_cookie.c | 4 ++-- src/qemu/qemu_migration_params.c | 15 +++++++-------- src/qemu/qemu_monitor.c | 2 +- src/qemu/qemu_slirp.c | 2 +- src/test/test_driver.c | 2 +- src/util/virbitmap.h | 2 +- src/util/vircommand.c | 2 +- src/util/virdnsmasq.c | 2 +- src/util/virhostcpu.c | 2 +- src/util/virjson.c | 2 +- src/util/virnetdev.c | 10 +++++----- src/util/virnuma.c | 4 ++-- src/util/virprocess.c | 4 ++-- src/util/virresctrl.c | 2 +- src/util/virstoragefile.c | 2 +- src/vmx/vmx.c | 2 +- tests/qemumonitorjsontest.c | 2 +- tests/testutils.c | 2 +- tests/virbitmaptest.c | 28 ++++++++++++++-------------- tools/virsh-domain.c | 4 ++-- tools/virt-host-validate-common.c | 2 +- 34 files changed, 68 insertions(+), 69 deletions(-) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 610e6e8..7115a88 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -1581,7 +1581,7 @@ virCapabilitiesHostNUMAInitFake(virCapsHostNUMAPtr caps) for (c = 0; c < nodeinfo.cores; c++) { g_autoptr(virBitmap) siblings = virBitmapNew(ncpus); for (t = 0; t < nodeinfo.threads; t++) - ignore_value(virBitmapSetBit(siblings, id + t)); + virBitmapSetBit(siblings, id + t); for (t = 0; t < nodeinfo.threads; t++) { if (virHostCPUGetOnline(id, &tmp) < 0) diff --git a/src/conf/checkpoint_conf.c b/src/conf/checkpoint_conf.c index 1f93595..ac40f86 100644 --- a/src/conf/checkpoint_conf.c +++ b/src/conf/checkpoint_conf.c @@ -351,7 +351,7 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr def) disk->name); goto cleanup; } - ignore_value(virBitmapSetBit(map, idx)); + virBitmapSetBit(map, idx); disk->idx = idx; if (STRNEQ(disk->name, def->parent.dom->disks[idx]->dst)) { diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 1068cbf..a28fba4 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -1694,7 +1694,7 @@ virDomainVirtioSerialAddrReserve(virDomainDefPtr def G_GNUC_UNUSED, return -1; } - ignore_value(virBitmapSetBit(map, info->addr.vioserial.port)); + virBitmapSetBit(map, info->addr.vioserial.port); return 0; } @@ -2201,7 +2201,7 @@ virDomainUSBAddressSetAddHub(virDomainUSBAddressSetPtr addrs, hub->info.addr.usb.bus, portStr); goto cleanup; } - ignore_value(virBitmapSetBit(targetHub->portmap, targetPort)); + virBitmapSetBit(targetHub->portmap, targetPort); targetHub->ports[targetPort] = newHub; newHub = NULL; @@ -2414,7 +2414,7 @@ virDomainUSBAddressReserve(virDomainDeviceInfoPtr info, return -1; } - ignore_value(virBitmapSetBit(targetHub->portmap, targetPort)); + virBitmapSetBit(targetHub->portmap, targetPort); return 0; } diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 386b04b..a067fb4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2009,7 +2009,7 @@ virDomainDefGetOnlineVcpumap(const virDomainDef *def) for (i = 0; i < def->maxvcpus; i++) { if (def->vcpus[i]->online) - ignore_value(virBitmapSetBit(ret, i)); + virBitmapSetBit(ret, i); } return ret; @@ -4483,7 +4483,7 @@ virDomainDefRejectDuplicateControllers(virDomainDefPtr def) cont->idx); goto cleanup; } - ignore_value(virBitmapSetBit(bitmaps[cont->type], cont->idx)); + virBitmapSetBit(bitmaps[cont->type], cont->idx); } ret = 0; diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index c540153..9aa55bd 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -1160,7 +1160,7 @@ virNodeDevCapNetParseXML(xmlXPathContextPtr ctxt, tmp); goto out; } - ignore_value(virBitmapSetBit(net->features, val)); + virBitmapSetBit(net->features, val); VIR_FREE(tmp); } diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 07336e9..68ba35f 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -695,7 +695,7 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def, disk->name); goto cleanup; } - ignore_value(virBitmapSetBit(map, idx)); + virBitmapSetBit(map, idx); disk->idx = idx; disk_snapshot = def->parent.dom->disks[idx]->snapshot; diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index fac5ff7..ca8c697 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1459,7 +1459,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, (const char*)nodes[i]->name); return NULL; } - ignore_value(virBitmapSetBit(def->target.features, f)); + virBitmapSetBit(def->target.features, f); } VIR_FREE(nodes); } diff --git a/src/libxl/libxl_capabilities.c b/src/libxl/libxl_capabilities.c index beac8c8..97a5cd0 100644 --- a/src/libxl/libxl_capabilities.c +++ b/src/libxl/libxl_capabilities.c @@ -316,7 +316,7 @@ libxlCapsInitNuma(libxl_ctx *ctx, virCapsPtr caps) for (j = 0; j < nr_cpus_node[node]; j++) { if (cpus[node][j].socket_id == cpu_topo[i].socket && cpus[node][j].core_id == cpu_topo[i].core) - ignore_value(virBitmapSetBit(cpus[node][j].siblings, i)); + virBitmapSetBit(cpus[node][j].siblings, i); } } diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 54cedd7..c27a9c4 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -5143,7 +5143,7 @@ networkUnplugBandwidth(virNetworkObjPtr obj, obj, network_driver->xmlopt) < 0) { tmp_floor_sum += ifaceBand->in->floor; virNetworkObjSetFloorSum(obj, tmp_floor_sum); - ignore_value(virBitmapSetBit(classIdMap, *class_id)); + virBitmapSetBit(classIdMap, *class_id); return ret; } /* update rate for non guaranteed NICs */ diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 1a2cb87..97bbadb 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -1996,7 +1996,7 @@ void virQEMUCapsSet(virQEMUCapsPtr qemuCaps, virQEMUCapsFlags flag) { - ignore_value(virBitmapSetBit(qemuCaps->flags, flag)); + virBitmapSetBit(qemuCaps->flags, flag); } diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 058cbda..92266fb 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -3063,7 +3063,7 @@ qemuDomainGetMemorySlotMap(const virDomainDef *def) mem = def->mems[i]; if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) - ignore_value(virBitmapSetBit(ret, mem->info.addr.dimm.slot)); + virBitmapSetBit(ret, mem->info.addr.dimm.slot); } return ret; @@ -3085,7 +3085,7 @@ qemuAssignMemoryDeviceSlot(virDomainMemoryDefPtr mem, return -1; } - ignore_value(virBitmapSetBit(slotmap, nextslot)); + virBitmapSetBit(slotmap, nextslot); mem->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM; mem->info.addr.dimm.slot = nextslot; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0ad6359..7d41c19 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14412,7 +14412,7 @@ qemuDomainSnapshotCreateInactiveExternal(virQEMUDriverPtr driver, /* If the target does not exist, we're going to create it possibly */ if (!virFileExists(snapdisk->src->path)) - ignore_value(virBitmapSetBit(created, i)); + virBitmapSetBit(created, i); if (virCommandRun(cmd, NULL) < 0) goto cleanup; @@ -22336,10 +22336,10 @@ qemuDomainGetGuestVcpusParams(virTypedParameterPtr *params, } if (info[i].online) - ignore_value(virBitmapSetBit(online, info[i].id)); + virBitmapSetBit(online, info[i].id); if (info[i].offlinable) - ignore_value(virBitmapSetBit(offlinable, info[i].id)); + virBitmapSetBit(offlinable, info[i].id); } #define ADD_BITMAP(name) \ diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 2691233..d94f2e4 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -6157,7 +6157,7 @@ qemuDomainSelectHotplugVcpuEntities(virDomainDefPtr def, goto error; } - ignore_value(virBitmapSetBit(ret, i)); + virBitmapSetBit(ret, i); } } else { *enable = false; @@ -6184,7 +6184,7 @@ qemuDomainSelectHotplugVcpuEntities(virDomainDefPtr def, goto error; } - ignore_value(virBitmapSetBit(ret, i)); + virBitmapSetBit(ret, i); } } diff --git a/src/qemu/qemu_migration_cookie.c b/src/qemu/qemu_migration_cookie.c index 81b557e..6eaa704 100644 --- a/src/qemu/qemu_migration_cookie.c +++ b/src/qemu/qemu_migration_cookie.c @@ -1156,11 +1156,11 @@ qemuMigrationCookieCapsXMLParse(xmlXPathContextPtr ctxt) if ((cap = qemuMigrationCapabilityTypeFromString(name)) < 0) VIR_DEBUG("unknown migration capability '%s'", name); else - ignore_value(virBitmapSetBit(caps->supported, cap)); + virBitmapSetBit(caps->supported, cap); if ((automatic = virXMLPropString(nodes[i], "auto")) && STREQ(automatic, "yes")) - ignore_value(virBitmapSetBit(caps->automatic, cap)); + virBitmapSetBit(caps->automatic, cap); VIR_FREE(name); VIR_FREE(automatic); diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c index fc28296..068f117 100644 --- a/src/qemu/qemu_migration_params.c +++ b/src/qemu/qemu_migration_params.c @@ -234,7 +234,7 @@ qemuMigrationParamsGetAlwaysOnCaps(qemuMigrationParty party) if (!(qemuMigrationParamsAlwaysOn[i].party & party)) continue; - ignore_value(virBitmapSetBit(caps, qemuMigrationParamsAlwaysOn[i].cap)); + virBitmapSetBit(caps, qemuMigrationParamsAlwaysOn[i].cap); } return caps; @@ -505,7 +505,7 @@ qemuMigrationParamsSetCompression(virTypedParameterPtr params, default: continue; } - ignore_value(virBitmapSetBit(migParams->caps, cap)); + virBitmapSetBit(migParams->caps, cap); } if ((migParams->params[QEMU_MIGRATION_PARAM_COMPRESS_LEVEL].set || @@ -526,8 +526,7 @@ qemuMigrationParamsSetCompression(virTypedParameterPtr params, if (!migParams->compMethods && (flags & VIR_MIGRATE_COMPRESSED)) { migParams->compMethods = 1ULL << QEMU_MIGRATION_COMPRESS_XBZRLE; - ignore_value(virBitmapSetBit(migParams->caps, - QEMU_MIGRATION_CAP_XBZRLE)); + virBitmapSetBit(migParams->caps, QEMU_MIGRATION_CAP_XBZRLE); } return 0; @@ -553,7 +552,7 @@ qemuMigrationParamsFromFlags(virTypedParameterPtr params, flags & qemuMigrationParamsFlagMap[i].flag) { VIR_DEBUG("Enabling migration capability '%s'", qemuMigrationCapabilityTypeToString(cap)); - ignore_value(virBitmapSetBit(migParams->caps, cap)); + virBitmapSetBit(migParams->caps, cap); } } @@ -1197,7 +1196,7 @@ qemuMigrationParamsCheck(virQEMUDriverPtr driver, VIR_DEBUG("Enabling migration capability '%s'", qemuMigrationCapabilityTypeToString(cap)); - ignore_value(virBitmapSetBit(migParams->caps, cap)); + virBitmapSetBit(migParams->caps, cap); } } @@ -1419,7 +1418,7 @@ qemuMigrationCapsCheck(virQEMUDriverPtr driver, if (cap < 0) { VIR_DEBUG("Unknown migration capability: '%s'", *capStr); } else { - ignore_value(virBitmapSetBit(priv->migrationCaps, cap)); + virBitmapSetBit(priv->migrationCaps, cap); VIR_DEBUG("Found migration capability: '%s'", *capStr); } } @@ -1429,7 +1428,7 @@ qemuMigrationCapsCheck(virQEMUDriverPtr driver, if (!migEvent) goto cleanup; - ignore_value(virBitmapSetBit(migEvent, QEMU_MIGRATION_CAP_EVENTS)); + virBitmapSetBit(migEvent, QEMU_MIGRATION_CAP_EVENTS); if (!(json = qemuMigrationCapsToJSON(migEvent, migEvent))) goto cleanup; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 637361d..d62d29e 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1942,7 +1942,7 @@ qemuMonitorGetCpuHalted(qemuMonitorPtr mon, for (i = 0; i < ncpuentries; i++) { if (cpuentries[i].halted) - ignore_value(virBitmapSetBit(ret, cpuentries[i].qemu_id)); + virBitmapSetBit(ret, cpuentries[i].qemu_id); } cleanup: diff --git a/src/qemu/qemu_slirp.c b/src/qemu/qemu_slirp.c index 3efc34b..b3dba8d 100644 --- a/src/qemu/qemu_slirp.c +++ b/src/qemu/qemu_slirp.c @@ -66,7 +66,7 @@ void qemuSlirpSetFeature(qemuSlirpPtr slirp, qemuSlirpFeature feature) { - ignore_value(virBitmapSetBit(slirp->features, feature)); + virBitmapSetBit(slirp->features, feature); } diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 993f405..2a04ccb 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1364,7 +1364,7 @@ testOpenDefault(virConnectPtr conn) virBitmapPtr siblings = virBitmapNew(16); if (!siblings) goto error; - ignore_value(virBitmapSetBit(siblings, i)); + virBitmapSetBit(siblings, i); privconn->cells[i / 8].cpus[(i % 8)].id = i; privconn->cells[i / 8].cpus[(i % 8)].socket_id = i / 8; privconn->cells[i / 8].cpus[(i % 8)].core_id = i % 8; diff --git a/src/util/virbitmap.h b/src/util/virbitmap.h index 7f1a109..caafc86 100644 --- a/src/util/virbitmap.h +++ b/src/util/virbitmap.h @@ -51,7 +51,7 @@ int virBitmapCopy(virBitmapPtr dst, virBitmapPtr src); * Set bit position @b in @bitmap */ int virBitmapSetBit(virBitmapPtr bitmap, size_t b) - ATTRIBUTE_NONNULL(1) G_GNUC_WARN_UNUSED_RESULT; + ATTRIBUTE_NONNULL(1); int virBitmapSetBitExpand(virBitmapPtr bitmap, size_t b) ATTRIBUTE_NONNULL(1) G_GNUC_WARN_UNUSED_RESULT; diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 5ce69ef..dfc0e11 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -471,7 +471,7 @@ virCommandMassCloseGetFDsLinux(virCommandPtr cmd G_GNUC_UNUSED, goto cleanup; } - ignore_value(virBitmapSetBit(fds, fd)); + virBitmapSetBit(fds, fd); } if (rc < 0) diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index f60577b..ff1f3d9 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -629,7 +629,7 @@ static void dnsmasqCapsSet(dnsmasqCapsPtr caps, dnsmasqCapsFlags flag) { - ignore_value(virBitmapSetBit(caps->flags, flag)); + virBitmapSetBit(caps->flags, flag); } diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c index 6cea755..a8fdc78 100644 --- a/src/util/virhostcpu.c +++ b/src/util/virhostcpu.c @@ -269,7 +269,7 @@ virHostCPUGetSiblingsList(unsigned int cpu) /* If the file doesn't exist, the threadis its only sibling */ ret = virBitmapNew(cpu + 1); if (ret) - ignore_value(virBitmapSetBit(ret, cpu)); + virBitmapSetBit(ret, cpu); } return ret; diff --git a/src/util/virjson.c b/src/util/virjson.c index 6921ecc..cd6cc9d 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -1263,7 +1263,7 @@ virJSONValueGetArrayAsBitmap(const virJSONValue *val, /* second pass sets the correct bits in the map */ for (i = 0; i < val->data.array.nvalues; i++) - ignore_value(virBitmapSetBit(*bitmap, elems[i])); + virBitmapSetBit(*bitmap, elems[i]); return 0; } diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 41dc5bd..1705311 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -2828,7 +2828,7 @@ virNetDevRDMAFeature(const char *ifname, if (virFileReadAll(ib_devpath, RESOURCE_FILE_LEN, &ib_res_buf) > 0 && STREQ(eth_res_buf, ib_res_buf)) { - ignore_value(virBitmapSetBit(*out, VIR_NET_DEV_FEAT_RDMA)); + virBitmapSetBit(*out, VIR_NET_DEV_FEAT_RDMA); break; } } @@ -2943,7 +2943,7 @@ virNetDevGetEthtoolFeatures(virBitmapPtr bitmap, for (i = 0; i < G_N_ELEMENTS(ethtool_cmds); i++) { cmd.cmd = ethtool_cmds[i].cmd; if (virNetDevFeatureAvailable(fd, ifr, &cmd)) - ignore_value(virBitmapSetBit(bitmap, ethtool_cmds[i].feat)); + virBitmapSetBit(bitmap, ethtool_cmds[i].feat); } # if HAVE_DECL_ETHTOOL_GFLAGS @@ -2951,7 +2951,7 @@ virNetDevGetEthtoolFeatures(virBitmapPtr bitmap, if (virNetDevFeatureAvailable(fd, ifr, &cmd)) { for (i = 0; i < G_N_ELEMENTS(flags); i++) { if (cmd.data & flags[i].cmd) - ignore_value(virBitmapSetBit(bitmap, flags[i].feat)); + virBitmapSetBit(bitmap, flags[i].feat); } } # endif @@ -3103,7 +3103,7 @@ virNetDevSwitchdevFeature(const char *ifname, if (tb[DEVLINK_ATTR_ESWITCH_MODE] && *(int *)RTA_DATA(tb[DEVLINK_ATTR_ESWITCH_MODE]) == DEVLINK_ESWITCH_MODE_SWITCHDEV) { - ignore_value(virBitmapSetBit(*out, VIR_NET_DEV_FEAT_SWITCHDEV)); + virBitmapSetBit(*out, VIR_NET_DEV_FEAT_SWITCHDEV); } ret = 0; @@ -3159,7 +3159,7 @@ virNetDevGetEthtoolGFeatures(virBitmapPtr bitmap, g_cmd->cmd = ETHTOOL_GFEATURES; g_cmd->size = GFEATURES_SIZE; if (virNetDevGFeatureAvailable(fd, ifr, g_cmd)) - ignore_value(virBitmapSetBit(bitmap, VIR_NET_DEV_FEAT_TXUDPTNL)); + virBitmapSetBit(bitmap, VIR_NET_DEV_FEAT_TXUDPTNL); return 0; } # else diff --git a/src/util/virnuma.c b/src/util/virnuma.c index eeca438..1220743 100644 --- a/src/util/virnuma.c +++ b/src/util/virnuma.c @@ -286,7 +286,7 @@ virNumaGetNodeCPUs(int node, for (i = 0; i < max_n_cpus; i++) { if (MASK_CPU_ISSET(mask, i)) { - ignore_value(virBitmapSetBit(cpumap, i)); + virBitmapSetBit(cpumap, i); ncpus++; } } @@ -1035,7 +1035,7 @@ virNumaGetHostMemoryNodeset(void) /* if we can't detect NUMA node size assume that it's present */ if (virNumaGetNodeMemory(i, &nodesize, NULL) < 0 || nodesize > 0) - ignore_value(virBitmapSetBit(nodeset, i)); + virBitmapSetBit(nodeset, i); } return nodeset; diff --git a/src/util/virprocess.c b/src/util/virprocess.c index a9afa2e..5ff02fe 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -520,7 +520,7 @@ virProcessGetAffinity(pid_t pid) for (i = 0; i < ncpus; i++) { /* coverity[overrun-local] */ if (CPU_ISSET_S(i, masklen, mask)) - ignore_value(virBitmapSetBit(ret, i)); + virBitmapSetBit(ret, i); } cleanup: @@ -573,7 +573,7 @@ virProcessGetAffinity(pid_t pid) for (i = 0; i < sizeof(mask) * 8; i++) if (CPU_ISSET(i, &mask)) - ignore_value(virBitmapSetBit(ret, i)); + virBitmapSetBit(ret, i); return ret; } diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 3563fc5..5a947c7 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -2081,7 +2081,7 @@ virResctrlAllocFindUnused(virResctrlAllocPtr alloc, return -1; for (i = last_pos; i < last_pos + need_bits; i++) - ignore_value(virBitmapSetBit(a_mask, i)); + virBitmapSetBit(a_mask, i); if (virResctrlAllocUpdateMask(alloc, level, type, cache, a_mask) < 0) goto cleanup; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 00d8e16..17472f2 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -843,7 +843,7 @@ qcow2GetFeatures(virBitmapPtr *features, bits = virReadBufInt64BE(buf + QCOW2v3_HDR_FEATURES_COMPATIBLE); for (i = 0; i < QCOW2_COMPATIBLE_FEATURE_LAST; i++) { if (bits & ((uint64_t) 1 << i)) - ignore_value(virBitmapSetBit(feat, qcow2CompatibleFeatureArray[i])); + virBitmapSetBit(feat, qcow2CompatibleFeatureArray[i]); } *features = feat; diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index a123a88..16bd30b 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -1547,7 +1547,7 @@ virVMXParseConfig(virVMXContext *ctx, goto cleanup; } - ignore_value(virBitmapSetBit(def->cpumask, number)); + virBitmapSetBit(def->cpumask, number); } } diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 741e36a..d5f205b 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -2259,7 +2259,7 @@ testQemuMonitorJSONqemuMonitorJSONGetMigrationCapabilities(const void *opaque) if (!bitmap) goto cleanup; - ignore_value(virBitmapSetBit(bitmap, QEMU_MIGRATION_CAP_XBZRLE)); + virBitmapSetBit(bitmap, QEMU_MIGRATION_CAP_XBZRLE); if (!(json = qemuMigrationCapsToJSON(bitmap, bitmap))) goto cleanup; diff --git a/tests/testutils.c b/tests/testutils.c index a1cd093..be9347e 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -987,7 +987,7 @@ virTestCapsBuildNUMATopology(int seq) if (!(cell_cpus[core_id].siblings = virBitmapNew(MAX_CPUS_IN_CELL))) goto error; - ignore_value(virBitmapSetBit(cell_cpus[core_id].siblings, id)); + virBitmapSetBit(cell_cpus[core_id].siblings, id); } id++; diff --git a/tests/virbitmaptest.c b/tests/virbitmaptest.c index 1c7dc1d..0bc7c87 100644 --- a/tests/virbitmaptest.c +++ b/tests/virbitmaptest.c @@ -156,7 +156,7 @@ test3(const void *data G_GNUC_UNUSED) goto error; for (i = 0; i < size; i++) - ignore_value(virBitmapSetBit(bitmap, i)); + virBitmapSetBit(bitmap, i); if (!virBitmapIsAllSet(bitmap)) goto error; @@ -321,8 +321,8 @@ test5(const void *v G_GNUC_UNUSED) if (virBitmapNextSetBit(bitmap, j) > 0) goto error; - ignore_value(virBitmapSetBit(bitmap, 2)); - ignore_value(virBitmapSetBit(bitmap, 15)); + virBitmapSetBit(bitmap, 2); + virBitmapSetBit(bitmap, 15); if (virBitmapToData(bitmap, &data2, &len2) < 0) goto error; @@ -376,7 +376,7 @@ test6(const void *v G_GNUC_UNUSED) VIR_FREE(str); - ignore_value(virBitmapSetBit(bitmap, 0)); + virBitmapSetBit(bitmap, 0); str = virBitmapFormat(bitmap); if (!str) goto error; @@ -386,8 +386,8 @@ test6(const void *v G_GNUC_UNUSED) VIR_FREE(str); - ignore_value(virBitmapSetBit(bitmap, 4)); - ignore_value(virBitmapSetBit(bitmap, 5)); + virBitmapSetBit(bitmap, 4); + virBitmapSetBit(bitmap, 5); str = virBitmapFormat(bitmap); if (!str) goto error; @@ -397,7 +397,7 @@ test6(const void *v G_GNUC_UNUSED) VIR_FREE(str); - ignore_value(virBitmapSetBit(bitmap, 6)); + virBitmapSetBit(bitmap, 6); str = virBitmapFormat(bitmap); if (!str) goto error; @@ -407,10 +407,10 @@ test6(const void *v G_GNUC_UNUSED) VIR_FREE(str); - ignore_value(virBitmapSetBit(bitmap, 13)); - ignore_value(virBitmapSetBit(bitmap, 14)); - ignore_value(virBitmapSetBit(bitmap, 15)); - ignore_value(virBitmapSetBit(bitmap, 16)); + virBitmapSetBit(bitmap, 13); + virBitmapSetBit(bitmap, 14); + virBitmapSetBit(bitmap, 15); + virBitmapSetBit(bitmap, 16); str = virBitmapFormat(bitmap); if (!str) goto error; @@ -420,8 +420,8 @@ test6(const void *v G_GNUC_UNUSED) VIR_FREE(str); - ignore_value(virBitmapSetBit(bitmap, 62)); - ignore_value(virBitmapSetBit(bitmap, 63)); + virBitmapSetBit(bitmap, 62); + virBitmapSetBit(bitmap, 63); str = virBitmapFormat(bitmap); if (!str) goto error; @@ -455,7 +455,7 @@ test7(const void *v G_GNUC_UNUSED) if (virBitmapIsAllSet(bitmap)) goto error; - ignore_value(virBitmapSetBit(bitmap, 1)); + virBitmapSetBit(bitmap, 1); if (virBitmapIsAllSet(bitmap)) goto error; diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index aaf3b9a..60dcce3 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6845,7 +6845,7 @@ virshDomainGetVcpuBitmap(vshControl *ctl, if ((nnodes = virXPathNodeSet("/domain/vcpus/vcpu", ctxt, &nodes)) <= 0) { /* if the specific vcpu state is missing provide a fallback */ for (i = 0; i < curvcpus; i++) - ignore_value(virBitmapSetBit(ret, i)); + virBitmapSetBit(ret, i); goto cleanup; } @@ -6858,7 +6858,7 @@ virshDomainGetVcpuBitmap(vshControl *ctl, continue; if (STREQ(online, "yes")) - ignore_value(virBitmapSetBit(ret, vcpuid)); + virBitmapSetBit(ret, vcpuid); VIR_FREE(online); } diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c index f68c9c7..45c7e24 100644 --- a/tools/virt-host-validate-common.c +++ b/tools/virt-host-validate-common.c @@ -238,7 +238,7 @@ virBitmapPtr virHostValidateGetCPUFlags(void) int value; if ((value = virHostValidateCPUFlagTypeFromString(tokens[i])) >= 0) - ignore_value(virBitmapSetBit(flags, value)); + virBitmapSetBit(flags, value); } virStringListFreeCount(tokens, ntokens); -- 1.8.3.1

On Thu, Jul 30, 2020 at 17:27:36 +0300, Nikolay Shirokovskiy wrote: What's the justification for this change? We enforce the return value check so that the caller is aware that it can fail.

Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/conf/domain_addr.c | 2 +- src/conf/domain_conf.c | 5 ++--- src/network/bridge_driver.c | 4 ++-- src/qemu/qemu_capabilities.c | 2 +- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_hotplug.c | 2 +- src/qemu/qemu_migration_params.c | 3 +-- src/util/virbitmap.h | 2 +- 9 files changed, 11 insertions(+), 13 deletions(-) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index a28fba4..76c5f51 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -2460,7 +2460,7 @@ virDomainUSBAddressRelease(virDomainUSBAddressSetPtr addrs, portStr))) return -1; - ignore_value(virBitmapClearBit(targetHub->portmap, targetPort)); + virBitmapClearBit(targetHub->portmap, targetPort); return 0; } diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a067fb4..27b58df 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3286,10 +3286,9 @@ virDomainIOThreadIDDefArrayInit(virDomainDefPtr def, /* Clear 0 since we don't use it, then mark those which are * already provided by the user */ - ignore_value(virBitmapClearBit(thrmap, 0)); + virBitmapClearBit(thrmap, 0); for (i = 0; i < def->niothreadids; i++) - ignore_value(virBitmapClearBit(thrmap, - def->iothreadids[i]->iothread_id)); + virBitmapClearBit(thrmap, def->iothreadids[i]->iothread_id); /* resize array */ if (VIR_REALLOC_N(def->iothreadids, iothreads) < 0) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index c27a9c4..bf90de1 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -5058,7 +5058,7 @@ networkPlugBandwidthImpl(virNetworkObjPtr obj, virNetworkObjSetFloorSum(obj, tmp_floor_sum); /* update status file */ if (virNetworkObjSaveStatus(driver->stateDir, obj, network_driver->xmlopt) < 0) { - ignore_value(virBitmapClearBit(classIdMap, next_id)); + virBitmapClearBit(classIdMap, next_id); tmp_floor_sum -= ifaceBand->in->floor; virNetworkObjSetFloorSum(obj, tmp_floor_sum); *class_id = 0; @@ -5137,7 +5137,7 @@ networkUnplugBandwidth(virNetworkObjPtr obj, virNetworkObjSetFloorSum(obj, tmp_floor_sum); /* return class ID */ - ignore_value(virBitmapClearBit(classIdMap, *class_id)); + virBitmapClearBit(classIdMap, *class_id); /* update status file */ if (virNetworkObjSaveStatus(driver->stateDir, obj, network_driver->xmlopt) < 0) { diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 97bbadb..3520463 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -2017,7 +2017,7 @@ void virQEMUCapsClear(virQEMUCapsPtr qemuCaps, virQEMUCapsFlags flag) { - ignore_value(virBitmapClearBit(qemuCaps->flags, flag)); + virBitmapClearBit(qemuCaps->flags, flag); } diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 2058290..016daed 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -279,7 +279,7 @@ qemuDomainDisableNamespace(virDomainObjPtr vm, qemuDomainObjPrivatePtr priv = vm->privateData; if (priv->namespaces) { - ignore_value(virBitmapClearBit(priv->namespaces, ns)); + virBitmapClearBit(priv->namespaces, ns); if (virBitmapIsAllClear(priv->namespaces)) { virBitmapFree(priv->namespaces); priv->namespaces = NULL; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7d41c19..57a0f1e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -22475,7 +22475,7 @@ qemuDomainSetGuestVcpus(virDomainPtr dom, info[i].online = !!state; info[i].modified = true; - ignore_value(virBitmapClearBit(map, info[i].id)); + virBitmapClearBit(map, info[i].id); } if (!virBitmapIsAllClear(map)) { diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index d94f2e4..5a1f7cd 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -6455,7 +6455,7 @@ qemuDomainFilterHotplugVcpuEntities(virDomainDefPtr def, } /* clear the subthreads */ - ignore_value(virBitmapClearBit(map, i)); + virBitmapClearBit(map, i); } } diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c index 068f117..100fcdf 100644 --- a/src/qemu/qemu_migration_params.c +++ b/src/qemu/qemu_migration_params.c @@ -1453,8 +1453,7 @@ qemuMigrationCapsCheck(virQEMUDriverPtr driver, * migration capabilities bitmap makes sure it won't be touched anywhere * else. */ - ignore_value(virBitmapClearBit(priv->migrationCaps, - QEMU_MIGRATION_CAP_EVENTS)); + virBitmapClearBit(priv->migrationCaps, QEMU_MIGRATION_CAP_EVENTS); ret = 0; diff --git a/src/util/virbitmap.h b/src/util/virbitmap.h index caafc86..af7ed53 100644 --- a/src/util/virbitmap.h +++ b/src/util/virbitmap.h @@ -61,7 +61,7 @@ int virBitmapSetBitExpand(virBitmapPtr bitmap, size_t b) * Clear bit position @b in @bitmap */ int virBitmapClearBit(virBitmapPtr bitmap, size_t b) - ATTRIBUTE_NONNULL(1) G_GNUC_WARN_UNUSED_RESULT; + ATTRIBUTE_NONNULL(1); int virBitmapClearBitExpand(virBitmapPtr bitmap, size_t b) ATTRIBUTE_NONNULL(1) G_GNUC_WARN_UNUSED_RESULT; -- 1.8.3.1

Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_migration_cookie.c | 4 ++-- src/qemu/qemu_migration_params.c | 10 +++++----- src/util/virbitmap.h | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_migration_cookie.c b/src/qemu/qemu_migration_cookie.c index 6eaa704..2c95c31 100644 --- a/src/qemu/qemu_migration_cookie.c +++ b/src/qemu/qemu_migration_cookie.c @@ -756,8 +756,8 @@ qemuMigrationCookieCapsXMLFormat(virBufferPtr buf, bool supported = false; bool automatic = false; - ignore_value(virBitmapGetBit(caps->supported, cap, &supported)); - ignore_value(virBitmapGetBit(caps->automatic, cap, &automatic)); + virBitmapGetBit(caps->supported, cap, &supported); + virBitmapGetBit(caps->automatic, cap, &automatic); if (supported) { virBufferAsprintf(buf, "<cap name='%s' auto='%s'/>\n", qemuMigrationCapabilityTypeToString(cap), diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c index 100fcdf..572c4b1 100644 --- a/src/qemu/qemu_migration_params.c +++ b/src/qemu/qemu_migration_params.c @@ -786,11 +786,11 @@ qemuMigrationCapsToJSON(virBitmapPtr caps, bool supported = false; bool state = false; - ignore_value(virBitmapGetBit(caps, bit, &supported)); + virBitmapGetBit(caps, bit, &supported); if (!supported) continue; - ignore_value(virBitmapGetBit(states, bit, &state)); + virBitmapGetBit(states, bit, &state); cap = virJSONValueNewObject(); @@ -1164,7 +1164,7 @@ qemuMigrationParamsCheck(virQEMUDriverPtr driver, for (cap = 0; cap < QEMU_MIGRATION_CAP_LAST; cap++) { bool state = false; - ignore_value(virBitmapGetBit(migParams->caps, cap, &state)); + virBitmapGetBit(migParams->caps, cap, &state); if (state && !qemuMigrationCapsGet(vm, cap)) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, @@ -1183,7 +1183,7 @@ qemuMigrationParamsCheck(virQEMUDriverPtr driver, bool remote = false; if (remoteCaps) - ignore_value(virBitmapGetBit(remoteCaps, cap, &remote)); + virBitmapGetBit(remoteCaps, cap, &remote); if (!remote) { VIR_DEBUG("Not enabling migration capability '%s'; it is " @@ -1473,7 +1473,7 @@ qemuMigrationCapsGet(virDomainObjPtr vm, bool enabled = false; if (priv->migrationCaps) - ignore_value(virBitmapGetBit(priv->migrationCaps, cap, &enabled)); + virBitmapGetBit(priv->migrationCaps, cap, &enabled); return enabled; } diff --git a/src/util/virbitmap.h b/src/util/virbitmap.h index af7ed53..c0b95ad 100644 --- a/src/util/virbitmap.h +++ b/src/util/virbitmap.h @@ -75,7 +75,7 @@ bool virBitmapIsBitSet(virBitmapPtr bitmap, size_t b) * Get setting of bit position @b in @bitmap and store in @result */ int virBitmapGetBit(virBitmapPtr bitmap, size_t b, bool *result) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) G_GNUC_WARN_UNUSED_RESULT; + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3); virBitmapPtr virBitmapNewString(const char *string) -- 1.8.3.1

On Thu, Jul 30, 2020 at 17:27:35 +0300, Nikolay Shirokovskiy wrote:
Most of bitmap setBit/clearBit/getBit users know that the bitmap index is not out of bound and thus don't check the return value. More precisely the stats is next:
Method all check =================================== virBitmapSetBit 85 14 virBitmapClearBit 15 3 virBitmapGetBit 15 6
where 'all' is the number of all occurences of the method and 'check' is the number of occurences with 'if (method' pattern.
Thus keeping the retvalue checking requirement produces more noise then helps. I guess we even can make these function return void as users can simply compare the index with the bitmap size.
Well. An ignore_value is not really expensive and it makes the callers aware that something needs to be checked. I don't really see the point of this. Additionally, individual patches are missing justification in the commit message. Mentioning it in the cover letter is not enough as that doesn't get comitted.

On 30.07.2020 17:56, Peter Krempa wrote:
On Thu, Jul 30, 2020 at 17:27:35 +0300, Nikolay Shirokovskiy wrote:
Most of bitmap setBit/clearBit/getBit users know that the bitmap index is not out of bound and thus don't check the return value. More precisely the stats is next:
Method all check =================================== virBitmapSetBit 85 14 virBitmapClearBit 15 3 virBitmapGetBit 15 6
where 'all' is the number of all occurences of the method and 'check' is the number of occurences with 'if (method' pattern.
Thus keeping the retvalue checking requirement produces more noise then helps. I guess we even can make these function return void as users can simply compare the index with the bitmap size.
Well. An ignore_value is not really expensive and it makes the callers aware that something needs to be checked.
The only condition these methods can fail is out of bound. Most of time it is known by the caller that there is no out of bound condition. So when compiler suggests to check error I personally go and read the code only to found it can not fail in my circumstances. At the same time it is quite obvious that these function can not produce something meaningful for out of bound. That's why I even thinking of why don't make these methods return void.
I don't really see the point of this.
Additionally, individual patches are missing justification in the commit message. Mentioning it in the cover letter is not enough as that doesn't get comitted.
I thought that writing same justification 3 times in a row will be too much. At the same time writing some short version will not explain things thoroughly. May be I should add good explanation only to the first patch. Nikolay

On 30.07.2020 18:49, Nikolay Shirokovskiy wrote:
On 30.07.2020 17:56, Peter Krempa wrote:
On Thu, Jul 30, 2020 at 17:27:35 +0300, Nikolay Shirokovskiy wrote:
Most of bitmap setBit/clearBit/getBit users know that the bitmap index is not out of bound and thus don't check the return value. More precisely the stats is next:
Method all check =================================== virBitmapSetBit 85 14 virBitmapClearBit 15 3 virBitmapGetBit 15 6
where 'all' is the number of all occurences of the method and 'check' is the number of occurences with 'if (method' pattern.
Thus keeping the retvalue checking requirement produces more noise then helps. I guess we even can make these function return void as users can simply compare the index with the bitmap size.
Well. An ignore_value is not really expensive and it makes the callers aware that something needs to be checked.
The only condition these methods can fail is out of bound. Most of time it is known by the caller that there is no out of bound condition. So when compiler suggests to check error I personally go and read the code only to found it can not fail in my circumstances. At the same time it is quite obvious that these function can not produce something meaningful for out of bound. That's why I even thinking of why don't make these methods return void.
I don't really see the point of this.
Additionally, individual patches are missing justification in the commit message. Mentioning it in the cover letter is not enough as that doesn't get comitted.
I thought that writing same justification 3 times in a row will be too much. At the same time writing some short version will not explain things thoroughly. May be I should add good explanation only to the first patch.
Hi, everyone. Is there other opinions on the topic or I can forget about the series and let it go?) Nikolay
participants (2)
-
Nikolay Shirokovskiy
-
Peter Krempa