Re: [libvirt] [PATCH 04/40] Simplify opening of Xen drivers

From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; Since the Xen driver was changed to only execute inside libvirtd, there is no scenario in which it will be opened from a non-privileged context. This all the code dealing with opening the sub-drivers can

be simplified to assume that they are always privileged. Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; --- src/xen/xen_driver.c | 148 +++++++++++++++++++++-------------------------- src/xen/xen_driver.h | 1 - src/xen/xen_hypervisor.c | 9 ++- src/xen/xen_hypervisor.h | 2 +- src/xen/xen_inotify.c | 8 +-- src/xen/xen_inotify.h | 11 ++-- src/xen/xend_internal.c | 9 ++- src/xen/xend_internal.h | 4 +- src/xen/xm_internal.c | 5 +- src/xen/xm_internal.h | 4 +- src/xen/xs_internal.c | 5 +- src/xen/xs_internal.h | 2 +- 12 files changed, 91 insertions(+), 117 deletions(-) diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 2ecb86f..b7f1ad4 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -86,14 +86,9 @@ static struct xenUnifiedDriver const * const drivers[XEN_UNIFIED_NR_DRIVERS] = { [XEN_UNIFIED_XEND_OFFSET] = &xenDaemonDriver, [XEN_UNIFIED_XS_OFFSET] = &xenStoreDriver, [XEN_UNIFIED_XM_OFFSET] = &xenXMDriver, -#if WITH_XEN_INOTIFY - [XEN_UNIFIED_INOTIFY_OFFSET] = &xenInotifyDriver, -#endif

}; -#if defined WITH_LIBVIRTD || defined __sun static bool inside_daemon = false; -#endif /** * xenNumaInit: @@ -200,14 +195,14 @@ done: return res; } -#ifdef WITH_LIBVIRTD - static int -xenUnifiedStateInitialize(bool privileged ATTRIBUTE_UNUSED, +xenUnifiedStateInitialize(bool privileged, virStateInhibitCallback callback ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) { - inside_daemon = true; + /* Don't allow driver to work in non-root libvirtd */ + if (privileged) + inside_daemon = true;

