[PATCH 0/4] Appease Clang
With its recent update Clang-13.0.1-rc1 started to report spurious warnings. Basically it is concerned about our use of g_auto*: g_autofree char *var = NULL; var = func(); It fails to see that @var is there to automatically free retval of func(). I've reported the bug here: https://bugs.llvm.org/show_bug.cgi?id=36627 But there are few places where we can change the code and appease Clang. The rest, where our code is correct - I'm "fixing" those places in patch 4/4 which exists only to show that after the problematic pattern is broken the build passes again: https://gitlab.com/MichalPrivoznik/libvirt/-/jobs/1532513684 Michal Prívozník (4): virpci: Avoid Clang false positive test: Drop unused @cfg from qemu*test virscsi: Drop @tmp from virSCSIDeviceListDel DO NOT MERGE src/rpc/virnetclient.c | 9 ++++++--- src/util/viridentity.c | 3 ++- src/util/virpci.c | 4 ++-- src/util/virscsi.c | 3 +-- tests/qemumigrationcookiexmltest.c | 2 -- tests/qemustatusxml2xmltest.c | 2 -- 6 files changed, 11 insertions(+), 12 deletions(-) -- 2.31.1
The virPCIDeviceIsBehindSwitchLackingACS() function checks whether given PCI device is not behind a switch that lacks ACS. It does so by starting at given device and traversing up, one parent at time towards the root. The parent device is obtained via virPCIDeviceGetParent() which allocates new virPCIDevice structure. For freeing the structure we use g_autoptr() and a temporary variable @tmp. However, Clang fails to understand our clever algorithm and complains that the variable is set but never used. This is obviously a false positive, but using a small trick we can shut Clang up. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virpci.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index 915a4903ca..f307580a53 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -2150,8 +2150,8 @@ virPCIDeviceIsBehindSwitchLackingACS(virPCIDevice *dev) return 1; } - tmp = parent; - ret = virPCIDeviceGetParent(parent, &parent); + tmp = g_steal_pointer(&parent); + ret = virPCIDeviceGetParent(tmp, &parent); if (ret < 0) return -1; } while (parent); -- 2.31.1
Please pick a better summary. Something along: virpci: Clarify lifetime of temporary object On Wed, Aug 25, 2021 at 14:54:56 +0200, Michal Privoznik wrote:
The virPCIDeviceIsBehindSwitchLackingACS() function checks whether given PCI device is not behind a switch that lacks ACS. It does so by starting at given device and traversing up, one parent at time towards the root. The parent device is obtained via virPCIDeviceGetParent() which allocates new virPCIDevice structure. For freeing the structure we use g_autoptr() and a temporary variable @tmp. However, Clang fails to understand our clever algorithm and complains that the variable is set but never used. This is obviously a false positive, but using a small trick we can shut Clang up.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virpci.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c index 915a4903ca..f307580a53 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -2150,8 +2150,8 @@ virPCIDeviceIsBehindSwitchLackingACS(virPCIDevice *dev) return 1; }
- tmp = parent; - ret = virPCIDeviceGetParent(parent, &parent); + tmp = g_steal_pointer(&parent); + ret = virPCIDeviceGetParent(tmp, &parent); if (ret < 0) return -1; } while (parent);
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
In qemumigrationcookiexmltest and qemustatusxml2xmltest there is @cfg variable that is unused. It's set via virQEMUDriverGetConfig() but then never used. Drop it. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/qemumigrationcookiexmltest.c | 2 -- tests/qemustatusxml2xmltest.c | 2 -- 2 files changed, 4 deletions(-) diff --git a/tests/qemumigrationcookiexmltest.c b/tests/qemumigrationcookiexmltest.c index ba7088c567..6ef5dc0ad6 100644 --- a/tests/qemumigrationcookiexmltest.c +++ b/tests/qemumigrationcookiexmltest.c @@ -401,7 +401,6 @@ static int mymain(void) { int ret = 0; - g_autoptr(virQEMUDriverConfig) cfg = NULL; g_autoptr(GHashTable) capslatest = NULL; g_autoptr(virConnect) conn = NULL; @@ -412,7 +411,6 @@ mymain(void) if (qemuTestDriverInit(&driver) < 0) return EXIT_FAILURE; - cfg = virQEMUDriverGetConfig(&driver); driver.privileged = true; if (!(conn = virGetConnect())) diff --git a/tests/qemustatusxml2xmltest.c b/tests/qemustatusxml2xmltest.c index d58f4b69da..cd43820551 100644 --- a/tests/qemustatusxml2xmltest.c +++ b/tests/qemustatusxml2xmltest.c @@ -77,7 +77,6 @@ mymain(void) { int ret = 0; g_autofree char *fakerootdir = NULL; - g_autoptr(virQEMUDriverConfig) cfg = NULL; g_autoptr(GHashTable) capslatest = testQemuGetLatestCaps(); g_autoptr(GHashTable) capscache = virHashNew(virObjectFreeHashData); g_autoptr(virConnect) conn = NULL; @@ -100,7 +99,6 @@ mymain(void) if (qemuTestDriverInit(&driver) < 0) return EXIT_FAILURE; - cfg = virQEMUDriverGetConfig(&driver); driver.privileged = true; if (!(conn = virGetConnect())) -- 2.31.1
On Wed, Aug 25, 2021 at 14:54:57 +0200, Michal Privoznik wrote:
In qemumigrationcookiexmltest and qemustatusxml2xmltest there is @cfg variable that is unused. It's set via virQEMUDriverGetConfig() but then never used. Drop it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/qemumigrationcookiexmltest.c | 2 -- tests/qemustatusxml2xmltest.c | 2 -- 2 files changed, 4 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Clang on Rawhide started to complain that @tmp variable in virSCSIDeviceListDel() is set but not used. This is obviously a false positive because the variable is used to free device stolen from the list. Anyway, we can do without the variable so in this specific case let's fix our code to appease Clang. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virscsi.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/util/virscsi.c b/src/util/virscsi.c index 2488b0e5f8..6a90d9002f 100644 --- a/src/util/virscsi.c +++ b/src/util/virscsi.c @@ -414,8 +414,7 @@ virSCSIDeviceListDel(virSCSIDeviceList *list, virSCSIDeviceUsedByInfoFree(dev->used_by[i]); VIR_DELETE_ELEMENT(dev->used_by, i, dev->n_used_by); } else { - g_autoptr(virSCSIDevice) tmp = NULL; - tmp = virSCSIDeviceListSteal(list, dev); + virSCSIDeviceFree(virSCSIDeviceListSteal(list, dev)); } break; } -- 2.31.1
On Wed, Aug 25, 2021 at 14:54:58 +0200, Michal Privoznik wrote:
Clang on Rawhide started to complain that @tmp variable in virSCSIDeviceListDel() is set but not used. This is obviously a false positive because the variable is used to free device stolen from the list. Anyway, we can do without the variable so in this specific case let's fix our code to appease Clang.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virscsi.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
This patch is intentionally wrong (it's leaking memory). I just include it to illustrate Clang bug. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/rpc/virnetclient.c | 9 ++++++--- src/util/viridentity.c | 3 ++- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index ffe2f343f9..eb215f13db 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; + GSource *source = NULL; GIOCondition ev; int ret; @@ -901,6 +901,7 @@ virNetClientTLSHandshake(virNetClient *client) ev, client->eventCtx, virNetClientIOEventTLS, client, NULL); + g_source_unref(source); return TRUE; } @@ -939,7 +940,7 @@ int virNetClientSetTLSSession(virNetClient *client, int ret; char buf[1]; int len; - g_autoptr(GSource) source = NULL; + GSource *source = NULL; #ifndef WIN32 sigset_t oldmask, blockedsigs; @@ -1028,6 +1029,7 @@ int virNetClientSetTLSSession(virNetClient *client, virObjectUnref(client->tls); client->tls = NULL; virObjectUnlock(client); + g_source_unref(source); return -1; } @@ -1664,7 +1666,7 @@ static int virNetClientIOEventLoop(virNetClient *client, #endif /* !WIN32 */ int timeout = -1; virNetMessage *msg = NULL; - g_autoptr(GSource) source = NULL; + GSource *source = NULL; GIOCondition ev = 0; struct virNetClientIOEventData data = { .client = client, @@ -1810,6 +1812,7 @@ static int virNetClientIOEventLoop(virNetClient *client, virNetClientMarkClose(client, closeReason); goto error; } + g_source_unref(source); } error: diff --git a/src/util/viridentity.c b/src/util/viridentity.c index e36e54ae4b..dbaacd6da5 100644 --- a/src/util/viridentity.c +++ b/src/util/viridentity.c @@ -134,12 +134,13 @@ virIdentity *virIdentityGetCurrent(void) */ int virIdentitySetCurrent(virIdentity *ident) { - g_autoptr(virIdentity) old = NULL; + virIdentity *old = NULL; if (virIdentityInitialize() < 0) return -1; old = virThreadLocalGet(&virIdentityCurrent); + g_clear_object(&old); if (virThreadLocalSet(&virIdentityCurrent, ident ? g_object_ref(ident) : NULL) < 0) { -- 2.31.1
participants (2)
-
Michal Privoznik -
Peter Krempa