util: replace atomic ops impls with g_atomic_int*

Libvirt's original atomic ops impls were largely copied
from GLib's code at the time. The only API difference
was that libvirt's virAtomicIntInc() would return a
value, but g_atomic_int_inc was void. We thus use
g_atomic_int_add(v, 1) instead, though this means
virAtomicIntInc() now returns the original value,
instead of the new value.

This rewrites libvirt's impl in terms of g_atomic_int*
as a short term conversion. The key motivation was to
quickly eliminate use of GNULIB's verify_expr() macro
which is not a direct match for G_STATIC_ASSERT_EXPR.
Long term all the callers should be updated to use
g_atomic_int* directly.
Reviewed-by: NPavel Hrdina <phrdina@redhat.com>
Signed-off-by: NDaniel P. Berrangé <berrange@redhat.com>

src/libxl/libxl_domain.c

@@ -1473,7 +1473,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver,
     if (virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0)
         goto destroy_dom;
 
-    if (virAtomicIntInc(&driver->nactive) == 1 && driver->inhibitCallback)
+    if (virAtomicIntInc(&driver->nactive) == 0 && driver->inhibitCallback)
         driver->inhibitCallback(true, driver->inhibitOpaque);
 
     /* finally we can call the 'started' hook script if any */

src/libxl/libxl_driver.c

@@ -446,7 +446,7 @@ libxlReconnectDomain(virDomainObjPtr vm,
         virDomainObjSetState(vm, VIR_DOMAIN_RUNNING,
                              VIR_DOMAIN_RUNNING_UNKNOWN);
 
-    if (virAtomicIntInc(&driver->nactive) == 1 && driver->inhibitCallback)
+    if (virAtomicIntInc(&driver->nactive) == 0 && driver->inhibitCallback)
         driver->inhibitCallback(true, driver->inhibitOpaque);
 
     /* Enable domain death events */

src/lxc/lxc_process.c

@@ -1468,7 +1468,7 @@ int virLXCProcessStart(virConnectPtr conn,
     if (virCommandHandshakeNotify(cmd) < 0)
         goto cleanup;
 
-    if (virAtomicIntInc(&driver->nactive) == 1 && driver->inhibitCallback)
+    if (virAtomicIntInc(&driver->nactive) == 0 && driver->inhibitCallback)
         driver->inhibitCallback(true, driver->inhibitOpaque);
 
     if (lxcContainerWaitForContinue(handshakefds[0]) < 0) {
@@ -1670,7 +1670,7 @@ virLXCProcessReconnectDomain(virDomainObjPtr vm,
         virDomainObjSetState(vm, VIR_DOMAIN_RUNNING,
                              VIR_DOMAIN_RUNNING_UNKNOWN);
 
-        if (virAtomicIntInc(&driver->nactive) == 1 && driver->inhibitCallback)
+        if (virAtomicIntInc(&driver->nactive) == 0 && driver->inhibitCallback)
             driver->inhibitCallback(true, driver->inhibitOpaque);
 
         if (!(priv->monitor = virLXCProcessConnectMonitor(driver, vm)))

src/nwfilter/nwfilter_dhcpsnoop.c

@@ -888,7 +888,7 @@ virNWFilterSnoopReqLeaseDel(virNWFilterSnoopReqPtr req,
  skip_instantiate:
     VIR_FREE(ipl);
 
-    virAtomicIntDecAndTest(&virNWFilterSnoopState.nLeases);
+    ignore_value(virAtomicIntDecAndTest(&virNWFilterSnoopState.nLeases));
 
  lease_not_found:
     VIR_FREE(ipstr);
@@ -1167,7 +1167,7 @@ static void virNWFilterDHCPDecodeWorker(void *jobdata, void *opaque)
                        _("Instantiation of rules failed on "
                          "interface '%s'"), req->binding->portdevname);
     }
-    virAtomicIntDecAndTest(job->qCtr);
+    ignore_value(virAtomicIntDecAndTest(job->qCtr));
     VIR_FREE(job);
 }
 
@@ -1568,7 +1568,7 @@ virNWFilterDHCPSnoopThread(void *req0)
             pcap_close(pcapConf[i].handle);
     }
 
-    virAtomicIntDecAndTest(&virNWFilterSnoopState.nThreads);
+    ignore_value(virAtomicIntDecAndTest(&virNWFilterSnoopState.nThreads));
 
     return;
 }