return 0; } @@ -216,8 +211,6 @@ static virStateDriver state_driver = { .stateInitialize = xenUnifiedStateInitialize, }; -#endif - /*----- Dispatch functions. -----*/ /* These dispatch functions follow the model used historically @@ -298,18 +291,15 @@ xenDomainXMLConfInit(void) static virDrvOpenStatus xenUnifiedConnectOpen(virConnectPtr conn, virConnectAuthPtr auth, unsigned int flags) { - int i, ret = VIR_DRV_OPEN_DECLINED; xenUnifiedPrivatePtr priv; char ebuf[1024]; -#ifdef __sun /* * Only the libvirtd instance can open this driver. * Everything else falls back to the remote driver. */ if (!inside_daemon) return VIR_DRV_OPEN_DECLINED; -#endif if (conn->uri == NULL) { if (!xenUnifiedProbe()) @@ -379,110 +369,108 @@ xenUnifiedConnectOpen(virConnectPtr conn, virConnectAuthPtr auth, unsigned int f priv->xshandle = NULL; - /* Hypervisor is only run with privilege & required to succeed */ - if (xenHavePrivilege()) { - VIR_DEBUG("Trying hypervisor sub-driver"); - if (xenHypervisorOpen(conn, auth, flags) == VIR_DRV_OPEN_SUCCESS) { - VIR_DEBUG("Activated hypervisor sub-driver"); - priv->opened[XEN_UNIFIED_HYPERVISOR_OFFSET] = 1; - } else { - goto fail; - } - } + /* Hypervisor required to succeed */ + VIR_DEBUG("Trying hypervisor sub-driver"); + if (xenHypervisorOpen(conn, auth, flags) < 0) + goto error; + VIR_DEBUG("Activated hypervisor sub-driver"); + priv->opened[XEN_UNIFIED_HYPERVISOR_OFFSET] = 1; - /* XenD is required to succeed if privileged */ + /* XenD is required to succeed */ VIR_DEBUG("Trying XenD sub-driver"); - if (xenDaemonOpen(conn, auth, flags) == VIR_DRV_OPEN_SUCCESS) { - VIR_DEBUG("Activated XenD sub-driver"); - priv->opened[XEN_UNIFIED_XEND_OFFSET] = 1; - - /* XenD is active, so try the xm & xs drivers too, both requird to - * succeed if root, optional otherwise */ - if (priv->xendConfigVersion <= XEND_CONFIG_VERSION_3_0_3) { - VIR_DEBUG("Trying XM sub-driver"); - if (xenXMOpen(conn, auth, flags) == VIR_DRV_OPEN_SUCCESS) { - VIR_DEBUG("Activated XM sub-driver"); - priv->opened[XEN_UNIFIED_XM_OFFSET] = 1; - } - } - VIR_DEBUG("Trying XS sub-driver"); - if (xenStoreOpen(conn, auth, flags) == VIR_DRV_OPEN_SUCCESS) { - VIR_DEBUG("Activated XS sub-driver"); - priv->opened[XEN_UNIFIED_XS_OFFSET] = 1; - } else { - if (xenHavePrivilege()) - goto fail; /* XS is mandatory when privileged */ - } - } else { - if (xenHavePrivilege()) { - goto fail; /* XenD is mandatory when privileged */ - } else { - VIR_DEBUG("Handing off for remote driver"); - ret = VIR_DRV_OPEN_DECLINED; /* Let remote_driver try instead */ - goto clean; - } - } + if (xenDaemonOpen(conn, auth, flags) < 0) + goto error; + VIR_DEBUG("Activated XenD sub-driver"); + priv->opened[XEN_UNIFIED_XEND_OFFSET] = 1; + + /* For old XenD, the XM driver is required to succeed */ + if (priv->xendConfigVersion <= XEND_CONFIG_VERSION_3_0_3) { + VIR_DEBUG("Trying XM sub-driver"); + if (xenXMOpen(conn, auth, flags) < 0) + goto error; + VIR_DEBUG("Activated XM sub-driver"); + priv->opened[XEN_UNIFIED_XM_OFFSET] = 1; + } + + VIR_DEBUG("Trying XS sub-driver"); + if (xenStoreOpen(conn, auth, flags) < 0) + goto error; + VIR_DEBUG("Activated XS sub-driver"); + priv->opened[XEN_UNIFIED_XS_OFFSET] = 1; xenNumaInit(conn); if (!(priv->caps = xenHypervisorMakeCapabilities(conn))) { - VIR_DEBUG("Failed to make capabilities"); - goto fail; + VIR_DEBUG("Errored to make capabilities");

+ goto error; } if (!(priv->xmlopt = xenDomainXMLConfInit())) - goto fail; + goto error; #if WITH_XEN_INOTIFY - if (xenHavePrivilege()) { - VIR_DEBUG("Trying Xen inotify sub-driver"); - if (xenInotifyOpen(conn, auth, flags) == VIR_DRV_OPEN_SUCCESS) { - VIR_DEBUG("Activated Xen inotify sub-driver"); - priv->opened[XEN_UNIFIED_INOTIFY_OFFSET] = 1; - } - } + VIR_DEBUG("Trying Xen inotify sub-driver"); + if (xenInotifyOpen(conn, auth, flags) < 0) + goto error;

