[libvirt] [PATCHv2 0/2] Fix starting of VM's on hosts without DBus compiled in or enabled
Hi, this series fixes regression on hosts without DBus that was introduced by the refactor to use systemd.machined to create cgroups. Version 2: As errors can't be unlogged, this version enhances the error reporting code to add conditional error logging. The only remaining issue is that if a user is running DBus but doesn't use systemd, the logs are still spammed with errors. Peter Krempa (2): virerror: Add support for suppressing reporting of errors cgroup: Don't fail to start a VM when DBus isn't compiled in or running daemon/libvirtd.c | 2 +- daemon/remote.c | 2 +- src/libvirt_private.syms | 2 +- src/node_device/node_device_hal.c | 2 +- src/nwfilter/nwfilter_driver.c | 4 ++-- src/rpc/virnetserver.c | 2 +- src/util/virdbus.c | 15 ++++++++------- src/util/virdbus.h | 2 +- src/util/virerror.c | 31 ++++++++++++++++++++----------- src/util/virerror.h | 21 +++++++++++++++------ src/util/virsystemd.c | 4 ++-- 11 files changed, 53 insertions(+), 34 deletions(-) -- 1.8.3.2
It might be desired to suppress reporting of errors in some cases when the failure isn't fatal but report them in other cases. This patch adds plumbing to simplify doing this and still allows to view the error when in debug mode. The new helper virReportErrorConditional() takes a new first argument. This boolean controls the reporting of the error. --- src/libvirt_private.syms | 2 +- src/util/virerror.c | 31 ++++++++++++++++++++----------- src/util/virerror.h | 21 +++++++++++++++------ 3 files changed, 36 insertions(+), 18 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a958d94..530ca34 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1330,7 +1330,7 @@ virErrorInitialize; virErrorSetErrnoFromLastError; virLastErrorIsSystemErrno; virRaiseErrorFull; -virReportErrorHelper; +virReportErrorHelperInternal; virReportOOMErrorFull; virReportSystemErrorFull; virSetError; diff --git a/src/util/virerror.c b/src/util/virerror.c index ca25678..14a06f4 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -1255,8 +1255,9 @@ virErrorMsg(virErrorNumber error, const char *info) } /** - * virReportErrorHelper: + * virReportErrorHelperInternal: * + * @report: boolean to control wether to raise the error or not * @domcode: the virErrorDomain indicating where it's coming from * @errorcode: the virErrorNumber code for the error * @filename: Source file error is dispatched from @@ -1268,12 +1269,14 @@ virErrorMsg(virErrorNumber error, const char *info) * Helper function to do most of the grunt work for individual driver * ReportError */ -void virReportErrorHelper(int domcode, - int errorcode, - const char *filename, - const char *funcname, - size_t linenr, - const char *fmt, ...) +void +virReportErrorHelperInternal(bool report, + int domcode, + int errorcode, + const char *filename, + const char *funcname, + size_t linenr, + const char *fmt, ...) { int save_errno = errno; va_list args; @@ -1289,10 +1292,16 @@ void virReportErrorHelper(int domcode, } virerr = virErrorMsg(errorcode, (errorMessage[0] ? errorMessage : NULL)); - virRaiseErrorFull(filename, funcname, linenr, - domcode, errorcode, VIR_ERR_ERROR, - virerr, errorMessage, NULL, - -1, -1, virerr, errorMessage); + if (report) { + virRaiseErrorFull(filename, funcname, linenr, + domcode, errorcode, VIR_ERR_ERROR, + virerr, errorMessage, NULL, + -1, -1, virerr, errorMessage); + } else { + VIR_DEBUG("Suppressed error at %s:%zu %s: %s %s", + filename, linenr, funcname, virerr, errorMessage); + } + errno = save_errno; } diff --git a/src/util/virerror.h b/src/util/virerror.h index 05e9950..39db8f6 100644 --- a/src/util/virerror.h +++ b/src/util/virerror.h @@ -47,12 +47,16 @@ void virRaiseErrorFull(const char *filename, const char *fmt, ...) ATTRIBUTE_FMT_PRINTF(12, 13); -void virReportErrorHelper(int domcode, int errcode, - const char *filename, - const char *funcname, - size_t linenr, - const char *fmt, ...) - ATTRIBUTE_FMT_PRINTF(6, 7); +void virReportErrorHelperInternal(bool report, + int domcode, int errcode, + const char *filename, + const char *funcname, + size_t linenr, + const char *fmt, ...) + ATTRIBUTE_FMT_PRINTF(7, 8); + +# define virReportErrorHelper(...) \ + virReportErrorHelperInternal(true, __VA_ARGS__) void virReportSystemErrorFull(int domcode, int theerrno, @@ -168,6 +172,11 @@ void virReportOOMErrorFull(int domcode, virReportErrorHelper(VIR_FROM_THIS, code, __FILE__, \ __FUNCTION__, __LINE__, __VA_ARGS__) +# define virReportErrorConditional(report, code, ...) \ + virReportErrorHelperInternal(report, VIR_FROM_THIS, code, __FILE__, \ + __FUNCTION__, __LINE__, __VA_ARGS__) + + int virSetError(virErrorPtr newerr); void virDispatchError(virConnectPtr conn); const char *virStrerror(int theerrno, char *errBuf, size_t errBufLen); -- 1.8.3.2
The systemd code to create cgroups has fallback means built in, but fails to engage them if a host isn't running DBus or libvirtd is compiled without DBus (yes, you still don't need to run DBus). This patch changes the return value in case DBus isn't available to so that the fallback code can be activated and cgroups are created manually. Additionally, the functions no longer report errors as they were reported over and over again and were no hard errors. --- daemon/libvirtd.c | 2 +- daemon/remote.c | 2 +- src/node_device/node_device_hal.c | 2 +- src/nwfilter/nwfilter_driver.c | 4 ++-- src/rpc/virnetserver.c | 2 +- src/util/virdbus.c | 15 ++++++++------- src/util/virdbus.h | 2 +- src/util/virsystemd.c | 4 ++-- 8 files changed, 17 insertions(+), 16 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index c9cd1a1..bea0190 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -921,7 +921,7 @@ static void daemonRunStateInit(void *opaque) dbus_connection_add_filter(sessionBus, handleSessionMessageFunc, srv, NULL); - systemBus = virDBusGetSystemBus(); + systemBus = virDBusGetSystemBus(true); if (systemBus != NULL) { dbus_connection_add_filter(systemBus, handleSystemMessageFunc, srv, NULL); diff --git a/daemon/remote.c b/daemon/remote.c index 03d5557..d385edf 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -2874,7 +2874,7 @@ remoteDispatchAuthPolkit(virNetServerPtr server ATTRIBUTE_UNUSED, (long long) callerPid, callerUid) < 0) goto authfail; - if (!(sysbus = virDBusGetSystemBus())) + if (!(sysbus = virDBusGetSystemBus(true))) goto authfail; VIR_INFO("Checking PID %lld running as %d", diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c index d94767c..de1b7f9 100644 --- a/src/node_device/node_device_hal.c +++ b/src/node_device/node_device_hal.c @@ -652,7 +652,7 @@ nodeStateInitialize(bool privileged ATTRIBUTE_UNUSED, } nodeDeviceLock(driverState); - if (!(sysbus = virDBusGetSystemBus())) { + if (!(sysbus = virDBusGetSystemBus(true))) { virErrorPtr verr = virGetLastError(); VIR_ERROR(_("DBus not available, disabling HAL driver: %s"), verr->message); diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 9bb4b4e..75290a9 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -100,7 +100,7 @@ nwfilterDriverRemoveDBusMatches(void) { DBusConnection *sysbus; - sysbus = virDBusGetSystemBus(); + sysbus = virDBusGetSystemBus(true); if (sysbus) { dbus_bus_remove_match(sysbus, DBUS_RULE_FWD_NAMEOWNERCHANGED, @@ -175,7 +175,7 @@ nwfilterStateInitialize(bool privileged, DBusConnection *sysbus = NULL; #if WITH_DBUS - sysbus = virDBusGetSystemBus(); + sysbus = virDBusGetSystemBus(true); #endif /* WITH_DBUS */ if (VIR_ALLOC(driverState) < 0) diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 2306e10..98c3594 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -755,7 +755,7 @@ static void virNetServerCallInhibit(virNetServerPtr srv, VIR_DEBUG("srv=%p what=%s who=%s why=%s mode=%s", srv, NULLSTR(what), NULLSTR(who), NULLSTR(why), NULLSTR(mode)); - if (!(systemBus = virDBusGetSystemBus())) + if (!(systemBus = virDBusGetSystemBus(true))) return; /* Only one outstanding call at a time */ diff --git a/src/util/virdbus.c b/src/util/virdbus.c index e88dc26..8333788 100644 --- a/src/util/virdbus.c +++ b/src/util/virdbus.c @@ -73,7 +73,7 @@ static void virDBusSystemBusInit(void) systembus = virDBusBusInit(DBUS_BUS_SYSTEM, &systemdbuserr); } -DBusConnection *virDBusGetSystemBus(void) +DBusConnection *virDBusGetSystemBus(bool fatal) { if (virOnce(&systemonce, virDBusSystemBusInit) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -82,9 +82,10 @@ DBusConnection *virDBusGetSystemBus(void) } if (!systembus) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to get DBus system bus connection: %s"), - systemdbuserr.message ? systemdbuserr.message : "watch setup failed"); + virReportErrorConditional(fatal, VIR_ERR_INTERNAL_ERROR, + _("Unable to get DBus system bus " + "connection: %s"), + systemdbuserr.message ? systemdbuserr.message : "watch setup failed"); return NULL; } @@ -1188,10 +1189,10 @@ int virDBusMessageRead(DBusMessage *msg, #else /* ! WITH_DBUS */ -DBusConnection *virDBusGetSystemBus(void) +DBusConnection *virDBusGetSystemBus(bool fatal) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("DBus support not compiled into this binary")); + virReportErrorConditional(fatal, VIR_ERR_INTERNAL_ERROR, "%s" + _("DBus support not compiled into this binary")); return NULL; } diff --git a/src/util/virdbus.h b/src/util/virdbus.h index 39de479..a30dbc3 100644 --- a/src/util/virdbus.h +++ b/src/util/virdbus.h @@ -31,7 +31,7 @@ # endif # include "internal.h" -DBusConnection *virDBusGetSystemBus(void); +DBusConnection *virDBusGetSystemBus(bool fatal); DBusConnection *virDBusGetSessionBus(void); int virDBusCallMethod(DBusConnection *conn, diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c index 251b846..f81c50b 100644 --- a/src/util/virsystemd.c +++ b/src/util/virsystemd.c @@ -145,8 +145,8 @@ int virSystemdCreateMachine(const char *name, char *username = NULL; char *slicename = NULL; - if (!(conn = virDBusGetSystemBus())) - return -1; + if (!(conn = virDBusGetSystemBus(false))) + return -2; if (privileged) { if (virAsprintf(&machinename, "%s-%s", drivername, name) < 0) -- 1.8.3.2
On Fri, Aug 16, 2013 at 04:48:31PM +0200, Peter Krempa wrote:
The systemd code to create cgroups has fallback means built in, but fails to engage them if a host isn't running DBus or libvirtd is compiled without DBus (yes, you still don't need to run DBus).
This patch changes the return value in case DBus isn't available to so that the fallback code can be activated and cgroups are created manually.
Additionally, the functions no longer report errors as they were reported over and over again and were no hard errors. --- daemon/libvirtd.c | 2 +- daemon/remote.c | 2 +- src/node_device/node_device_hal.c | 2 +- src/nwfilter/nwfilter_driver.c | 4 ++-- src/rpc/virnetserver.c | 2 +- src/util/virdbus.c | 15 ++++++++------- src/util/virdbus.h | 2 +- src/util/virsystemd.c | 4 ++-- 8 files changed, 17 insertions(+), 16 deletions(-)
diff --git a/src/util/virdbus.h b/src/util/virdbus.h index 39de479..a30dbc3 100644 --- a/src/util/virdbus.h +++ b/src/util/virdbus.h @@ -31,7 +31,7 @@ # endif # include "internal.h"
-DBusConnection *virDBusGetSystemBus(void); +DBusConnection *virDBusGetSystemBus(bool fatal); DBusConnection *virDBusGetSessionBus(void);
I'm really not at all a fan of 'bool fatal' like args to control error reporting. I think in this case we should have a bool virDBusHasSystemBus(void) method, which calls can use to determine if dbus is available. If not available then they can take a separate codepath as needed. This avoids the need to try something we expect to fail and then try to discard the errors. This would make the first patch uneccessary too. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (2)
-
Daniel P. Berrange -
Peter Krempa