Re: [libvirt] lock manager and libguestfs use (was: Re: RFC [2/3]: The lock manager plugin driver API)
On Wed, Nov 10, 2010 at 04:42:01PM +0000, Daniel P. Berrange wrote:
On Wed, Nov 10, 2010 at 04:17:13PM +0000, Richard W.M. Jones wrote:
> On Wed, Nov 10, 2010 at 03:52:49PM +0000, Daniel P. Berrange wrote:
> > 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'
>
> What sort of thing could 'foo' be?
>
> > entry point, because such use could be dangerous to safety.
>
> You should just rename the lock manager .so file in this case. But
> it'd be best to write an enduring minimum API and ABI now to avoid
> having to do this.
>
> > The
> > load_drv() method allows that to be handled. If load_drv() fails
> > libvirt/libguestfs *must* refuse to run any VMs.
>
> Well, libguestfs has other alternatives. Ignoring the lock manager is
> one, allowing read-only calls to proceed is another one.
Allowing read-only calls to continue isn't guarenteed to be safe.
The lock manager plugin may wish to deny even readonly usage, to
ensure that there aren't data consistency complication with NFS
close-to-open behaviour during migration. If a lock manager is
configured for use and it fails, not running any VMs is the only
safe thing, until the admin has fixed the config problem.
This is entirely separate from load_drv though. You can deny
read-only access when I try to acquire the resource.
What I'm wondering, and you're not answering, is what future event are
you anticipating which would make the outline API
(http://pastebin.ca/1987261) need to be completely overhauled? I
can't imagine it ...
> There are still issues with this. Multiple threads can call
> guestfs_launch (on different handles), resulting in reentrant calls to
> load_drv. I'm not convinced that the API as proposed is safe against
> this sort of thing:
>
> http://lists.fedoraproject.org/pipermail/devel/2010-January/129117.html
As mentioned below, these APIs are intended for doing global
are [not]?
state management, instead they are for version/feature
compatibility checks.
But back to my original point, if these fail I'm left SOL. This would
prevent libguestfs from working under nearly every condition, and all
I could suggest to users would be that something needs to be upgraded.
A basic minimum always-supported API is a must here, unless there is
genuinely some system out there that has such bizarre locking that we
would need to change how this works completely, and if there is such a
system perhaps we should consider it in the API now.
It isn't about the locking preventing unload. There are a
variety
of scenarios in which is it unsafe to unload *any* dlopen'd
library. eg, if the module has set a thread local variable with
a destructor, it is not safe to unload the module unless all
threads have terminated, or all thread locals cleaned up. The
module has no way to ensure this, so the only safe option is to
never dlclose() it, otherwise you'll get SIGBUS or SIGSEGV.
This isn't true. An API which allowed futures should also allow those
futures to be cancelled (from either the caller or callee side), and
it should cancel them in the __attribute__((destructor)). In any
case, the API proposed doesn't seem to have this.
> This is unfriendly to other callers ...
Unfortunately this is a no-win situation. Either the plugin impls
have to parse the XML, or the plugin callers have to. On balance
there are more plugin impls than callers, and one of the callers
already knows how to parse the XML, so I put the burden of XML
parsing on the caller rather than the plugin impl.
That doesn't make it friendly.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into Xen guests.
http://et.redhat.com/~rjones/virt-p2v