+ VIR_DEBUG("Activated Xen inotify sub-driver"); + priv->opened[XEN_UNIFIED_INOTIFY_OFFSET] = 1; #endif if (virAsprintf(&priv->saveDir, "%s", XEN_SAVE_DIR) == -1) { virReportOOMError(); - goto fail; + goto error; } if (virFileMakePath(priv->saveDir) < 0) { - VIR_ERROR(_("Failed to create save dir '%s': %s"), priv->saveDir, + VIR_ERROR(_("Errored to create save dir '%s': %s"), priv->saveDir, virStrerror(errno, ebuf, sizeof(ebuf))); - goto fail; + goto error; } return VIR_DRV_OPEN_SUCCESS; -fail: - ret = VIR_DRV_OPEN_ERROR; -clean: +error: VIR_DEBUG("Failed to activate a mandatory sub-driver"); - for (i = 0 ; i < XEN_UNIFIED_NR_DRIVERS ; i++) - if (priv->opened[i]) - drivers[i]->xenClose(conn); +#if WITH_XEN_INOTIFY + if (priv->opened[XEN_UNIFIED_INOTIFY_OFFSET]) + xenInotifyClose(conn); +#endif + if (priv->opened[XEN_UNIFIED_XM_OFFSET]) + xenXMClose(conn); + if (priv->opened[XEN_UNIFIED_XS_OFFSET]) + xenStoreClose(conn); + if (priv->opened[XEN_UNIFIED_XEND_OFFSET]) + xenDaemonClose(conn); + if (priv->opened[XEN_UNIFIED_HYPERVISOR_OFFSET]) + xenHypervisorClose(conn); virMutexDestroy(&priv->lock); VIR_FREE(priv->saveDir); VIR_FREE(priv); conn->privateData = NULL; - return ret; + return VIR_DRV_OPEN_ERROR; } static int xenUnifiedConnectClose(virConnectPtr conn) { xenUnifiedPrivatePtr priv = conn->privateData; - int i; virObjectUnref(priv->caps); virObjectUnref(priv->xmlopt); virDomainEventStateFree(priv->domainEvents); - for (i = 0; i < XEN_UNIFIED_NR_DRIVERS; ++i) - if (priv->opened[i]) - drivers[i]->xenClose(conn); +#if WITH_XEN_INOTIFY + if (priv->opened[XEN_UNIFIED_INOTIFY_OFFSET]) + xenInotifyClose(conn); +#endif + if (priv->opened[XEN_UNIFIED_XM_OFFSET]) + xenXMClose(conn); + if (priv->opened[XEN_UNIFIED_XS_OFFSET]) + xenStoreClose(conn); + if (priv->opened[XEN_UNIFIED_XEND_OFFSET]) + xenDaemonClose(conn); + if (priv->opened[XEN_UNIFIED_HYPERVISOR_OFFSET]) + xenHypervisorClose(conn); VIR_FREE(priv->saveDir); virMutexDestroy(&priv->lock); @@ -2485,9 +2473,7 @@ static virDriver xenUnifiedDriver = { int xenRegister(void) { -#ifdef WITH_LIBVIRTD if (virRegisterStateDriver(&state_driver) == -1) return -1; -#endif return virRegisterDriver(&xenUnifiedDriver); } diff --git a/src/xen/xen_driver.h b/src/xen/xen_driver.h index c39e9be..70c1226 100644 --- a/src/xen/xen_driver.h +++ b/src/xen/xen_driver.h @@ -93,7 +93,6 @@ extern int xenRegister (void); * structure with direct calls in xen_unified.c. */ struct xenUnifiedDriver { - virDrvConnectClose xenClose; /* Only mandatory callback; all others may be NULL */ virDrvConnectGetVersion xenVersion; virDrvConnectGetHostname xenGetHostname; virDrvDomainSuspend xenDomainSuspend; diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c index d9941ec..6b41898 100644 --- a/src/xen/xen_hypervisor.c +++ b/src/xen/xen_hypervisor.c @@ -880,7 +880,6 @@ typedef struct xen_op_v2_dom xen_op_v2_dom; static unsigned long long xenHypervisorGetMaxMemory(virDomainPtr domain); struct xenUnifiedDriver xenHypervisorDriver = { - .xenClose = xenHypervisorClose, .xenVersion = xenHypervisorGetVersion, .xenDomainSuspend = xenHypervisorPauseDomain, .xenDomainResume = xenHypervisorResumeDomain, @@ -2184,7 +2183,7 @@ VIR_ONCE_GLOBAL_INIT(xenHypervisor) * * Returns 0 or -1 in case of error. */ -virDrvOpenStatus +int xenHypervisorOpen(virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, unsigned int flags) @@ -2192,10 +2191,10 @@ xenHypervisorOpen(virConnectPtr conn, int ret; xenUnifiedPrivatePtr priv = conn->privateData; - virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR); + virCheckFlags(VIR_CONNECT_RO, -1); if (xenHypervisorInitialize() < 0) - return VIR_DRV_OPEN_ERROR; + return -1; priv->handle = -1; @@ -2207,7 +2206,7 @@ xenHypervisorOpen(virConnectPtr conn, priv->handle = ret; - return VIR_DRV_OPEN_SUCCESS; + return 0; } /** diff --git a/src/xen/xen_hypervisor.h b/src/xen/xen_hypervisor.h index 786e301..86dca88 100644 --- a/src/xen/xen_hypervisor.h +++ b/src/xen/xen_hypervisor.h @@ -53,7 +53,7 @@ virDomainPtr char * xenHypervisorDomainGetOSType (virDomainPtr dom); -virDrvOpenStatus +int xenHypervisorOpen (virConnectPtr conn, virConnectAuthPtr auth, unsigned int flags); diff --git a/src/xen/xen_inotify.c b/src/xen/xen_inotify.c index d83708c..b032bba 100644 --- a/src/xen/xen_inotify.c +++ b/src/xen/xen_inotify.c @@ -44,10 +44,6 @@ #define VIR_FROM_THIS VIR_FROM_XEN_INOTIFY -struct xenUnifiedDriver xenInotifyDriver = { - .xenClose = xenInotifyClose, -}; - static int xenInotifyXenCacheLookup(virConnectPtr conn, const char *filename, @@ -349,7 +345,7 @@ cleanup: * * Returns 0 or -1 in case of error. */ -virDrvOpenStatus +int xenInotifyOpen(virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, unsigned int flags) @@ -359,7 +355,7 @@ xenInotifyOpen(virConnectPtr conn, char *path; xenUnifiedPrivatePtr priv = conn->privateData; - virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR); + virCheckFlags(VIR_CONNECT_RO, -1); if (priv->configDir) { priv->useXenConfigCache = 1; diff --git a/src/xen/xen_inotify.h b/src/xen/xen_inotify.h index 8b31b50..6055c88 100644 --- a/src/xen/xen_inotify.h +++ b/src/xen/xen_inotify.h @@ -24,13 +24,10 @@ # define __VIR_XEN_INOTIFY_H__ # include "internal.h" -# include "driver.h" -extern struct xenUnifiedDriver xenInotifyDriver; - -virDrvOpenStatus xenInotifyOpen (virConnectPtr conn, - virConnectAuthPtr auth, - unsigned int flags); -int xenInotifyClose (virConnectPtr conn); +int xenInotifyOpen(virConnectPtr conn, + virConnectAuthPtr auth, + unsigned int flags); +int xenInotifyClose(virConnectPtr conn); #endif diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index e1f0708..eb3e63e 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -1231,15 +1231,15 @@ error: * * Returns 0 in case of success, -1 in case of error. */ -virDrvOpenStatus +int xenDaemonOpen(virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, unsigned int flags) { char *port = NULL; - int ret = VIR_DRV_OPEN_ERROR; + int ret = -1; - virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR); + virCheckFlags(VIR_CONNECT_RO, -1); /* Switch on the scheme, which we expect to be NULL (file), * "http" or "xen". @@ -1286,7 +1286,7 @@ xenDaemonOpen(virConnectPtr conn, } done: - ret = VIR_DRV_OPEN_SUCCESS; + ret = 0; failed: VIR_FREE(port); @@ -3652,7 +3652,6 @@ xenDaemonDomainBlockPeek(virDomainPtr domain, } struct xenUnifiedDriver xenDaemonDriver = { - .xenClose = xenDaemonClose, .xenVersion = xenDaemonGetVersion, .xenDomainSuspend = xenDaemonDomainSuspend, .xenDomainResume = xenDaemonDomainResume, diff --git a/src/xen/xend_internal.h b/src/xen/xend_internal.h index 06c75e1..e5c0896 100644 --- a/src/xen/xend_internal.h +++ b/src/xen/xend_internal.h @@ -95,8 +95,8 @@ xenDaemonDomainFetch(virConnectPtr xend, /* refactored ones */ -virDrvOpenStatus xenDaemonOpen(virConnectPtr conn, virConnectAuthPtr auth, - unsigned int flags); +int xenDaemonOpen(virConnectPtr conn, virConnectAuthPtr auth, + unsigned int flags); int xenDaemonClose(virConnectPtr conn); int xenDaemonGetVersion(virConnectPtr conn, unsigned long *hvVer); int xenDaemonNodeGetInfo(virConnectPtr conn, virNodeInfoPtr info); diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index 8580793..1b4d1cf 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -81,7 +81,6 @@ static int xenXMDomainDetachDeviceFlags(virDomainPtr domain, const char *xml, #define XM_XML_ERROR "Invalid xml" struct xenUnifiedDriver xenXMDriver = { - .xenClose = xenXMClose, .xenDomainGetMaxMemory = xenXMDomainGetMaxMemory, .xenDomainSetMaxMemory = xenXMDomainSetMaxMemory, .xenDomainSetMemory = xenXMDomainSetMemory, @@ -419,14 +418,14 @@ xenXMConfigCacheRefresh(virConnectPtr conn) * us watch for changes (see separate driver), otherwise we poll * every few seconds */ -virDrvOpenStatus +int xenXMOpen(virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, unsigned int flags) { xenUnifiedPrivatePtr priv = conn->privateData; - virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR); + virCheckFlags(VIR_CONNECT_RO, -1); priv->configDir = XM_CONFIG_DIR; diff --git a/src/xen/xm_internal.h b/src/xen/xm_internal.h index df77ac8..6424c41 100644 --- a/src/xen/xm_internal.h +++ b/src/xen/xm_internal.h @@ -36,8 +36,8 @@ int xenXMConfigCacheRefresh (virConnectPtr conn); int xenXMConfigCacheAddFile(virConnectPtr conn, const char *filename); int xenXMConfigCacheRemoveFile(virConnectPtr conn, const char *filename); -virDrvOpenStatus xenXMOpen(virConnectPtr conn, virConnectAuthPtr auth, - unsigned int flags); +int xenXMOpen(virConnectPtr conn, virConnectAuthPtr auth, + unsigned int flags);

