On Wed, Nov 10, 2010 at 03:01:01PM +0000, Richard W.M. Jones wrote:
> [Resurrecting this old thread which has just been brought to my attention]
>
> On Fri, Sep 10, 2010 at 05:00:52PM +0100, Daniel P. Berrange wrote:
> >
> > typedef struct _virLockManagerDriver virLockManagerDriver;
> > typedef virLockManagerDriver *virLockManagerDriverPtr;
> >
> > /* Which callbacks are supported */
> > typedef enum {
> > VIR_LOCK_MANAGER_DRIVER_RESOURCES = (1 << 0),
> > VIR_LOCK_MANAGER_DRIVER_MIGRATE = (1 << 0),
> > VIR_LOCK_MANAGER_DRIVER_HOTPLUG = (1 << 1),
> > };
> >
> > struct _virLockManagerDriver {
> > /**
> > * @version: the newest implemented plugin ABI version
> > * @flags: the implemented plugin optional extras
> > */
> > unsigned int version;
> > unsigned int flags;
> >
> > /**
> > * load_drv:
> > * @version: the libvirt requested plugin ABI version
> > * @flags: the libvirt requested plugin optional extras
> > *
> > * Allow the plugin to validate the libvirt requested
> > * plugin version / flags. This allows it to reject
> > * use of versions which are too old. A plugin may
> > * be loaded multiple times, for different libvirt
> > * drivers
> > *
> > * Returns -1 if the requested version/flags were inadequate
> > */
> > int (*load_drv)(unsigned int version,
> > unsigned int flags);
> >
> > /**
> > * unload_drv:
> > *
> > * Called to release any resources prior to the plugin
> > * being unloaded from memory. Returns -1 to prevent
> > * plugin unloading.
> > */
> > int (*[un]load_drv)(void);
>
> There are various user-unfriendly aspects to this:
>
> (1) If load_drv fails, you've left me up the creek. What can I as a
> caller do? Tell the user to upgrade their software? Carry on
> regardless?
The load_drv() method there is to allow the plugin implementation
to block its use with a libvirtd that is too old. eg, say we add
a new API 'foo', and the plugin implements that API. It wants to
block its use in any libvirt which doesn't know about the 'foo'
entry point, because such use could be dangerous to safety. The
load_drv() method allows that to be handled. If load_drv() fails
libvirt/libguestfs *must* refuse to run any VMs.
> (2) Without compiler help it is hard to call load_drv/unload_drv at
> the right time in a thread-safe way.
In libvirt context they are loaded when the QEMU driver is
initialized in libvirtd which is still single threaded.
In libguestfs, you've no global "driver" state, so I'd
suggest it is easiest to just call load_drv before starting
the VM, and unload_drv after stopping it. The API contract
requires that a plugin needs to cope with being loaded
multiple times.
> (3) Having all the functions buried inside a structure means I cannot
> use ordinary techniques (ie. dlsym) to find out if a function is
> supported. Because I cannot use dlsym, I'm forced to use hard-coded
> ABI checks!
You're thinking only about libvirtd checking what a plugin
supports. The load_drv() method is about the *opposite*,
allowing the plugin to check what libvirtd supports. You
can't do that with dlsym, or __attribute__().
> I think this is better done using the techniques that ELF or any
> self-respecting dynamic loader already provides:
>
> Use __attribute__((constructor)) and __attribute__((destructor)) to
> automatically and safely start and stop the lock manager (solves (2)).
>
> Guarantee a minimum ABI in the first version (solves (1)).
In addition to the reasons already mentioned for constructor not
being able to replace load_drv, "destructor" can't replace the
unload_drv method. The unload_drv method is there to allow the
plugin to refuse to allow module unloading from the app.
The load/unload API entry points aren't really intended for one
time module state initialization. They are for doing compatibility
and safety checks, which require parameters & return values that
you can't do with constructors.
If a plugin does want todo global state initialization then it
can of course use __attribute__((constructor)) and
__attribute__((destructor)), and leave unload_drv/load_Drv to
be no-op
> Provide top-level functions in the lockmgr.so which correspond to
> functions I can call directly, ie:
>
> virLockManagerDoFoo (virLockManagerPtr *impl, int foo_arg);
>
> and let me find out if they are supported by calling dlsym (solves (1) & (3)).
>
> If you need to change an API, add another function:
>
> virLockManagerDoFoo2 (virLockManagerPtr *impl, int foo_arg,
> int oops_we_forgot_this_arg);
>
> As for the specifics, where is the lock manager DSO going to sit in
> the filesystem?
Not locked down yet, but likely $LIBDIR/libvirt/plugins/lock_manager/$NAME.so
>
> Thanks for explaining the specifics of this on IRC. It currently
> looks like this:
>
> impl->load_drv(LIBVIRT_LOCK_VERSION, 0);
>
> impl->new_drv(VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN, 0);
>
> impl->set_parameter_drv("id", 2);
> impl->set_parameter_drv("name", "foo-guest');
> impl->set_parameter_drv("uuid", "c7a2edbd-edaf-9455-926a-d65c16db1809");
>
> impl->acquire_resource(VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK,
> "/some/big/disk/path",
> 0);
>
> impl->acquire_resource(VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK,
> "/some/big/iso/file/path",
> VIR_LOCK_MANAGER_RESOURCE_READONLY);
>
>
> if (!(impl->startup_drv())
> ...cannot obtain lock, so bail out...
>
> ...run QEMU...
>
> impl->shutdown_drv
>
> The thing here is that I have to translate the XML into something that
> the lock manager wants. Why not let me just pass the XML? Or the
> virDomainPtr?
At the point where libvirt drivers invoke the plugin, there is no
virDomainPtr in available, only the internal objects virDomainDefPtr
which aren't something we want to expose since they're not ABI
stable. In addition from the context where the plugin executes, it
likely won't be able to use the libvirt APIs safely without causing
deadlock in libvirt drivers.
It isn't passing the XML because we didn't want the lock manager
implementations to all have to re-implement XML parsing, and again
the libvirt driver code doesn't have the XML around at this point.
Regards,
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list