Re: [libvirt] [PATCH 03/14] Refactor creation of lock manager plugins

On 12/11/2012 01:41 PM, Daniel P. Berrange wrote: > From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; > > Refactor virLockManagerPluginNew() so that the caller does > not need to pass in the config file path itself - just the > config directory and driver name. > > Fix QEMU to actually pass in a config file when creating the > default lock manager plugin, rather than NULL. > > Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; > --- > src/locking/lock_manager.c | 20 +++++++++++++++++--- > src/locking/lock_manager.h | 3 ++- > src/qemu/qemu_conf.c | 12 +++++------- > src/qemu/qemu_conf.h | 3 ++- > src/qemu/qemu_driver.c | 24 ++++++++++-------------- > 5 files changed, 36 insertions(+), 26 deletions(-) > @@ -136,6 +137,16 @@ virLockManagerPluginPtr virLockManagerPluginNew(const char *name, > virLockManagerPluginPtr plugin = NULL; > const char *moddir = getenv("LIBVIRT_LOCK_MANAGER_PLUGIN_DIR"); > char *modfile = NULL; > + char *configFile = NULL; > + > + VIR_DEBUG("name=%s driverName=%s configDir=%s flags=%x", > + name, driverName, NULLSTR(configDir), flags); This says configDir might be NULL,

> + > + if (virAsprintf(&configFile, "%s/%s-%s.conf", > + configDir, driverName, name) < 0) { but this unconditionally dereferences it. Which way did you mean to go? > @@ -708,16 +707,15 @@ qemuStartup(bool privileged, > } > VIR_FREE(rundir); > > - base = virGetUserConfigDirectory(); > - if (!base) > + if (!(qemu_driver->configBaseDir = virGetUserConfigDirectory())) > goto error; > - if (virAsprintf(&qemu_driver->libDir, "%s/qemu/lib", base) == -1) > + if (virAsprintf(&qemu_driver->libDir, "%s/qemu/lib", qemu_driver->configBaseDir) == -1) This makes for some long lines; is it worth wrapping at the ',' now that you are using a longer name? Or maybe worth declaring: const char *base = qemu_driver->configBaseDir; to avoid having to touch these lines?