[libvirt] [snmp PATCH 00/20] Misc cleanups and improvements

These are not pushed. I'll wait couple of moments if there is somebody who has opinion. If not I will push them. Michal Privoznik (20): libvirtGuestTable.c: Free duplicated domain name libvirtSnmp.c: Retab and realign configure: Drop support for libvirt older than 0.9.0 configure.ac: Drop useless "-lvirt" in LIBVIRT_LIBS showError: Switch to less ancient error reporting libvirtSnmp: Modernize libvirtSnmpLoadGuests libvirtSnmp: Modernize insertGuest libvirtSnmp: Rewrite some functions src: Fix header file defines src: Fix includes in header files libvirtRegisterEvents: Drop pthread_attr_init libvirtSnmp: turn showError() into printf-like function libvirtSnmpError: Drop 'extern' for printLibvirtError() libvirtSnmp: Drop 'extern' from function headers libvirtSnmp: s/showError/printLibvirtError/ libvirtSnmpError: Introduce and use printSystemError libvirtSnmp: Fix type of libvirtUnregisterEvents() libvirtSnmp: Report libvirt errors if no domain is found libvirtSnmpInit: Don't report errors from libvirtRegisterEvents() .gitignore: Ignore tags file .gitignore | 1 + configure.ac | 31 +- src/Makefile.am | 18 - src/event_poll.c | 724 ------------------------------------- src/event_poll.h | 132 ------- src/ignore-value.h | 64 ---- src/internal.h | 138 ------- src/libvirtGuestTable.c | 4 +- src/libvirtNotifications.h | 8 +- src/libvirtSnmp.c | 436 ++++++++-------------- src/libvirtSnmp.h | 30 +- src/libvirtSnmpError.c | 83 ++++- src/libvirtSnmpError.h | 23 +- src/memory.c | 313 ---------------- src/memory.h | 212 ----------- src/threads.c | 251 ------------- src/threads.h | 101 ------ src/util.c | 105 ------ src/util.h | 38 -- 19 files changed, 261 insertions(+), 2451 deletions(-) delete mode 100644 src/event_poll.c delete mode 100644 src/event_poll.h delete mode 100644 src/ignore-value.h delete mode 100644 src/internal.h delete mode 100644 src/memory.c delete mode 100644 src/memory.h delete mode 100644 src/threads.c delete mode 100644 src/threads.h delete mode 100644 src/util.c delete mode 100644 src/util.h -- 2.18.1

The domain name is duplicated in insertGuest() but it's never freed leading to the following memleak: 46 bytes in 6 blocks are definitely lost in loss record 795 of 944 at 0x4C2CF0F: malloc (vg_replace_malloc.c:299) by 0x6E6FE79: strdup (in /lib64/libc-2.26.so) by 0x10E665: insertGuest (libvirtSnmp.c:160) by 0x10EA7D: libvirtSnmpLoadGuests (libvirtSnmp.c:285) by 0x10DB01: libvirtGuestTable_container_load (libvirtGuestTable_data_access.c:230) by 0x1133E3: _cache_load (libvirtGuestTable_interface.c:1557) by 0x5F2955B: _cache_load (in /usr/lib64/libnetsnmpagent.so.30.0.3) by 0x5F2A7FF: netsnmp_cache_helper_handler (in /usr/lib64/libnetsnmpagent.so.30.0.3) by 0x5F3CE2C: netsnmp_call_next_handler (in /usr/lib64/libnetsnmpagent.so.30.0.3) by 0x5F316B6: table_helper_handler (in /usr/lib64/libnetsnmpagent.so.30.0.3) by 0x5F3C8E6: netsnmp_call_handlers (in /usr/lib64/libnetsnmpagent.so.30.0.3) by 0x5F4A65E: handle_var_requests (in /usr/lib64/libnetsnmpagent.so.30.0.3) Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirtGuestTable.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/libvirtGuestTable.c b/src/libvirtGuestTable.c index cb4c59f..80645f2 100644 --- a/src/libvirtGuestTable.c +++ b/src/libvirtGuestTable.c @@ -157,9 +157,7 @@ void libvirtGuestTable_rowreq_ctx_cleanup(libvirtGuestTable_rowreq_ctx *rowreq_c netsnmp_assert(NULL != rowreq_ctx); - /* - * TODO:211:o: |-> Perform extra libvirtGuestTable rowreq cleanup. - */ + free(rowreq_ctx->data.libvirtGuestName); } /* libvirtGuestTable_rowreq_ctx_cleanup */ /** -- 2.18.1