src/qemu/qemu_process.c

@@ -5571,7 +5571,7 @@ qemuProcessInit(virQEMUDriverPtr driver,
         qemuDomainSetFakeReboot(driver, vm, false);
         virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_STARTING_UP);
 
-        if (virAtomicIntInc(&driver->nactive) == 1 && driver->inhibitCallback)
+        if (virAtomicIntInc(&driver->nactive) == 0 && driver->inhibitCallback)
             driver->inhibitCallback(true, driver->inhibitOpaque);
 
         /* Run an early hook to set-up missing devices */
@@ -8139,7 +8139,7 @@ qemuProcessReconnect(void *opaque)
             goto error;
     }
 
-    if (virAtomicIntInc(&driver->nactive) == 1 && driver->inhibitCallback)
+    if (virAtomicIntInc(&driver->nactive) == 0 && driver->inhibitCallback)
         driver->inhibitCallback(true, driver->inhibitOpaque);
 
  cleanup:

src/util/viratomic.h

 /*
  * viratomic.h: atomic integer operations
  *
- * Copyright (C) 2012 Red Hat, Inc.
- *
- * Based on code taken from GLib 2.32, under the LGPLv2+
- *
- * Copyright (C) 2011 Ryan Lortie
+ * Copyright (C) 2012-2020 Red Hat, Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -21,18 +17,15 @@
  * License along with this library.  If not, see
  * <http://www.gnu.org/licenses/>.
  *
+ * APIs in this header should no longer be used. Direct
+ * use of the g_atomic APIs is preferred & existing code
+ * should be converted as needed.
  */
 
 #pragma once
 
 #include "internal.h"
 
-#ifdef VIR_ATOMIC_OPS_GCC
-# define VIR_STATIC /* Nothing; we just never define the functions */
-#else
-# define VIR_STATIC static
-#endif
-
 /**
  * virAtomicIntGet:
  * Gets the current value of atomic.
@@ -40,8 +33,7 @@
  * This call acts as a full compiler and hardware memory barrier
  * (before the get)
  */
-VIR_STATIC int virAtomicIntGet(volatile int *atomic)
-    ATTRIBUTE_NONNULL(1);
+#define virAtomicIntGet(v) g_atomic_int_get(v)
 
 /**
  * virAtomicIntSet:
@@ -50,21 +42,18 @@ VIR_STATIC int virAtomicIntGet(volatile int *atomic)
  * This call acts as a full compiler and hardware memory barrier
  * (after the set)
  */
-VIR_STATIC void virAtomicIntSet(volatile int *atomic,
-                                int newval)
-    ATTRIBUTE_NONNULL(1);
+#define virAtomicIntSet(i, newv) g_atomic_int_set(i, newv)
 
 /**
  * virAtomicIntInc:
  * Increments the value of atomic by 1.
  *
  * Think of this operation as an atomic version of
- * { *atomic += 1; return *atomic; }
+ * { tmp = *atomic; *atomic += 1; return tmp; }
  *
  * This call acts as a full compiler and hardware memory barrier.
  */
-VIR_STATIC int virAtomicIntInc(volatile int *atomic)
-    ATTRIBUTE_NONNULL(1);
+#define virAtomicIntInc(i) g_atomic_int_add(i, 1)
 
 /**
  * virAtomicIntDecAndTest:
@@ -75,8 +64,7 @@ VIR_STATIC int virAtomicIntInc(volatile int *atomic)
  *
  * This call acts as a full compiler and hardware memory barrier.
  */
-VIR_STATIC bool virAtomicIntDecAndTest(volatile int *atomic)
-    ATTRIBUTE_NONNULL(1);
+#define virAtomicIntDecAndTest(i) (!!g_atomic_int_dec_and_test(i))
 
 /**
  * virAtomicIntCompareExchange:
@@ -91,10 +79,8 @@ VIR_STATIC bool virAtomicIntDecAndTest(volatile int *atomic)
  *
  * This call acts as a full compiler and hardware memory barrier.
  */
