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.
> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> index 6527146..482989f 100644
> --- a/src/qemu/qemu_cgroup.c
> +++ b/src/qemu/qemu_cgroup.c
> @@ -48,15 +48,20 @@ static const char *const defaultDeviceACL[] = {
> bool qemuCgroupControllerActive(virQEMUDriverPtr driver,
> int controller)
> {
> + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> + bool ret = false;
Missing empty line.
> if (driver->cgroup == NULL)
> - return false;
> + goto cleanup;
> if (controller < 0 || controller >= VIR_CGROUP_CONTROLLER_LAST)
> - return false;
> + goto cleanup;
> if (!virCgroupMounted(driver->cgroup, controller))
> - return false;
> - if (driver->cgroupControllers & (1 << controller))
> - return true;
> - return false;
> + goto cleanup;
> + if (cfg->cgroupControllers & (1 << controller))
> + ret = true;
> +
> +cleanup:
> + virObjectUnref(cfg);
> + return ret;
> }
>
> static int
...
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index f6273c1..ef04634 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
...
> @@ -3283,6 +3288,7 @@ qemuBuildHostNetStr(virDomainNetDefPtr net,
> virBuffer buf = VIR_BUFFER_INITIALIZER;
> enum virDomainNetType netType = virDomainNetGetActualType(net);
> const char *brname = NULL;
> + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>
> if (net->script && netType != VIR_DOMAIN_NET_TYPE_ETHERNET) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
This continues with
_("scripts are not supported on interfaces of type %s"),
virDomainNetTypeToString(netType));
return NULL;
}
and you would leak the reference to virQEMUDriverConfigPtr.
...
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index ee48333..46c1892 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
...
> @@ -70,68 +88,221 @@ void qemuDriverUnlock(virQEMUDriverPtr driver)
> }
>
>
> -int qemuLoadDriverConfig(virQEMUDriverPtr driver,
> - const char *filename) {
> - virConfPtr conf = NULL;
> - virConfValuePtr p;
> - char *user = NULL;
> - char *group = NULL;
> - int ret = -1;
> - int i;
> +virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged)
> +{
> + virQEMUDriverConfigPtr cfg;
> +
> + if (virQEMUConfigInitialize() < 0)
> + return NULL;
> +
> + if (!(cfg = virObjectNew(virQEMUDriverConfigClass)))
> + return NULL;
> +
> + cfg->privileged = privileged;
> + cfg->uri = privileged ? "qemu:///system" : "qemu:///session";
> +
> + 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->group = 0;
> + }
Looks like it would make sense to merge the two if statements above into
one.
> + cfg->dynamicOwnership = privileged ? true : false;
cfg->dynamicOwnership = privileged; would be enough, there are bools on
both sides.
...
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 18c4109..6c328d6 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
...
> @@ -1866,11 +1867,11 @@ qemuDomainChangeGraphics(virQEMUDriverPtr driver,
> STRNEQ_NULLABLE(olddev->data.vnc.auth.passwd,
> dev->data.vnc.auth.passwd)) {
> VIR_DEBUG("Updating password on VNC server %p %p",
> - dev->data.vnc.auth.passwd, driver->vncPassword);
> + dev->data.vnc.auth.passwd, cfg->vncPassword);
> ret = qemuDomainChangeGraphicsPasswords(driver, vm,
> VIR_DOMAIN_GRAPHICS_TYPE_VNC,
> &dev->data.vnc.auth,
> - driver->vncPassword);
> + cfg->vncPassword);
> if (ret < 0)
> return ret;
This one should be goto cleanup;
>
...
> @@ -1926,11 +1927,11 @@ qemuDomainChangeGraphics(virQEMUDriverPtr driver,
> STRNEQ_NULLABLE(olddev->data.spice.auth.passwd,
> dev->data.spice.auth.passwd)) {
> VIR_DEBUG("Updating password on SPICE server %p %p",
> - dev->data.spice.auth.passwd, driver->spicePassword);
> + dev->data.spice.auth.passwd, cfg->spicePassword);
> ret = qemuDomainChangeGraphicsPasswords(driver, vm,
> VIR_DOMAIN_GRAPHICS_TYPE_SPICE,
> &dev->data.spice.auth,
> - driver->spicePassword);
> + cfg->spicePassword);
>
> if (ret < 0)
> return ret;
And this one too.
...
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index a2ce007..d1d3d95 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
...
> @@ -3515,6 +3550,7 @@ int qemuProcessStart(virConnectPtr conn,
> char *nodeset = NULL;
> virBitmapPtr nodemask = NULL;
> unsigned int stop_flags;
> + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>
> /* Okay, these are just internal flags,
> * but doesn't hurt to check */
This continues as
virCheckFlags(VIR_QEMU_PROCESS_START_COLD |
VIR_QEMU_PROCESS_START_PAUSED |
VIR_QEMU_PROCESS_START_AUTODESROY, -1);
and you would leak virQEMUDriverConfigPtr reference.
...
Nice refactoring but quite hard to follow especially the part that
initializes qemu driver structure.
You gain bonus points for not breaking make check syntax-check :-)
Jirka
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list