[libvirt PATCH 0/4] Reinstate -Wunused-but-set-variable
Personally, I feel like all the cases caught by new Clang's -Wunused-but-set-variable behavior were also unnecessarily difficult to read for humans, we should be more obvious when relying purely on side effects of g_auto and the warning is worth keeping. Even if not, the first two patches should not be controversial. Ján Tomko (4): qemu: remove unused 'cfg' variables util: virIdentitySetCurrent: only unref the old identity on success rpc: mark source returned by virEventGLibAddSocketWatch as unused Revert "meson: avoid bogus warnings from clang and g_autoptr" build-aux/syntax-check.mk | 2 +- meson.build | 21 --------------------- src/qemu/qemu_driver.c | 9 --------- src/rpc/virnetclient.c | 6 +++--- src/util/viridentity.c | 4 +++- 5 files changed, 7 insertions(+), 35 deletions(-) -- 2.31.1
Unused as of: commit effeee5c2fcec19fcaad627690a6a0ba0025e35f qemu: driver: Use 'qemuDomainSaveStatus' for saving status XML This function extracts the config from the vm object, so the caller no longer needs to do it. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_driver.c | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f1f961c51c..dfffd6b373 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1837,7 +1837,6 @@ static int qemuDomainSuspend(virDomainPtr dom) qemuDomainObjPrivate *priv; virDomainPausedReason reason; int state; - g_autoptr(virQEMUDriverConfig) cfg = NULL; if (!(vm = qemuDomainObjFromDomain(dom))) return -1; @@ -1845,7 +1844,6 @@ static int qemuDomainSuspend(virDomainPtr dom) if (virDomainSuspendEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - cfg = virQEMUDriverGetConfig(driver); priv = vm->privateData; if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_SUSPEND) < 0) @@ -1890,13 +1888,10 @@ static int qemuDomainResume(virDomainPtr dom) int ret = -1; int state; int reason; - g_autoptr(virQEMUDriverConfig) cfg = NULL; if (!(vm = qemuDomainObjFromDomain(dom))) return -1; - cfg = virQEMUDriverGetConfig(driver); - if (virDomainResumeEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -20237,8 +20232,6 @@ qemuDomainAgentSetResponseTimeout(virDomainPtr dom, int timeout, unsigned int flags) { - virQEMUDriver *driver = dom->conn->privateData; - g_autoptr(virQEMUDriverConfig) cfg = NULL; virDomainObj *vm = NULL; int ret = -1; @@ -20255,8 +20248,6 @@ qemuDomainAgentSetResponseTimeout(virDomainPtr dom, if (!(vm = qemuDomainObjFromDomain(dom))) return -1; - cfg = virQEMUDriverGetConfig(driver); - if (virDomainAgentSetResponseTimeoutEnsureACL(dom->conn, vm->def) < 0) goto cleanup; -- 2.31.1
In the unlikely case that we were unable to set the new identity, we would unref the old one even though it still could be in the thread-local storage. Fixes: c6825d88137cb8e4debdf4310e45ee23cb5698c0 Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/viridentity.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/util/viridentity.c b/src/util/viridentity.c index e36e54ae4b..70843ecf9f 100644 --- a/src/util/viridentity.c +++ b/src/util/viridentity.c @@ -134,7 +134,7 @@ virIdentity *virIdentityGetCurrent(void) */ int virIdentitySetCurrent(virIdentity *ident) { - g_autoptr(virIdentity) old = NULL; + virIdentity *old = NULL; if (virIdentityInitialize() < 0) return -1; @@ -150,6 +150,8 @@ int virIdentitySetCurrent(virIdentity *ident) return -1; } + if (old) + g_object_unref(old); return 0; } -- 2.31.1
Two users of virEventGLibAddSocketWatch care about the GSource it returns. The other three free it by assigning it to an autofreed variable. Mark them with G_GNUC_UNUSED to make this obvious to the reader and the compiler. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/rpc/virnetclient.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index ffe2f343f9..f526ad89ec 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -882,7 +882,7 @@ virNetClientIOEventTLS(int fd, static gboolean virNetClientTLSHandshake(virNetClient *client) { - g_autoptr(GSource) source = NULL; + g_autoptr(GSource) G_GNUC_UNUSED source = NULL; GIOCondition ev; int ret; @@ -939,7 +939,7 @@ int virNetClientSetTLSSession(virNetClient *client, int ret; char buf[1]; int len; - g_autoptr(GSource) source = NULL; + g_autoptr(GSource) G_GNUC_UNUSED source = NULL; #ifndef WIN32 sigset_t oldmask, blockedsigs; @@ -1664,7 +1664,7 @@ static int virNetClientIOEventLoop(virNetClient *client, #endif /* !WIN32 */ int timeout = -1; virNetMessage *msg = NULL; - g_autoptr(GSource) source = NULL; + g_autoptr(GSource) G_GNUC_UNUSED source = NULL; GIOCondition ev = 0; struct virNetClientIOEventData data = { .client = client, -- 2.31.1
Commit 345996c6208b281233074362a8d81295e2e711d4 disabled the -Wunused-but-set-variable warning on CLang, beacuse it warned on variables that were unread, but we relied on the side effects of their destructors. Reinstate the warning now that all the occurrences have been fixed. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- build-aux/syntax-check.mk | 2 +- meson.build | 21 --------------------- 2 files changed, 1 insertion(+), 22 deletions(-) diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk index 5c5a2a8771..2058af0b77 100644 --- a/build-aux/syntax-check.mk +++ b/build-aux/syntax-check.mk @@ -1589,7 +1589,7 @@ exclude_file_name_regexp--sc_prohibit_canonicalize_file_name = \ ^(build-aux/syntax-check\.mk|tests/virfilemock\.c)$$ exclude_file_name_regexp--sc_prohibit_raw_allocation = \ - ^(meson\.build|docs/advanced-tests\.rst|src/util/viralloc\.[ch]|examples/.*|tests/(securityselinuxhelper|(vircgroup|nss)mock|commandhelper)\.c|tools/wireshark/src/packet-libvirt\.c|tools/nss/libvirt_nss(_leases|_macs)?\.c)$$ + ^(docs/advanced-tests\.rst|src/util/viralloc\.[ch]|examples/.*|tests/(securityselinuxhelper|(vircgroup|nss)mock|commandhelper)\.c|tools/wireshark/src/packet-libvirt\.c|tools/nss/libvirt_nss(_leases|_macs)?\.c)$$ exclude_file_name_regexp--sc_prohibit_readlink = \ ^src/(util/virutil|lxc/lxc_container)\.c$$ diff --git a/meson.build b/meson.build index a8c99ac4b1..9d493ccd9e 100644 --- a/meson.build +++ b/meson.build @@ -470,27 +470,6 @@ if get_option('warning_level') == '2' supported_cc_flags += [ '-Wno-unused-function' ] endif - if supported_cc_flags.contains('-Wunused-but-set-variable') - # Clang complains about unused variables in many scenarios related - # to attribute((cleanup)) aka g_auto* - w_unused_but_set_var_args = [ '-Wunused-but-set-variable', '-Werror' ] - w_unused_but_set_var_code = ''' - static inline void free_pointer (void *p) { - void **pp = (void**)p; - free (*pp); - } - - int main(void) { - __attribute__((cleanup(free_pointer))) char *buffer = 0x0; - buffer = 0x1; - } - ''' - # We previously turned on unused-but-set-variable, so we must turn - # it off again explicitly now. - if not cc.compiles(w_unused_but_set_var_code, args: w_unused_but_set_var_args) - supported_cc_flags += [ '-Wno-unused-but-set-variable' ] - endif - endif endif add_project_arguments(supported_cc_flags, language: 'c') -- 2.31.1
On 9/4/21 9:25 PM, Ján Tomko wrote:
Personally, I feel like all the cases caught by new Clang's -Wunused-but-set-variable behavior were also unnecessarily difficult to read for humans, we should be more obvious when relying purely on side effects of g_auto and the warning is worth keeping.
Even if not, the first two patches should not be controversial.
Ján Tomko (4): qemu: remove unused 'cfg' variables util: virIdentitySetCurrent: only unref the old identity on success rpc: mark source returned by virEventGLibAddSocketWatch as unused Revert "meson: avoid bogus warnings from clang and g_autoptr"
build-aux/syntax-check.mk | 2 +- meson.build | 21 --------------------- src/qemu/qemu_driver.c | 9 --------- src/rpc/virnetclient.c | 6 +++--- src/util/viridentity.c | 4 +++- 5 files changed, 7 insertions(+), 35 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Ján Tomko -
Michal Prívozník