On Tue, Dec 11, 2012 at 02:57:10PM -0700, Eric Blake wrote:
> On 12/11/2012 01:41 PM, Daniel P. Berrange wrote:
> > From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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,
The NULLSTR is a mistake - the original param allowed NULLs,
but the new one does not.
>
> > +
> > + 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?
Yep, I'll do that.
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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list