-VIR_STATIC bool virAtomicIntCompareExchange(volatile int *atomic,
-                                            int oldval,
-                                            int newval)
-    ATTRIBUTE_NONNULL(1);
+#define virAtomicIntCompareExchange(i, oldi, newi) \
+    (!!g_atomic_int_compare_and_exchange(i, oldi, newi))
 
 /**
  * virAtomicIntAdd:
@@ -105,9 +91,7 @@ VIR_STATIC bool virAtomicIntCompareExchange(volatile int *atomic,
  *
  * This call acts as a full compiler and hardware memory barrier.
  */
-VIR_STATIC int virAtomicIntAdd(volatile int *atomic,
-                               int val)
-    ATTRIBUTE_NONNULL(1);
+#define virAtomicIntAdd(i, v) g_atomic_int_add(i, v)
 
 /**
  * virAtomicIntAnd:
@@ -119,9 +103,7 @@ VIR_STATIC int virAtomicIntAdd(volatile int *atomic,
  * Think of this operation as an atomic version of
  * { tmp = *atomic; *atomic &= val; return tmp; }
  */
-VIR_STATIC unsigned int virAtomicIntAnd(volatile unsigned int *atomic,
-                                        unsigned int val)
-    ATTRIBUTE_NONNULL(1);
+#define virAtomicIntAnd(i, v) g_atomic_int_and(i, v)
 
 /**
  * virAtomicIntOr:
@@ -133,9 +115,7 @@ VIR_STATIC unsigned int virAtomicIntAnd(volatile unsigned int *atomic,
  *
  * This call acts as a full compiler and hardware memory barrier.
  */
-VIR_STATIC unsigned int virAtomicIntOr(volatile unsigned int *atomic,
-                                       unsigned int val)
-    ATTRIBUTE_NONNULL(1);
+#define virAtomicIntOr(i, v) g_atomic_int_or(i, v)
 
 /**
  * virAtomicIntXor:
@@ -147,305 +127,4 @@ VIR_STATIC unsigned int virAtomicIntOr(volatile unsigned int *atomic,
  *
  * This call acts as a full compiler and hardware memory barrier.
  */
