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

2013-05-08 Thread Daniel P. Berrange 
On Fri, May 03, 2013 at 04:12:29PM -0600, Jim Fehlig wrote:
 Daniel P. Berrange wrote:

  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?

The xenInotifyDriver table had one callback in it - the 'close' method.
Since we directly call xenInotifyClose, we don't need this in the
driver table anymore.

   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'?

Yep, ok.

  +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 :).

Hah, yes, search + replace gone too far.

 
  +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.

I've audited the xenInotifyOpen method and the only reasons
it would return -1 are all genuine fatal errors which we
should having been honouring.


Regards,
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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2013-05-03 Thread Jim Fehlig 
Daniel P. Berrange wrote:
 From: Daniel P. Berrange berra...@redhat.com

 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 berra...@redhat.com
 ---
  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;
 -}

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

2013-05-02 Thread Daniel P. Berrange 
From: Daniel P. Berrange berra...@redhat.com

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 berra...@redhat.com
---
 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)
+