int xenXMClose(virConnectPtr conn); const char *xenXMGetType(virConnectPtr conn); int xenXMDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info); diff --git a/src/xen/xs_internal.c b/src/xen/xs_internal.c index 393e5f9..eecdcae 100644 --- a/src/xen/xs_internal.c +++ b/src/xen/xs_internal.c @@ -58,7 +58,6 @@ static void xenStoreWatchEvent(int watch, int fd, int events, void *data); static void xenStoreWatchListFree(xenStoreWatchListPtr list); struct xenUnifiedDriver xenStoreDriver = { - .xenClose = xenStoreClose, .xenDomainShutdown = xenStoreDomainShutdown, .xenDomainReboot = xenStoreDomainReboot, .xenDomainGetOSType = xenStoreDomainGetOSType, @@ -218,14 +217,14 @@ virDomainGetVMInfo(virDomainPtr domain, const char *vm, const char *name) * * Returns 0 or -1 in case of error. */ -virDrvOpenStatus +int xenStoreOpen(virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, unsigned int flags) { xenUnifiedPrivatePtr priv = conn->privateData; - virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR); + virCheckFlags(VIR_CONNECT_RO, -1); if (flags & VIR_CONNECT_RO) priv->xshandle = xs_daemon_open_readonly(); diff --git a/src/xen/xs_internal.h b/src/xen/xs_internal.h index 84d0d29..29f0165 100644 --- a/src/xen/xs_internal.h +++ b/src/xen/xs_internal.h @@ -29,7 +29,7 @@ extern struct xenUnifiedDriver xenStoreDriver; int xenStoreInit (void); -virDrvOpenStatus xenStoreOpen (virConnectPtr conn, +int xenStoreOpen (virConnectPtr conn, virConnectAuthPtr auth, unsigned int flags);

int xenStoreClose (virConnectPtr conn);