-VIR_STATIC unsigned int virAtomicIntXor(volatile unsigned int *atomic,
-                                        unsigned int val)
-    ATTRIBUTE_NONNULL(1);
-
-#undef VIR_STATIC
-
-#ifdef VIR_ATOMIC_OPS_GCC
-
-# define virAtomicIntGet(atomic) \
-    (__extension__ ({ \
-            (void)verify_expr(sizeof(*(atomic)) == sizeof(int), 0); \
-            (void)(0 ? *(atomic) ^ *(atomic) : 0); \
-            __sync_synchronize(); \
-            (int)*(atomic); \
-        }))
-# define virAtomicIntSet(atomic, newval) \
-    (__extension__ ({ \
-            (void)verify_expr(sizeof(*(atomic)) == sizeof(int), 0); \
-            (void)(0 ? *(atomic) ^ (newval) : 0); \
-            *(atomic) = (newval); \
-            __sync_synchronize(); \
-        }))
-# define virAtomicIntInc(atomic) \
-    (__extension__ ({ \
-            (void)verify_expr(sizeof(*(atomic)) == sizeof(int), 0); \
-            (void)(0 ? *(atomic) ^ *(atomic) : 0); \
-            __sync_add_and_fetch((atomic), 1); \
-        }))
-# define virAtomicIntDecAndTest(atomic) \
-    (__extension__ ({ \
-            (void)verify_expr(sizeof(*(atomic)) == sizeof(int), 0); \
-            (void)(0 ? *(atomic) ^ *(atomic) : 0); \
-            __sync_fetch_and_sub((atomic), 1) == 1; \
-        }))
-# define virAtomicIntCompareExchange(atomic, oldval, newval) \
-    (__extension__ ({ \
-            (void)verify_expr(sizeof(*(atomic)) == sizeof(int), 0); \
-            (void)(0 ? *(atomic) ^ (newval) ^ (oldval) : 0); \
-            (bool)__sync_bool_compare_and_swap((atomic), \
-                                               (oldval), (newval)); \
-        }))
-# define virAtomicIntAdd(atomic, val) \
-    (__extension__ ({ \
-            (void)verify_expr(sizeof(*(atomic)) == sizeof(int), 0); \
-            (void)(0 ? *(atomic) ^ (val) : 0); \
-            (int) __sync_fetch_and_add((atomic), (val)); \
-        }))
-# define virAtomicIntAnd(atomic, val) \
-    (__extension__ ({ \
-            (void)verify_expr(sizeof(*(atomic)) == sizeof(int), 0); \
-            (void) (0 ? *(atomic) ^ (val) : 0); \
-            (unsigned int) __sync_fetch_and_and((atomic), (val)); \
-        }))
-# define virAtomicIntOr(atomic, val) \
-    (__extension__ ({ \
-            (void)verify_expr(sizeof(*(atomic)) == sizeof(int), 0); \
-            (void) (0 ? *(atomic) ^ (val) : 0); \
-            (unsigned int) __sync_fetch_and_or((atomic), (val)); \
-        }))
-# define virAtomicIntXor(atomic, val) \
-    (__extension__ ({ \
-            (void)verify_expr(sizeof(*(atomic)) == sizeof(int), 0); \
-            (void) (0 ? *(atomic) ^ (val) : 0); \
-            (unsigned int) __sync_fetch_and_xor((atomic), (val)); \
-        }))
-
-
-#else
-
-# ifdef VIR_ATOMIC_OPS_WIN32
-
-#  include <winsock2.h>
-#  include <windows.h>
-#  include <intrin.h>
-#  if !defined(_M_AMD64) && !defined (_M_IA64) && !defined(_M_X64)
-#   define InterlockedAnd _InterlockedAnd
-#   define InterlockedOr _InterlockedOr
-#   define InterlockedXor _InterlockedXor
-#  endif
-
-/*
- * http://msdn.microsoft.com/en-us/library/ms684122(v=vs.85).aspx
- */
-static inline int
-virAtomicIntGet(volatile int *atomic)
-{
-    MemoryBarrier();
-    return *atomic;
-}
-
-static inline void
-virAtomicIntSet(volatile int *atomic,
-                int newval)
-{
-    *atomic = newval;
-    MemoryBarrier();
-}
-
-static inline int
-virAtomicIntInc(volatile int *atomic)
-{
-    return InterlockedIncrement((volatile LONG *)atomic);
-}
-
-static inline bool
-virAtomicIntDecAndTest(volatile int *atomic)
-{
-    return InterlockedDecrement((volatile LONG *)atomic) == 0;
-}
-
-static inline bool
-virAtomicIntCompareExchange(volatile int *atomic,
-                            int oldval,
-                            int newval)
-{
-    return InterlockedCompareExchange((volatile LONG *)atomic, newval, oldval) == oldval;
-}
-
-static inline int
-virAtomicIntAdd(volatile int *atomic,
-                int val)
-{
-    return InterlockedExchangeAdd((volatile LONG *)atomic, val);
-}
-
-static inline unsigned int
-virAtomicIntAnd(volatile unsigned int *atomic,
-                unsigned int val)
-{
-    return InterlockedAnd((volatile LONG *)atomic, val);
-}
-
-static inline unsigned int
-virAtomicIntOr(volatile unsigned int *atomic,
-               unsigned int val)
-{
-    return InterlockedOr((volatile LONG *)atomic, val);
-}
-
-static inline unsigned int
-virAtomicIntXor(volatile unsigned int *atomic,
-                unsigned int val)
-{
-    return InterlockedXor((volatile LONG *)atomic, val);
-}
-
-
-# else
-#  ifdef VIR_ATOMIC_OPS_PTHREAD
-#   include <pthread.h>
-
-extern pthread_mutex_t virAtomicLock;
-
-static inline int
-virAtomicIntGet(volatile int *atomic)
-{
-    int value;
-
-    pthread_mutex_lock(&virAtomicLock);
-    value = *atomic;
-    pthread_mutex_unlock(&virAtomicLock);
-
-    return value;
-}
-
-static inline void
-virAtomicIntSet(volatile int *atomic,
-                int value)
-{
-    pthread_mutex_lock(&virAtomicLock);
-    *atomic = value;
-    pthread_mutex_unlock(&virAtomicLock);
-}
-
-static inline int
-virAtomicIntInc(volatile int *atomic)
-{
-    int value;
-
-    pthread_mutex_lock(&virAtomicLock);
-    value = ++(*atomic);
-    pthread_mutex_unlock(&virAtomicLock);
-
-    return value;
-}
-
-static inline bool
-virAtomicIntDecAndTest(volatile int *atomic)
-{
-    bool is_zero;
-
-    pthread_mutex_lock(&virAtomicLock);
-    is_zero = --(*atomic) == 0;
-    pthread_mutex_unlock(&virAtomicLock);
-
-    return is_zero;
-}
-
-static inline bool
-virAtomicIntCompareExchange(volatile int *atomic,
-                            int oldval,
-                            int newval)
-{
-    bool success;
-
-    pthread_mutex_lock(&virAtomicLock);
-
-    if ((success = (*atomic == oldval)))
-        *atomic = newval;
-
-    pthread_mutex_unlock(&virAtomicLock);
-
-    return success;
-}
-
-static inline int
-virAtomicIntAdd(volatile int *atomic,
-                int val)
-{
-    int oldval;
-
-    pthread_mutex_lock(&virAtomicLock);
-    oldval = *atomic;
-    *atomic = oldval + val;
-    pthread_mutex_unlock(&virAtomicLock);
-
-    return oldval;
-}
-
-static inline unsigned int
-virAtomicIntAnd(volatile unsigned int *atomic,
-                unsigned int val)
-{
-    unsigned int oldval;
-
-    pthread_mutex_lock(&virAtomicLock);
-    oldval = *atomic;
-    *atomic = oldval & val;
-    pthread_mutex_unlock(&virAtomicLock);
-
-    return oldval;
-}
-
-static inline unsigned int
-virAtomicIntOr(volatile unsigned int *atomic,
-               unsigned int val)
-{
-    unsigned int oldval;
-
-    pthread_mutex_lock(&virAtomicLock);
-    oldval = *atomic;
-    *atomic = oldval | val;
-    pthread_mutex_unlock(&virAtomicLock);
-
-    return oldval;
-}
-
-static inline unsigned int
-virAtomicIntXor(volatile unsigned int *atomic,
-                unsigned int val)
-{
-    unsigned int oldval;
-
-    pthread_mutex_lock(&virAtomicLock);
-    oldval = *atomic;
-    *atomic = oldval ^ val;
-    pthread_mutex_unlock(&virAtomicLock);
-
-    return oldval;
-}
-
-
-#  else
-#   error "No atomic integer impl for this platform"
-#  endif
-# endif
-
-/* The int/unsigned int casts here ensure that you can
- * pass either an int or unsigned int to all atomic op
- * functions, in the same way that we can with GCC
- * atomic op helpers.
- */
-# define virAtomicIntGet(atomic) \
-    virAtomicIntGet((int *)atomic)
-# define virAtomicIntSet(atomic, val) \
-    virAtomicIntSet((int *)atomic, val)
-# define virAtomicIntInc(atomic) \
-    virAtomicIntInc((int *)atomic)
-# define virAtomicIntDecAndTest(atomic) \
-    virAtomicIntDecAndTest((int *)atomic)
-# define virAtomicIntCompareExchange(atomic, oldval, newval) \
-    virAtomicIntCompareExchange((int *)atomic, oldval, newval)
-# define virAtomicIntAdd(atomic, val) \
-    virAtomicIntAdd((int *)atomic, val)
-# define virAtomicIntAnd(atomic, val) \
-    virAtomicIntAnd((unsigned int *)atomic, val)
-# define virAtomicIntOr(atomic, val) \
-    virAtomicIntOr((unsigned int *)atomic, val)
-# define virAtomicIntXor(atomic, val) \
-    virAtomicIntXor((unsigned int *)atomic, val)
-
-#endif
+#define virAtomicIntXor(i, v) g_atomic_int_xor(i, v)

src/util/virprocess.c

@@ -1021,7 +1021,7 @@ int virProcessGetStartTime(pid_t pid,
                            unsigned long long *timestamp)
 {
     static int warned;
-    if (virAtomicIntInc(&warned) == 1) {
+    if (virAtomicIntInc(&warned) == 0) {
         VIR_WARN("Process start time of pid %lld not available on this platform",
                  (long long) pid);
     }

tests/viratomictest.c

@@ -50,7 +50,7 @@ testTypes(const void *data G_GNUC_UNUSED)
     testAssertEq(virAtomicIntAdd(&u, 1), 5);
     testAssertEq(u, 6);
 
-    testAssertEq(virAtomicIntInc(&u), 7);
+    testAssertEq(virAtomicIntInc(&u), 6);
     testAssertEq(u, 7);
 
     res = virAtomicIntDecAndTest(&u);