Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
> 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
>
s/This/Thus/ ?
> be simplified to assume that they are always privileged.
>
> Signed-off-by: Daniel P. Berrange <berrange@xxxxxxxxxx>
> ---
> 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
>
Looks like this was never used, so just removing it right? But there are
a lot of loops in this file with 'drivers[i]->', where it might be
possible that i == XEN_UNIFIED_INOTIFY_OFFSET?
> };
>
> -#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;
>
Seems the name 'inside_daemon' is no longer appropriate. Should it be
something like 'is_privileged'?
> 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");
>
Maybe one too many instances of 'fail' replaced with 'error'? I think
"Failed to make capabilities" is better than "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;
>
The old code silently continued on if xenInotifyOpen() didn't return
success.
> + 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);
>
Can be condensed to one line now.
> 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);
>
Heh, different styles used throughout these files. This code has been
around for a looong time...
I'm out of time now and will have to continue with reviews next week.
Regards,
Jim
> int xenStoreClose (virConnectPtr conn);
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list