[libvirt] [PATCH 00/11] Replace virXXXFree calls with virObjectUnref
Based on some recent review comments and a bit of internal IRC, this set of patches will change all the existing virXXXFree calls found in daemon/* and src/* to be virObjectUnref instead and then add a rule to inhibit usage unless the string/call is found in docs/*, tests/*, examples/*, tools/*, cfg.mk, libvirt_public.syms, include/libvirt/libvirt-*.h, and src/libvirt-*.c Most of this was brute force to start with and adding the rule afterwards caught a few oddball places. The reason for not wanting to call a virXXXFree function is because it will reset the last error, potentially clearing something and resulting in a message "an error was encountered but the cause is unknown". There were some places in the code which saved the last error, called the free function, then restored the last error. Those have now been adjusted to avoid that processing. Anything that required checking for a non NULL pointer prior to calling the virXXXFree is now not necessary since the virObjectUnref will check for NULL before continuing; whereas, the virXXXFree functions didn't allow NULL. If usage of the virXXXFree function is found during syntax-check, a message such as the following will be displayed: include/libvirt/libvirt-storage.h:256:int virStoragePoolFree (virStoragePoolPtr pool); include/libvirt/libvirt-storage.h:336:int virStorageVolFree (virStorageVolPtr vol); maint.mk: avoid using virXXXFree, use virObjectUnref instead make: *** [sc_prohibit_virXXXFree] Error 1 John Ferlan (11): rpc: Replace virXXXFree with virObjectUnref Replace virDomainFree with virObjectUnref Replace virNetworkFree with virObjectUnref Replace virNodeDeviceFree with virObjectUnref Replace virStorageVolFree with virObjectUnref Replace virStoragePoolFree with virObjectUnref Replace virStreamFree with virObjectUnref Replace virSecretFree with virObjectUnref Replace virNWFilterFree with virObjectUnref Replace virInterfaceFree with virObjectUnref Replace virDomainSnapshotFree with virObjectUnref cfg.mk | 12 +++ daemon/remote.c | 168 ++++++++++++-------------------- daemon/stream.c | 2 +- src/conf/domain_event.c | 4 +- src/conf/network_conf.c | 6 +- src/conf/network_event.c | 2 +- src/conf/node_device_conf.c | 6 +- src/conf/storage_conf.c | 6 +- src/conf/virchrdev.c | 4 +- src/esx/esx_driver.c | 2 +- src/fdstream.c | 2 +- src/hyperv/hyperv_driver.c | 2 +- src/interface/interface_backend_netcf.c | 6 +- src/interface/interface_backend_udev.c | 2 +- src/libxl/libxl_conf.c | 7 +- src/locking/sanlock_helper.c | 3 +- src/lxc/lxc_driver.c | 8 +- src/lxc/lxc_process.c | 8 +- src/nwfilter/nwfilter_driver.c | 6 +- src/qemu/qemu_command.c | 8 +- src/qemu/qemu_driver.c | 4 +- src/qemu/qemu_hotplug.c | 8 +- src/remote/remote_driver.c | 76 +++++++-------- src/rpc/gendispatch.pl | 17 ++-- src/secret/secret_driver.c | 6 +- src/storage/storage_backend.c | 4 +- src/storage/storage_backend_fs.c | 4 +- src/storage/storage_backend_rbd.c | 3 +- src/storage/storage_driver.c | 20 +--- src/uml/uml_conf.c | 2 +- src/vbox/vbox_common.c | 6 +- src/xenconfig/xen_common.c | 2 +- src/xenconfig/xen_sxpr.c | 2 +- 33 files changed, 155 insertions(+), 263 deletions(-) -- 1.9.3
Modify the various virXXXFree calls to only call virObjectUnref. Calling the public API will reset the last error thus clearing out a pending error. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/rpc/gendispatch.pl | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl index 80f35b3..0dc167a 100755 --- a/src/rpc/gendispatch.pl +++ b/src/rpc/gendispatch.pl @@ -469,8 +469,7 @@ elsif ($mode eq "server") { " goto cleanup;\n"); push(@args_list, "dev"); push(@free_list, - " if (dev)\n" . - " virNodeDeviceFree(dev);"); + " virObjectUnref(dev);"); } foreach my $args_member (@{$call->{args_members}}) { @@ -486,8 +485,7 @@ elsif ($mode eq "server") { " goto cleanup;\n"); push(@args_list, "$2"); push(@free_list, - " if ($2)\n" . - " vir${type_name}Free($2);"); + " virObjectUnref($2);"); } elsif ($args_member =~ m/^remote_nonnull_domain_snapshot /) { push(@vars_list, "virDomainPtr dom = NULL"); push(@vars_list, "virDomainSnapshotPtr snapshot = NULL"); @@ -499,10 +497,8 @@ elsif ($mode eq "server") { " goto cleanup;\n"); push(@args_list, "snapshot"); push(@free_list, - " if (snapshot)\n" . - " virDomainSnapshotFree(snapshot);\n" . - " if (dom)\n" . - " virDomainFree(dom);"); + " virObjectUnref(snapshot);\n" . + " virObjectUnref(dom);"); } elsif ($args_member =~ m/^(?:remote_string|remote_uuid) (\S+)<\S+>;/) { if (! @args_list) { push(@args_list, "priv->conn"); @@ -694,8 +690,7 @@ elsif ($mode eq "server") { push(@vars_list, "vir${type_name}Ptr $2 = NULL"); push(@ret_list, "make_nonnull_$1(&ret->$2, $2);"); push(@free_list, - " if ($2)\n" . - " vir${type_name}Free($2);"); + " virObjectUnref($2);"); $single_ret_var = $2; $single_ret_by_ref = 0; $single_ret_check = " == NULL"; @@ -845,7 +840,7 @@ elsif ($mode eq "server") { push(@free_list_on_error, " virStreamAbort(st);"); push(@free_list_on_error, " daemonFreeClientStream(client, stream);"); push(@free_list_on_error, "} else {"); - push(@free_list_on_error, " virStreamFree(st);"); + push(@free_list_on_error, " virObjectUnref(st);"); push(@free_list_on_error, "}"); } -- 1.9.3
On 12/01/14 16:56, John Ferlan wrote:
Modify the various virXXXFree calls to only call virObjectUnref. Calling the public API will reset the last error thus clearing out a pending error.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/rpc/gendispatch.pl | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)
ACK for consistency, although the remote driver is saving the error prior to calling the *Free() apis that would clear it out. Peter
Since virDomainFree will call virObjectUnref anyway, let's just use that directly so as to avoid the possibility that we inadvertently clear out a pending error message when using the public API. Signed-off-by: John Ferlan <jferlan@redhat.com> --- cfg.mk | 12 ++++ daemon/remote.c | 131 +++++++++++++++---------------------------- src/conf/domain_event.c | 4 +- src/esx/esx_driver.c | 2 +- src/hyperv/hyperv_driver.c | 2 +- src/locking/sanlock_helper.c | 3 +- src/qemu/qemu_driver.c | 2 +- src/remote/remote_driver.c | 45 ++++++++------- src/vbox/vbox_common.c | 6 +- 9 files changed, 86 insertions(+), 121 deletions(-) diff --git a/cfg.mk b/cfg.mk index 3f35479..c772110 100644 --- a/cfg.mk +++ b/cfg.mk @@ -988,6 +988,15 @@ sc_prohibit_system_error_with_vir_err: halt='do not use virReportSystemError with VIR_ERR_* error codes' \ $(_sc_search_regexp) +# Rule to prohibit usage of virXXXFree within library, daemon, remote, etc. +# functions. There's a corresponding exclude to allow usage within tests, +# docs, examples, tools, src/libvirt-*.c, and include/libvirt/libvirt-*.h +sc_prohibit_virXXXFree: + @prohibit='\bvirDomainFree\b' \ + exclude='sc_prohibit_virXXXFree' \ + halt='avoid using 'virXXXFree', use 'virObjectUnref' instead' \ + $(_sc_search_regexp) + # We don't use this feature of maint.mk. prev_version_file = /dev/null @@ -1175,3 +1184,6 @@ exclude_file_name_regexp--sc_prohibit_useless_translation = \ exclude_file_name_regexp--sc_prohibit_devname = \ ^(tools/virsh.pod|cfg.mk|docs/.*)$$ + +exclude_file_name_regexp--sc_prohibit_virXXXFree = \ + ^(docs/|tests/|examples/|tools/|cfg.mk|src/libvirt_public.syms|include/libvirt/libvirt-domain.h|src/libvirt-(domain|qemu).c$$) diff --git a/daemon/remote.c b/daemon/remote.c index 75ae982..00b3e21 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -1347,8 +1347,7 @@ remoteDispatchDomainGetSchedulerType(virNetServerPtr server ATTRIBUTE_UNUSED, cleanup: if (rv < 0) virNetMessageSaveError(rerr); - if (dom) - virDomainFree(dom); + virObjectUnref(dom); return rv; } @@ -1555,8 +1554,7 @@ remoteDispatchDomainGetSchedulerParameters(virNetServerPtr server ATTRIBUTE_UNUS if (rv < 0) virNetMessageSaveError(rerr); virTypedParamsFree(params, nparams); - if (dom) - virDomainFree(dom); + virObjectUnref(dom); return rv; } @@ -1613,7 +1611,7 @@ remoteDispatchConnectListAllDomains(virNetServerPtr server ATTRIBUTE_UNUSED, virNetMessageSaveError(rerr); if (doms && ndomains > 0) { for (i = 0; i < ndomains; i++) - virDomainFree(doms[i]); + virObjectUnref(doms[i]); VIR_FREE(doms); } return rv; @@ -1666,8 +1664,7 @@ remoteDispatchDomainGetSchedulerParametersFlags(virNetServerPtr server ATTRIBUTE if (rv < 0) virNetMessageSaveError(rerr); virTypedParamsFree(params, nparams); - if (dom) - virDomainFree(dom); + virObjectUnref(dom); return rv; } @@ -1724,8 +1721,7 @@ remoteDispatchDomainMemoryStats(virNetServerPtr server ATTRIBUTE_UNUSED, cleanup: if (rv < 0) virNetMessageSaveError(rerr); - if (dom) - virDomainFree(dom); + virObjectUnref(dom); VIR_FREE(stats); return rv; } @@ -1780,8 +1776,7 @@ remoteDispatchDomainBlockPeek(virNetServerPtr server ATTRIBUTE_UNUSED, virNetMessageSaveError(rerr); VIR_FREE(ret->buffer.buffer_val); } - if (dom) - virDomainFree(dom); + virObjectUnref(dom); return rv; } @@ -1844,8 +1839,7 @@ remoteDispatchDomainBlockStatsFlags(virNetServerPtr server ATTRIBUTE_UNUSED, if (rv < 0) virNetMessageSaveError(rerr); virTypedParamsFree(params, nparams); - if (dom) - virDomainFree(dom); + virObjectUnref(dom); return rv; } @@ -1897,8 +1891,7 @@ remoteDispatchDomainMemoryPeek(virNetServerPtr server ATTRIBUTE_UNUSED, virNetMessageSaveError(rerr); VIR_FREE(ret->buffer.buffer_val); } - if (dom) - virDomainFree(dom); + virObjectUnref(dom); return rv; } @@ -1941,8 +1934,7 @@ remoteDispatchDomainGetSecurityLabel(virNetServerPtr server ATTRIBUTE_UNUSED, cleanup: if (rv < 0) virNetMessageSaveError(rerr); - if (dom) - virDomainFree(dom); + virObjectUnref(dom); VIR_FREE(seclabel); return rv; } @@ -2001,8 +1993,7 @@ remoteDispatchDomainGetSecurityLabelList(virNetServerPtr server ATTRIBUTE_UNUSED cleanup: if (rv < 0) virNetMessageSaveError(rerr); - if (dom) - virDomainFree(dom); + virObjectUnref(dom); VIR_FREE(seclabels); return rv; } @@ -2107,8 +2098,7 @@ remoteDispatchDomainGetVcpuPinInfo(virNetServerPtr server ATTRIBUTE_UNUSED, if (rv < 0) virNetMessageSaveError(rerr); VIR_FREE(cpumaps); - if (dom) - virDomainFree(dom); + virObjectUnref(dom); return rv; } @@ -2143,8 +2133,7 @@ remoteDispatchDomainPinEmulator(virNetServerPtr server ATTRIBUTE_UNUSED, cleanup: if (rv < 0) virNetMessageSaveError(rerr); - if (dom) - virDomainFree(dom); + virObjectUnref(dom); return rv; } @@ -2194,8 +2183,7 @@ remoteDispatchDomainGetEmulatorPinInfo(virNetServerPtr server ATTRIBUTE_UNUSED, if (rv < 0) virNetMessageSaveError(rerr); VIR_FREE(cpumaps); - if (dom) - virDomainFree(dom); + virObjectUnref(dom); return rv; } @@ -2276,8 +2264,7 @@ remoteDispatchDomainGetVcpus(virNetServerPtr server ATTRIBUTE_UNUSED, } VIR_FREE(cpumaps); VIR_FREE(info); - if (dom) - virDomainFree(dom); + virObjectUnref(dom); return rv; } @@ -2446,8 +2433,7 @@ remoteDispatchDomainGetMemoryParameters(virNetServerPtr server ATTRIBUTE_UNUSED, if (rv < 0) virNetMessageSaveError(rerr); virTypedParamsFree(params, nparams); - if (dom) - virDomainFree(dom); + virObjectUnref(dom); return rv; } @@ -2509,8 +2495,7 @@ remoteDispatchDomainGetNumaParameters(virNetServerPtr server ATTRIBUTE_UNUSED, if (rv < 0) virNetMessageSaveError(rerr); virTypedParamsFree(params, nparams); - if (dom) - virDomainFree(dom); + virObjectUnref(dom); return rv; } @@ -2572,8 +2557,7 @@ remoteDispatchDomainGetBlkioParameters(virNetServerPtr server ATTRIBUTE_UNUSED, if (rv < 0) virNetMessageSaveError(rerr); virTypedParamsFree(params, nparams); - if (dom) - virDomainFree(dom); + virObjectUnref(dom); return rv; } @@ -2757,8 +2741,7 @@ remoteDispatchDomainGetBlockJobInfo(virNetServerPtr server ATTRIBUTE_UNUSED, cleanup: if (rv < 0) virNetMessageSaveError(rerr); - if (dom) - virDomainFree(dom); + virObjectUnref(dom); return rv; } @@ -2820,8 +2803,7 @@ remoteDispatchDomainGetBlockIoTune(virNetServerPtr server ATTRIBUTE_UNUSED, if (rv < 0) virNetMessageSaveError(rerr); virTypedParamsFree(params, nparams); - if (dom) - virDomainFree(dom); + virObjectUnref(dom); return rv; } @@ -3618,8 +3600,7 @@ remoteDispatchDomainGetState(virNetServerPtr server ATTRIBUTE_UNUSED, cleanup: if (rv < 0) virNetMessageSaveError(rerr); - if (dom) - virDomainFree(dom); + virObjectUnref(dom); return rv; } @@ -3774,8 +3755,7 @@ remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr server ATTRI VIR_FREE(callback); if (rv < 0) virNetMessageSaveError(rerr); - if (dom) - virDomainFree(dom); + virObjectUnref(dom); virMutexUnlock(&priv->lock); return rv; } @@ -3915,8 +3895,7 @@ qemuDispatchDomainMonitorCommand(virNetServerPtr server ATTRIBUTE_UNUSED, cleanup: if (rv < 0) virNetMessageSaveError(rerr); - if (dom) - virDomainFree(dom); + virObjectUnref(dom); return rv; } @@ -3967,8 +3946,7 @@ remoteDispatchDomainMigrateBegin3(virNetServerPtr server ATTRIBUTE_UNUSED, cleanup: if (rv < 0) virNetMessageSaveError(rerr); - if (dom) - virDomainFree(dom); + virObjectUnref(dom); return rv; } @@ -4079,8 +4057,7 @@ remoteDispatchDomainMigratePerform3(virNetServerPtr server ATTRIBUTE_UNUSED, cleanup: if (rv < 0) virNetMessageSaveError(rerr); - if (dom) - virDomainFree(dom); + virObjectUnref(dom); return rv; } @@ -4133,8 +4110,7 @@ remoteDispatchDomainMigrateFinish3(virNetServerPtr server ATTRIBUTE_UNUSED, virNetMessageSaveError(rerr); VIR_FREE(cookieout); } - if (dom) - virDomainFree(dom); + virObjectUnref(dom); return rv; } @@ -4170,8 +4146,7 @@ remoteDispatchDomainMigrateConfirm3(virNetServerPtr server ATTRIBUTE_UNUSED, cleanup: if (rv < 0) virNetMessageSaveError(rerr); - if (dom) - virDomainFree(dom); + virObjectUnref(dom); return rv; } @@ -4262,8 +4237,7 @@ remoteDispatchDomainOpenGraphics(virNetServerPtr server ATTRIBUTE_UNUSED, VIR_FORCE_CLOSE(fd); if (rv < 0) virNetMessageSaveError(rerr); - if (dom) - virDomainFree(dom); + virObjectUnref(dom); return rv; } @@ -4306,8 +4280,7 @@ remoteDispatchDomainOpenGraphicsFd(virNetServerPtr server ATTRIBUTE_UNUSED, if (rv < 0) virNetMessageSaveError(rerr); - if (dom) - virDomainFree(dom); + virObjectUnref(dom); return rv; } @@ -4371,8 +4344,7 @@ remoteDispatchDomainGetInterfaceParameters(virNetServerPtr server ATTRIBUTE_UNUS if (rv < 0) virNetMessageSaveError(rerr); virTypedParamsFree(params, nparams); - if (dom) - virDomainFree(dom); + virObjectUnref(dom); return rv; } @@ -4443,8 +4415,7 @@ remoteDispatchDomainGetCPUStats(virNetServerPtr server ATTRIBUTE_UNUSED, if (rv < 0) virNetMessageSaveError(rerr); virTypedParamsFree(params, args->ncpus * args->nparams); - if (dom) - virDomainFree(dom); + virObjectUnref(dom); return rv; } @@ -4498,8 +4469,7 @@ remoteDispatchDomainGetDiskErrors(virNetServerPtr server ATTRIBUTE_UNUSED, cleanup: if (rv < 0) virNetMessageSaveError(rerr); - if (dom) - virDomainFree(dom); + virObjectUnref(dom); if (errors && len > 0) { size_t i; for (i = 0; i < len; i++) @@ -4564,8 +4534,7 @@ remoteDispatchDomainListAllSnapshots(virNetServerPtr server ATTRIBUTE_UNUSED, cleanup: if (rv < 0) virNetMessageSaveError(rerr); - if (dom) - virDomainFree(dom); + virObjectUnref(dom); if (snaps && nsnaps > 0) { for (i = 0; i < nsnaps; i++) virDomainSnapshotFree(snaps[i]); @@ -4635,8 +4604,7 @@ remoteDispatchDomainSnapshotListAllChildren(virNetServerPtr server ATTRIBUTE_UNU virNetMessageSaveError(rerr); if (snapshot) virDomainSnapshotFree(snapshot); - if (dom) - virDomainFree(dom); + virObjectUnref(dom); if (snaps && nsnaps > 0) { for (i = 0; i < nsnaps; i++) virDomainSnapshotFree(snaps[i]); @@ -5214,8 +5182,7 @@ lxcDispatchDomainOpenNamespace(virNetServerPtr server ATTRIBUTE_UNUSED, cleanup: if (rv < 0) virNetMessageSaveError(rerr); - if (dom) - virDomainFree(dom); + virObjectUnref(dom); return rv; } @@ -5265,8 +5232,7 @@ remoteDispatchDomainGetJobStats(virNetServerPtr server ATTRIBUTE_UNUSED, if (rv < 0) virNetMessageSaveError(rerr); virTypedParamsFree(params, nparams); - if (dom) - virDomainFree(dom); + virObjectUnref(dom); return rv; } @@ -5323,8 +5289,7 @@ remoteDispatchDomainMigrateBegin3Params(virNetServerPtr server ATTRIBUTE_UNUSED, virTypedParamsFree(params, nparams); if (rv < 0) virNetMessageSaveError(rerr); - if (dom) - virDomainFree(dom); + virObjectUnref(dom); return rv; } @@ -5514,8 +5479,7 @@ remoteDispatchDomainMigratePerform3Params(virNetServerPtr server ATTRIBUTE_UNUSE virTypedParamsFree(params, nparams); if (rv < 0) virNetMessageSaveError(rerr); - if (dom) - virDomainFree(dom); + virObjectUnref(dom); return rv; } @@ -5575,8 +5539,7 @@ remoteDispatchDomainMigrateFinish3Params(virNetServerPtr server ATTRIBUTE_UNUSED virNetMessageSaveError(rerr); VIR_FREE(cookieout); } - if (dom) - virDomainFree(dom); + virObjectUnref(dom); return rv; } @@ -5627,8 +5590,7 @@ remoteDispatchDomainMigrateConfirm3Params(virNetServerPtr server ATTRIBUTE_UNUSE virTypedParamsFree(params, nparams); if (rv < 0) virNetMessageSaveError(rerr); - if (dom) - virDomainFree(dom); + virObjectUnref(dom); return rv; } @@ -5728,8 +5690,7 @@ remoteDispatchDomainCreateXMLWithFiles(virNetServerPtr server ATTRIBUTE_UNUSED, VIR_FREE(files); if (rv < 0) virNetMessageSaveError(rerr); - if (dom) - virDomainFree(dom); + virObjectUnref(dom); return rv; } @@ -5779,8 +5740,7 @@ static int remoteDispatchDomainCreateWithFiles(virNetServerPtr server ATTRIBUTE_ VIR_FREE(files); if (rv < 0) virNetMessageSaveError(rerr); - if (dom) - virDomainFree(dom); + virObjectUnref(dom); return rv; } @@ -5975,8 +5935,7 @@ qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr server ATTRIBUTE_U VIR_FREE(callback); if (rv < 0) virNetMessageSaveError(rerr); - if (dom) - virDomainFree(dom); + virObjectUnref(dom); virMutexUnlock(&priv->lock); return rv; } @@ -6061,8 +6020,7 @@ remoteDispatchDomainGetTime(virNetServerPtr server ATTRIBUTE_UNUSED, cleanup: if (rv < 0) virNetMessageSaveError(rerr); - if (dom) - virDomainFree(dom); + virObjectUnref(dom); return rv; } @@ -6475,8 +6433,7 @@ remoteDispatchDomainGetFSInfo(virNetServerPtr server ATTRIBUTE_UNUSED, VIR_FREE(ret->info.info_val); } } - if (dom) - virDomainFree(dom); + virObjectUnref(dom); if (ninfo >= 0) for (i = 0; i < ninfo; i++) virDomainFSInfoFree(info[i]); diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c index d1042bf..2786c1e 100644 --- a/src/conf/domain_event.c +++ b/src/conf/domain_event.c @@ -1544,7 +1544,7 @@ virDomainEventDispatchDefaultFunc(virConnectPtr conn, VIR_WARN("Unexpected event ID %d", event->eventID); cleanup: - virDomainFree(dom); + virObjectUnref(dom); } @@ -1618,7 +1618,7 @@ virDomainQemuMonitorEventDispatchFunc(virConnectPtr conn, qemuMonitorEvent->micros, qemuMonitorEvent->details, data->opaque); - virDomainFree(dom); + virObjectUnref(dom); } diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 1cbebb9..e45ae2d 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -5121,7 +5121,7 @@ esxConnectListAllDomains(virConnectPtr conn, cleanup: if (doms) { for (id = 0; id < count; id++) - virDomainFree(doms[id]); + virObjectUnref(doms[id]); VIR_FREE(doms); } diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index ece943e..ea9ba85 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -1311,7 +1311,7 @@ hypervConnectListAllDomains(virConnectPtr conn, cleanup: if (doms) { for (i = 0; i < count; ++i) - virDomainFree(doms[i]); + virObjectUnref(doms[i]); VIR_FREE(doms); } diff --git a/src/locking/sanlock_helper.c b/src/locking/sanlock_helper.c index 52db6cb..d8d294f 100644 --- a/src/locking/sanlock_helper.c +++ b/src/locking/sanlock_helper.c @@ -106,8 +106,7 @@ main(int argc, char **argv) } cleanup: - if (dom) - virDomainFree(dom); + virObjectUnref(dom); if (conn) virConnectClose(conn); VIR_FREE(xml); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a956c59..8d4b393 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1010,7 +1010,7 @@ qemuStateStop(void) cleanup: if (domains) { for (i = 0; i < numDomains; i++) - virDomainFree(domains[i]); + virObjectUnref(domains[i]); VIR_FREE(domains); } VIR_FREE(flags); diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 22f0c88..48e2e25 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -1645,8 +1645,7 @@ remoteConnectListAllDomains(virConnectPtr conn, cleanup: if (doms) { for (i = 0; i < ret.domains.domains_len; i++) - if (doms[i]) - virDomainFree(doms[i]); + virObjectUnref(doms[i]); VIR_FREE(doms); } @@ -4764,7 +4763,7 @@ remoteDomainBuildEventLifecycleHelper(virConnectPtr conn, return; event = virDomainEventLifecycleNewFromDom(dom, msg->event, msg->detail); - virDomainFree(dom); + virObjectUnref(dom); remoteEventQueue(priv, event, callbackID); } @@ -4802,7 +4801,7 @@ remoteDomainBuildEventRebootHelper(virConnectPtr conn, return; event = virDomainEventRebootNewFromDom(dom); - virDomainFree(dom); + virObjectUnref(dom); remoteEventQueue(priv, event, callbackID); } @@ -4839,7 +4838,7 @@ remoteDomainBuildEventRTCChangeHelper(virConnectPtr conn, return; event = virDomainEventRTCChangeNewFromDom(dom, msg->offset); - virDomainFree(dom); + virObjectUnref(dom); remoteEventQueue(priv, event, callbackID); } @@ -4876,7 +4875,7 @@ remoteDomainBuildEventWatchdogHelper(virConnectPtr conn, return; event = virDomainEventWatchdogNewFromDom(dom, msg->action); - virDomainFree(dom); + virObjectUnref(dom); remoteEventQueue(priv, event, callbackID); } @@ -4916,7 +4915,7 @@ remoteDomainBuildEventIOErrorHelper(virConnectPtr conn, msg->srcPath, msg->devAlias, msg->action); - virDomainFree(dom); + virObjectUnref(dom); remoteEventQueue(priv, event, callbackID); } @@ -4958,7 +4957,7 @@ remoteDomainBuildEventIOErrorReasonHelper(virConnectPtr conn, msg->action, msg->reason); - virDomainFree(dom); + virObjectUnref(dom); remoteEventQueue(priv, event, callbackID); } @@ -4997,7 +4996,7 @@ remoteDomainBuildEventBlockJobHelper(virConnectPtr conn, event = virDomainEventBlockJobNewFromDom(dom, msg->path, msg->type, msg->status); - virDomainFree(dom); + virObjectUnref(dom); remoteEventQueue(priv, event, callbackID); } @@ -5037,7 +5036,7 @@ remoteDomainBuildEventBlockJob2(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, event = virDomainEventBlockJob2NewFromDom(dom, msg->dst, msg->type, msg->status); - virDomainFree(dom); + virObjectUnref(dom); remoteEventQueue(priv, event, msg->callbackID); } @@ -5091,7 +5090,7 @@ remoteDomainBuildEventGraphicsHelper(virConnectPtr conn, msg->authScheme, subject); - virDomainFree(dom); + virObjectUnref(dom); remoteEventQueue(priv, event, callbackID); return; @@ -5115,7 +5114,7 @@ remoteDomainBuildEventGraphicsHelper(virConnectPtr conn, VIR_FREE(subject->identities); VIR_FREE(subject); } - virDomainFree(dom); + virObjectUnref(dom); return; } static void @@ -5152,7 +5151,7 @@ remoteDomainBuildEventControlErrorHelper(virConnectPtr conn, event = virDomainEventControlErrorNewFromDom(dom); - virDomainFree(dom); + virObjectUnref(dom); remoteEventQueue(priv, event, callbackID); } @@ -5195,7 +5194,7 @@ remoteDomainBuildEventDiskChangeHelper(virConnectPtr conn, msg->devAlias, msg->reason); - virDomainFree(dom); + virObjectUnref(dom); remoteEventQueue(priv, event, callbackID); } @@ -5236,7 +5235,7 @@ remoteDomainBuildEventTrayChangeHelper(virConnectPtr conn, msg->devAlias, msg->reason); - virDomainFree(dom); + virObjectUnref(dom); remoteEventQueue(priv, event, callbackID); } @@ -5275,7 +5274,7 @@ remoteDomainBuildEventPMWakeupHelper(virConnectPtr conn, event = virDomainEventPMWakeupNewFromDom(dom, reason); - virDomainFree(dom); + virObjectUnref(dom); remoteEventQueue(priv, event, callbackID); } @@ -5315,7 +5314,7 @@ remoteDomainBuildEventPMSuspendHelper(virConnectPtr conn, event = virDomainEventPMSuspendNewFromDom(dom, reason); - virDomainFree(dom); + virObjectUnref(dom); remoteEventQueue(priv, event, callbackID); } @@ -5354,7 +5353,7 @@ remoteDomainBuildEventBalloonChangeHelper(virConnectPtr conn, return; event = virDomainEventBalloonChangeNewFromDom(dom, msg->actual); - virDomainFree(dom); + virObjectUnref(dom); remoteEventQueue(priv, event, callbackID); } @@ -5394,7 +5393,7 @@ remoteDomainBuildEventPMSuspendDiskHelper(virConnectPtr conn, event = virDomainEventPMSuspendDiskNewFromDom(dom, reason); - virDomainFree(dom); + virObjectUnref(dom); remoteEventQueue(priv, event, callbackID); } @@ -5434,7 +5433,7 @@ remoteDomainBuildEventDeviceRemovedHelper(virConnectPtr conn, event = virDomainEventDeviceRemovedNewFromDom(dom, msg->devAlias); - virDomainFree(dom); + virObjectUnref(dom); remoteEventQueue(priv, event, callbackID); } @@ -5485,7 +5484,7 @@ remoteDomainBuildEventCallbackTunable(virNetClientProgramPtr prog ATTRIBUTE_UNUS event = virDomainEventTunableNewFromDom(dom, params, nparams); - virDomainFree(dom); + virObjectUnref(dom); remoteEventQueue(priv, event, msg->callbackID); } @@ -5508,7 +5507,7 @@ remoteDomainBuildEventCallbackAgentLifecycle(virNetClientProgramPtr prog ATTRIBU event = virDomainEventAgentLifecycleNewFromDom(dom, msg->state, msg->reason); - virDomainFree(dom); + virObjectUnref(dom); remoteEventQueue(priv, event, msg->callbackID); } @@ -5555,7 +5554,7 @@ remoteDomainBuildQemuMonitorEvent(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, msg->event, msg->seconds, msg->micros, msg->details ? *msg->details : NULL); - virDomainFree(dom); + virObjectUnref(dom); remoteEventQueue(priv, event, msg->callbackID); } diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index bffec82..e3d89f3 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -7497,10 +7497,8 @@ vboxConnectListAllDomains(virConnectPtr conn, cleanup: if (doms) { - for (i = 0; i < count; i++) { - if (doms[i]) - virDomainFree(doms[i]); - } + for (i = 0; i < count; i++) + virObjectUnref(doms[i]); } VIR_FREE(doms); -- 1.9.3
On 12/01/14 16:56, John Ferlan wrote:
Since virDomainFree will call virObjectUnref anyway, let's just use that directly so as to avoid the possibility that we inadvertently clear out a pending error message when using the public API.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- cfg.mk | 12 ++++ daemon/remote.c | 131 +++++++++++++++---------------------------- src/conf/domain_event.c | 4 +- src/esx/esx_driver.c | 2 +- src/hyperv/hyperv_driver.c | 2 +- src/locking/sanlock_helper.c | 3 +- src/qemu/qemu_driver.c | 2 +- src/remote/remote_driver.c | 45 ++++++++------- src/vbox/vbox_common.c | 6 +- 9 files changed, 86 insertions(+), 121 deletions(-)
ACK, the remote driver was okay regarding rewriting of the error, although few of the *ListAllDomain functions might have the error overwritten. Peter
Since virNetworkFree will call virObjectUnref anyway, let's just use that directly so as to avoid the possibility that we inadvertently clear out a pending error message when using the public API. Signed-off-by: John Ferlan <jferlan@redhat.com> --- cfg.mk | 4 ++-- daemon/remote.c | 7 +++---- src/conf/network_conf.c | 6 ++---- src/conf/network_event.c | 2 +- src/libxl/libxl_conf.c | 7 +------ src/lxc/lxc_driver.c | 8 +------- src/lxc/lxc_process.c | 8 +------- src/qemu/qemu_command.c | 8 +------- src/qemu/qemu_hotplug.c | 8 +------- src/remote/remote_driver.c | 5 ++--- src/uml/uml_conf.c | 2 +- src/xenconfig/xen_common.c | 2 +- src/xenconfig/xen_sxpr.c | 2 +- 13 files changed, 18 insertions(+), 51 deletions(-) diff --git a/cfg.mk b/cfg.mk index c772110..b5f853b 100644 --- a/cfg.mk +++ b/cfg.mk @@ -992,7 +992,7 @@ sc_prohibit_system_error_with_vir_err: # functions. There's a corresponding exclude to allow usage within tests, # docs, examples, tools, src/libvirt-*.c, and include/libvirt/libvirt-*.h sc_prohibit_virXXXFree: - @prohibit='\bvirDomainFree\b' \ + @prohibit='\bvir(Domain|Network)Free\b' \ exclude='sc_prohibit_virXXXFree' \ halt='avoid using 'virXXXFree', use 'virObjectUnref' instead' \ $(_sc_search_regexp) @@ -1186,4 +1186,4 @@ exclude_file_name_regexp--sc_prohibit_devname = \ ^(tools/virsh.pod|cfg.mk|docs/.*)$$ exclude_file_name_regexp--sc_prohibit_virXXXFree = \ - ^(docs/|tests/|examples/|tools/|cfg.mk|src/libvirt_public.syms|include/libvirt/libvirt-domain.h|src/libvirt-(domain|qemu).c$$) + ^(docs/|tests/|examples/|tools/|cfg.mk|src/libvirt_public.syms|include/libvirt/libvirt-(domain|network).h|src/libvirt-(domain|qemu|network).c$$) diff --git a/daemon/remote.c b/daemon/remote.c index 00b3e21..edae335 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -4790,7 +4790,7 @@ remoteDispatchConnectListAllNetworks(virNetServerPtr server ATTRIBUTE_UNUSED, virNetMessageSaveError(rerr); if (nets && nnets > 0) { for (i = 0; i < nnets; i++) - virNetworkFree(nets[i]); + virObjectUnref(nets[i]); VIR_FREE(nets); } return rv; @@ -5816,8 +5816,7 @@ remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server ATTRIBUTE_UN VIR_FREE(callback); if (rv < 0) virNetMessageSaveError(rerr); - if (net) - virNetworkFree(net); + virObjectUnref(net); virMutexUnlock(&priv->lock); return rv; } @@ -6202,7 +6201,7 @@ remoteDispatchNetworkGetDHCPLeases(virNetServerPtr server ATTRIBUTE_UNUSED, virNetworkDHCPLeaseFree(leases[i]); VIR_FREE(leases); } - virNetworkFree(net); + virObjectUnref(net); return rv; } diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 067334e..a249e32 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -4463,10 +4463,8 @@ virNetworkObjListExport(virConnectPtr conn, cleanup: if (tmp_nets) { - for (i = 0; i < nnets; i++) { - if (tmp_nets[i]) - virNetworkFree(tmp_nets[i]); - } + for (i = 0; i < nnets; i++) + virObjectUnref(tmp_nets[i]); } VIR_FREE(tmp_nets); diff --git a/src/conf/network_event.c b/src/conf/network_event.c index 991591a..8623940 100644 --- a/src/conf/network_event.c +++ b/src/conf/network_event.c @@ -119,7 +119,7 @@ virNetworkEventDispatchDefaultFunc(virConnectPtr conn, VIR_WARN("Unexpected event ID %d", event->eventID); cleanup: - virNetworkFree(net); + virObjectUnref(net); } diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index e296ffc..0555b91 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -1058,7 +1058,6 @@ libxlMakeNic(virDomainDefPtr def, char *brname = NULL; virNetworkPtr network; virConnectPtr conn; - virErrorPtr errobj; if (!(conn = virConnectOpen("xen:///system"))) return -1; @@ -1078,11 +1077,7 @@ libxlMakeNic(virDomainDefPtr def, VIR_FREE(brname); - /* Preserve any previous failure */ - errobj = virSaveLastError(); - virNetworkFree(network); - virSetError(errobj); - virFreeError(errobj); + virObjectUnref(network); virObjectUnref(conn); if (fail) return -1; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 93db1ee..97caee3 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -4178,19 +4178,13 @@ lxcDomainAttachDeviceNetLive(virConnectPtr conn, virNetworkPtr network; char *brname = NULL; bool fail = false; - virErrorPtr errobj; if (!(network = virNetworkLookupByName(conn, net->data.network.name))) goto cleanup; if (!(brname = virNetworkGetBridgeName(network))) fail = true; - /* Make sure any above failure is preserved */ - errobj = virSaveLastError(); - virNetworkFree(network); - virSetError(errobj); - virFreeError(errobj); - + virObjectUnref(network); if (fail) goto cleanup; diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 9208f02..de574a9 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -386,7 +386,6 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn, virNetworkPtr network; char *brname = NULL; bool fail = false; - virErrorPtr errobj; if (!(network = virNetworkLookupByName(conn, net->data.network.name))) @@ -394,12 +393,7 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn, if (!(brname = virNetworkGetBridgeName(network))) fail = true; - /* Make sure any above failure is preserved */ - errobj = virSaveLastError(); - virNetworkFree(network); - virSetError(errobj); - virFreeError(errobj); - + virObjectUnref(network); if (fail) goto cleanup; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 4ed6506..1831323 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -298,7 +298,6 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK) { bool fail = false; - virErrorPtr errobj; virNetworkPtr network = virNetworkLookupByName(conn, net->data.network.name); if (!network) @@ -307,12 +306,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, if (!(brname = virNetworkGetBridgeName(network))) fail = true; - /* Make sure any above failure is preserved */ - errobj = virSaveLastError(); - virNetworkFree(network); - virSetError(errobj); - virFreeError(errobj); - + virObjectUnref(network); if (fail) return ret; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index f2740f4..1971b0a 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1763,7 +1763,6 @@ qemuDomainNetGetBridgeName(virConnectPtr conn, virDomainNetDefPtr net) if (VIR_STRDUP(brname, tmpbr) < 0) goto cleanup; } else if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK) { - virErrorPtr errobj; virNetworkPtr network; if (!(network = virNetworkLookupByName(conn, net->data.network.name))) { @@ -1774,12 +1773,7 @@ qemuDomainNetGetBridgeName(virConnectPtr conn, virDomainNetDefPtr net) } brname = virNetworkGetBridgeName(network); - /* Make sure any above failure is preserved */ - errobj = virSaveLastError(); - virNetworkFree(network); - virSetError(errobj); - virFreeError(errobj); - + virObjectUnref(network); } else { virReportError(VIR_ERR_INTERNAL_ERROR, _("Interface type %d has no bridge name"), diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 48e2e25..c23a087 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -3063,8 +3063,7 @@ remoteConnectListAllNetworks(virConnectPtr conn, cleanup: if (tmp_nets) { for (i = 0; i < ret.nets.nets_len; i++) - if (tmp_nets[i]) - virNetworkFree(tmp_nets[i]); + virObjectUnref(tmp_nets[i]); VIR_FREE(tmp_nets); } @@ -5529,7 +5528,7 @@ remoteNetworkBuildEventLifecycle(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, event = virNetworkEventLifecycleNew(net->name, net->uuid, msg->event, msg->detail); - virNetworkFree(net); + virObjectUnref(net); remoteEventQueue(priv, event, msg->callbackID); } diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c index 9dcd4ae..7a5d62b 100644 --- a/src/uml/uml_conf.c +++ b/src/uml/uml_conf.c @@ -213,7 +213,7 @@ umlBuildCommandLineNet(virConnectPtr conn, goto error; } bridge = virNetworkGetBridgeName(network); - virNetworkFree(network); + virObjectUnref(network); if (bridge == NULL) goto error; diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index 29c6c34..7f4ec89 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -1245,7 +1245,7 @@ xenFormatNet(virConnectPtr conn, return -1; } bridge = virNetworkGetBridgeName(network); - virNetworkFree(network); + virObjectUnref(network); if (!bridge) { virReportError(VIR_ERR_INTERNAL_ERROR, _("network %s is not active"), diff --git a/src/xenconfig/xen_sxpr.c b/src/xenconfig/xen_sxpr.c index a667814..d8783e9 100644 --- a/src/xenconfig/xen_sxpr.c +++ b/src/xenconfig/xen_sxpr.c @@ -1915,7 +1915,7 @@ xenFormatSxprNet(virConnectPtr conn, } bridge = virNetworkGetBridgeName(network); - virNetworkFree(network); + virObjectUnref(network); if (!bridge) { virReportError(VIR_ERR_INTERNAL_ERROR, _("network %s is not active"), -- 1.9.3
On 12/01/14 16:56, John Ferlan wrote:
Since virNetworkFree will call virObjectUnref anyway, let's just use that directly so as to avoid the possibility that we inadvertently clear out a pending error message when using the public API.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- cfg.mk | 4 ++-- daemon/remote.c | 7 +++---- src/conf/network_conf.c | 6 ++---- src/conf/network_event.c | 2 +- src/libxl/libxl_conf.c | 7 +------ src/lxc/lxc_driver.c | 8 +------- src/lxc/lxc_process.c | 8 +------- src/qemu/qemu_command.c | 8 +------- src/qemu/qemu_hotplug.c | 8 +------- src/remote/remote_driver.c | 5 ++--- src/uml/uml_conf.c | 2 +- src/xenconfig/xen_common.c | 2 +- src/xenconfig/xen_sxpr.c | 2 +- 13 files changed, 18 insertions(+), 51 deletions(-)
ACK
On 12/02/2014 10:14 AM, Peter Krempa wrote:
On 12/01/14 16:56, John Ferlan wrote:
Since virNetworkFree will call virObjectUnref anyway, let's just use that directly so as to avoid the possibility that we inadvertently clear out a pending error message when using the public API.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- cfg.mk | 4 ++-- daemon/remote.c | 7 +++---- src/conf/network_conf.c | 6 ++---- src/conf/network_event.c | 2 +- src/libxl/libxl_conf.c | 7 +------ src/lxc/lxc_driver.c | 8 +------- src/lxc/lxc_process.c | 8 +------- src/qemu/qemu_command.c | 8 +------- src/qemu/qemu_hotplug.c | 8 +------- src/remote/remote_driver.c | 5 ++--- src/uml/uml_conf.c | 2 +- src/xenconfig/xen_common.c | 2 +- src/xenconfig/xen_sxpr.c | 2 +- 13 files changed, 18 insertions(+), 51 deletions(-)
ACK
Note that this will require a rebase, as I pushed my patch that changed a single occurrence (because we may not want to backport this larger patch)
Since virNodeDeviceFree will call virObjectUnref anyway, let's just use that directly so as to avoid the possibility that we inadvertently clear out a pending error message when using the public API. Signed-off-by: John Ferlan <jferlan@redhat.com> --- cfg.mk | 4 ++-- daemon/remote.c | 5 ++--- src/conf/node_device_conf.c | 6 ++---- src/remote/remote_driver.c | 3 +-- 4 files changed, 7 insertions(+), 11 deletions(-) diff --git a/cfg.mk b/cfg.mk index b5f853b..97e573b 100644 --- a/cfg.mk +++ b/cfg.mk @@ -992,7 +992,7 @@ sc_prohibit_system_error_with_vir_err: # functions. There's a corresponding exclude to allow usage within tests, # docs, examples, tools, src/libvirt-*.c, and include/libvirt/libvirt-*.h sc_prohibit_virXXXFree: - @prohibit='\bvir(Domain|Network)Free\b' \ + @prohibit='\bvir(Domain|Network|NodeDevice)Free\b' \ exclude='sc_prohibit_virXXXFree' \ halt='avoid using 'virXXXFree', use 'virObjectUnref' instead' \ $(_sc_search_regexp) @@ -1186,4 +1186,4 @@ exclude_file_name_regexp--sc_prohibit_devname = \ ^(tools/virsh.pod|cfg.mk|docs/.*)$$ exclude_file_name_regexp--sc_prohibit_virXXXFree = \ - ^(docs/|tests/|examples/|tools/|cfg.mk|src/libvirt_public.syms|include/libvirt/libvirt-(domain|network).h|src/libvirt-(domain|qemu|network).c$$) + ^(docs/|tests/|examples/|tools/|cfg.mk|src/libvirt_public.syms|include/libvirt/libvirt-(domain|network|nodedev).h|src/libvirt-(domain|qemu|network|nodedev).c$$) diff --git a/daemon/remote.c b/daemon/remote.c index edae335..cfda8d8 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -3375,8 +3375,7 @@ remoteDispatchNodeDeviceGetParent(virNetServerPtr server ATTRIBUTE_UNUSED, cleanup: if (rv < 0) virNetMessageSaveError(rerr); - if (dev) - virNodeDeviceFree(dev); + virObjectUnref(dev); return rv; } @@ -4908,7 +4907,7 @@ remoteDispatchConnectListAllNodeDevices(virNetServerPtr server ATTRIBUTE_UNUSED, virNetMessageSaveError(rerr); if (devices && ndevices > 0) { for (i = 0; i < ndevices; i++) - virNodeDeviceFree(devices[i]); + virObjectUnref(devices[i]); VIR_FREE(devices); } return rv; diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 2a609e4..03b88a2 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -1825,10 +1825,8 @@ virNodeDeviceObjListExport(virConnectPtr conn, cleanup: if (tmp_devices) { - for (i = 0; i < ndevices; i++) { - if (tmp_devices[i]) - virNodeDeviceFree(tmp_devices[i]); - } + for (i = 0; i < ndevices; i++) + virObjectUnref(tmp_devices[i]); } VIR_FREE(tmp_devices); diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index c23a087..d03b159 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -3387,8 +3387,7 @@ remoteConnectListAllNodeDevices(virConnectPtr conn, cleanup: if (tmp_devices) { for (i = 0; i < ret.devices.devices_len; i++) - if (tmp_devices[i]) - virNodeDeviceFree(tmp_devices[i]); + virObjectUnref(tmp_devices[i]); VIR_FREE(tmp_devices); } -- 1.9.3
On 12/01/14 16:56, John Ferlan wrote:
Since virNodeDeviceFree will call virObjectUnref anyway, let's just use that directly so as to avoid the possibility that we inadvertently clear out a pending error message when using the public API.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- cfg.mk | 4 ++-- daemon/remote.c | 5 ++--- src/conf/node_device_conf.c | 6 ++---- src/remote/remote_driver.c | 3 +-- 4 files changed, 7 insertions(+), 11 deletions(-)
ACK
Since virStorageVolFree will call virObjectUnref anyway, let's just use that directly so as to avoid the possibility that we inadvertently clear out a pending error message when using the public API. Signed-off-by: John Ferlan <jferlan@redhat.com> --- cfg.mk | 4 ++-- daemon/remote.c | 2 +- src/remote/remote_driver.c | 3 +-- src/storage/storage_driver.c | 9 +++------ 4 files changed, 7 insertions(+), 11 deletions(-) diff --git a/cfg.mk b/cfg.mk index 97e573b..5da7b22 100644 --- a/cfg.mk +++ b/cfg.mk @@ -992,7 +992,7 @@ sc_prohibit_system_error_with_vir_err: # functions. There's a corresponding exclude to allow usage within tests, # docs, examples, tools, src/libvirt-*.c, and include/libvirt/libvirt-*.h sc_prohibit_virXXXFree: - @prohibit='\bvir(Domain|Network|NodeDevice)Free\b' \ + @prohibit='\bvir(Domain|Network|NodeDevice|StorageVol)Free\b' \ exclude='sc_prohibit_virXXXFree' \ halt='avoid using 'virXXXFree', use 'virObjectUnref' instead' \ $(_sc_search_regexp) @@ -1186,4 +1186,4 @@ exclude_file_name_regexp--sc_prohibit_devname = \ ^(tools/virsh.pod|cfg.mk|docs/.*)$$ exclude_file_name_regexp--sc_prohibit_virXXXFree = \ - ^(docs/|tests/|examples/|tools/|cfg.mk|src/libvirt_public.syms|include/libvirt/libvirt-(domain|network|nodedev).h|src/libvirt-(domain|qemu|network|nodedev).c$$) + ^(docs/|tests/|examples/|tools/|cfg.mk|src/test/test_driver.c|src/libvirt_public.syms|include/libvirt/libvirt-(domain|network|nodedev|storage).h|src/libvirt-(domain|qemu|network|nodedev|storage).c$$) diff --git a/daemon/remote.c b/daemon/remote.c index cfda8d8..644f10e 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -4728,7 +4728,7 @@ remoteDispatchStoragePoolListAllVolumes(virNetServerPtr server ATTRIBUTE_UNUSED, virNetMessageSaveError(rerr); if (vols && nvols > 0) { for (i = 0; i < nvols; i++) - virStorageVolFree(vols[i]); + virObjectUnref(vols[i]); VIR_FREE(vols); } if (pool) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index d03b159..b89984e 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -3749,8 +3749,7 @@ remoteStoragePoolListAllVolumes(virStoragePoolPtr pool, cleanup: if (tmp_vols) { for (i = 0; i < ret.vols.vols_len; i++) - if (tmp_vols[i]) - virStorageVolFree(tmp_vols[i]); + virObjectUnref(tmp_vols[i]); VIR_FREE(tmp_vols); } diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 88dea34..0fcbc4e 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1282,10 +1282,8 @@ storagePoolListAllVolumes(virStoragePoolPtr pool, cleanup: if (tmp_vols) { - for (i = 0; i < nvols; i++) { - if (tmp_vols[i]) - virStorageVolFree(tmp_vols[i]); - } + for (i = 0; i < nvols; i++) + virObjectUnref(tmp_vols[i]); VIR_FREE(tmp_vols); } @@ -3148,8 +3146,7 @@ virStorageTranslateDiskSourcePool(virConnectPtr conn, savedError = virSaveLastError(); if (pool) virStoragePoolFree(pool); - if (vol) - virStorageVolFree(vol); + virObjectUnref(vol); if (savedError) { virSetError(savedError); virFreeError(savedError); -- 1.9.3
On 12/01/14 16:56, John Ferlan wrote:
Since virStorageVolFree will call virObjectUnref anyway, let's just use that directly so as to avoid the possibility that we inadvertently clear out a pending error message when using the public API.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- cfg.mk | 4 ++-- daemon/remote.c | 2 +- src/remote/remote_driver.c | 3 +-- src/storage/storage_driver.c | 9 +++------ 4 files changed, 7 insertions(+), 11 deletions(-)
ACK
Since virStoragePoolFree will call virObjectUnref anyway, let's just use that directly so as to avoid the possibility that we inadvertently clear out a pending error message when using the public API. Signed-off-by: John Ferlan <jferlan@redhat.com> --- cfg.mk | 2 +- daemon/remote.c | 5 ++--- src/conf/storage_conf.c | 6 ++---- src/remote/remote_driver.c | 3 +-- src/storage/storage_driver.c | 11 +---------- 5 files changed, 7 insertions(+), 20 deletions(-) diff --git a/cfg.mk b/cfg.mk index 5da7b22..4766f0b 100644 --- a/cfg.mk +++ b/cfg.mk @@ -992,7 +992,7 @@ sc_prohibit_system_error_with_vir_err: # functions. There's a corresponding exclude to allow usage within tests, # docs, examples, tools, src/libvirt-*.c, and include/libvirt/libvirt-*.h sc_prohibit_virXXXFree: - @prohibit='\bvir(Domain|Network|NodeDevice|StorageVol)Free\b' \ + @prohibit='\bvir(Domain|Network|NodeDevice|StorageVol|StoragePool)Free\b' \ exclude='sc_prohibit_virXXXFree' \ halt='avoid using 'virXXXFree', use 'virObjectUnref' instead' \ $(_sc_search_regexp) diff --git a/daemon/remote.c b/daemon/remote.c index 644f10e..fe1b13c 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -4665,7 +4665,7 @@ remoteDispatchConnectListAllStoragePools(virNetServerPtr server ATTRIBUTE_UNUSED virNetMessageSaveError(rerr); if (pools && npools > 0) { for (i = 0; i < npools; i++) - virStoragePoolFree(pools[i]); + virObjectUnref(pools[i]); VIR_FREE(pools); } return rv; @@ -4731,8 +4731,7 @@ remoteDispatchStoragePoolListAllVolumes(virNetServerPtr server ATTRIBUTE_UNUSED, virObjectUnref(vols[i]); VIR_FREE(vols); } - if (pool) - virStoragePoolFree(pool); + virObjectUnref(pool); return rv; } diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index f75e862..3987470 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2523,10 +2523,8 @@ virStoragePoolObjListExport(virConnectPtr conn, cleanup: if (tmp_pools) { - for (i = 0; i < npools; i++) { - if (tmp_pools[i]) - virStoragePoolFree(tmp_pools[i]); - } + for (i = 0; i < npools; i++) + virObjectUnref(tmp_pools[i]); } VIR_FREE(tmp_pools); diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index b89984e..830e1d4 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -3682,8 +3682,7 @@ remoteConnectListAllStoragePools(virConnectPtr conn, cleanup: if (tmp_pools) { for (i = 0; i < ret.pools.pools_len; i++) - if (tmp_pools[i]) - virStoragePoolFree(tmp_pools[i]); + virObjectUnref(tmp_pools[i]); VIR_FREE(tmp_pools); } diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 0fcbc4e..99ccb49 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -3006,7 +3006,6 @@ virStorageTranslateDiskSourcePool(virConnectPtr conn, char *poolxml = NULL; virStorageVolInfo info; int ret = -1; - virErrorPtr savedError = NULL; if (def->src->type != VIR_STORAGE_TYPE_VOLUME) return 0; @@ -3142,16 +3141,8 @@ virStorageTranslateDiskSourcePool(virConnectPtr conn, ret = 0; cleanup: - if (ret < 0) - savedError = virSaveLastError(); - if (pool) - virStoragePoolFree(pool); + virObjectUnref(pool); virObjectUnref(vol); - if (savedError) { - virSetError(savedError); - virFreeError(savedError); - } - VIR_FREE(poolxml); virStoragePoolDefFree(pooldef); return ret; -- 1.9.3
On 12/01/14 16:56, John Ferlan wrote:
Since virStoragePoolFree will call virObjectUnref anyway, let's just use that directly so as to avoid the possibility that we inadvertently clear out a pending error message when using the public API.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- cfg.mk | 2 +- daemon/remote.c | 5 ++--- src/conf/storage_conf.c | 6 ++---- src/remote/remote_driver.c | 3 +-- src/storage/storage_driver.c | 11 +---------- 5 files changed, 7 insertions(+), 20 deletions(-)
ACK
Since virStreamFree will call virObjectUnref anyway, let's just use that directly so as to avoid the possibility that we inadvertently clear out a pending error message when using the public API. Signed-off-by: John Ferlan <jferlan@redhat.com> --- cfg.mk | 4 ++-- daemon/remote.c | 2 +- daemon/stream.c | 2 +- src/conf/virchrdev.c | 4 ++-- src/fdstream.c | 2 +- src/remote/remote_driver.c | 2 +- 6 files changed, 8 insertions(+), 8 deletions(-) diff --git a/cfg.mk b/cfg.mk index 4766f0b..c7dfb76 100644 --- a/cfg.mk +++ b/cfg.mk @@ -992,7 +992,7 @@ sc_prohibit_system_error_with_vir_err: # functions. There's a corresponding exclude to allow usage within tests, # docs, examples, tools, src/libvirt-*.c, and include/libvirt/libvirt-*.h sc_prohibit_virXXXFree: - @prohibit='\bvir(Domain|Network|NodeDevice|StorageVol|StoragePool)Free\b' \ + @prohibit='\bvir(Domain|Network|NodeDevice|StorageVol|StoragePool|Stream)Free\b' \ exclude='sc_prohibit_virXXXFree' \ halt='avoid using 'virXXXFree', use 'virObjectUnref' instead' \ $(_sc_search_regexp) @@ -1186,4 +1186,4 @@ exclude_file_name_regexp--sc_prohibit_devname = \ ^(tools/virsh.pod|cfg.mk|docs/.*)$$ exclude_file_name_regexp--sc_prohibit_virXXXFree = \ - ^(docs/|tests/|examples/|tools/|cfg.mk|src/test/test_driver.c|src/libvirt_public.syms|include/libvirt/libvirt-(domain|network|nodedev|storage).h|src/libvirt-(domain|qemu|network|nodedev|storage).c$$) + ^(docs/|tests/|examples/|tools/|cfg.mk|src/test/test_driver.c|src/libvirt_public.syms|include/libvirt/libvirt-(domain|network|nodedev|storage|stream).h|src/libvirt-(domain|qemu|network|nodedev|storage|stream).c$$) diff --git a/daemon/remote.c b/daemon/remote.c index fe1b13c..b2f1c46 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -5414,7 +5414,7 @@ remoteDispatchDomainMigratePrepareTunnel3Params(virNetServerPtr server ATTRIBUTE virStreamAbort(st); daemonFreeClientStream(client, stream); } else { - virStreamFree(st); + virObjectUnref(st); } } return rv; diff --git a/daemon/stream.c b/daemon/stream.c index 88bc858..dfe0bf9 100644 --- a/daemon/stream.c +++ b/daemon/stream.c @@ -383,7 +383,7 @@ int daemonFreeClientStream(virNetServerClientPtr client, msg = tmp; } - virStreamFree(stream->st); + virObjectUnref(stream->st); VIR_FREE(stream); return ret; diff --git a/src/conf/virchrdev.c b/src/conf/virchrdev.c index 022fe71..3e7df7e 100644 --- a/src/conf/virchrdev.c +++ b/src/conf/virchrdev.c @@ -221,7 +221,7 @@ static void virChrdevHashEntryFree(void *data, virStreamPtr st = data; /* free stream reference */ - virStreamFree(st); + virObjectUnref(st); /* delete lock file */ virChrdevLockFileRemove(dev); @@ -439,7 +439,7 @@ int virChrdevOpen(virChrdevsPtr devs, if (added) virHashRemoveEntry(devs->hash, path); else - virStreamFree(st); + virObjectUnref(st); virSetError(savedError); virFreeError(savedError); diff --git a/src/fdstream.c b/src/fdstream.c index 9ff7e2a..a020cdd 100644 --- a/src/fdstream.c +++ b/src/fdstream.c @@ -188,7 +188,7 @@ static void virFDStreamEvent(int watch ATTRIBUTE_UNUSED, static void virFDStreamCallbackFree(void *opaque) { virStreamPtr st = opaque; - virStreamFree(st); + virObjectUnref(st); } diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 830e1d4..2eb251a 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -5691,7 +5691,7 @@ static void remoteStreamCallbackFree(void *opaque) if (!cbdata->cb && cbdata->ff) (cbdata->ff)(cbdata->opaque); - virStreamFree(cbdata->st); + virObjectUnref(cbdata->st); VIR_FREE(opaque); } -- 1.9.3
On 12/01/14 16:56, John Ferlan wrote:
Since virStreamFree will call virObjectUnref anyway, let's just use that directly so as to avoid the possibility that we inadvertently clear out a pending error message when using the public API.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- cfg.mk | 4 ++-- daemon/remote.c | 2 +- daemon/stream.c | 2 +- src/conf/virchrdev.c | 4 ++-- src/fdstream.c | 2 +- src/remote/remote_driver.c | 2 +- 6 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/conf/virchrdev.c b/src/conf/virchrdev.c index 022fe71..3e7df7e 100644 --- a/src/conf/virchrdev.c +++ b/src/conf/virchrdev.c
@@ -439,7 +439,7 @@ int virChrdevOpen(virChrdevsPtr devs, if (added) virHashRemoveEntry(devs->hash, path); else - virStreamFree(st); + virObjectUnref(st);
virSetError(savedError); virFreeError(savedError);
virHashRemoveEntry doesn't taint the error so you can get rid of the code that saves error and returns it
diff --git a/src/fdstream.c b/src/fdstream.c index 9ff7e2a..a020cdd 100644 --- a/src/fdstream.c +++ b/src/fdstream.c @@ -188,7 +188,7 @@ static void virFDStreamEvent(int watch ATTRIBUTE_UNUSED, static void virFDStreamCallbackFree(void *opaque) { virStreamPtr st = opaque; - virStreamFree(st); + virObjectUnref(st);
virObjectUnref() takes void * so you could get rid of the intermediate variable.
}
ACK with or without changes. Peter
Since virSecretFree will call virObjectUnref anyway, let's just use that directly so as to avoid the possibility that we inadvertently clear out a pending error message when using the public API. Signed-off-by: John Ferlan <jferlan@redhat.com> --- cfg.mk | 4 ++-- daemon/remote.c | 5 ++--- src/remote/remote_driver.c | 3 +-- src/secret/secret_driver.c | 6 ++---- src/storage/storage_backend.c | 4 ++-- src/storage/storage_backend_fs.c | 4 ++-- src/storage/storage_backend_rbd.c | 3 +-- 7 files changed, 12 insertions(+), 17 deletions(-) diff --git a/cfg.mk b/cfg.mk index c7dfb76..e91cddf 100644 --- a/cfg.mk +++ b/cfg.mk @@ -992,7 +992,7 @@ sc_prohibit_system_error_with_vir_err: # functions. There's a corresponding exclude to allow usage within tests, # docs, examples, tools, src/libvirt-*.c, and include/libvirt/libvirt-*.h sc_prohibit_virXXXFree: - @prohibit='\bvir(Domain|Network|NodeDevice|StorageVol|StoragePool|Stream)Free\b' \ + @prohibit='\bvir(Domain|Network|NodeDevice|StorageVol|StoragePool|Stream|Secret)Free\b' \ exclude='sc_prohibit_virXXXFree' \ halt='avoid using 'virXXXFree', use 'virObjectUnref' instead' \ $(_sc_search_regexp) @@ -1186,4 +1186,4 @@ exclude_file_name_regexp--sc_prohibit_devname = \ ^(tools/virsh.pod|cfg.mk|docs/.*)$$ exclude_file_name_regexp--sc_prohibit_virXXXFree = \ - ^(docs/|tests/|examples/|tools/|cfg.mk|src/test/test_driver.c|src/libvirt_public.syms|include/libvirt/libvirt-(domain|network|nodedev|storage|stream).h|src/libvirt-(domain|qemu|network|nodedev|storage|stream).c$$) + ^(docs/|tests/|examples/|tools/|cfg.mk|src/test/test_driver.c|src/libvirt_public.syms|include/libvirt/libvirt-(domain|network|nodedev|storage|stream|secret).h|src/libvirt-(domain|qemu|network|nodedev|storage|stream|secret).c$$) diff --git a/daemon/remote.c b/daemon/remote.c index b2f1c46..60d05e1 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -3565,8 +3565,7 @@ remoteDispatchSecretGetValue(virNetServerPtr server ATTRIBUTE_UNUSED, cleanup: if (rv < 0) virNetMessageSaveError(rerr); - if (secret) - virSecretFree(secret); + virObjectUnref(secret); return rv; } @@ -5024,7 +5023,7 @@ remoteDispatchConnectListAllSecrets(virNetServerPtr server ATTRIBUTE_UNUSED, virNetMessageSaveError(rerr); if (secrets && nsecrets > 0) { for (i = 0; i < nsecrets; i++) - virSecretFree(secrets[i]); + virObjectUnref(secrets[i]); VIR_FREE(secrets); } return rv; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 2eb251a..431a092 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -3518,8 +3518,7 @@ remoteConnectListAllSecrets(virConnectPtr conn, cleanup: if (tmp_secrets) { for (i = 0; i < ret.secrets.secrets_len; i++) - if (tmp_secrets[i]) - virSecretFree(tmp_secrets[i]); + virObjectUnref(tmp_secrets[i]); VIR_FREE(tmp_secrets); } diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index cd04b20..c7a51bb 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -700,10 +700,8 @@ secretConnectListAllSecrets(virConnectPtr conn, cleanup: secretDriverUnlock(driver); if (tmp_secrets) { - for (i = 0; i < ret_nsecrets; i ++) { - if (tmp_secrets[i]) - virSecretFree(tmp_secrets[i]); - } + for (i = 0; i < ret_nsecrets; i ++) + virObjectUnref(tmp_secrets[i]); } VIR_FREE(tmp_secrets); diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 98720f6..b990a82 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -506,7 +506,7 @@ virStorageGenerateSecretUUID(virConnectPtr conn, if (tmp == NULL) return 0; - virSecretFree(tmp); + virObjectUnref(tmp); } virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -589,7 +589,7 @@ virStorageGenerateQcowEncryption(virConnectPtr conn, if (ret != 0 && conn->secretDriver->secretUndefine != NULL) conn->secretDriver->secretUndefine(secret); - virSecretFree(secret); + virObjectUnref(secret); } virBufferFreeAndReset(&buf); virSecretDefFree(def); diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index f4bf5d6..0ee1d09 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1201,7 +1201,7 @@ virStorageBackendFileSystemVolRefresh(virConnectPtr conn, if (VIR_ALLOC_N(vol->target.encryption->secrets, 1) < 0 || VIR_ALLOC(encsec) < 0) { VIR_FREE(vol->target.encryption->secrets); - virSecretFree(sec); + virObjectUnref(sec); return -1; } @@ -1210,7 +1210,7 @@ virStorageBackendFileSystemVolRefresh(virConnectPtr conn, encsec->type = VIR_STORAGE_ENCRYPTION_SECRET_TYPE_PASSPHRASE; virSecretGetUUID(sec, encsec->uuid); - virSecretFree(sec); + virObjectUnref(sec); } } diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 5a16eff..57182de 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -225,8 +225,7 @@ static int virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, VIR_FREE(secret_value); VIR_FREE(rados_key); - if (secret != NULL) - virSecretFree(secret); + virObjectUnref(secret); virBufferFreeAndReset(&mon_host); VIR_FREE(mon_buff); -- 1.9.3
On 12/01/14 16:56, John Ferlan wrote:
Since virSecretFree will call virObjectUnref anyway, let's just use that directly so as to avoid the possibility that we inadvertently clear out a pending error message when using the public API.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- cfg.mk | 4 ++-- daemon/remote.c | 5 ++--- src/remote/remote_driver.c | 3 +-- src/secret/secret_driver.c | 6 ++---- src/storage/storage_backend.c | 4 ++-- src/storage/storage_backend_fs.c | 4 ++-- src/storage/storage_backend_rbd.c | 3 +-- 7 files changed, 12 insertions(+), 17 deletions(-)
ACK, Peter
Since virNWFilterFree will call virObjectUnref anyway, let's just use that directly so as to avoid the possibility that we inadvertently clear out a pending error message when using the public API. Signed-off-by: John Ferlan <jferlan@redhat.com> --- cfg.mk | 4 ++-- daemon/remote.c | 2 +- src/nwfilter/nwfilter_driver.c | 6 ++---- src/remote/remote_driver.c | 3 +-- 4 files changed, 6 insertions(+), 9 deletions(-) diff --git a/cfg.mk b/cfg.mk index e91cddf..bc9310d 100644 --- a/cfg.mk +++ b/cfg.mk @@ -992,7 +992,7 @@ sc_prohibit_system_error_with_vir_err: # functions. There's a corresponding exclude to allow usage within tests, # docs, examples, tools, src/libvirt-*.c, and include/libvirt/libvirt-*.h sc_prohibit_virXXXFree: - @prohibit='\bvir(Domain|Network|NodeDevice|StorageVol|StoragePool|Stream|Secret)Free\b' \ + @prohibit='\bvir(Domain|Network|NodeDevice|StorageVol|StoragePool|Stream|Secret|NWFilter)Free\b' \ exclude='sc_prohibit_virXXXFree' \ halt='avoid using 'virXXXFree', use 'virObjectUnref' instead' \ $(_sc_search_regexp) @@ -1186,4 +1186,4 @@ exclude_file_name_regexp--sc_prohibit_devname = \ ^(tools/virsh.pod|cfg.mk|docs/.*)$$ exclude_file_name_regexp--sc_prohibit_virXXXFree = \ - ^(docs/|tests/|examples/|tools/|cfg.mk|src/test/test_driver.c|src/libvirt_public.syms|include/libvirt/libvirt-(domain|network|nodedev|storage|stream|secret).h|src/libvirt-(domain|qemu|network|nodedev|storage|stream|secret).c$$) + ^(docs/|tests/|examples/|tools/|cfg.mk|src/test/test_driver.c|src/libvirt_public.syms|include/libvirt/libvirt-(domain|network|nodedev|storage|stream|secret|nwfilter).h|src/libvirt-(domain|qemu|network|nodedev|storage|stream|secret|nwfilter).c$$) diff --git a/daemon/remote.c b/daemon/remote.c index 60d05e1..dc00424 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -4964,7 +4964,7 @@ remoteDispatchConnectListAllNWFilters(virNetServerPtr server ATTRIBUTE_UNUSED, virNetMessageSaveError(rerr); if (filters && nfilters > 0) { for (i = 0; i < nfilters; i++) - virNWFilterFree(filters[i]); + virObjectUnref(filters[i]); VIR_FREE(filters); } return rv; diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index b41253d..0a04d5d 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -547,10 +547,8 @@ nwfilterConnectListAllNWFilters(virConnectPtr conn, cleanup: nwfilterDriverUnlock(driver); if (tmp_filters) { - for (i = 0; i < nfilters; i ++) { - if (tmp_filters[i]) - virNWFilterFree(tmp_filters[i]); - } + for (i = 0; i < nfilters; i ++) + virObjectUnref(tmp_filters[i]); } VIR_FREE(tmp_filters); diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 431a092..28bb46a 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -3452,8 +3452,7 @@ remoteConnectListAllNWFilters(virConnectPtr conn, cleanup: if (tmp_filters) { for (i = 0; i < ret.filters.filters_len; i++) - if (tmp_filters[i]) - virNWFilterFree(tmp_filters[i]); + virObjectUnref(tmp_filters[i]); VIR_FREE(tmp_filters); } -- 1.9.3
On 12/01/14 16:56, John Ferlan wrote:
Since virNWFilterFree will call virObjectUnref anyway, let's just use that directly so as to avoid the possibility that we inadvertently clear out a pending error message when using the public API.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- cfg.mk | 4 ++-- daemon/remote.c | 2 +- src/nwfilter/nwfilter_driver.c | 6 ++---- src/remote/remote_driver.c | 3 +-- 4 files changed, 6 insertions(+), 9 deletions(-)
ACK
Since virInterfac3Free will call virObjectUnref anyway, let's just use that directly so as to avoid the possibility that we inadvertently clear out a pending error message when using the public API. Signed-off-by: John Ferlan <jferlan@redhat.com> --- cfg.mk | 4 ++-- daemon/remote.c | 2 +- src/interface/interface_backend_netcf.c | 6 ++---- src/interface/interface_backend_udev.c | 2 +- src/remote/remote_driver.c | 3 +-- 5 files changed, 7 insertions(+), 10 deletions(-) diff --git a/cfg.mk b/cfg.mk index bc9310d..e8ff5f5 100644 --- a/cfg.mk +++ b/cfg.mk @@ -992,7 +992,7 @@ sc_prohibit_system_error_with_vir_err: # functions. There's a corresponding exclude to allow usage within tests, # docs, examples, tools, src/libvirt-*.c, and include/libvirt/libvirt-*.h sc_prohibit_virXXXFree: - @prohibit='\bvir(Domain|Network|NodeDevice|StorageVol|StoragePool|Stream|Secret|NWFilter)Free\b' \ + @prohibit='\bvir(Domain|Network|NodeDevice|StorageVol|StoragePool|Stream|Secret|NWFilter|Interface)Free\b' \ exclude='sc_prohibit_virXXXFree' \ halt='avoid using 'virXXXFree', use 'virObjectUnref' instead' \ $(_sc_search_regexp) @@ -1186,4 +1186,4 @@ exclude_file_name_regexp--sc_prohibit_devname = \ ^(tools/virsh.pod|cfg.mk|docs/.*)$$ exclude_file_name_regexp--sc_prohibit_virXXXFree = \ - ^(docs/|tests/|examples/|tools/|cfg.mk|src/test/test_driver.c|src/libvirt_public.syms|include/libvirt/libvirt-(domain|network|nodedev|storage|stream|secret|nwfilter).h|src/libvirt-(domain|qemu|network|nodedev|storage|stream|secret|nwfilter).c$$) + ^(docs/|tests/|examples/|tools/|cfg.mk|src/test/test_driver.c|src/libvirt_public.syms|include/libvirt/libvirt-(domain|network|nodedev|storage|stream|secret|nwfilter|interface).h|src/libvirt-(domain|qemu|network|nodedev|storage|stream|secret|nwfilter|interface).c$$) diff --git a/daemon/remote.c b/daemon/remote.c index dc00424..3e4dc1f 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -4846,7 +4846,7 @@ remoteDispatchConnectListAllInterfaces(virNetServerPtr server ATTRIBUTE_UNUSED, virNetMessageSaveError(rerr); if (ifaces && nifaces > 0) { for (i = 0; i < nifaces; i++) - virInterfaceFree(ifaces[i]); + virObjectUnref(ifaces[i]); VIR_FREE(ifaces); } return rv; diff --git a/src/interface/interface_backend_netcf.c b/src/interface/interface_backend_netcf.c index 1734329..116d84e 100644 --- a/src/interface/interface_backend_netcf.c +++ b/src/interface/interface_backend_netcf.c @@ -675,10 +675,8 @@ netcfConnectListAllInterfaces(virConnectPtr conn, VIR_FREE(names); if (tmp_iface_objs) { - for (i = 0; i < niface_objs; i++) { - if (tmp_iface_objs[i]) - virInterfaceFree(tmp_iface_objs[i]); - } + for (i = 0; i < niface_objs; i++) + virObjectUnref(tmp_iface_objs[i]); VIR_FREE(tmp_iface_objs); } diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c index e9fd941..d4f8c0f 100644 --- a/src/interface/interface_backend_udev.c +++ b/src/interface/interface_backend_udev.c @@ -471,7 +471,7 @@ udevConnectListAllInterfaces(virConnectPtr conn, if (ifaces) { for (tmp_count = 0; tmp_count < count; tmp_count++) - virInterfaceFree(ifaces_list[tmp_count]); + virObjectUnref(ifaces_list[tmp_count]); } VIR_FREE(ifaces_list); diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 28bb46a..3dc3ecb 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -3321,8 +3321,7 @@ remoteConnectListAllInterfaces(virConnectPtr conn, cleanup: if (tmp_ifaces) { for (i = 0; i < ret.ifaces.ifaces_len; i++) - if (tmp_ifaces[i]) - virInterfaceFree(tmp_ifaces[i]); + virObjectUnref(tmp_ifaces[i]); } VIR_FREE(tmp_ifaces); -- 1.9.3
On 12/01/14 16:56, John Ferlan wrote:
Since virInterfac3Free will call virObjectUnref anyway, let's just use that directly so as to avoid the possibility that we inadvertently clear out a pending error message when using the public API.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- cfg.mk | 4 ++-- daemon/remote.c | 2 +- src/interface/interface_backend_netcf.c | 6 ++---- src/interface/interface_backend_udev.c | 2 +- src/remote/remote_driver.c | 3 +-- 5 files changed, 7 insertions(+), 10 deletions(-)
ACK
Since virDomainSnapshotFree will call virObjectUnref anyway, let's just use that directly so as to avoid the possibility that we inadvertently clear out a pending error message when using the public API. Signed-off-by: John Ferlan <jferlan@redhat.com> --- cfg.mk | 4 ++-- daemon/remote.c | 7 +++---- src/qemu/qemu_driver.c | 2 +- src/remote/remote_driver.c | 6 ++---- 4 files changed, 8 insertions(+), 11 deletions(-) diff --git a/cfg.mk b/cfg.mk index e8ff5f5..21f83c3 100644 --- a/cfg.mk +++ b/cfg.mk @@ -992,7 +992,7 @@ sc_prohibit_system_error_with_vir_err: # functions. There's a corresponding exclude to allow usage within tests, # docs, examples, tools, src/libvirt-*.c, and include/libvirt/libvirt-*.h sc_prohibit_virXXXFree: - @prohibit='\bvir(Domain|Network|NodeDevice|StorageVol|StoragePool|Stream|Secret|NWFilter|Interface)Free\b' \ + @prohibit='\bvir(Domain|Network|NodeDevice|StorageVol|StoragePool|Stream|Secret|NWFilter|Interface|DomainSnapshot)Free\b' \ exclude='sc_prohibit_virXXXFree' \ halt='avoid using 'virXXXFree', use 'virObjectUnref' instead' \ $(_sc_search_regexp) @@ -1186,4 +1186,4 @@ exclude_file_name_regexp--sc_prohibit_devname = \ ^(tools/virsh.pod|cfg.mk|docs/.*)$$ exclude_file_name_regexp--sc_prohibit_virXXXFree = \ - ^(docs/|tests/|examples/|tools/|cfg.mk|src/test/test_driver.c|src/libvirt_public.syms|include/libvirt/libvirt-(domain|network|nodedev|storage|stream|secret|nwfilter|interface).h|src/libvirt-(domain|qemu|network|nodedev|storage|stream|secret|nwfilter|interface).c$$) + ^(docs/|tests/|examples/|tools/|cfg.mk|src/test/test_driver.c|src/libvirt_public.syms|include/libvirt/libvirt-(domain|network|nodedev|storage|stream|secret|nwfilter|interface|domain-snapshot).h|src/libvirt-(domain|qemu|network|nodedev|storage|stream|secret|nwfilter|interface|domain-snapshot).c$$) diff --git a/daemon/remote.c b/daemon/remote.c index 3e4dc1f..d657a09 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -4535,7 +4535,7 @@ remoteDispatchDomainListAllSnapshots(virNetServerPtr server ATTRIBUTE_UNUSED, virObjectUnref(dom); if (snaps && nsnaps > 0) { for (i = 0; i < nsnaps; i++) - virDomainSnapshotFree(snaps[i]); + virObjectUnref(snaps[i]); VIR_FREE(snaps); } return rv; @@ -4600,12 +4600,11 @@ remoteDispatchDomainSnapshotListAllChildren(virNetServerPtr server ATTRIBUTE_UNU cleanup: if (rv < 0) virNetMessageSaveError(rerr); - if (snapshot) - virDomainSnapshotFree(snapshot); + virObjectUnref(snapshot); virObjectUnref(dom); if (snaps && nsnaps > 0) { for (i = 0; i < nsnaps; i++) - virDomainSnapshotFree(snaps[i]); + virObjectUnref(snaps[i]); VIR_FREE(snaps); } return rv; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8d4b393..1130474 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14090,7 +14090,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, cfg->snapshotDir) < 0) { /* if writing of metadata fails, error out rather than trying * to silently carry on without completing the snapshot */ - virDomainSnapshotFree(snapshot); + virObjectUnref(snapshot); snapshot = NULL; virReportError(VIR_ERR_INTERNAL_ERROR, _("unable to save metadata for snapshot %s"), diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 3dc3ecb..999f16d 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -6766,8 +6766,7 @@ remoteDomainListAllSnapshots(virDomainPtr dom, cleanup: if (snaps) { for (i = 0; i < ret.snapshots.snapshots_len; i++) - if (snaps[i]) - virDomainSnapshotFree(snaps[i]); + virObjectUnref(snaps[i]); VIR_FREE(snaps); } @@ -6833,8 +6832,7 @@ remoteDomainSnapshotListAllChildren(virDomainSnapshotPtr parent, cleanup: if (snaps) { for (i = 0; i < ret.snapshots.snapshots_len; i++) - if (snaps[i]) - virDomainSnapshotFree(snaps[i]); + virObjectUnref(snaps[i]); VIR_FREE(snaps); } -- 1.9.3
On 12/01/14 16:56, John Ferlan wrote:
Since virDomainSnapshotFree will call virObjectUnref anyway, let's just use that directly so as to avoid the possibility that we inadvertently clear out a pending error message when using the public API.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- cfg.mk | 4 ++-- daemon/remote.c | 7 +++---- src/qemu/qemu_driver.c | 2 +- src/remote/remote_driver.c | 6 ++---- 4 files changed, 8 insertions(+), 11 deletions(-)
ACK
On 12/01/2014 10:56 AM, John Ferlan wrote:
Based on some recent review comments and a bit of internal IRC, this set of patches will change all the existing virXXXFree calls found in daemon/* and src/* to be virObjectUnref instead and then add a rule to inhibit usage unless the string/call is found in docs/*, tests/*, examples/*, tools/*, cfg.mk, libvirt_public.syms, include/libvirt/libvirt-*.h, and src/libvirt-*.c
Most of this was brute force to start with and adding the rule afterwards caught a few oddball places.
The reason for not wanting to call a virXXXFree function is because it will reset the last error, potentially clearing something and resulting in a message "an error was encountered but the cause is unknown". There were some places in the code which saved the last error, called the free function, then restored the last error. Those have now been adjusted to avoid that processing. Anything that required checking for a non NULL pointer prior to calling the virXXXFree is now not necessary since the virObjectUnref will check for NULL before continuing; whereas, the virXXXFree functions didn't allow NULL.
If usage of the virXXXFree function is found during syntax-check, a message such as the following will be displayed:
include/libvirt/libvirt-storage.h:256:int virStoragePoolFree (virStoragePoolPtr pool); include/libvirt/libvirt-storage.h:336:int virStorageVolFree (virStorageVolPtr vol); maint.mk: avoid using virXXXFree, use virObjectUnref instead make: *** [sc_prohibit_virXXXFree] Error 1
John Ferlan (11): rpc: Replace virXXXFree with virObjectUnref Replace virDomainFree with virObjectUnref Replace virNetworkFree with virObjectUnref Replace virNodeDeviceFree with virObjectUnref Replace virStorageVolFree with virObjectUnref Replace virStoragePoolFree with virObjectUnref Replace virStreamFree with virObjectUnref Replace virSecretFree with virObjectUnref Replace virNWFilterFree with virObjectUnref Replace virInterfaceFree with virObjectUnref Replace virDomainSnapshotFree with virObjectUnref
cfg.mk | 12 +++ daemon/remote.c | 168 ++++++++++++-------------------- daemon/stream.c | 2 +- src/conf/domain_event.c | 4 +- src/conf/network_conf.c | 6 +- src/conf/network_event.c | 2 +- src/conf/node_device_conf.c | 6 +- src/conf/storage_conf.c | 6 +- src/conf/virchrdev.c | 4 +- src/esx/esx_driver.c | 2 +- src/fdstream.c | 2 +- src/hyperv/hyperv_driver.c | 2 +- src/interface/interface_backend_netcf.c | 6 +- src/interface/interface_backend_udev.c | 2 +- src/libxl/libxl_conf.c | 7 +- src/locking/sanlock_helper.c | 3 +- src/lxc/lxc_driver.c | 8 +- src/lxc/lxc_process.c | 8 +- src/nwfilter/nwfilter_driver.c | 6 +- src/qemu/qemu_command.c | 8 +- src/qemu/qemu_driver.c | 4 +- src/qemu/qemu_hotplug.c | 8 +- src/remote/remote_driver.c | 76 +++++++-------- src/rpc/gendispatch.pl | 17 ++-- src/secret/secret_driver.c | 6 +- src/storage/storage_backend.c | 4 +- src/storage/storage_backend_fs.c | 4 +- src/storage/storage_backend_rbd.c | 3 +- src/storage/storage_driver.c | 20 +--- src/uml/uml_conf.c | 2 +- src/vbox/vbox_common.c | 6 +- src/xenconfig/xen_common.c | 2 +- src/xenconfig/xen_sxpr.c | 2 +- 33 files changed, 155 insertions(+), 263 deletions(-)
Thanks for the review - Laine: all set - rebase/merge picked that up and no thoughts to backporting this as a whole! - I adjusted patch 7 to remove the savedError logic and avoid the extraneous intermediate variable - I adjusted patch 10 to fix the commit message from "virInterfac3Free" to be "virInterfaceFree" and pushed (after making sure my Coverity build was happy too!) John
participants (3)
-
John Ferlan -
Laine Stump -
Peter Krempa