This file contains tabs, is misaligned and ugly. Fix this. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirtSnmp.c | 175 +++++++++++++++++++++++----------------------- 1 file changed, 87 insertions(+), 88 deletions(-) diff --git a/src/libvirtSnmp.c b/src/libvirtSnmp.c index 2bd8f01..805cd6f 100644 --- a/src/libvirtSnmp.c +++ b/src/libvirtSnmp.c @@ -98,18 +98,18 @@ showError(virConnectPtr conn) break; case -1: - snmp_log(LOG_ERR, "Parameter error when attempting to get last error\n"); + snmp_log(LOG_ERR, "Parameter error when attempting to get last error\n"); break; default: - snmp_log(LOG_ERR, "libvirt reported: \"%s\"\n", err->message); + snmp_log(LOG_ERR, "libvirt reported: \"%s\"\n", err->message); break; } virResetError(err); free(err); -out: + out: return; } @@ -185,7 +185,7 @@ insertGuest(netsnmp_container *container, virDomainPtr domain) goto out; } -out: + out: return ret; } @@ -218,8 +218,8 @@ libvirtSnmpLoadGuests(netsnmp_container *container) } numIds = virConnectListDomains(conn, - idList, - numActiveDomains); + idList, + numActiveDomains); if (-1 == numIds) { ret = -1; @@ -233,7 +233,7 @@ libvirtSnmpLoadGuests(netsnmp_container *container) if (NULL == domain) { printf("Failed to lookup domain\n"); showError(conn); - ret = -1; + ret = -1; goto out; } @@ -263,8 +263,8 @@ libvirtSnmpLoadGuests(netsnmp_container *container) } numNames = virConnectListDefinedDomains(conn, - nameList, - numDefinedDomains); + nameList, + numDefinedDomains); if (-1 == numNames) { ret = -1; @@ -290,16 +290,17 @@ libvirtSnmpLoadGuests(netsnmp_container *container) goto out_inact; } -out_inact: + out_inact: free(nameList); -out: + out: free(idList); return ret; } /* Polling thread function */ void * -pollingThreadFunc(void *foo) { +pollingThreadFunc(void *foo) +{ while (run) { #ifdef LIBVIRT_OLD if (virEventPollRunOnce() < 0) { @@ -341,8 +342,7 @@ libvirtRegisterEvents(virConnectPtr conn) { pthread_attr_init(&thread_attr); pthread_attr_setdetachstate(&thread_attr, PTHREAD_CREATE_JOINABLE); - if (pthread_create - (&poll_thread, &thread_attr, pollingThreadFunc, NULL)) + if (pthread_create(&poll_thread, &thread_attr, pollingThreadFunc, NULL)) return -1; pthread_attr_destroy(&thread_attr); @@ -373,13 +373,12 @@ int libvirtSnmpInit(void) if (virEventPollInit() < 0) return -1; - virEventRegisterImpl( - virEventPollAddHandle, - virEventPollUpdateHandle, - virEventPollRemoveHandle, - virEventPollAddTimeout, - virEventPollUpdateTimeout, - virEventPollRemoveTimeout); + virEventRegisterImpl(virEventPollAddHandle, + virEventPollUpdateHandle, + virEventPollRemoveHandle, + virEventPollAddTimeout, + virEventPollUpdateTimeout, + virEventPollRemoveTimeout); #else virEventRegisterDefaultImpl(); #endif @@ -437,95 +436,95 @@ void libvirtSnmpShutdown(void) int libvirtSnmpCheckDomainExists(unsigned char *uuid) { - virDomainPtr d = virDomainLookupByUUID(conn, uuid); - if (d == NULL) - return -1; + virDomainPtr d = virDomainLookupByUUID(conn, uuid); + if (d == NULL) + return -1; - virDomainFree(d); - return 0; + virDomainFree(d); + return 0; } int libvirtSnmpCreate(unsigned char *uuid, int state) { - virDomainPtr dom; - int ret; - unsigned int flags = 0; + virDomainPtr dom; + int ret; + unsigned int flags = 0; - dom = virDomainLookupByUUID(conn, uuid); - if (dom == NULL) { - printf("Cannot find domain to create\n"); - return -1; - } + dom = virDomainLookupByUUID(conn, uuid); + if (dom == NULL) { + printf("Cannot find domain to create\n"); + return -1; + } - switch(state) { - case VIR_DOMAIN_RUNNING: - flags = 0; - break; - case VIR_DOMAIN_PAUSED: - flags = VIR_DOMAIN_START_PAUSED; - break; - default: - printf("Can't create domain with state %d\n", flags); - return -1; - } + switch(state) { + case VIR_DOMAIN_RUNNING: + flags = 0; + break; + case VIR_DOMAIN_PAUSED: + flags = VIR_DOMAIN_START_PAUSED; + break; + default: + printf("Can't create domain with state %d\n", flags); + return -1; + } - ret = virDomainCreateWithFlags(dom, flags); - if (ret) { - showError(conn); - } - virDomainFree(dom); - return ret; + ret = virDomainCreateWithFlags(dom, flags); + if (ret) { + showError(conn); + } + virDomainFree(dom); + return ret; } int libvirtSnmpDestroy(unsigned char *uuid) { - virDomainPtr dom; - int ret; + virDomainPtr dom; + int ret; - dom = virDomainLookupByUUID(conn, uuid); - if (dom == NULL) { - printf("Cannot find domain to destroy\n"); - return -1; - } + dom = virDomainLookupByUUID(conn, uuid); + if (dom == NULL) { + printf("Cannot find domain to destroy\n"); + return -1; + } - ret = virDomainDestroy(dom); - if (ret) { - showError(conn); - } - virDomainFree(dom); - return ret; + ret = virDomainDestroy(dom); + if (ret) { + showError(conn); + } + virDomainFree(dom); + return ret; } int libvirtSnmpChangeState(unsigned char *uuid, int newstate, int oldstate) { - virDomainPtr dom; - int ret = 0; + virDomainPtr dom; + int ret = 0; - dom = virDomainLookupByUUID(conn, uuid); - if (dom == NULL) { - printf("Cannot find domain to change\n"); - return 1; - } + dom = virDomainLookupByUUID(conn, uuid); + if (dom == NULL) { + printf("Cannot find domain to change\n"); + return 1; + } - if (oldstate == VIR_DOMAIN_RUNNING && newstate == VIR_DOMAIN_PAUSED) - ret = virDomainSuspend(dom); - else if (oldstate == VIR_DOMAIN_PAUSED && newstate == VIR_DOMAIN_RUNNING) - ret = virDomainResume(dom); - else if (newstate == VIR_DOMAIN_SHUTDOWN) - ret = virDomainShutdown(dom); - else { - printf("Wrong state transition from %d to %d\n", oldstate, newstate); - ret = -1; - goto out; - } + if (oldstate == VIR_DOMAIN_RUNNING && newstate == VIR_DOMAIN_PAUSED) + ret = virDomainSuspend(dom); + else if (oldstate == VIR_DOMAIN_PAUSED && newstate == VIR_DOMAIN_RUNNING) + ret = virDomainResume(dom); + else if (newstate == VIR_DOMAIN_SHUTDOWN) + ret = virDomainShutdown(dom); + else { + printf("Wrong state transition from %d to %d\n", oldstate, newstate); + ret = -1; + goto out; + } - if (ret != 0) { - showError(conn); - } -out: - virDomainFree(dom); - return ret; + if (ret != 0) { + showError(conn); + } + out: + virDomainFree(dom); + return ret; } -- 2.18.1

Historically, we wanted libvirt-snmp to build on RHEL-6.0 which contained ancient libvirt. It's not the case anymore - after six years it is safe to assume everybody upgraded to something newer. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- configure.ac | 29 +- src/Makefile.am | 18 -- src/event_poll.c | 724 --------------------------------------------- src/event_poll.h | 132 --------- src/ignore-value.h | 64 ---- src/internal.h | 138 --------- src/libvirtSnmp.c | 24 +- src/memory.c | 313 -------------------- src/memory.h | 212 ------------- src/threads.c | 251 ---------------- src/threads.h | 101 ------- src/util.c | 105 ------- src/util.h | 38 --- 13 files changed, 2 insertions(+), 2147 deletions(-) delete mode 100644 src/event_poll.c delete mode 100644 src/event_poll.h delete mode 100644 src/ignore-value.h delete mode 100644 src/internal.h delete mode 100644 src/memory.c delete mode 100644 src/memory.h delete mode 100644 src/threads.c delete mode 100644 src/threads.h delete mode 100644 src/util.c delete mode 100644 src/util.h diff --git a/configure.ac b/configure.ac index b8cc784..7834c34 100644 --- a/configure.ac +++ b/configure.ac @@ -23,39 +23,12 @@ dnl dnl do we have libvirt installed? -LIBVIRT_REQUIRED=0.8.0 +LIBVIRT_REQUIRED=0.9.0 PKG_CHECK_MODULES(LIBVIRT, libvirt >= $LIBVIRT_REQUIRED) LIBVIRT_LIBS="$LIBVIRT_LIBS -lvirt" AC_SUBST(LIBVIRT_LIBS) -dnl do we have old libvirt? -save_CFLAGS="$CFLAGS" -save_LDFLAGS="$LDFLAGS" -save_LIBS="$LIBS" -CFLAGS="$CFLAGS $LIBVIRT_CFLAGS" -LDFLAGS="$CFLAGS $LIBVIRT_LIBS" -AC_CHECK_LIB([virt], [virEventRunDefaultImpl], [old=0], [old=1]) -CFLAGS="$save_CFLAGS" -LDFLAGS="$save_LDFLAGS" -LIBS="$save_LIBS" -if test $old = 1 ; then - AC_DEFINE_UNQUOTED([LIBVIRT_OLD], ["$old"], [we are using old libvirt - which does not have new event api]) - AC_CHECK_FUNCS([gettimeofday]) - AC_CHECK_FUNCS([memmove]) - AC_CHECK_HEADERS([fcntl.h]) - AC_CHECK_HEADERS([stddef.h]) - AC_CHECK_HEADERS([sys/time.h]) - AC_CHECK_TYPES([ptrdiff_t]) - AC_C_INLINE - AC_FUNC_REALLOC - AC_HEADER_STDBOOL - AC_TYPE_PID_T - AC_HEADER_TIME -fi -AM_CONDITIONAL([LIBVIRT_OLD], [test $old = 1]) - SNMP_CONFIG="net-snmp-config" SNMP_CFLAGS="" SNMP_LIBS="" diff --git a/src/Makefile.am b/src/Makefile.am index 622a280..0c9b856 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1,19 +1,5 @@ bin_PROGRAMS=libvirtMib_subagent -LIBVIRT_OLD_SRCS = \ - threads.c \ - event_poll.c \ - memory.c \ - util.c - -LIBVIRT_OLD_HDRS = \ - internal.h \ - ignore-value.h \ - threads.h \ - event_poll.h \ - memory.h \ - util.h - USER_SRCS = \ libvirtGuestTable_data_get.c \ libvirtGuestTable_data_set.c \ @@ -61,10 +47,6 @@ libvirtMib_subagent_LDADD= \ $(LIBVIRT_LIBS) \ $(SNMP_LIBS) -if LIBVIRT_OLD -libvirtMib_subagent_SOURCES+=${LIBVIRT_OLD_SRCS} ${LIBVIRT_OLD_HDRS} -endif - EXTRA_DIST = LIBVIRT-MIB.txt install-data-local: diff --git a/src/event_poll.c b/src/event_poll.c deleted file mode 100644 index f8c4a8b..0000000 --- a/src/event_poll.c +++ /dev/null @@ -1,724 +0,0 @@ -/* - * event.c: event loop for monitoring file handles - * - * Copyright (C) 2007, 2010-2011 Red Hat, Inc. - * Copyright (C) 2007 Daniel P. Berrange - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library; if not, write to the Free Software - * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA - * - * Author: Daniel P. Berrange <berrange@redhat.com> - */ - -#include <config.h> - -#include <stdlib.h> -#include <string.h> -#include <poll.h> -#include <sys/time.h> -#include <errno.h> -#include <unistd.h> - -#include "threads.h" -#include "event_poll.h" -#include "memory.h" -#include "util.h" -#include "ignore-value.h" - -#define EVENT_DEBUG(fmt, ...) VIR_DEBUG(fmt, __VA_ARGS__) - -static int virEventPollInterruptLocked(void); - -/* State for a single file handle being monitored */ -struct virEventPollHandle { - int watch; - int fd; - int events; - virEventHandleCallback cb; - virFreeCallback ff; - void *opaque; - int deleted; -}; - -/* State for a single timer being generated */ -struct virEventPollTimeout { - int timer; - int frequency; - unsigned long long expiresAt; - virEventTimeoutCallback cb; - virFreeCallback ff; - void *opaque; - int deleted; -}; - -/* Allocate extra slots for virEventPollHandle/virEventPollTimeout - records in this multiple */ -#define EVENT_ALLOC_EXTENT 10 - -/* State for the main event loop */ -struct virEventPollLoop { - virMutex lock; - int running; - virThread leader; - int wakeupfd[2]; - size_t handlesCount; - size_t handlesAlloc; - struct virEventPollHandle *handles; - size_t timeoutsCount; - size_t timeoutsAlloc; - struct virEventPollTimeout *timeouts; -}; - -/* Only have one event loop */ -static struct virEventPollLoop eventLoop; - -/* Unique ID for the next FD watch to be registered */ -static int nextWatch = 1; - -/* Unique ID for the next timer to be registered */ -static int nextTimer = 1; - -/* - * Register a callback for monitoring file handle events. - * NB, it *must* be safe to call this from within a callback - * For this reason we only ever append to existing list. - */ -int virEventPollAddHandle(int fd, int events, - virEventHandleCallback cb, - void *opaque, - virFreeCallback ff) { - int watch; - EVENT_DEBUG("Add handle fd=%d events=%d cb=%p opaque=%p", fd, events, cb, opaque); - virMutexLock(&eventLoop.lock); - if (eventLoop.handlesCount == eventLoop.handlesAlloc) { - EVENT_DEBUG("Used %zu handle slots, adding at least %d more", - eventLoop.handlesAlloc, EVENT_ALLOC_EXTENT); - if (VIR_RESIZE_N(eventLoop.handles, eventLoop.handlesAlloc, - eventLoop.handlesCount, EVENT_ALLOC_EXTENT) < 0) { - virMutexUnlock(&eventLoop.lock); - return -1; - } - } - - watch = nextWatch++; - - eventLoop.handles[eventLoop.handlesCount].watch = watch; - eventLoop.handles[eventLoop.handlesCount].fd = fd; - eventLoop.handles[eventLoop.handlesCount].events = - virEventPollToNativeEvents(events); - eventLoop.handles[eventLoop.handlesCount].cb = cb; - eventLoop.handles[eventLoop.handlesCount].ff = ff; - eventLoop.handles[eventLoop.handlesCount].opaque = opaque; - eventLoop.handles[eventLoop.handlesCount].deleted = 0; - - eventLoop.handlesCount++; - - virEventPollInterruptLocked(); - virMutexUnlock(&eventLoop.lock); - - return watch; -} - -void virEventPollUpdateHandle(int watch, int events) { - int i; - EVENT_DEBUG("Update handle w=%d e=%d", watch, events); - - if (watch <= 0) { - VIR_WARN("Ignoring invalid update watch %d", watch); - return; - } - - virMutexLock(&eventLoop.lock); - for (i = 0 ; i < eventLoop.handlesCount ; i++) { - if (eventLoop.handles[i].watch == watch) { - eventLoop.handles[i].events = - virEventPollToNativeEvents(events); - virEventPollInterruptLocked(); - break; - } - } - virMutexUnlock(&eventLoop.lock); -} - -/* - * Unregister a callback from a file handle - * NB, it *must* be safe to call this from within a callback - * For this reason we only ever set a flag in the existing list. - * Actual deletion will be done out-of-band - */ -int virEventPollRemoveHandle(int watch) { - int i; - EVENT_DEBUG("Remove handle w=%d", watch); - - if (watch <= 0) { - VIR_WARN("Ignoring invalid remove watch %d", watch); - return -1; - } - - virMutexLock(&eventLoop.lock); - for (i = 0 ; i < eventLoop.handlesCount ; i++) { - if (eventLoop.handles[i].deleted) - continue; - - if (eventLoop.handles[i].watch == watch) { - EVENT_DEBUG("mark delete %d %d", i, eventLoop.handles[i].fd); - eventLoop.handles[i].deleted = 1; - virEventPollInterruptLocked(); - virMutexUnlock(&eventLoop.lock); - return 0; - } - } - virMutexUnlock(&eventLoop.lock); - return -1; -} - - -/* - * Register a callback for a timer event - * NB, it *must* be safe to call this from within a callback - * For this reason we only ever append to existing list. - */ -int virEventPollAddTimeout(int frequency, - virEventTimeoutCallback cb, - void *opaque, - virFreeCallback ff) { - struct timeval now; - int ret; - EVENT_DEBUG("Adding timer %d with %d ms freq", nextTimer, frequency); - if (gettimeofday(&now, NULL) < 0) { - return -1; - } - - virMutexLock(&eventLoop.lock); - if (eventLoop.timeoutsCount == eventLoop.timeoutsAlloc) { - EVENT_DEBUG("Used %zu timeout slots, adding at least %d more", - eventLoop.timeoutsAlloc, EVENT_ALLOC_EXTENT); - if (VIR_RESIZE_N(eventLoop.timeouts, eventLoop.timeoutsAlloc, - eventLoop.timeoutsCount, EVENT_ALLOC_EXTENT) < 0) { - virMutexUnlock(&eventLoop.lock); - return -1; - } - } - - eventLoop.timeouts[eventLoop.timeoutsCount].timer = nextTimer++; - eventLoop.timeouts[eventLoop.timeoutsCount].frequency = frequency; - eventLoop.timeouts[eventLoop.timeoutsCount].cb = cb; - eventLoop.timeouts[eventLoop.timeoutsCount].ff = ff; - eventLoop.timeouts[eventLoop.timeoutsCount].opaque = opaque; - eventLoop.timeouts[eventLoop.timeoutsCount].deleted = 0; - eventLoop.timeouts[eventLoop.timeoutsCount].expiresAt = - frequency >= 0 ? frequency + - (((unsigned long long)now.tv_sec)*1000) + - (((unsigned long long)now.tv_usec)/1000) : 0; - - eventLoop.timeoutsCount++; - ret = nextTimer-1; - virEventPollInterruptLocked(); - virMutexUnlock(&eventLoop.lock); - return ret; -} - -void virEventPollUpdateTimeout(int timer, int frequency) { - struct timeval tv; - int i; - EVENT_DEBUG("Updating timer %d timeout with %d ms freq", timer, frequency); - - if (timer <= 0) { - VIR_WARN("Ignoring invalid update timer %d", timer); - return; - } - - if (gettimeofday(&tv, NULL) < 0) { - return; - } - - virMutexLock(&eventLoop.lock); - for (i = 0 ; i < eventLoop.timeoutsCount ; i++) { - if (eventLoop.timeouts[i].timer == timer) { - eventLoop.timeouts[i].frequency = frequency; - eventLoop.timeouts[i].expiresAt = - frequency >= 0 ? frequency + - (((unsigned long long)tv.tv_sec)*1000) + - (((unsigned long long)tv.tv_usec)/1000) : 0; - virEventPollInterruptLocked(); - break; - } - } - virMutexUnlock(&eventLoop.lock); -} - -/* - * Unregister a callback for a timer - * NB, it *must* be safe to call this from within a callback - * For this reason we only ever set a flag in the existing list. - * Actual deletion will be done out-of-band - */ -int virEventPollRemoveTimeout(int timer) { - int i; - EVENT_DEBUG("Remove timer %d", timer); - - if (timer <= 0) { - VIR_WARN("Ignoring invalid remove timer %d", timer); - return -1; - } - - virMutexLock(&eventLoop.lock); - for (i = 0 ; i < eventLoop.timeoutsCount ; i++) { - if (eventLoop.timeouts[i].deleted) - continue; - - if (eventLoop.timeouts[i].timer == timer) { - eventLoop.timeouts[i].deleted = 1; - virEventPollInterruptLocked(); - virMutexUnlock(&eventLoop.lock); - return 0; - } - } - virMutexUnlock(&eventLoop.lock); - return -1; -} - -/* Iterates over all registered timeouts and determine which - * will be the first to expire. - * @timeout: filled with expiry time of soonest timer, or -1 if - * no timeout is pending - * returns: 0 on success, -1 on error - */ -static int virEventPollCalculateTimeout(int *timeout) { - unsigned long long then = 0; - int i; - EVENT_DEBUG("Calculate expiry of %zu timers", eventLoop.timeoutsCount); - /* Figure out if we need a timeout */ - for (i = 0 ; i < eventLoop.timeoutsCount ; i++) { - if (eventLoop.timeouts[i].frequency < 0) - continue; - - EVENT_DEBUG("Got a timeout scheduled for %llu", eventLoop.timeouts[i].expiresAt); - if (then == 0 || - eventLoop.timeouts[i].expiresAt < then) - then = eventLoop.timeouts[i].expiresAt; - } - - /* Calculate how long we should wait for a timeout if needed */ - if (then > 0) { - struct timeval tv; - - if (gettimeofday(&tv, NULL) < 0) { - perror("Unable to get current time"); - return -1; - } - - *timeout = then - - ((((unsigned long long)tv.tv_sec)*1000) + - (((unsigned long long)tv.tv_usec)/1000)); - - if (*timeout < 0) - *timeout = 0; - } else { - *timeout = -1; - } - - EVENT_DEBUG("Timeout at %llu due in %d ms", then, *timeout); - - return 0; -} - -/* - * Allocate a pollfd array containing data for all registered - * file handles. The caller must free the returned data struct - * returns: the pollfd array, or NULL on error - */ -static struct pollfd *virEventPollMakePollFDs(int *nfds) { - struct pollfd *fds; - int i; - - *nfds = 0; - for (i = 0 ; i < eventLoop.handlesCount ; i++) { - if (eventLoop.handles[i].events && !eventLoop.handles[i].deleted) - (*nfds)++; - } - - /* Setup the poll file handle data structs */ - if (VIR_ALLOC_N(fds, *nfds) < 0) { - perror("unable to allocate memory"); - return NULL; - } - - *nfds = 0; - for (i = 0 ; i < eventLoop.handlesCount ; i++) { - EVENT_DEBUG("Prepare n=%d w=%d, f=%d e=%d d=%d", i, - eventLoop.handles[i].watch, - eventLoop.handles[i].fd, - eventLoop.handles[i].events, - eventLoop.handles[i].deleted); - if (!eventLoop.handles[i].events || eventLoop.handles[i].deleted) - continue; - fds[*nfds].fd = eventLoop.handles[i].fd; - fds[*nfds].events = eventLoop.handles[i].events; - fds[*nfds].revents = 0; - (*nfds)++; - //EVENT_DEBUG("Wait for %d %d", eventLoop.handles[i].fd, eventLoop.handles[i].events); - } - - return fds; -} - - -/* - * Iterate over all timers and determine if any have expired. - * Invoke the user supplied callback for each timer whose - * expiry time is met, and schedule the next timeout. Does - * not try to 'catch up' on time if the actual expiry time - * was later than the requested time. - * - * This method must cope with new timers being registered - * by a callback, and must skip any timers marked as deleted. - * - * Returns 0 upon success, -1 if an error occurred - */ -static int virEventPollDispatchTimeouts(void) { - struct timeval tv; - unsigned long long now; - int i; - /* Save this now - it may be changed during dispatch */ - int ntimeouts = eventLoop.timeoutsCount; - VIR_DEBUG("Dispatch %d", ntimeouts); - - if (gettimeofday(&tv, NULL) < 0) { - perror("Unable to get current time"); - return -1; - } - now = (((unsigned long long)tv.tv_sec)*1000) + - (((unsigned long long)tv.tv_usec)/1000); - - for (i = 0 ; i < ntimeouts ; i++) { - if (eventLoop.timeouts[i].deleted || eventLoop.timeouts[i].frequency < 0) - continue; - - /* Add 20ms fuzz so we don't pointlessly spin doing - * <10ms sleeps, particularly on kernels with low HZ - * it is fine that a timer expires 20ms earlier than - * requested - */ - if (eventLoop.timeouts[i].expiresAt <= (now+20)) { - virEventTimeoutCallback cb = eventLoop.timeouts[i].cb; - int timer = eventLoop.timeouts[i].timer; - void *opaque = eventLoop.timeouts[i].opaque; - eventLoop.timeouts[i].expiresAt = - now + eventLoop.timeouts[i].frequency; - - virMutexUnlock(&eventLoop.lock); - (cb)(timer, opaque); - virMutexLock(&eventLoop.lock); - } - } - return 0; -} - - -/* Iterate over all file handles and dispatch any which - * have pending events listed in the poll() data. Invoke - * the user supplied callback for each handle which has - * pending events - * - * This method must cope with new handles being registered - * by a callback, and must skip any handles marked as deleted. - * - * Returns 0 upon success, -1 if an error occurred - */ -static int virEventPollDispatchHandles(int nfds, struct pollfd *fds) { - int i, n; - VIR_DEBUG("Dispatch %d", nfds); - - /* NB, use nfds not eventLoop.handlesCount, because new - * fds might be added on end of list, and they're not - * in the fds array we've got */ - for (i = 0, n = 0 ; n < nfds && i < eventLoop.handlesCount ; n++) { - while ((eventLoop.handles[i].fd != fds[n].fd || - eventLoop.handles[i].events == 0) && - i < eventLoop.handlesCount) { - i++; - } - if (i == eventLoop.handlesCount) - break; - - VIR_DEBUG("i=%d w=%d", i, eventLoop.handles[i].watch); - if (eventLoop.handles[i].deleted) { - EVENT_DEBUG("Skip deleted n=%d w=%d f=%d", i, - eventLoop.handles[i].watch, eventLoop.handles[i].fd); - continue; - } - - if (fds[n].revents) { - virEventHandleCallback cb = eventLoop.handles[i].cb; - int watch = eventLoop.handles[i].watch; - void *opaque = eventLoop.handles[i].opaque; - int hEvents = virEventPollFromNativeEvents(fds[n].revents); - EVENT_DEBUG("Dispatch n=%d f=%d w=%d e=%d %p", i, - fds[n].fd, watch, fds[n].revents, opaque); - virMutexUnlock(&eventLoop.lock); - (cb)(watch, fds[n].fd, hEvents, opaque); - virMutexLock(&eventLoop.lock); - } - } - - return 0; -} - - -/* Used post dispatch to actually remove any timers that - * were previously marked as deleted. This asynchronous - * cleanup is needed to make dispatch re-entrant safe. - */ -static void virEventPollCleanupTimeouts(void) { - int i; - size_t gap; - VIR_DEBUG("Cleanup %zu", eventLoop.timeoutsCount); - - /* Remove deleted entries, shuffling down remaining - * entries as needed to form contiguous series - */ - for (i = 0 ; i < eventLoop.timeoutsCount ; ) { - if (!eventLoop.timeouts[i].deleted) { - i++; - continue; - } - - EVENT_DEBUG("Purging timeout %d with id %d", i, - eventLoop.timeouts[i].timer); - if (eventLoop.timeouts[i].ff) { - virFreeCallback ff = eventLoop.timeouts[i].ff; - void *opaque = eventLoop.timeouts[i].opaque; - virMutexUnlock(&eventLoop.lock); - ff(opaque); - virMutexLock(&eventLoop.lock); - } - - if ((i+1) < eventLoop.timeoutsCount) { - memmove(eventLoop.timeouts+i, - eventLoop.timeouts+i+1, - sizeof(struct virEventPollTimeout)*(eventLoop.timeoutsCount - -(i+1))); - } - eventLoop.timeoutsCount--; - } - - /* Release some memory if we've got a big chunk free */ - gap = eventLoop.timeoutsAlloc - eventLoop.timeoutsCount; - if (eventLoop.timeoutsCount == 0 || - (gap > eventLoop.timeoutsCount && gap > EVENT_ALLOC_EXTENT)) { - EVENT_DEBUG("Found %zu out of %zu timeout slots used, releasing %zu", - eventLoop.timeoutsCount, eventLoop.timeoutsAlloc, gap); - VIR_SHRINK_N(eventLoop.timeouts, eventLoop.timeoutsAlloc, gap); - } -} - -/* Used post dispatch to actually remove any handles that - * were previously marked as deleted. This asynchronous - * cleanup is needed to make dispatch re-entrant safe. - */ -static void virEventPollCleanupHandles(void) { - int i; - size_t gap; - VIR_DEBUG("Cleanup %zu", eventLoop.handlesCount); - - /* Remove deleted entries, shuffling down remaining - * entries as needed to form contiguous series - */ - for (i = 0 ; i < eventLoop.handlesCount ; ) { - if (!eventLoop.handles[i].deleted) { - i++; - continue; - } - - if (eventLoop.handles[i].ff) { - virFreeCallback ff = eventLoop.handles[i].ff; - void *opaque = eventLoop.handles[i].opaque; - virMutexUnlock(&eventLoop.lock); - ff(opaque); - virMutexLock(&eventLoop.lock); - } - - if ((i+1) < eventLoop.handlesCount) { - memmove(eventLoop.handles+i, - eventLoop.handles+i+1, - sizeof(struct virEventPollHandle)*(eventLoop.handlesCount - -(i+1))); - } - eventLoop.handlesCount--; - } - - /* Release some memory if we've got a big chunk free */ - gap = eventLoop.handlesAlloc - eventLoop.handlesCount; - if (eventLoop.handlesCount == 0 || - (gap > eventLoop.handlesCount && gap > EVENT_ALLOC_EXTENT)) { - EVENT_DEBUG("Found %zu out of %zu handles slots used, releasing %zu", - eventLoop.handlesCount, eventLoop.handlesAlloc, gap); - VIR_SHRINK_N(eventLoop.handles, eventLoop.handlesAlloc, gap); - } -} - -/* - * Run a single iteration of the event loop, blocking until - * at least one file handle has an event, or a timer expires - */ -int virEventPollRunOnce(void) { - struct pollfd *fds = NULL; - int ret, timeout, nfds; - - virMutexLock(&eventLoop.lock); - eventLoop.running = 1; - virThreadSelf(&eventLoop.leader); - - virEventPollCleanupTimeouts(); - virEventPollCleanupHandles(); - - if (!(fds = virEventPollMakePollFDs(&nfds)) || - virEventPollCalculateTimeout(&timeout) < 0) - goto error; - - virMutexUnlock(&eventLoop.lock); - - retry: - EVENT_DEBUG("Poll on %d handles %p timeout %d", nfds, fds, timeout); - ret = poll(fds, nfds, timeout); - if (ret < 0) { - EVENT_DEBUG("Poll got error event %d", errno); - if (errno == EINTR) { - goto retry; - } - perror("Unable to poll on file handles"); - goto error_unlocked; - } - EVENT_DEBUG("Poll got %d event(s)", ret); - - virMutexLock(&eventLoop.lock); - if (virEventPollDispatchTimeouts() < 0) - goto error; - - if (ret > 0 && - virEventPollDispatchHandles(nfds, fds) < 0) - goto error; - - virEventPollCleanupTimeouts(); - virEventPollCleanupHandles(); - - eventLoop.running = 0; - virMutexUnlock(&eventLoop.lock); - VIR_FREE(fds); - return 0; - -error: - virMutexUnlock(&eventLoop.lock); -error_unlocked: - VIR_FREE(fds); - return -1; -} - - -static void virEventPollHandleWakeup(int watch ATTRIBUTE_UNUSED, - int fd, - int events ATTRIBUTE_UNUSED, - void *opaque ATTRIBUTE_UNUSED) -{ - char c; - virMutexLock(&eventLoop.lock); - ignore_value(saferead(fd, &c, sizeof(c))); - virMutexUnlock(&eventLoop.lock); -} - -int virEventPollInit(void) -{ - if (virMutexInit(&eventLoop.lock) < 0) { - perror("Unable to initialize mutex"); - return -1; - } - - if (pipe(eventLoop.wakeupfd) < 0 || - virSetNonBlock(eventLoop.wakeupfd[0]) < 0 || - virSetNonBlock(eventLoop.wakeupfd[1]) < 0 || - virSetCloseExec(eventLoop.wakeupfd[0]) < 0 || - virSetCloseExec(eventLoop.wakeupfd[1]) < 0) { - perror("Unable to setup wakeup pipe"); - return -1; - } - - if (virEventPollAddHandle(eventLoop.wakeupfd[0], - VIR_EVENT_HANDLE_READABLE, - virEventPollHandleWakeup, NULL, NULL) < 0) { - fprintf(stderr, "Unable to add handle %d to event loop", - eventLoop.wakeupfd[0]); - return -1; - } - - return 0; -} - -static int virEventPollInterruptLocked(void) -{ - char c = '\0'; - - if (!eventLoop.running || - virThreadIsSelf(&eventLoop.leader)) { - VIR_DEBUG("Skip interrupt, %d %d", eventLoop.running, - virThreadID(&eventLoop.leader)); - return 0; - } - - VIR_DEBUG0("Interrupting"); - if (safewrite(eventLoop.wakeupfd[1], &c, sizeof(c)) != sizeof(c)) - return -1; - return 0; -} - -int virEventPollInterrupt(void) -{ - int ret; - virMutexLock(&eventLoop.lock); - ret = virEventPollInterruptLocked(); - virMutexUnlock(&eventLoop.lock); - return ret; -} - -int -virEventPollToNativeEvents(int events) -{ - int ret = 0; - if(events & VIR_EVENT_HANDLE_READABLE) - ret |= POLLIN; - if(events & VIR_EVENT_HANDLE_WRITABLE) - ret |= POLLOUT; - if(events & VIR_EVENT_HANDLE_ERROR) - ret |= POLLERR; - if(events & VIR_EVENT_HANDLE_HANGUP) - ret |= POLLHUP; - return ret; -} - -int -virEventPollFromNativeEvents(int events) -{ - int ret = 0; - if(events & POLLIN) - ret |= VIR_EVENT_HANDLE_READABLE; - if(events & POLLOUT) - ret |= VIR_EVENT_HANDLE_WRITABLE; - if(events & POLLERR) - ret |= VIR_EVENT_HANDLE_ERROR; - if(events & POLLNVAL) /* Treat NVAL as error, since libvirt doesn't distinguish */ - ret |= VIR_EVENT_HANDLE_ERROR; - if(events & POLLHUP) - ret |= VIR_EVENT_HANDLE_HANGUP; - return ret; -} diff --git a/src/event_poll.h b/src/event_poll.h deleted file mode 100644 index 4ab3789..0000000 --- a/src/event_poll.h +++ /dev/null @@ -1,132 +0,0 @@ -/* - * event.h: event loop for monitoring file handles - * - * Copyright (C) 2007 Daniel P. Berrange - * Copyright (C) 2007 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 - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library; if not, write to the Free Software - * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA - * - * Author: Daniel P. Berrange <berrange@redhat.com> - */ - -#ifndef __VIR_EVENT_POLL_H__ -# define __VIR_EVENT_POLL_H__ - -# include "internal.h" - -/** - * virEventPollAddHandle: register a callback for monitoring file handle events - * - * @fd: file handle to monitor for events - * @events: bitset of events to watch from POLLnnn constants - * @cb: callback to invoke when an event occurs - * @opaque: user data to pass to callback - * - * returns -1 if the file handle cannot be registered, 0 upon success - */ -int virEventPollAddHandle(int fd, int events, - virEventHandleCallback cb, - void *opaque, - virFreeCallback ff); - -/** - * virEventPollUpdateHandle: change event set for a monitored file handle - * - * @watch: watch whose handle to update - * @events: bitset of events to watch from POLLnnn constants - * - * Will not fail if fd exists - */ -void virEventPollUpdateHandle(int watch, int events); - -/** - * virEventPollRemoveHandle: unregister a callback from a file handle - * - * @watch: watch whose handle to remove - * - * returns -1 if the file handle was not registered, 0 upon success - */ -int virEventPollRemoveHandle(int watch); - -/** - * virEventPollAddTimeout: register a callback for a timer event - * - * @frequency: time between events in milliseconds - * @cb: callback to invoke when an event occurs - * @opaque: user data to pass to callback - * - * Setting frequency to -1 will disable the timer. Setting the frequency - * to zero will cause it to fire on every event loop iteration. - * - * returns -1 if the file handle cannot be registered, a positive - * integer timer id upon success - */ -int virEventPollAddTimeout(int frequency, - virEventTimeoutCallback cb, - void *opaque, - virFreeCallback ff); - -/** - * virEventPollUpdateTimeout: change frequency for a timer - * - * @timer: timer id to change - * @frequency: time between events in milliseconds - * - * Setting frequency to -1 will disable the timer. Setting the frequency - * to zero will cause it to fire on every event loop iteration. - * - * Will not fail if timer exists - */ -void virEventPollUpdateTimeout(int timer, int frequency); - -/** - * virEventPollRemoveTimeout: unregister a callback for a timer - * - * @timer: the timer id to remove - * - * returns -1 if the timer was not registered, 0 upon success - */ -int virEventPollRemoveTimeout(int timer); - -/** - * virEventPollInit: Initialize the event loop - * - * returns -1 if initialization failed - */ -int virEventPollInit(void); - -/** - * virEventPollRunOnce: run a single iteration of the event loop. - * - * Blocks the caller until at least one file handle has an - * event or the first timer expires. - * - * returns -1 if the event monitoring failed - */ -int virEventPollRunOnce(void); - -int virEventPollFromNativeEvents(int events); -int virEventPollToNativeEvents(int events); - - -/** - * virEventPollInterrupt: wakeup any thread waiting in poll() - * - * return -1 if wakup failed - */ -int virEventPollInterrupt(void); - - -#endif /* __VIRTD_EVENT_H__ */ diff --git a/src/ignore-value.h b/src/ignore-value.h deleted file mode 100644 index 0df1c01..0000000 --- a/src/ignore-value.h +++ /dev/null @@ -1,64 +0,0 @@ -/* -*- buffer-read-only: t -*- vi: set ro: */ -/* DO NOT EDIT! GENERATED AUTOMATICALLY! */ -/* ignore a function return without a compiler warning - - Copyright (C) 2008-2011 Free Software Foundation, Inc. - - This program is free software: you can redistribute it and/or modify - it under the terms of the GNU Lesser General Public License as published by - the Free Software Foundation; either version 2.1 of the License, or - (at your option) any later version. - - This program is distributed in the hope that it will be useful, - but WITHOUT ANY WARRANTY; without even the implied warranty of - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - GNU Lesser General Public License for more details. - - You should have received a copy of the GNU Lesser General Public License - along with this program. If not, see <http://www.gnu.org/licenses/>. */ - -/* Written by Jim Meyering, Eric Blake and Pádraig Brady. */ - -/* Use "ignore_value" to avoid a warning when using a function declared with - gcc's warn_unused_result attribute, but for which you really do want to - ignore the result. Traditionally, people have used a "(void)" cast to - indicate that a function's return value is deliberately unused. However, - if the function is declared with __attribute__((warn_unused_result)), - gcc issues a warning even with the cast. - - Caution: most of the time, you really should heed gcc's warning, and - check the return value. However, in those exceptional cases in which - you're sure you know what you're doing, use this function. - - For the record, here's one of the ignorable warnings: - "copy.c:233: warning: ignoring return value of 'fchown', - declared with attribute warn_unused_result". */ - -#ifndef _GL_IGNORE_VALUE_H -# define _GL_IGNORE_VALUE_H - -# ifndef _GL_ATTRIBUTE_DEPRECATED -/* The __attribute__((__deprecated__)) feature - is available in gcc versions 3.1 and newer. */ -# if __GNUC__ < 3 || (__GNUC__ == 3 && __GNUC_MINOR__ < 1) -# define _GL_ATTRIBUTE_DEPRECATED /* empty */ -# else -# define _GL_ATTRIBUTE_DEPRECATED __attribute__ ((__deprecated__)) -# endif -# endif - -/* The __attribute__((__warn_unused_result__)) feature - is available in gcc versions 3.4 and newer, - while the typeof feature has been available since 2.7 at least. */ -# if __GNUC__ < 3 || (__GNUC__ == 3 && __GNUC_MINOR__ < 4) -# define ignore_value(x) ((void) (x)) -# else -# define ignore_value(x) (({ __typeof__ (x) __x = (x); (void) __x; })) -# endif - -/* ignore_value works for scalars, pointers and aggregates; - deprecate ignore_ptr. */ -static inline void _GL_ATTRIBUTE_DEPRECATED -ignore_ptr (void *p) { (void) p; } /* deprecated: use ignore_value */ - -#endif diff --git a/src/internal.h b/src/internal.h deleted file mode 100644 index 4910059..0000000 --- a/src/internal.h +++ /dev/null @@ -1,138 +0,0 @@ -/* - * internal.h: internal definitions just used by code from the library - * - * Copy: Copyright (C) 2005-2006, 2010-2011 Red Hat, Inc. - * - * See libvirt's COPYING.LIB for the License of this software - * - */ - -#ifndef __INTERNAL_H__ -# define __INTERNAL_H__ - -# include <stdio.h> -# include <stdlib.h> -# include <stdbool.h> -# include <stddef.h> -# include <errno.h> -# include <string.h> -# include <libvirt/libvirt.h> -# include <libvirt/virterror.h> - -/* C99 uses __func__. __FUNCTION__ is legacy. */ -# ifndef __GNUC__ -# define __FUNCTION__ __func__ -# endif - -# ifdef __GNUC__ - -# ifndef __GNUC_PREREQ -# if defined __GNUC__ && defined __GNUC_MINOR__ -# define __GNUC_PREREQ(maj, min) \ - ((__GNUC__ << 16) + __GNUC_MINOR__ >= ((maj) << 16) + (min)) -# else -# define __GNUC_PREREQ(maj,min) 0 -# endif - -/* Work around broken limits.h on debian etch */ -# if defined _GCC_LIMITS_H_ && ! defined ULLONG_MAX -# define ULLONG_MAX ULONG_LONG_MAX -# endif - -# endif /* __GNUC__ */ - -/** - * ATTRIBUTE_UNUSED: - * - * Macro to flag conciously unused parameters to functions - */ -# ifndef ATTRIBUTE_UNUSED -# define ATTRIBUTE_UNUSED __attribute__((__unused__)) -# endif - -/** - * ATTRIBUTE_SENTINEL: - * - * Macro to check for NULL-terminated varargs lists - */ -# ifndef ATTRIBUTE_SENTINEL -# if __GNUC_PREREQ (4, 0) -# define ATTRIBUTE_SENTINEL __attribute__((__sentinel__)) -# else -# define ATTRIBUTE_SENTINEL -# endif -# endif - -/** - * ATTRIBUTE_FMT_PRINTF - * - * Macro used to check printf like functions, if compiling - * with gcc. - * - * We use gnulib which guarentees we always have GNU style - * printf format specifiers even on broken Win32 platforms - * hence we have to force 'gnu_printf' for new GCC - */ -# ifndef ATTRIBUTE_FMT_PRINTF -# if __GNUC_PREREQ (4, 4) -# define ATTRIBUTE_FMT_PRINTF(fmtpos,argpos) __attribute__((__format__ (gnu_printf, fmtpos,argpos))) -# else -# define ATTRIBUTE_FMT_PRINTF(fmtpos,argpos) __attribute__((__format__ (printf, fmtpos,argpos))) -# endif -# endif - -# ifndef ATTRIBUTE_RETURN_CHECK -# if __GNUC_PREREQ (3, 4) -# define ATTRIBUTE_RETURN_CHECK __attribute__((__warn_unused_result__)) -# else -# define ATTRIBUTE_RETURN_CHECK -# endif -# endif - -/** - * ATTRIBUTE_PACKED - * - * force a structure to be packed, i.e. not following architecture and - * compiler best alignments for its sub components. It's needed for example - * for the network filetering code when defining the content of raw - * ethernet packets. - * Others compiler than gcc may use something different e.g. #pragma pack(1) - */ -# ifndef ATTRIBUTE_PACKED -# if __GNUC_PREREQ (3, 3) -# define ATTRIBUTE_PACKED __attribute__((packed)) -# else -# error "Need an __attribute__((packed)) equivalent" -# endif -# endif - -# ifndef ATTRIBUTE_NONNULL -# if __GNUC_PREREQ (3, 3) -# define ATTRIBUTE_NONNULL(m) __attribute__((__nonnull__(m))) -# else -# define ATTRIBUTE_NONNULL(m) -# endif -# endif - -# else -# ifndef ATTRIBUTE_UNUSED -# define ATTRIBUTE_UNUSED -# endif -# ifndef ATTRIBUTE_FMT_PRINTF -# define ATTRIBUTE_FMT_PRINTF(...) -# endif -# ifndef ATTRIBUTE_RETURN_CHECK -# define ATTRIBUTE_RETURN_CHECK -# endif -# endif /* __GNUC__ */ - -extern int verbose; - -#define VIR_DEBUG0(fmt) if (verbose) fprintf(stderr, "%s:%d :: " fmt "\n", \ - __func__, __LINE__) -#define VIR_DEBUG(fmt, ...) if (verbose) fprintf(stderr, "%s:%d: " fmt "\n", \ - __func__, __LINE__, __VA_ARGS__) -#define VIR_WARN(fmt, ...) if (verbose) fprintf(stderr, "%s:%d: " fmt "\n", \ - __func__, __LINE__, __VA_ARGS__) - -#endif diff --git a/src/libvirtSnmp.c b/src/libvirtSnmp.c index 805cd6f..111c97b 100644 --- a/src/libvirtSnmp.c +++ b/src/libvirtSnmp.c @@ -24,17 +24,12 @@ #include <stdio.h> #include <stdlib.h> -#include <sys/types.h> -#include <sys/poll.h> #include <pthread.h> #include <signal.h> #include "libvirtSnmp.h" #include "libvirtGuestTable.h" /* include our MIB structures*/ #include "libvirtNotifications.h" -#ifdef LIBVIRT_OLD -# include "event_poll.h" -#endif #define DEBUG0(fmt) if (verbose) printf("%s:%d :: " fmt "\n", \ __func__, __LINE__) @@ -302,11 +297,7 @@ void * pollingThreadFunc(void *foo) { while (run) { -#ifdef LIBVIRT_OLD - if (virEventPollRunOnce() < 0) { -#else if (virEventRunDefaultImpl() < 0) { -#endif showError(conn); pthread_exit(NULL); } @@ -368,21 +359,8 @@ int libvirtSnmpInit(void) verbose = verbose_env != NULL; /* if we don't already have registered callback */ - if (callbackRet == -1) { -#ifdef LIBVIRT_OLD - if (virEventPollInit() < 0) - return -1; - - virEventRegisterImpl(virEventPollAddHandle, - virEventPollUpdateHandle, - virEventPollRemoveHandle, - virEventPollAddTimeout, - virEventPollUpdateTimeout, - virEventPollRemoveTimeout); -#else + if (callbackRet == -1) virEventRegisterDefaultImpl(); -#endif - } /* TODO: configure the URI */ /* Use libvirt env variable LIBVIRT_DEFAULT_URI by default*/ diff --git a/src/memory.c b/src/memory.c deleted file mode 100644 index bfa32a8..0000000 --- a/src/memory.c +++ /dev/null @@ -1,313 +0,0 @@ -/* - * memory.c: safer memory allocation - * - * Copyright (C) 2010-2011 Red Hat, Inc. - * Copyright (C) 2008 Daniel P. Berrange - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library; if not, write to the Free Software - * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA - * - */ - -#include <config.h> -#include <stdlib.h> - -#include "memory.h" -#include "ignore-value.h" - - -#if TEST_OOM -static int testMallocNext = 0; -static int testMallocFailFirst = 0; -static int testMallocFailLast = 0; -static void (*testMallocHook)(int, void*) = NULL; -static void *testMallocHookData = NULL; - -void virAllocTestInit(void) -{ - testMallocNext = 1; - testMallocFailFirst = 0; - testMallocFailLast = 0; -} - -int virAllocTestCount(void) -{ - return testMallocNext - 1; -} - -void virAllocTestHook(void (*func)(int, void*), void *data) -{ - testMallocHook = func; - testMallocHookData = data; -} - -void virAllocTestOOM(int n, int m) -{ - testMallocNext = 1; - testMallocFailFirst = n; - testMallocFailLast = n + m - 1; -} - -static int virAllocTestFail(void) -{ - int fail = 0; - if (testMallocNext == 0) - return 0; - - fail = - testMallocNext >= testMallocFailFirst && - testMallocNext <= testMallocFailLast; - - if (fail && testMallocHook) - (testMallocHook)(testMallocNext, testMallocHookData); - - testMallocNext++; - return fail; -} -#endif - - -/** - * virAlloc: - * @ptrptr: pointer to pointer for address of allocated memory - * @size: number of bytes to allocate - * - * Allocate 'size' bytes of memory. Return the address of the - * allocated memory in 'ptrptr'. The newly allocated memory is - * filled with zeros. - * - * Returns -1 on failure to allocate, zero on success - */ -int virAlloc(void *ptrptr, size_t size) -{ -#if TEST_OOM - if (virAllocTestFail()) { - *(void **)ptrptr = NULL; - return -1; - } -#endif - - *(void **)ptrptr = calloc(1, size); - if (*(void **)ptrptr == NULL) - return -1; - return 0; -} - -/** - * virAllocN: - * @ptrptr: pointer to pointer for address of allocated memory - * @size: number of bytes to allocate - * @count: number of elements to allocate - * - * Allocate an array of memory 'count' elements long, - * each with 'size' bytes. Return the address of the - * allocated memory in 'ptrptr'. The newly allocated - * memory is filled with zeros. - * - * Returns -1 on failure to allocate, zero on success - */ -int virAllocN(void *ptrptr, size_t size, size_t count) -{ -#if TEST_OOM - if (virAllocTestFail()) { - *(void **)ptrptr = NULL; - return -1; - } -#endif - - *(void**)ptrptr = calloc(count, size); - if (*(void**)ptrptr == NULL) - return -1; - return 0; -} - -/** - * virReallocN: - * @ptrptr: pointer to pointer for address of allocated memory - * @size: number of bytes to allocate - * @count: number of elements in array - * - * Resize the block of memory in 'ptrptr' to be an array of - * 'count' elements, each 'size' bytes in length. Update 'ptrptr' - * with the address of the newly allocated memory. On failure, - * 'ptrptr' is not changed and still points to the original memory - * block. Any newly allocated memory in 'ptrptr' is uninitialized. - * - * Returns -1 on failure to allocate, zero on success - */ -int virReallocN(void *ptrptr, size_t size, size_t count) -{ - void *tmp; -#if TEST_OOM - if (virAllocTestFail()) - return -1; -#endif - - if (xalloc_oversized(count, size)) { - errno = ENOMEM; - return -1; - } - tmp = realloc(*(void**)ptrptr, size * count); - if (!tmp && (size * count)) - return -1; - *(void**)ptrptr = tmp; - return 0; -} - -/** - * virExpandN: - * @ptrptr: pointer to pointer for address of allocated memory - * @size: number of bytes per element - * @countptr: pointer to number of elements in array - * @add: number of elements to add - * - * Resize the block of memory in 'ptrptr' to be an array of - * '*countptr' + 'add' elements, each 'size' bytes in length. - * Update 'ptrptr' and 'countptr' with the details of the newly - * allocated memory. On failure, 'ptrptr' and 'countptr' are not - * changed. Any newly allocated memory in 'ptrptr' is zero-filled. - * - * Returns -1 on failure to allocate, zero on success - */ -int virExpandN(void *ptrptr, size_t size, size_t *countptr, size_t add) -{ - int ret; - - if (*countptr + add < *countptr) { - errno = ENOMEM; - return -1; - } - ret = virReallocN(ptrptr, size, *countptr + add); - if (ret == 0) { - memset(*(char **)ptrptr + (size * *countptr), 0, size * add); - *countptr += add; - } - return ret; -} - -/** - * virResizeN: - * @ptrptr: pointer to pointer for address of allocated memory - * @size: number of bytes per element - * @allocptr: pointer to number of elements allocated in array - * @count: number of elements currently used in array - * @add: minimum number of additional elements to support in array - * - * If 'count' + 'add' is larger than '*allocptr', then resize the - * block of memory in 'ptrptr' to be an array of at least 'count' + - * 'add' elements, each 'size' bytes in length. Update 'ptrptr' and - * 'allocptr' with the details of the newly allocated memory. On - * failure, 'ptrptr' and 'allocptr' are not changed. Any newly - * allocated memory in 'ptrptr' is zero-filled. - * - * Returns -1 on failure to allocate, zero on success - */ -int virResizeN(void *ptrptr, size_t size, size_t *allocptr, size_t count, - size_t add) -{ - size_t delta; - - if (count + add < count) { - errno = ENOMEM; - return -1; - } - if (count + add <= *allocptr) - return 0; - - delta = count + add - *allocptr; - if (delta < *allocptr / 2) - delta = *allocptr / 2; - return virExpandN(ptrptr, size, allocptr, delta); -} - -/** - * virShrinkN: - * @ptrptr: pointer to pointer for address of allocated memory - * @size: number of bytes per element - * @countptr: pointer to number of elements in array - * @toremove: number of elements to remove - * - * Resize the block of memory in 'ptrptr' to be an array of - * '*countptr' - 'toremove' elements, each 'size' bytes in length. - * Update 'ptrptr' and 'countptr' with the details of the newly - * allocated memory. If 'toremove' is larger than 'countptr', free - * the entire array. - */ -void virShrinkN(void *ptrptr, size_t size, size_t *countptr, size_t toremove) -{ - if (toremove < *countptr) - ignore_value(virReallocN(ptrptr, size, *countptr -= toremove)); - else { - virFree(ptrptr); - *countptr = 0; - } -} - - -/** - * Vir_Alloc_Var: - * @ptrptr: pointer to hold address of allocated memory - * @struct_size: size of initial struct - * @element_size: size of array elements - * @count: number of array elements to allocate - * - * Allocate struct_size bytes plus an array of 'count' elements, each - * of size element_size. This sort of allocation is useful for - * receiving the data of certain ioctls and other APIs which return a - * struct in which the last element is an array of undefined length. - * The caller of this type of API is expected to know the length of - * the array that will be returned and allocate a suitable buffer to - * contain the returned data. C99 refers to these variable length - * objects as structs containing flexible array members. - * - * Returns -1 on failure, 0 on success - */ -int virAllocVar(void *ptrptr, size_t struct_size, size_t element_size, size_t count) -{ - size_t alloc_size = 0; - -#if TEST_OOM - if (virAllocTestFail()) - return -1; -#endif - - if (VIR_ALLOC_VAR_OVERSIZED(struct_size, count, element_size)) { - errno = ENOMEM; - return -1; - } - - alloc_size = struct_size + (element_size * count); - *(void **)ptrptr = calloc(1, alloc_size); - if (*(void **)ptrptr == NULL) - return -1; - return 0; -} - - -/** - * virFree: - * @ptrptr: pointer to pointer for address of memory to be freed - * - * Release the chunk of memory in the pointer pointed to by - * the 'ptrptr' variable. After release, 'ptrptr' will be - * updated to point to NULL. - */ -void virFree(void *ptrptr) -{ - int save_errno = errno; - - free(*(void**)ptrptr); - *(void**)ptrptr = NULL; - errno = save_errno; -} diff --git a/src/memory.h b/src/memory.h deleted file mode 100644 index 66b4c42..0000000 --- a/src/memory.h +++ /dev/null @@ -1,212 +0,0 @@ -/* - * memory.c: safer memory allocation - * - * Copyright (C) 2010 Red Hat, Inc. - * Copyright (C) 2008 Daniel P. Berrange - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library; if not, write to the Free Software - * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA - * - */ - - -#ifndef __VIR_MEMORY_H_ -# define __VIR_MEMORY_H_ - -# include "internal.h" - -/* Return 1 if an array of N objects, each of size S, cannot exist due - to size arithmetic overflow. S must be positive and N must be - nonnegative. This is a macro, not an inline function, so that it - works correctly even when SIZE_MAX < N. - - By gnulib convention, SIZE_MAX represents overflow in size - calculations, so the conservative dividend to use here is - SIZE_MAX - 1, since SIZE_MAX might represent an overflowed value. - However, malloc (SIZE_MAX) fails on all known hosts where - sizeof (ptrdiff_t) <= sizeof (size_t), so do not bother to test for - exactly-SIZE_MAX allocations on such hosts; this avoids a test and - branch when S is known to be 1. */ -# ifndef xalloc_oversized -# define xalloc_oversized(n, s) \ - ((size_t) (sizeof (ptrdiff_t) <= sizeof (size_t) ? -1 : -2) / (s) < (n)) -# endif - - - -/* Don't call these directly - use the macros below */ -int virAlloc(void *ptrptr, size_t size) ATTRIBUTE_RETURN_CHECK - ATTRIBUTE_NONNULL(1); -int virAllocN(void *ptrptr, size_t size, size_t count) ATTRIBUTE_RETURN_CHECK - ATTRIBUTE_NONNULL(1); -int virReallocN(void *ptrptr, size_t size, size_t count) ATTRIBUTE_RETURN_CHECK - ATTRIBUTE_NONNULL(1); -int virExpandN(void *ptrptr, size_t size, size_t *count, size_t add) - ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3); -int virResizeN(void *ptrptr, size_t size, size_t *alloc, size_t count, - size_t desired) - ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3); -void virShrinkN(void *ptrptr, size_t size, size_t *count, size_t toremove) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3); -int virAllocVar(void *ptrptr, - size_t struct_size, - size_t element_size, - size_t count) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1); -void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); - -/** - * VIR_ALLOC: - * @ptr: pointer to hold address of allocated memory - * - * Allocate sizeof(*ptr) bytes of memory and store - * the address of allocated memory in 'ptr'. Fill the - * newly allocated memory with zeros. - * - * Returns -1 on failure, 0 on success - */ -# define VIR_ALLOC(ptr) virAlloc(&(ptr), sizeof(*(ptr))) - -/** - * VIR_ALLOC_N: - * @ptr: pointer to hold address of allocated memory - * @count: number of elements to allocate - * - * Allocate an array of 'count' elements, each sizeof(*ptr) - * bytes long and store the address of allocated memory in - * 'ptr'. Fill the newly allocated memory with zeros. - * - * Returns -1 on failure, 0 on success - */ -# define VIR_ALLOC_N(ptr, count) virAllocN(&(ptr), sizeof(*(ptr)), (count)) - -/** - * VIR_REALLOC_N: - * @ptr: pointer to hold address of allocated memory - * @count: number of elements to allocate - * - * Re-allocate an array of 'count' elements, each sizeof(*ptr) - * bytes long and store the address of allocated memory in - * 'ptr'. If 'ptr' grew, the added memory is uninitialized. - * - * Returns -1 on failure, 0 on success - */ -# define VIR_REALLOC_N(ptr, count) virReallocN(&(ptr), sizeof(*(ptr)), (count)) - -/** - * VIR_EXPAND_N: - * @ptr: pointer to hold address of allocated memory - * @count: variable tracking number of elements currently allocated - * @add: number of elements to add - * - * Re-allocate an array of 'count' elements, each sizeof(*ptr) - * bytes long, to be 'count' + 'add' elements long, then store the - * address of allocated memory in 'ptr' and the new size in 'count'. - * The new elements are filled with zero. - * - * Returns -1 on failure, 0 on success - */ -# define VIR_EXPAND_N(ptr, count, add) \ - virExpandN(&(ptr), sizeof(*(ptr)), &(count), add) - -/** - * VIR_RESIZE_N: - * @ptr: pointer to hold address of allocated memory - * @alloc: variable tracking number of elements currently allocated - * @count: number of elements currently in use - * @add: minimum number of elements to additionally support - * - * Blindly using VIR_EXPAND_N(array, alloc, 1) in a loop scales - * quadratically, because every iteration must copy contents from - * all prior iterations. But amortized linear scaling can be achieved - * by tracking allocation size separately from the number of used - * elements, and growing geometrically only as needed. - * - * If 'count' + 'add' is larger than 'alloc', then geometrically reallocate - * the array of 'alloc' elements, each sizeof(*ptr) bytes long, and store - * the address of allocated memory in 'ptr' and the new size in 'alloc'. - * The new elements are filled with zero. - * - * Returns -1 on failure, 0 on success - */ -# define VIR_RESIZE_N(ptr, alloc, count, add) \ - virResizeN(&(ptr), sizeof(*(ptr)), &(alloc), count, add) - -/** - * VIR_SHRINK_N: - * @ptr: pointer to hold address of allocated memory - * @count: variable tracking number of elements currently allocated - * @remove: number of elements to remove - * - * Re-allocate an array of 'count' elements, each sizeof(*ptr) - * bytes long, to be 'count' - 'remove' elements long, then store the - * address of allocated memory in 'ptr' and the new size in 'count'. - * If 'count' <= 'remove', the entire array is freed. - * - * No return value. - */ -# define VIR_SHRINK_N(ptr, count, remove) \ - virShrinkN(&(ptr), sizeof(*(ptr)), &(count), remove) - -/* - * VIR_ALLOC_VAR_OVERSIZED: - * @M: size of base structure - * @N: number of array elements in trailing array - * @S: size of trailing array elements - * - * Check to make sure that the requested allocation will not cause - * arithmetic overflow in the allocation size. The check is - * essentially the same as that in gnulib's xalloc_oversized. - */ -# define VIR_ALLOC_VAR_OVERSIZED(M, N, S) ((((size_t)-1) - (M)) / (S) < (N)) - -/** - * VIR_ALLOC_VAR: - * @ptr: pointer to hold address of allocated memory - * @type: element type of trailing array - * @count: number of array elements to allocate - * - * Allocate sizeof(*ptr) bytes plus an array of 'count' elements, each - * sizeof('type'). This sort of allocation is useful for receiving - * the data of certain ioctls and other APIs which return a struct in - * which the last element is an array of undefined length. The caller - * of this type of API is expected to know the length of the array - * that will be returned and allocate a suitable buffer to contain the - * returned data. C99 refers to these variable length objects as - * structs containing flexible array members. - - * Returns -1 on failure, 0 on success - */ -# define VIR_ALLOC_VAR(ptr, type, count) \ - virAllocVar(&(ptr), sizeof(*(ptr)), sizeof(type), (count)) - -/** - * VIR_FREE: - * @ptr: pointer holding address to be freed - * - * Free the memory stored in 'ptr' and update to point - * to NULL. - */ -# define VIR_FREE(ptr) virFree(&(ptr)) - - -# if TEST_OOM -void virAllocTestInit(void); -int virAllocTestCount(void); -void virAllocTestOOM(int n, int m); -void virAllocTestHook(void (*func)(int, void*), void *data); -# endif - - - -#endif /* __VIR_MEMORY_H_ */ diff --git a/src/threads.c b/src/threads.c deleted file mode 100644 index 8987173..0000000 --- a/src/threads.c +++ /dev/null @@ -1,251 +0,0 @@ -/* - * threads.c: basic thread synchronization primitives - * - * Copyright (C) 2009-2010 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 - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library; if not, write to the Free Software - * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA - * - */ - -#include <config.h> - -#include <unistd.h> -#include <inttypes.h> -#if HAVE_SYS_SYSCALL_H -# include <sys/syscall.h> -#endif - -#include "threads.h" -#include "memory.h" - - -/* Nothing special required for pthreads */ -int virThreadInitialize(void) -{ - return 0; -} - -void virThreadOnExit(void) -{ -} - - -int virMutexInit(virMutexPtr m) -{ - int ret; - pthread_mutexattr_t attr; - pthread_mutexattr_init(&attr); - pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_NORMAL); - ret = pthread_mutex_init(&m->lock, &attr); - pthread_mutexattr_destroy(&attr); - if (ret != 0) { - errno = ret; - return -1; - } - return 0; -} - -int virMutexInitRecursive(virMutexPtr m) -{ - int ret; - pthread_mutexattr_t attr; - pthread_mutexattr_init(&attr); - pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); - ret = pthread_mutex_init(&m->lock, &attr); - pthread_mutexattr_destroy(&attr); - if (ret != 0) { - errno = ret; - return -1; - } - return 0; -} - -void virMutexDestroy(virMutexPtr m) -{ - pthread_mutex_destroy(&m->lock); -} - -void virMutexLock(virMutexPtr m){ - pthread_mutex_lock(&m->lock); -} - -void virMutexUnlock(virMutexPtr m) -{ - pthread_mutex_unlock(&m->lock); -} - - -int virCondInit(virCondPtr c) -{ - int ret; - if ((ret = pthread_cond_init(&c->cond, NULL)) != 0) { - errno = ret; - return -1; - } - return 0; -} - -int virCondDestroy(virCondPtr c) -{ - int ret; - if ((ret = pthread_cond_destroy(&c->cond)) != 0) { - errno = ret; - return -1; - } - return 0; -} - -int virCondWait(virCondPtr c, virMutexPtr m) -{ - int ret; - if ((ret = pthread_cond_wait(&c->cond, &m->lock)) != 0) { - errno = ret; - return -1; - } - return 0; -} - -int virCondWaitUntil(virCondPtr c, virMutexPtr m, unsigned long long whenms) -{ - int ret; - struct timespec ts; - - ts.tv_sec = whenms / 1000; - ts.tv_nsec = (whenms % 1000) * 1000; - - if ((ret = pthread_cond_timedwait(&c->cond, &m->lock, &ts)) != 0) { - errno = ret; - return -1; - } - return 0; -} - -void virCondSignal(virCondPtr c) -{ - pthread_cond_signal(&c->cond); -} - -void virCondBroadcast(virCondPtr c) -{ - pthread_cond_broadcast(&c->cond); -} - -struct virThreadArgs { - virThreadFunc func; - void *opaque; -}; - -static void *virThreadHelper(void *data) -{ - struct virThreadArgs *args = data; - args->func(args->opaque); - VIR_FREE(args); - return NULL; -} - -int virThreadCreate(virThreadPtr thread, - bool joinable, - virThreadFunc func, - void *opaque) -{ - struct virThreadArgs *args; - pthread_attr_t attr; - int ret = -1; - int err; - - if ((err = pthread_attr_init(&attr)) != 0) - goto cleanup; - if (VIR_ALLOC(args) < 0) { - err = ENOMEM; - goto cleanup; - } - - args->func = func; - args->opaque = opaque; - - if (!joinable) - pthread_attr_setdetachstate(&attr, 1); - - err = pthread_create(&thread->thread, &attr, virThreadHelper, args); - if (err != 0) { - VIR_FREE(args); - goto cleanup; - } - /* New thread owns 'args' in success case, so don't free */ - - ret = 0; -cleanup: - pthread_attr_destroy(&attr); - if (ret < 0) - errno = err; - return ret; -} - -void virThreadSelf(virThreadPtr thread) -{ - thread->thread = pthread_self(); -} - -bool virThreadIsSelf(virThreadPtr thread) -{ - return pthread_equal(pthread_self(), thread->thread) ? true : false; -} - -/* For debugging use only; this result is not guaranteed unique on BSD - * systems when pthread_t is a 64-bit pointer. */ -int virThreadSelfID(void) -{ -#if defined(HAVE_SYS_SYSCALL_H) && defined(SYS_gettid) - pid_t tid; - tid = syscall(SYS_gettid); - return (int)tid; -#else - return (int)pthread_self(); -#endif -} - -/* For debugging use only; this result is not guaranteed unique on BSD - * systems when pthread_t is a 64-bit pointer, nor does it match the - * thread id of virThreadSelfID on Linux. */ -int virThreadID(virThreadPtr thread) -{ - return (int)(uintptr_t)thread->thread; -} - -void virThreadJoin(virThreadPtr thread) -{ - pthread_join(thread->thread, NULL); -} - -int virThreadLocalInit(virThreadLocalPtr l, - virThreadLocalCleanup c) -{ - int ret; - if ((ret = pthread_key_create(&l->key, c)) != 0) { - errno = ret; - return -1; - } - return 0; -} - -void *virThreadLocalGet(virThreadLocalPtr l) -{ - return pthread_getspecific(l->key); -} - -void virThreadLocalSet(virThreadLocalPtr l, void *val) -{ - pthread_setspecific(l->key, val); -} diff --git a/src/threads.h b/src/threads.h deleted file mode 100644 index ae81273..0000000 --- a/src/threads.h +++ /dev/null @@ -1,101 +0,0 @@ -/* - * threads.h: basic thread synchronization primitives - * - * Copyright (C) 2009-2010 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 - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library; if not, write to the Free Software - * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA - * - */ - -#ifndef __THREADS_H_ -# define __THREADS_H_ - -# include "internal.h" -# include <pthread.h> - -typedef struct virMutex virMutex; -typedef virMutex *virMutexPtr; - -typedef struct virCond virCond; -typedef virCond *virCondPtr; - -typedef struct virThreadLocal virThreadLocal; -typedef virThreadLocal *virThreadLocalPtr; - -typedef struct virThread virThread; -typedef virThread *virThreadPtr; - - -int virThreadInitialize(void) ATTRIBUTE_RETURN_CHECK; -void virThreadOnExit(void); - -typedef void (*virThreadFunc)(void *opaque); - -int virThreadCreate(virThreadPtr thread, - bool joinable, - virThreadFunc func, - void *opaque) ATTRIBUTE_RETURN_CHECK; -void virThreadSelf(virThreadPtr thread); -bool virThreadIsSelf(virThreadPtr thread); -void virThreadJoin(virThreadPtr thread); - -/* These next two functions are for debugging only, since they are not - * guaranteed to give unique values for distinct threads on all - * architectures, nor are the two functions guaranteed to give the same - * value for the same thread. */ -int virThreadSelfID(void); -int virThreadID(virThreadPtr thread); - -int virMutexInit(virMutexPtr m) ATTRIBUTE_RETURN_CHECK; -int virMutexInitRecursive(virMutexPtr m) ATTRIBUTE_RETURN_CHECK; -void virMutexDestroy(virMutexPtr m); - -void virMutexLock(virMutexPtr m); -void virMutexUnlock(virMutexPtr m); - - - -int virCondInit(virCondPtr c) ATTRIBUTE_RETURN_CHECK; -int virCondDestroy(virCondPtr c) ATTRIBUTE_RETURN_CHECK; - -int virCondWait(virCondPtr c, virMutexPtr m) ATTRIBUTE_RETURN_CHECK; -int virCondWaitUntil(virCondPtr c, virMutexPtr m, unsigned long long whenms) ATTRIBUTE_RETURN_CHECK; -void virCondSignal(virCondPtr c); -void virCondBroadcast(virCondPtr c); - - -typedef void (*virThreadLocalCleanup)(void *); -int virThreadLocalInit(virThreadLocalPtr l, - virThreadLocalCleanup c) ATTRIBUTE_RETURN_CHECK; -void *virThreadLocalGet(virThreadLocalPtr l); -void virThreadLocalSet(virThreadLocalPtr l, void*); - -struct virMutex { - pthread_mutex_t lock; -}; - -struct virCond { - pthread_cond_t cond; -}; - -struct virThread { - pthread_t thread; -}; - -struct virThreadLocal { - pthread_key_t key; -}; - -#endif diff --git a/src/util.c b/src/util.c deleted file mode 100644 index 30d4e47..0000000 --- a/src/util.c +++ /dev/null @@ -1,105 +0,0 @@ -/* - * utils.c: common, generic utility functions - * - * Copyright (C) 2006-2011 Red Hat, Inc. - * Copyright (C) 2006 Daniel P. Berrange - * Copyright (C) 2006, 2007 Binary Karma - * Copyright (C) 2006 Shuveb Hussain - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library; if not, write to the Free Software - * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA - * - * Author: Daniel P. Berrange <berrange@redhat.com> - * File created Jul 18, 2007 - Shuveb Hussain <shuveb@binarykarma.com> - */ - -#include "util.h" -#include <unistd.h> -#include <fcntl.h> - -/* Like read(), but restarts after EINTR */ -ssize_t -saferead(int fd, void *buf, size_t count) -{ - size_t nread = 0; - while (count > 0) { - ssize_t r = read(fd, buf, count); - if (r < 0 && errno == EINTR) - continue; - if (r < 0) - return r; - if (r == 0) - return nread; - buf = (char *)buf + r; - count -= r; - nread += r; - } - return nread; -} - -/* Like write(), but restarts after EINTR */ -ssize_t -safewrite(int fd, const void *buf, size_t count) -{ - size_t nwritten = 0; - while (count > 0) { - ssize_t r = write(fd, buf, count); - - if (r < 0 && errno == EINTR) - continue; - if (r < 0) - return r; - if (r == 0) - return nwritten; - buf = (const char *)buf + r; - count -= r; - nwritten += r; - } - return nwritten; -} - -int virSetBlocking(int fd, bool blocking) { - int flags; - if ((flags = fcntl(fd, F_GETFL)) < 0) - return -1; - if (blocking) - flags &= ~O_NONBLOCK; - else - flags |= O_NONBLOCK; - if ((fcntl(fd, F_SETFL, flags)) < 0) - return -1; - return 0; -} - -int virSetNonBlock(int fd) { - return virSetBlocking(fd, false); -} - -int virSetCloseExec(int fd) -{ - return virSetInherit(fd, false); -} - -int virSetInherit(int fd, bool inherit) { - int flags; - if ((flags = fcntl(fd, F_GETFD)) < 0) - return -1; - if (inherit) - flags &= ~FD_CLOEXEC; - else - flags |= FD_CLOEXEC; - if ((fcntl(fd, F_SETFD, flags)) < 0) - return -1; - return 0; -} diff --git a/src/util.h b/src/util.h deleted file mode 100644 index a3d289e..0000000 --- a/src/util.h +++ /dev/null @@ -1,38 +0,0 @@ -/* - * utils.h: common, generic utility functions - * - * Copyright (C) 2010-2011 Red Hat, Inc. - * Copyright (C) 2006, 2007 Binary Karma - * Copyright (C) 2006 Shuveb Hussain - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library; if not, write to the Free Software - * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA - * - * File created Jul 18, 2007 - Shuveb Hussain <shuveb@binarykarma.com> - */ - -#ifndef __VIR_UTIL_H__ -#define __VIR_UTIL_H__ - -#include "internal.h" - -int virSetBlocking(int fd, bool blocking) ATTRIBUTE_RETURN_CHECK; -int virSetNonBlock(int fd) ATTRIBUTE_RETURN_CHECK; -int virSetInherit(int fd, bool inherit) ATTRIBUTE_RETURN_CHECK; -int virSetCloseExec(int fd) ATTRIBUTE_RETURN_CHECK; - -ssize_t saferead(int fd, void *buf, size_t count) ATTRIBUTE_RETURN_CHECK; -ssize_t safewrite(int fd, const void *buf, size_t count) ATTRIBUTE_RETURN_CHECK; - -#endif -- 2.18.1

We are using PKG_CHECK_MODULES() which sets LIBVIRT_LIBS and LIBVIRT_CFLAGS. There is no need to modify the former and put "-lvirt" into it. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- configure.ac | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index 7834c34..1b4112e 100644 --- a/configure.ac +++ b/configure.ac @@ -26,8 +26,8 @@ dnl do we have libvirt installed? LIBVIRT_REQUIRED=0.9.0 PKG_CHECK_MODULES(LIBVIRT, libvirt >= $LIBVIRT_REQUIRED) -LIBVIRT_LIBS="$LIBVIRT_LIBS -lvirt" AC_SUBST(LIBVIRT_LIBS) +AC_SUBST(LIBVIRT_CFLAGS) SNMP_CONFIG="net-snmp-config" SNMP_CFLAGS="" -- 2.18.1

So far, the function calls virConnCopyLastError() which is suboptimal since we have virGetLastErrorMessage(). Switch to that. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- configure.ac | 2 +- src/libvirtSnmp.c | 31 +++---------------------------- 2 files changed, 4 insertions(+), 29 deletions(-) diff --git a/configure.ac b/configure.ac index 1b4112e..baac0e1 100644 --- a/configure.ac +++ b/configure.ac @@ -23,7 +23,7 @@ dnl dnl do we have libvirt installed? -LIBVIRT_REQUIRED=0.9.0 +LIBVIRT_REQUIRED=1.0.6 PKG_CHECK_MODULES(LIBVIRT, libvirt >= $LIBVIRT_REQUIRED) AC_SUBST(LIBVIRT_LIBS) diff --git a/src/libvirtSnmp.c b/src/libvirtSnmp.c index 111c97b..22cf8f0 100644 --- a/src/libvirtSnmp.c +++ b/src/libvirtSnmp.c @@ -76,36 +76,11 @@ stop(int sig) static void showError(virConnectPtr conn) { - int ret; - virErrorPtr err; + const char *err = virGetLastErrorMessage(); - err = malloc(sizeof(*err)); - if (NULL == err) { - printf("Could not allocate memory for error data\n"); - goto out; - } + snmp_log(LOG_ERR, "libvirt reported: \"%s\"\n", err); - ret = virConnCopyLastError(conn, err); - - switch (ret) { - case 0: - snmp_log(LOG_ERR, "No error found\n"); - break; - - case -1: - snmp_log(LOG_ERR, "Parameter error when attempting to get last error\n"); - break; - - default: - snmp_log(LOG_ERR, "libvirt reported: \"%s\"\n", err->message); - break; - } - - virResetError(err); - free(err); - - out: - return; + virResetLastError(); } -- 2.18.1

Since libvirt-0.9.13 we have this atomic API for listing domains. Use that instead of the old ones. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirtSnmp.c | 107 +++++++--------------------------------------- 1 file changed, 16 insertions(+), 91 deletions(-) diff --git a/src/libvirtSnmp.c b/src/libvirtSnmp.c index 22cf8f0..589fd03 100644 --- a/src/libvirtSnmp.c +++ b/src/libvirtSnmp.c @@ -165,105 +165,30 @@ insertGuest(netsnmp_container *container, virDomainPtr domain) int libvirtSnmpLoadGuests(netsnmp_container *container) { - int ret = 0, i, numIds, numActiveDomains; - int numNames, numDefinedDomains; - int *idList = NULL; - char **nameList = NULL; - virDomainPtr domain = NULL; + int ret = -1; + int i; + int ndomains; + virDomainPtr *domains = NULL; - numActiveDomains = virConnectNumOfDomains(conn); - if (-1 == numActiveDomains) { - ret = -1; - printf("Failed to get number of active domains\n"); + if ((ndomains = virConnectListAllDomains(conn, &domains, 0)) < 0) { + printf("Failed to list all domains\n"); showError(conn); - goto out; + goto cleanup; } - idList = malloc(sizeof(*idList) * numActiveDomains); - if (NULL == idList) { - ret = -1; - printf("Could not allocate memory for list of active domains\n"); - goto out; + for (i = 0; i < ndomains; i++) { + if (insertGuest(container, domains[i]) < 0) + goto cleanup; } - numIds = virConnectListDomains(conn, - idList, - numActiveDomains); - - if (-1 == numIds) { - ret = -1; - printf("Could not get list of defined domains from hypervisor\n"); - showError(conn); - goto out; + ret = 0; + cleanup: + if (domains) { + for (i = 0; i < ndomains; i++) + virDomainFree(domains[i]); + free(domains); } - - for (i = 0 ; i < numIds ; i++) { - domain = virDomainLookupByID(conn, *(idList + i)); - if (NULL == domain) { - printf("Failed to lookup domain\n"); - showError(conn); - ret = -1; - goto out; - } - - ret = insertGuest(container, domain); - - virDomainFree(domain); - - if (-1 == ret) - goto out; - } - - /* Inactive domains */ - numDefinedDomains = virConnectNumOfDefinedDomains(conn); - if (-1 == numDefinedDomains) { - ret = -1; - printf("Failed to get number of defined domains\n"); - showError(conn); - goto out; - } - - nameList = malloc(sizeof(*nameList) * numDefinedDomains); - - if (NULL == nameList) { - ret = -1; - printf("Could not allocate memory for list of defined domains\n"); - goto out_inact; - } - - numNames = virConnectListDefinedDomains(conn, - nameList, - numDefinedDomains); - - if (-1 == numNames) { - ret = -1; - printf("Could not get list of defined domains from hypervisor\n"); - showError(conn); - goto out_inact; - } - - for (i = 0 ; i < numNames ; i++) { - domain = virDomainLookupByName(conn, *(nameList + i)); - if (NULL == domain) { - printf("Failed to lookup domain\n"); - showError(conn); - ret = -1; - goto out_inact; - } - - ret = insertGuest(container, domain); - - virDomainFree(domain); - - if (-1 == ret) - goto out_inact; - } - - out_inact: - free(nameList); - out: - free(idList); return ret; } -- 2.18.1

Firstly, this style of comparison makes my eyes bleed: if (-1 == some_int) Secondly, the retval is initialized to zero and then in every error path it is set to -1. Le sigh. Thirdly, virDomainGetName() can't fail at the point we are calling it (it can fail iff passed virDomainPtr is invalid, but that can't be the case). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirtSnmp.c | 68 +++++++++++++++++++++-------------------------- 1 file changed, 31 insertions(+), 37 deletions(-) diff --git a/src/libvirtSnmp.c b/src/libvirtSnmp.c index 589fd03..1e7e345 100644 --- a/src/libvirtSnmp.c +++ b/src/libvirtSnmp.c @@ -87,54 +87,47 @@ showError(virConnectPtr conn) static int insertGuest(netsnmp_container *container, virDomainPtr domain) { - int ret = 0; virDomainInfo info; libvirtGuestTable_rowreq_ctx *row_ctx = NULL; - const char *name = NULL; - unsigned char uuid[16]; + const char *name; + unsigned char uuid[VIR_UUID_BUFLEN]; - if (-1 == virDomainGetInfo(domain, &info)) { + if (virDomainGetInfo(domain, &info) < 0) { printf("Failed to get domain info\n"); showError(conn); - ret = -1; - goto out; + goto error; } /* create new row in the container */ - row_ctx = libvirtGuestTable_allocate_rowreq_ctx(NULL); - if (!row_ctx) { + if (!(row_ctx = libvirtGuestTable_allocate_rowreq_ctx(NULL))) { snmp_log(LOG_ERR, "Error creating row"); - ret = -1; - goto out; + goto error; } /* set the index of the row */ - ret = virDomainGetUUID(domain, uuid); - if (ret) { - snmp_log(LOG_ERR, "Cannot get UUID"); - libvirtGuestTable_release_rowreq_ctx(row_ctx); - ret = -1; - goto out; + if (virDomainGetUUID(domain, uuid) < 0) { + printf("Failed to get domain UUID\n"); + showError(conn); + goto error; } - if (MFD_SUCCESS != libvirtGuestTable_indexes_set(row_ctx, (char*) uuid, - sizeof(uuid))) { + + if (libvirtGuestTable_indexes_set(row_ctx, + (char*) uuid, + sizeof(uuid)) != MFD_SUCCESS) { snmp_log(LOG_ERR, "Error setting row index"); - libvirtGuestTable_release_rowreq_ctx(row_ctx); - ret = -1; - goto out; + goto error; } /* set the data */ - name = virDomainGetName(domain); - if (name) - row_ctx->data.libvirtGuestName = strdup(name); - else - row_ctx->data.libvirtGuestName = strdup(""); - if (!row_ctx->data.libvirtGuestName) { + if (!(name = virDomainGetName(domain))) { + printf("Failed to get domain name\n"); + showError(conn); + goto error; + } + + if (!(row_ctx->data.libvirtGuestName = strdup(name))) { snmp_log(LOG_ERR, "Not enough memory for domain name '%s'", name); - libvirtGuestTable_release_rowreq_ctx(row_ctx); - ret = -1; - goto out; + goto error; } row_ctx->data.libvirtGuestState = info.state; @@ -147,16 +140,17 @@ insertGuest(netsnmp_container *container, virDomainPtr domain) row_ctx->data.libvirtGuestRowStatus = ROWSTATUS_ACTIVE; - ret = CONTAINER_INSERT(container, row_ctx); - if (ret) { + if (CONTAINER_INSERT(container, row_ctx) < 0) { snmp_log(LOG_ERR, "Cannot insert domain '%s' to container", name); - libvirtGuestTable_release_rowreq_ctx(row_ctx); - ret = -1; - goto out; + goto error; } - out: - return ret; + return 0; + + error: + if (row_ctx) + libvirtGuestTable_release_rowreq_ctx(row_ctx); + return -1; } /* -- 2.18.1

Some function initialize retval to 0 and then for every error path they reset it to -1. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirtSnmp.c | 50 +++++++++++++++++++++++------------------------ 1 file changed, 24 insertions(+), 26 deletions(-) diff --git a/src/libvirtSnmp.c b/src/libvirtSnmp.c index 1e7e345..4ad6fab 100644 --- a/src/libvirtSnmp.c +++ b/src/libvirtSnmp.c @@ -258,9 +258,7 @@ int libvirtSnmpInit(void) /* TODO: configure the URI */ /* Use libvirt env variable LIBVIRT_DEFAULT_URI by default*/ - conn = virConnectOpenAuth(NULL, virConnectAuthPtrDefault, 0); - - if (NULL == conn) { + if (!(conn = virConnectOpenAuth(NULL, virConnectAuthPtrDefault, 0))) { printf("No connection to hypervisor\n"); showError(conn); return -1; @@ -283,6 +281,7 @@ libvirtDeinitTimer(int timer ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) void libvirtSnmpShutdown(void) { int timer; + int rc; /* in case server is being stopped, run is still 1, so let's * shutdown the thread in a clean way */ @@ -299,8 +298,9 @@ void libvirtSnmpShutdown(void) printf("Failed to unregister domain events\n"); } - if (0 != virConnectClose(conn)) { - printf("Failed to disconnect from hypervisor\n"); + if ((rc = virConnectClose(conn))) { + printf("Failed to disconnect from hypervisor. " + "Leaked references: %d\n", rc); showError(conn); } } @@ -320,18 +320,11 @@ int libvirtSnmpCreate(unsigned char *uuid, int state) { virDomainPtr dom; - int ret; + int ret = -1; unsigned int flags = 0; - dom = virDomainLookupByUUID(conn, uuid); - if (dom == NULL) { - printf("Cannot find domain to create\n"); - return -1; - } - switch(state) { case VIR_DOMAIN_RUNNING: - flags = 0; break; case VIR_DOMAIN_PAUSED: flags = VIR_DOMAIN_START_PAUSED; @@ -341,10 +334,18 @@ libvirtSnmpCreate(unsigned char *uuid, int state) return -1; } - ret = virDomainCreateWithFlags(dom, flags); - if (ret) { - showError(conn); + if (!(dom = virDomainLookupByUUID(conn, uuid))) { + printf("Cannot find domain to create\n"); + return -1; } + + if (virDomainCreateWithFlags(dom, flags) < 0) { + showError(conn); + goto cleanup; + } + + ret = 0; + cleanup: virDomainFree(dom); return ret; } @@ -353,27 +354,26 @@ int libvirtSnmpDestroy(unsigned char *uuid) { virDomainPtr dom; - int ret; - dom = virDomainLookupByUUID(conn, uuid); - if (dom == NULL) { + if (!(dom = virDomainLookupByUUID(conn, uuid))) { printf("Cannot find domain to destroy\n"); return -1; } - ret = virDomainDestroy(dom); - if (ret) { + if (virDomainDestroy(dom) < 0) { showError(conn); + return -1; } + virDomainFree(dom); - return ret; + return 0; } int libvirtSnmpChangeState(unsigned char *uuid, int newstate, int oldstate) { virDomainPtr dom; - int ret = 0; + int ret = -1; dom = virDomainLookupByUUID(conn, uuid); if (dom == NULL) { @@ -389,13 +389,11 @@ libvirtSnmpChangeState(unsigned char *uuid, int newstate, int oldstate) ret = virDomainShutdown(dom); else { printf("Wrong state transition from %d to %d\n", oldstate, newstate); - ret = -1; goto out; } - if (ret != 0) { + if (ret < 0) showError(conn); - } out: virDomainFree(dom); return ret; -- 2.18.1

To avoid multiple includes, header files start with: #ifndef __SOMETHING__ # define __SOMETHING__ Well, SOMETHING should be the file name, and there should be a space after hash and before 'define'. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirtNotifications.h | 6 +++--- src/libvirtSnmp.h | 6 +++--- src/libvirtSnmpError.h | 6 +++--- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/libvirtNotifications.h b/src/libvirtNotifications.h index 0be8fdc..ee7a2e3 100644 --- a/src/libvirtNotifications.h +++ b/src/libvirtNotifications.h @@ -20,8 +20,8 @@ * Author: Michal Privoznik <mprivozn@redhat.com> */ -#ifndef LIBVIRTNOTIFICATIONS_H -#define LIBVIRTNOTIFICATIONS_H +#ifndef __LIBVIRT_NOTIFICATIONS_H__ +# define __LIBVIRT_NOTIFICATIONS_H__ #include "libvirtSnmp.h" @@ -30,4 +30,4 @@ */ int send_libvirtGuestNotif_trap(virDomainPtr dom); -#endif /* LIBVIRTNOTIFICATIONS_H */ +#endif /* __LIBVIRT_NOTIFICATIONS_H__ */ diff --git a/src/libvirtSnmp.h b/src/libvirtSnmp.h index bac9988..2c5b0e5 100644 --- a/src/libvirtSnmp.h +++ b/src/libvirtSnmp.h @@ -20,8 +20,8 @@ * Author: Michal Privoznik <mprivozn@redhat.com> */ -#ifndef __VIR_SNMP_H__ -#define __VIR_SNMP_H__ +#ifndef __LIBVIRT_SNMP_H__ +# define __LIBVIRT_SNMP_H__ /* standard libvirt includes */ #include <libvirt/libvirt.h> @@ -60,5 +60,5 @@ libvirtSnmpDestroy(unsigned char *uuid); extern int libvirtSnmpChangeState(unsigned char *uuid, int newstate, int oldstate); -#endif /* __VIR_SNMP_H__ */ +#endif /* __LIBVIRT_SNMP_H__ */ diff --git a/src/libvirtSnmpError.h b/src/libvirtSnmpError.h index 518cc03..6d59478 100644 --- a/src/libvirtSnmpError.h +++ b/src/libvirtSnmpError.h @@ -20,12 +20,12 @@ * Author: Michal Privoznik <mprivozn@redhat.com> */ -#ifndef __VIR_SNMP_ERROR_H__ -#define __VIR_SNMP_ERROR_H__ +#ifndef __LIBVIRT_SNMP_ERROR_H__ +# define __LIBVIRT_SNMP_ERROR_H__ #include <libvirt/libvirt.h> #include <libvirt/virterror.h> extern void printLibvirtError(const char *msg); -#endif +#endif /* __LIBVIRT_SNMP_ERROR_H__ */ -- 2.18.1

On Thursday, 18 October 2018 14:26:47 CET Michal Privoznik wrote:
To avoid multiple includes, header files start with:
#ifndef __SOMETHING__ # define __SOMETHING__
Well, SOMETHING should be the file name, and there should be a space after hash and before 'define'.
Note that identifier start that with two underscores (or even with two underscores in their name, IIRC) are reserved for the system. I know the likelyhood of a name conflict with a system identifier with LIBVIRT in the name is very low, still I saw people reporting these "reserved names used" issues in the past. I'd simply use LIBVIRTSNMP_ (or something like that) as prefix. -- Pino Toscano

A header file should include another header file if and only if an there exists a symbol that requires the inclusion. For instance, if a header file declares the following function: int function(virDomainPtr dom); then it is okay to have the header file include libvirt.h. Unfortunately, some of our header files have needless includes. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirtNotifications.h | 2 +- src/libvirtSnmp.c | 2 ++ src/libvirtSnmp.h | 10 +++------- src/libvirtSnmpError.c | 7 ++++++- src/libvirtSnmpError.h | 3 --- 5 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/libvirtNotifications.h b/src/libvirtNotifications.h index ee7a2e3..6c532b0 100644 --- a/src/libvirtNotifications.h +++ b/src/libvirtNotifications.h @@ -23,7 +23,7 @@ #ifndef __LIBVIRT_NOTIFICATIONS_H__ # define __LIBVIRT_NOTIFICATIONS_H__ -#include "libvirtSnmp.h" +# include <libvirt/libvirt.h> /* * function declarations diff --git a/src/libvirtSnmp.c b/src/libvirtSnmp.c index 4ad6fab..3b5c17f 100644 --- a/src/libvirtSnmp.c +++ b/src/libvirtSnmp.c @@ -26,6 +26,8 @@ #include <stdlib.h> #include <pthread.h> #include <signal.h> +#include <libvirt/libvirt.h> +#include <libvirt/virterror.h> #include "libvirtSnmp.h" #include "libvirtGuestTable.h" /* include our MIB structures*/ diff --git a/src/libvirtSnmp.h b/src/libvirtSnmp.h index 2c5b0e5..24707bd 100644 --- a/src/libvirtSnmp.h +++ b/src/libvirtSnmp.h @@ -23,14 +23,10 @@ #ifndef __LIBVIRT_SNMP_H__ # define __LIBVIRT_SNMP_H__ -/* standard libvirt includes */ -#include <libvirt/libvirt.h> -#include <libvirt/virterror.h> - /* standard Net-SNMP includes */ -#include <net-snmp/net-snmp-config.h> -#include <net-snmp/net-snmp-includes.h> -#include <net-snmp/agent/net-snmp-agent-includes.h> +# include <net-snmp/net-snmp-config.h> +# include <net-snmp/net-snmp-includes.h> +# include <net-snmp/agent/net-snmp-agent-includes.h> /* * Populate libvirtGuestTable into given container. diff --git a/src/libvirtSnmpError.c b/src/libvirtSnmpError.c index f5d546d..1678bcb 100644 --- a/src/libvirtSnmpError.c +++ b/src/libvirtSnmpError.c @@ -20,8 +20,13 @@ * Author: Michal Privoznik <mprivozn@redhat.com> */ -#include "libvirtSnmpError.h" +#include <config.h> + #include <stdio.h> +#include <libvirt/libvirt.h> +#include <libvirt/virterror.h> + +#include "libvirtSnmpError.h" /** * Print libvirt error diff --git a/src/libvirtSnmpError.h b/src/libvirtSnmpError.h index 6d59478..e28ad7c 100644 --- a/src/libvirtSnmpError.h +++ b/src/libvirtSnmpError.h @@ -23,9 +23,6 @@ #ifndef __LIBVIRT_SNMP_ERROR_H__ # define __LIBVIRT_SNMP_ERROR_H__ -#include <libvirt/libvirt.h> -#include <libvirt/virterror.h> - extern void printLibvirtError(const char *msg); #endif /* __LIBVIRT_SNMP_ERROR_H__ */ -- 2.18.1

The threads are JOINABLE by default. No need to go through pthread_attr_* circus to set what is default anyway. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirtSnmp.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/libvirtSnmp.c b/src/libvirtSnmp.c index 3b5c17f..88f2ec6 100644 --- a/src/libvirtSnmp.c +++ b/src/libvirtSnmp.c @@ -205,7 +205,7 @@ pollingThreadFunc(void *foo) int libvirtRegisterEvents(virConnectPtr conn) { struct sigaction action_stop; - pthread_attr_t thread_attr; + int ret = -1; memset(&action_stop, 0, sizeof action_stop); @@ -226,15 +226,12 @@ libvirtRegisterEvents(virConnectPtr conn) { return -1; /* we need a thread to poll for events */ - pthread_attr_init(&thread_attr); - pthread_attr_setdetachstate(&thread_attr, PTHREAD_CREATE_JOINABLE); + if (pthread_create(&poll_thread, NULL, pollingThreadFunc, NULL)) + goto cleanup; - if (pthread_create(&poll_thread, &thread_attr, pollingThreadFunc, NULL)) - return -1; - - pthread_attr_destroy(&thread_attr); - - return 0; + ret = 0; + cleanup: + return ret; } /* Unregister domain events collection */ -- 2.18.1

On Thu, Oct 18, 2018 at 02:26:49PM +0200, Michal Privoznik wrote:
The threads are JOINABLE by default. No need to go through pthread_attr_* circus to set what is default anyway.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirtSnmp.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/src/libvirtSnmp.c b/src/libvirtSnmp.c index 3b5c17f..88f2ec6 100644 --- a/src/libvirtSnmp.c +++ b/src/libvirtSnmp.c @@ -205,7 +205,7 @@ pollingThreadFunc(void *foo) int libvirtRegisterEvents(virConnectPtr conn) { struct sigaction action_stop; - pthread_attr_t thread_attr; + int ret = -1;
memset(&action_stop, 0, sizeof action_stop);
@@ -226,15 +226,12 @@ libvirtRegisterEvents(virConnectPtr conn) { return -1;
/* we need a thread to poll for events */ - pthread_attr_init(&thread_attr); - pthread_attr_setdetachstate(&thread_attr, PTHREAD_CREATE_JOINABLE); + if (pthread_create(&poll_thread, NULL, pollingThreadFunc, NULL)) + goto cleanup;
- if (pthread_create(&poll_thread, &thread_attr, pollingThreadFunc, NULL)) - return -1; - - pthread_attr_destroy(&thread_attr); - - return 0; + ret = 0; + cleanup:
Not really a need for cleanup here, but I'm guessing you just want to have everything follow the same pattern.
+ return ret; }
/* Unregister domain events collection */ -- 2.18.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

Currently, the following pattern is used with showError: printf("Some error message\n"); showError(conn); Not only showError does not need connection pointer, those two lines can be merged into a single one: showError("Some error message"); Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirtSnmp.c | 79 ++++++++++++++++++++++++++++++++--------------- 1 file changed, 54 insertions(+), 25 deletions(-) diff --git a/src/libvirtSnmp.c b/src/libvirtSnmp.c index 88f2ec6..7aa83a1 100644 --- a/src/libvirtSnmp.c +++ b/src/libvirtSnmp.c @@ -43,6 +43,17 @@ #define ATTRIBUTE_UNUSED __attribute__((__unused__)) #endif +# ifndef ATTRIBUTE_FMT_PRINTF +# ifndef __clang__ +# define ATTRIBUTE_FMT_PRINTF(fmtpos, argpos) \ + __attribute__((__format__ (__gnu_printf__, fmtpos, argpos))) +# else +# define ATTRIBUTE_FMT_PRINTF(fmtpos, argpos) \ + __attribute__((__format__ (__printf__, fmtpos, argpos))) +# endif +# endif + + int verbose = 0; virConnectPtr conn; int callbackRet = -1; @@ -76,11 +87,31 @@ stop(int sig) } static void -showError(virConnectPtr conn) +ATTRIBUTE_FMT_PRINTF(1,2) +showError(const char *fmt, ...) { - const char *err = virGetLastErrorMessage(); + const char *libvirtErr = virGetLastErrorMessage(); + char ebuf[1024]; + int rc; + int size = 0; + va_list ap; - snmp_log(LOG_ERR, "libvirt reported: \"%s\"\n", err); + va_start(ap, fmt); + rc = vsnprintf(ebuf, sizeof(ebuf), fmt, ap); + size += rc; + va_end(ap); + + if (rc < 0 || size >= sizeof(ebuf)) + return; + + rc = snprintf(ebuf + size, sizeof(ebuf) - size, ": %s\n", libvirtErr); + size += rc; + + if (rc < 0 || size >= sizeof(ebuf)) + return; + + fputs(ebuf, stderr); + snmp_log(LOG_ERR, "%s", ebuf); virResetLastError(); } @@ -95,8 +126,7 @@ insertGuest(netsnmp_container *container, virDomainPtr domain) unsigned char uuid[VIR_UUID_BUFLEN]; if (virDomainGetInfo(domain, &info) < 0) { - printf("Failed to get domain info\n"); - showError(conn); + showError("Failed to get domain info"); goto error; } @@ -108,8 +138,7 @@ insertGuest(netsnmp_container *container, virDomainPtr domain) /* set the index of the row */ if (virDomainGetUUID(domain, uuid) < 0) { - printf("Failed to get domain UUID\n"); - showError(conn); + showError("Failed to get domain UUID"); goto error; } @@ -122,8 +151,7 @@ insertGuest(netsnmp_container *container, virDomainPtr domain) /* set the data */ if (!(name = virDomainGetName(domain))) { - printf("Failed to get domain name\n"); - showError(conn); + showError("Failed to get domain name"); goto error; } @@ -167,8 +195,7 @@ libvirtSnmpLoadGuests(netsnmp_container *container) virDomainPtr *domains = NULL; if ((ndomains = virConnectListAllDomains(conn, &domains, 0)) < 0) { - printf("Failed to list all domains\n"); - showError(conn); + showError("Failed to list all domains"); goto cleanup; } @@ -194,7 +221,7 @@ pollingThreadFunc(void *foo) { while (run) { if (virEventRunDefaultImpl() < 0) { - showError(conn); + showError("Failed to run event loop"); pthread_exit(NULL); } } @@ -222,8 +249,10 @@ libvirtRegisterEvents(virConnectPtr conn) { (domainEventCallback), NULL, myFreeFunc); - if (callbackRet == -1) - return -1; + if (callbackRet == -1) { + showError("Failed to register libvirt event handler"); + goto cleanup; + } /* we need a thread to poll for events */ if (pthread_create(&poll_thread, NULL, pollingThreadFunc, NULL)) @@ -258,8 +287,7 @@ int libvirtSnmpInit(void) /* TODO: configure the URI */ /* Use libvirt env variable LIBVIRT_DEFAULT_URI by default*/ if (!(conn = virConnectOpenAuth(NULL, virConnectAuthPtrDefault, 0))) { - printf("No connection to hypervisor\n"); - showError(conn); + showError("No connection to hypervisor"); return -1; } @@ -298,9 +326,8 @@ void libvirtSnmpShutdown(void) } if ((rc = virConnectClose(conn))) { - printf("Failed to disconnect from hypervisor. " - "Leaked references: %d\n", rc); - showError(conn); + showError("Failed to disconnect from hypervisor. " + "Leaked references: %d\n", rc); } } @@ -339,7 +366,7 @@ libvirtSnmpCreate(unsigned char *uuid, int state) } if (virDomainCreateWithFlags(dom, flags) < 0) { - showError(conn); + showError("Failed to create domain: %s", virDomainGetName(dom)); goto cleanup; } @@ -353,6 +380,7 @@ int libvirtSnmpDestroy(unsigned char *uuid) { virDomainPtr dom; + int ret = -1; if (!(dom = virDomainLookupByUUID(conn, uuid))) { printf("Cannot find domain to destroy\n"); @@ -360,10 +388,12 @@ libvirtSnmpDestroy(unsigned char *uuid) } if (virDomainDestroy(dom) < 0) { - showError(conn); - return -1; + showError("Failed to destroy domain %s", virDomainGetName(dom)); + goto cleanup; } + ret = 0; + cleanup: virDomainFree(dom); return 0; } @@ -374,8 +404,7 @@ libvirtSnmpChangeState(unsigned char *uuid, int newstate, int oldstate) virDomainPtr dom; int ret = -1; - dom = virDomainLookupByUUID(conn, uuid); - if (dom == NULL) { + if (!(dom = virDomainLookupByUUID(conn, uuid))) { printf("Cannot find domain to change\n"); return 1; } @@ -392,7 +421,7 @@ libvirtSnmpChangeState(unsigned char *uuid, int newstate, int oldstate) } if (ret < 0) - showError(conn); + showError("Failed to change state of %s", virDomainGetName(dom)); out: virDomainFree(dom); return ret; -- 2.18.1

This function is not extern. Drop the keyword. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirtSnmpError.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libvirtSnmpError.h b/src/libvirtSnmpError.h index e28ad7c..8dae0cb 100644 --- a/src/libvirtSnmpError.h +++ b/src/libvirtSnmpError.h @@ -23,6 +23,6 @@ #ifndef __LIBVIRT_SNMP_ERROR_H__ # define __LIBVIRT_SNMP_ERROR_H__ -extern void printLibvirtError(const char *msg); +void printLibvirtError(const char *msg); #endif /* __LIBVIRT_SNMP_ERROR_H__ */ -- 2.18.1

These functions are not extern. Drop the keyword. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirtSnmp.h | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/libvirtSnmp.h b/src/libvirtSnmp.h index 24707bd..f109bb4 100644 --- a/src/libvirtSnmp.h +++ b/src/libvirtSnmp.h @@ -31,29 +31,29 @@ /* * Populate libvirtGuestTable into given container. */ -extern int +int libvirtSnmpLoadGuests(netsnmp_container *container); -extern int +int libvirtSnmpInit(void); -extern void +void libvirtSnmpShutdown(void); /** * Check that domain with given UUID exists. * Return 0 if so, -1 if not. */ -extern int +int libvirtSnmpCheckDomainExists(unsigned char *uuid); -extern int +int libvirtSnmpCreate(unsigned char *uuid, int state); -extern int +int libvirtSnmpDestroy(unsigned char *uuid); -extern int +int libvirtSnmpChangeState(unsigned char *uuid, int newstate, int oldstate); #endif /* __LIBVIRT_SNMP_H__ */ -- 2.18.1

We have two function that do merely the same. Merge them into one. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirtSnmp.c | 65 ++++++++---------------------------------- src/libvirtSnmpError.c | 38 ++++++++++++++++++++---- src/libvirtSnmpError.h | 13 ++++++++- 3 files changed, 57 insertions(+), 59 deletions(-) diff --git a/src/libvirtSnmp.c b/src/libvirtSnmp.c index 7aa83a1..97bb927 100644 --- a/src/libvirtSnmp.c +++ b/src/libvirtSnmp.c @@ -30,6 +30,7 @@ #include <libvirt/virterror.h> #include "libvirtSnmp.h" +#include "libvirtSnmpError.h" #include "libvirtGuestTable.h" /* include our MIB structures*/ #include "libvirtNotifications.h" @@ -43,17 +44,6 @@ #define ATTRIBUTE_UNUSED __attribute__((__unused__)) #endif -# ifndef ATTRIBUTE_FMT_PRINTF -# ifndef __clang__ -# define ATTRIBUTE_FMT_PRINTF(fmtpos, argpos) \ - __attribute__((__format__ (__gnu_printf__, fmtpos, argpos))) -# else -# define ATTRIBUTE_FMT_PRINTF(fmtpos, argpos) \ - __attribute__((__format__ (__printf__, fmtpos, argpos))) -# endif -# endif - - int verbose = 0; virConnectPtr conn; int callbackRet = -1; @@ -86,37 +76,6 @@ stop(int sig) run = 0; } -static void -ATTRIBUTE_FMT_PRINTF(1,2) -showError(const char *fmt, ...) -{ - const char *libvirtErr = virGetLastErrorMessage(); - char ebuf[1024]; - int rc; - int size = 0; - va_list ap; - - va_start(ap, fmt); - rc = vsnprintf(ebuf, sizeof(ebuf), fmt, ap); - size += rc; - va_end(ap); - - if (rc < 0 || size >= sizeof(ebuf)) - return; - - rc = snprintf(ebuf + size, sizeof(ebuf) - size, ": %s\n", libvirtErr); - size += rc; - - if (rc < 0 || size >= sizeof(ebuf)) - return; - - fputs(ebuf, stderr); - snmp_log(LOG_ERR, "%s", ebuf); - - virResetLastError(); -} - - static int insertGuest(netsnmp_container *container, virDomainPtr domain) { @@ -126,7 +85,7 @@ insertGuest(netsnmp_container *container, virDomainPtr domain) unsigned char uuid[VIR_UUID_BUFLEN]; if (virDomainGetInfo(domain, &info) < 0) { - showError("Failed to get domain info"); + printLibvirtError("Failed to get domain info"); goto error; } @@ -138,7 +97,7 @@ insertGuest(netsnmp_container *container, virDomainPtr domain) /* set the index of the row */ if (virDomainGetUUID(domain, uuid) < 0) { - showError("Failed to get domain UUID"); + printLibvirtError("Failed to get domain UUID"); goto error; } @@ -151,7 +110,7 @@ insertGuest(netsnmp_container *container, virDomainPtr domain) /* set the data */ if (!(name = virDomainGetName(domain))) { - showError("Failed to get domain name"); + printLibvirtError("Failed to get domain name"); goto error; } @@ -195,7 +154,7 @@ libvirtSnmpLoadGuests(netsnmp_container *container) virDomainPtr *domains = NULL; if ((ndomains = virConnectListAllDomains(conn, &domains, 0)) < 0) { - showError("Failed to list all domains"); + printLibvirtError("Failed to list all domains"); goto cleanup; } @@ -221,7 +180,7 @@ pollingThreadFunc(void *foo) { while (run) { if (virEventRunDefaultImpl() < 0) { - showError("Failed to run event loop"); + printLibvirtError("Failed to run event loop"); pthread_exit(NULL); } } @@ -250,7 +209,7 @@ libvirtRegisterEvents(virConnectPtr conn) { NULL, myFreeFunc); if (callbackRet == -1) { - showError("Failed to register libvirt event handler"); + printLibvirtError("Failed to register libvirt event handler"); goto cleanup; } @@ -287,7 +246,7 @@ int libvirtSnmpInit(void) /* TODO: configure the URI */ /* Use libvirt env variable LIBVIRT_DEFAULT_URI by default*/ if (!(conn = virConnectOpenAuth(NULL, virConnectAuthPtrDefault, 0))) { - showError("No connection to hypervisor"); + printLibvirtError("No connection to hypervisor"); return -1; } @@ -326,7 +285,7 @@ void libvirtSnmpShutdown(void) } if ((rc = virConnectClose(conn))) { - showError("Failed to disconnect from hypervisor. " + printLibvirtError("Failed to disconnect from hypervisor. " "Leaked references: %d\n", rc); } } @@ -366,7 +325,7 @@ libvirtSnmpCreate(unsigned char *uuid, int state) } if (virDomainCreateWithFlags(dom, flags) < 0) { - showError("Failed to create domain: %s", virDomainGetName(dom)); + printLibvirtError("Failed to create domain: %s", virDomainGetName(dom)); goto cleanup; } @@ -388,7 +347,7 @@ libvirtSnmpDestroy(unsigned char *uuid) } if (virDomainDestroy(dom) < 0) { - showError("Failed to destroy domain %s", virDomainGetName(dom)); + printLibvirtError("Failed to destroy domain %s", virDomainGetName(dom)); goto cleanup; } @@ -421,7 +380,7 @@ libvirtSnmpChangeState(unsigned char *uuid, int newstate, int oldstate) } if (ret < 0) - showError("Failed to change state of %s", virDomainGetName(dom)); + printLibvirtError("Failed to change state of %s", virDomainGetName(dom)); out: virDomainFree(dom); return ret; diff --git a/src/libvirtSnmpError.c b/src/libvirtSnmpError.c index 1678bcb..c356aae 100644 --- a/src/libvirtSnmpError.c +++ b/src/libvirtSnmpError.c @@ -25,15 +25,43 @@ #include <stdio.h> #include <libvirt/libvirt.h> #include <libvirt/virterror.h> +#include <net-snmp/net-snmp-config.h> +#include <net-snmp/net-snmp-includes.h> +#include <net-snmp/agent/net-snmp-agent-includes.h> #include "libvirtSnmpError.h" /** - * Print libvirt error - * @msg Error message + * printLibvirtError: + * @fmt: Error message format string + * + * Print and reset Libvirt error. */ -void printLibvirtError(const char *msg) { - virErrorPtr err = virGetLastError(); +void +printLibvirtError(const char *fmt, ...) +{ + const char *libvirtErr = virGetLastErrorMessage(); + char ebuf[1024]; + int rc; + int size = 0; + va_list ap; - fprintf(stderr, "%s: %s", msg, err ? err->message : "Unknown error"); + va_start(ap, fmt); + rc = vsnprintf(ebuf, sizeof(ebuf), fmt, ap); + size += rc; + va_end(ap); + + if (rc < 0 || size >= sizeof(ebuf)) + return; + + rc = snprintf(ebuf + size, sizeof(ebuf) - size, ": %s\n", libvirtErr); + size += rc; + + if (rc < 0 || size >= sizeof(ebuf)) + return; + + fputs(ebuf, stderr); + snmp_log(LOG_ERR, "%s", ebuf); + + virResetLastError(); } diff --git a/src/libvirtSnmpError.h b/src/libvirtSnmpError.h index 8dae0cb..46c98f0 100644 --- a/src/libvirtSnmpError.h +++ b/src/libvirtSnmpError.h @@ -23,6 +23,17 @@ #ifndef __LIBVIRT_SNMP_ERROR_H__ # define __LIBVIRT_SNMP_ERROR_H__ -void printLibvirtError(const char *msg); +# ifndef ATTRIBUTE_FMT_PRINTF +# ifndef __clang__ +# define ATTRIBUTE_FMT_PRINTF(fmtpos, argpos) \ + __attribute__((__format__ (__gnu_printf__, fmtpos, argpos))) +# else +# define ATTRIBUTE_FMT_PRINTF(fmtpos, argpos) \ + __attribute__((__format__ (__printf__, fmtpos, argpos))) +# endif +# endif + +void printLibvirtError(const char *fmt, ...) + ATTRIBUTE_FMT_PRINTF(1, 2); #endif /* __LIBVIRT_SNMP_ERROR_H__ */ -- 2.18.1

This function formats passed string and prints stringified system error. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirtSnmp.c | 8 ++++++-- src/libvirtSnmpError.c | 38 ++++++++++++++++++++++++++++++++++++++ src/libvirtSnmpError.h | 3 +++ 3 files changed, 47 insertions(+), 2 deletions(-) diff --git a/src/libvirtSnmp.c b/src/libvirtSnmp.c index 97bb927..3a93bc6 100644 --- a/src/libvirtSnmp.c +++ b/src/libvirtSnmp.c @@ -189,8 +189,10 @@ pollingThreadFunc(void *foo) /* Function to register domain lifecycle events collection */ int -libvirtRegisterEvents(virConnectPtr conn) { +libvirtRegisterEvents(virConnectPtr conn) +{ struct sigaction action_stop; + int rc; int ret = -1; memset(&action_stop, 0, sizeof action_stop); @@ -214,8 +216,10 @@ libvirtRegisterEvents(virConnectPtr conn) { } /* we need a thread to poll for events */ - if (pthread_create(&poll_thread, NULL, pollingThreadFunc, NULL)) + if ((rc = pthread_create(&poll_thread, NULL, pollingThreadFunc, NULL))) { + printSystemError(rc, "Unable to create polling thread"); goto cleanup; + } ret = 0; cleanup: diff --git a/src/libvirtSnmpError.c b/src/libvirtSnmpError.c index c356aae..33a65c2 100644 --- a/src/libvirtSnmpError.c +++ b/src/libvirtSnmpError.c @@ -65,3 +65,41 @@ printLibvirtError(const char *fmt, ...) virResetLastError(); } + + +/** + * printSystemError: + * @theerrno: the errno value + * @fmt: Error message format string + * + * Print system error with @theerrno translated into human readable form. + */ +void +printSystemError(int theerrno, const char *fmt, ...) +{ + char ebuf[1024]; + char sysebuf[1024]; + int rc; + int size = 0; + va_list ap; + + if (!strerror_r(theerrno, sysebuf, sizeof(sysebuf))) + return; + + va_start(ap, fmt); + rc = vsnprintf(ebuf, sizeof(ebuf), fmt, ap); + size += rc; + va_end(ap); + + if (rc < 0 || size >= sizeof(ebuf)) + return; + + rc = snprintf(ebuf + size, sizeof(ebuf) - size, ": %s\n", sysebuf); + size += rc; + + if (rc < 0 || size >= sizeof(ebuf)) + return; + + fputs(ebuf, stderr); + snmp_log(LOG_ERR, "%s", ebuf); +} diff --git a/src/libvirtSnmpError.h b/src/libvirtSnmpError.h index 46c98f0..e8822d1 100644 --- a/src/libvirtSnmpError.h +++ b/src/libvirtSnmpError.h @@ -36,4 +36,7 @@ void printLibvirtError(const char *fmt, ...) ATTRIBUTE_FMT_PRINTF(1, 2); +void printSystemError(int theerrno, const char *fmt, ...) + ATTRIBUTE_FMT_PRINTF(2, 3); + #endif /* __LIBVIRT_SNMP_ERROR_H__ */ -- 2.18.1

This function returns only a single value. It makes no sense to have it return that. Make it return void. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirtSnmp.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/libvirtSnmp.c b/src/libvirtSnmp.c index 3a93bc6..2f2f406 100644 --- a/src/libvirtSnmp.c +++ b/src/libvirtSnmp.c @@ -227,14 +227,14 @@ libvirtRegisterEvents(virConnectPtr conn) } /* Unregister domain events collection */ -int +static void libvirtUnregisterEvents(virConnectPtr conn) { - void *status; + if (callbackRet < 0) + return; virConnectDomainEventDeregisterAny(conn, callbackRet); callbackRet = -1; - return 0; } int libvirtSnmpInit(void) @@ -284,13 +284,11 @@ void libvirtSnmpShutdown(void) if (timer != -1) virEventRemoveTimeout(timer); - if (libvirtUnregisterEvents(conn)) { - printf("Failed to unregister domain events\n"); - } + libvirtUnregisterEvents(conn); if ((rc = virConnectClose(conn))) { printLibvirtError("Failed to disconnect from hypervisor. " - "Leaked references: %d\n", rc); + "Leaked references: %d\n", rc); } } -- 2.18.1

On Thu, Oct 18, 2018 at 02:26:55PM +0200, Michal Privoznik wrote:
This function returns only a single value. It makes no sense to have it return that. Make it return void.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirtSnmp.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/src/libvirtSnmp.c b/src/libvirtSnmp.c index 3a93bc6..2f2f406 100644 --- a/src/libvirtSnmp.c +++ b/src/libvirtSnmp.c @@ -227,14 +227,14 @@ libvirtRegisterEvents(virConnectPtr conn) }
/* Unregister domain events collection */ -int +static void libvirtUnregisterEvents(virConnectPtr conn) { - void *status; + if (callbackRet < 0) + return;
virConnectDomainEventDeregisterAny(conn, callbackRet); callbackRet = -1; - return 0; }
int libvirtSnmpInit(void) @@ -284,13 +284,11 @@ void libvirtSnmpShutdown(void) if (timer != -1) virEventRemoveTimeout(timer);
- if (libvirtUnregisterEvents(conn)) { - printf("Failed to unregister domain events\n"); - } + libvirtUnregisterEvents(conn);
if ((rc = virConnectClose(conn))) { printLibvirtError("Failed to disconnect from hypervisor. " - "Leaked references: %d\n", rc); + "Leaked references: %d\n", rc);
This hunk probably should've been part of the s/showError/printLibvirtError/ patch
} }
-- 2.18.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

Instead of plain printf(), report proper libvirt error. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirtSnmp.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libvirtSnmp.c b/src/libvirtSnmp.c index 2f2f406..c334967 100644 --- a/src/libvirtSnmp.c +++ b/src/libvirtSnmp.c @@ -322,7 +322,7 @@ libvirtSnmpCreate(unsigned char *uuid, int state) } if (!(dom = virDomainLookupByUUID(conn, uuid))) { - printf("Cannot find domain to create\n"); + printLibvirtError("No such domain"); return -1; } @@ -344,7 +344,7 @@ libvirtSnmpDestroy(unsigned char *uuid) int ret = -1; if (!(dom = virDomainLookupByUUID(conn, uuid))) { - printf("Cannot find domain to destroy\n"); + printLibvirtError("No such domain"); return -1; } @@ -366,7 +366,7 @@ libvirtSnmpChangeState(unsigned char *uuid, int newstate, int oldstate) int ret = -1; if (!(dom = virDomainLookupByUUID(conn, uuid))) { - printf("Cannot find domain to change\n"); + printLibvirtError("No such domain"); return 1; } -- 2.18.1

The libvirtRegisterEvents() function now reports errors on its own. No need to duplicate the effort. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirtSnmp.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/libvirtSnmp.c b/src/libvirtSnmp.c index c334967..e63942b 100644 --- a/src/libvirtSnmp.c +++ b/src/libvirtSnmp.c @@ -254,10 +254,8 @@ int libvirtSnmpInit(void) return -1; } - if ((callbackRet == -1) && libvirtRegisterEvents(conn)) { - printf("Unable to register domain events\n"); + if ((callbackRet == -1) && libvirtRegisterEvents(conn)) return -1; - } return 0; } -- 2.18.1

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index ce79f12..dd930b7 100644 --- a/.gitignore +++ b/.gitignore @@ -16,6 +16,7 @@ ChangeLog /INSTALL Makefile Makefile.in +tags /aclocal.m4 /autom4te.cache/ /build-aux/compile -- 2.18.1

On Thu, Oct 18, 2018 at 02:26:58PM +0200, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- .gitignore | 1 + 1 file changed, 1 insertion(+)
diff --git a/.gitignore b/.gitignore index ce79f12..dd930b7 100644 --- a/.gitignore +++ b/.gitignore @@ -16,6 +16,7 @@ ChangeLog /INSTALL Makefile Makefile.in +tags /aclocal.m4 /autom4te.cache/ /build-aux/compile -- 2.18.1
I prefer what I learnt here: https://www.redhat.com/archives/libosinfo/2018-October/msg00006.html But it's up to you if you want to follow that (hint: you should).
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, Oct 18, 2018 at 02:26:38PM +0200, Michal Privoznik wrote:
These are not pushed. I'll wait couple of moments if there is somebody who has opinion. If not I will push them.
Except the libvirt-snmp internals this looks good. Few nits posted for particular patches, really just nits. It's a nice clean-up, it definitely makes sense to drop support for too old libvirt IMHO. If you want to make the (+)/(-) ratio even smaller you can reorganize some ofthe patches about error printing and formatting as you are adding some support, then moving it, then putting it in another function and so on. But I don't think the readability is too much impacted by that. It would just make for a nicer git history and even more awesome ratio ;) Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
participants (3)
-
Martin Kletzander
-
Michal Privoznik
-
Pino Toscano