Re: [PATCH v3 01/13] Introduce a virQEMUDriverConfigPtr object

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Feb 01, 2013 at 09:20:25PM +0100, Jiri Denemark wrote:
> On Fri, Feb 01, 2013 at 11:18:23 +0000, Daniel P. Berrange wrote:
> > From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
> > 
> > Currently the virQEMUDriverPtr struct contains an wide variety
> > of data with varying access needs. Move all the static config
> > data into a dedicated virQEMUDriverConfigPtr object. The only
> > locking requirement is to hold the driver lock, while obtaining
> > an instance of virQEMUDriverConfigPtr. Once a reference is held
> > on the config object, it can be used completely lockless since
> > it is immutable.
> > 
> > NB, not all APIs correctly hold the driver lock while getting
> > a reference to the config object in this patch. This is safe
> > for now since the config is never updated on the fly. Later
> > patches will address this fully.
> > 
> ...
> >  15 files changed, 1099 insertions(+), 755 deletions(-)
> 
> Uff. I don't want to go through this patch more than once so please
> don't send v2, just send a diff between v1 and v2, which should be much
> easier to review.

Here are the changes I propose - should adress every point you
mention.

Daniel

diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 482989f..376a21d 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -50,6 +50,7 @@ bool qemuCgroupControllerActive(virQEMUDriverPtr driver,
 {
     virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
     bool ret = false;
+
     if (driver->cgroup == NULL)
         goto cleanup;
     if (controller < 0 || controller >= VIR_CGROUP_CONTROLLER_LAST)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index ef04634..fbc5481 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3294,6 +3294,7 @@ qemuBuildHostNetStr(virDomainNetDefPtr net,
         virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                        _("scripts are not supported on interfaces of type %s"),
                        virDomainNetTypeToString(netType));
+        virObjectUnref(cfg);
         return NULL;
     }
 
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 46c1892..454aa35 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -104,16 +104,13 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged)
     if (privileged) {
         if (virGetUserID(QEMU_USER, &cfg->user) < 0)
             goto error;
-    } else {
-        cfg->user = 0;
-    }
-    if (privileged) {
         if (virGetGroupID(QEMU_GROUP, &cfg->group) < 0)
             goto error;
     } else {
+        cfg->user = 0;
         cfg->group = 0;
     }
-    cfg->dynamicOwnership = privileged ? true : false;
+    cfg->dynamicOwnership = privileged;
 
     cfg->cgroupControllers =
         (1 << VIR_CGROUP_CONTROLLER_CPU) |
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 6c328d6..d2b717b 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1873,7 +1873,7 @@ qemuDomainChangeGraphics(virQEMUDriverPtr driver,
                                                     &dev->data.vnc.auth,
                                                     cfg->vncPassword);
             if (ret < 0)
-                return ret;
+                goto cleanup;
 
             /* Steal the new dev's  char * reference */
             VIR_FREE(olddev->data.vnc.auth.passwd);
@@ -1934,7 +1934,7 @@ qemuDomainChangeGraphics(virQEMUDriverPtr driver,
                                                     cfg->spicePassword);
 
             if (ret < 0)
-                return ret;
+                goto cleanup;
 
             /* Steal the new dev's char * reference */
             VIR_FREE(olddev->data.spice.auth.passwd);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index d1d3d95..bf97edd 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3550,7 +3550,7 @@ int qemuProcessStart(virConnectPtr conn,
     char *nodeset = NULL;
     virBitmapPtr nodemask = NULL;
     unsigned int stop_flags;
-    virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+    virQEMUDriverConfigPtr cfg;
 
     /* Okay, these are just internal flags,
      * but doesn't hurt to check */
@@ -3558,6 +3558,8 @@ int qemuProcessStart(virConnectPtr conn,
                   VIR_QEMU_PROCESS_START_PAUSED |
                   VIR_QEMU_PROCESS_START_AUTODESROY, -1);
 
+    cfg = virQEMUDriverGetConfig(driver);
+
     /* From now on until domain security labeling is done:
      * if any operation fails and we goto cleanup, we must not
      * restore any security label as we would overwrite labels


-- 
|: 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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list


[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]