5:35 a.m.
The virConnectOpen method is unfortuantely rather special. While there is
a virConnect object available, the current rule is that drivers must report
errors against the global error object, because upon failure no virConnect
object will be returned to the caller. Unforatunately, despite frequent
cleanups of code getting this wrong, I've just audited the remote driver
and found another 20 or so places where its wrong. This is clearly not a
sustainable approach to error reporting.
The guarenteed correct solution is actually rather simple
- Always report errors against the virConnect object, even in the driver
open method
- In the cleanup patch of do_open() in libvirt.c, if no global error is
set, copy the error from the virConnect object's virError, into the
global virError.
With this in place we can change all the drivers back to always report
against the error object, and thus cleanup some disgusting code like
__virRaiseError (in_open ? NULL : conn, ...
To just
__virRaiseError (conn, ...
Daniel
Index: src/libvirt.c
===================================================================
RCS file: /data/cvs/libvirt/src/libvirt.c,v
retrieving revision 1.151
diff -u -r1.151 libvirt.c
--- src/libvirt.c 18 Aug 2008 09:24:46 -0000 1.151
+++ src/libvirt.c 19 Aug 2008 10:29:51 -0000
@@ -735,10 +735,8 @@
name = "xen:///";
ret = virGetConnect();
- if (ret == NULL) {
- virLibConnError(NULL, VIR_ERR_NO_MEMORY, _("allocating connection"));
+ if (ret == NULL)
return NULL;
- }
uri = xmlParseURI (name);
if (!uri) {
@@ -839,7 +837,21 @@
failed:
if (ret->driver) ret->driver->close (ret);
if (uri) xmlFreeURI(uri);
- virUnrefConnect(ret);
+
+ /* If not global error was set, copy any error set
+ in the connection object we're about to dispose of */
+ if (__lastErr.code == VIR_ERR_OK) {
+ memcpy(&__lastErr, &ret->err, sizeof(ret->err));
+ memset(&ret->err, 0, sizeof(ret->err));
+ }
+
+ /* Still no error set, then raise a generic error */
+ if (__lastErr.code == VIR_ERR_OK)
+ virLibConnError (NULL, VIR_ERR_INTERNAL_ERROR,
+ _("unable to open connection"));
+
+ virUnrefConnect(ret);
+
return NULL;
}
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
8:42 a.m.
On Tue, Aug 19, 2008 at 11:35:18AM +0100, Daniel P. Berrange wrote:
The virConnectOpen method is unfortuantely rather special. While
there is
a virConnect object available, the current rule is that drivers must report
errors against the global error object, because upon failure no virConnect
object will be returned to the caller. Unforatunately, despite frequent
cleanups of code getting this wrong, I've just audited the remote driver
and found another 20 or so places where its wrong. This is clearly not a
sustainable approach to error reporting.
The guarenteed correct solution is actually rather simple
- Always report errors against the virConnect object, even in the driver
open method
- In the cleanup patch of do_open() in libvirt.c, if no global error is
set, copy the error from the virConnect object's virError, into the
global virError.
With this in place we can change all the drivers back to always report
against the error object, and thus cleanup some disgusting code like
__virRaiseError (in_open ? NULL : conn, ...
To just
__virRaiseError (conn, ...
Okay, make sense, it's better to be more flexible in one location
and simplify code everywhere else.
+1
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
8:22 a.m.
On Tue, Aug 19, 2008 at 11:35:18AM +0100, Daniel P. Berrange wrote:
The guarenteed correct solution is actually rather simple
- Always report errors against the virConnect object, even in the driver
open method
- In the cleanup patch of do_open() in libvirt.c, if no global error is
set, copy the error from the virConnect object's virError, into the
global virError.
+1 , although unfortunately this isn't thread-safe. Nothing can be
thread-safe unless we change the API.
Rich.
--
Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones
virt-top is 'top' for virtual machines. Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://et.redhat.com/~rjones/virt-top
1:24 p.m.
On Wed, Aug 20, 2008 at 02:22:58PM +0100, Richard W.M. Jones wrote:
On Tue, Aug 19, 2008 at 11:35:18AM +0100, Daniel P. Berrange wrote:
> The guarenteed correct solution is actually rather simple
>
> - Always report errors against the virConnect object, even in the driver
> open method
>
> - In the cleanup patch of do_open() in libvirt.c, if no global error is
> set, copy the error from the virConnect object's virError, into the
> global virError.
+1 , although unfortunately this isn't thread-safe. Nothing can be
thread-safe unless we change the API.
Well we're not making it any less thread-safe. You alrady have to use the
virGetLastError() global func - we're simply making sure it actually
records all errors.
To make it thread-safe we'll need to add a real virGetThreadLastError()
API, which is something on my todo list - with that new apps can just
call thevirGetThreadLastError() exclusively and never need to know the
distinction between global/connection errors which causes so much
trouble. I'm fairly sure I can preseve existing semantics at same
time with some suitable internal cleverness.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
1:35 a.m.
On Wed, Aug 20, 2008 at 07:24:59PM +0100, Daniel P. Berrange wrote:
On Wed, Aug 20, 2008 at 02:22:58PM +0100, Richard W.M. Jones wrote:
> On Tue, Aug 19, 2008 at 11:35:18AM +0100, Daniel P. Berrange wrote:
> > The guarenteed correct solution is actually rather simple
> >
> > - Always report errors against the virConnect object, even in the driver
> > open method
> >
> > - In the cleanup patch of do_open() in libvirt.c, if no global error is
> > set, copy the error from the virConnect object's virError, into the
> > global virError.
>
> +1 , although unfortunately this isn't thread-safe. Nothing can be
> thread-safe unless we change the API.
Well we're not making it any less thread-safe. You alrady have to use the
virGetLastError() global func - we're simply making sure it actually
records all errors.
To make it thread-safe we'll need to add a real virGetThreadLastError()
API, which is something on my todo list - with that new apps can just
call thevirGetThreadLastError() exclusively and never need to know the
distinction between global/connection errors which causes so much
trouble. I'm fairly sure I can preseve existing semantics at same
time with some suitable internal cleverness.
I really wished we could avoid thread local storage mess, and in
general anything having to do with API exported global variables. In
general (I mean for the vast majority of the userland code dealing with
libvirt) there is always a domain or connection object where we can plug
the error, and provide it in-context. The only exception is the
connection creation, maybe this means we need to provide a better entry
point for this, but I would really prefer to not expose the notion of
thread at the API level.
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
3:41 a.m.
On Thu, Aug 21, 2008 at 08:35:25AM +0200, Daniel Veillard wrote:
On Wed, Aug 20, 2008 at 07:24:59PM +0100, Daniel P. Berrange wrote:
> On Wed, Aug 20, 2008 at 02:22:58PM +0100, Richard W.M. Jones wrote:
> > On Tue, Aug 19, 2008 at 11:35:18AM +0100, Daniel P. Berrange wrote:
> > > The guarenteed correct solution is actually rather simple
> > >
> > > - Always report errors against the virConnect object, even in the driver
> > > open method
> > >
> > > - In the cleanup patch of do_open() in libvirt.c, if no global error is
> > > set, copy the error from the virConnect object's virError, into the
> > > global virError.
> >
> > +1 , although unfortunately this isn't thread-safe. Nothing can be
> > thread-safe unless we change the API.
>
> Well we're not making it any less thread-safe. You alrady have to use the
> virGetLastError() global func - we're simply making sure it actually
> records all errors.
>
> To make it thread-safe we'll need to add a real virGetThreadLastError()
> API, which is something on my todo list - with that new apps can just
> call thevirGetThreadLastError() exclusively and never need to know the
> distinction between global/connection errors which causes so much
> trouble. I'm fairly sure I can preseve existing semantics at same
> time with some suitable internal cleverness.
I really wished we could avoid thread local storage mess, and in
general anything having to do with API exported global variables. In
general (I mean for the vast majority of the userland code dealing with
libvirt) there is always a domain or connection object where we can plug
the error, and provide it in-context. The only exception is the
connection creation, maybe this means we need to provide a better entry
point for this, but I would really prefer to not expose the notion of
thread at the API level.
Well you see I want to be able to use the same virConnectPtr object
from multiple threads. I've looked at how todo this and it actually
won't be very hard once we have a way to report thread-local errors.
Re-using a single virConnectPtr object is important because when talking
to a remote libvirtd instance, you don't want to have to open multiple
connections for each thread in your app, because that will require the
user to re-authenticate multiple times (or for the app to store your
credentials - not cool)
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
4:30 a.m.
On Thu, Aug 21, 2008 at 09:41:12AM +0100, Daniel P. Berrange wrote:
On Thu, Aug 21, 2008 at 08:35:25AM +0200, Daniel Veillard wrote:
> > To make it thread-safe we'll need to add a real virGetThreadLastError()
> > API, which is something on my todo list - with that new apps can just
> > call thevirGetThreadLastError() exclusively and never need to know the
> > distinction between global/connection errors which causes so much
> > trouble. I'm fairly sure I can preseve existing semantics at same
> > time with some suitable internal cleverness.
>
> I really wished we could avoid thread local storage mess, and in
> general anything having to do with API exported global variables. In
> general (I mean for the vast majority of the userland code dealing with
> libvirt) there is always a domain or connection object where we can plug
> the error, and provide it in-context. The only exception is the
> connection creation, maybe this means we need to provide a better entry
> point for this, but I would really prefer to not expose the notion of
> thread at the API level.
Well you see I want to be able to use the same virConnectPtr object
from multiple threads. I've looked at how todo this and it actually
won't be very hard once we have a way to report thread-local errors.
not very hard ??? You expect to put a global lock in the connection
The threading problem is a pandora box, once you provide the
functionality people will use it in ways you didn't expect. You will see
things like people closing the connection in one thread while another
thread is still using it. I don't think you can catch all scenarios
so i'm quite worried about suggesting it will be usable and then facing
undebuggable scenarios coming from lost users.
Re-using a single virConnectPtr object is important because when
talking
to a remote libvirtd instance, you don't want to have to open multiple
connections for each thread in your app, because that will require the
user to re-authenticate multiple times (or for the app to store your
credentials - not cool)
you keep one thread per connection. You will have to lock anyway,
it's not like you will gain anything by trying to parallelize on a
single connection. I really don't see what you're trying to do. Even
entry points without side effect will need some kind of locking.
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
4:42 a.m.
On Thu, Aug 21, 2008 at 11:30:11AM +0200, Daniel Veillard wrote:
On Thu, Aug 21, 2008 at 09:41:12AM +0100, Daniel P. Berrange wrote:
> On Thu, Aug 21, 2008 at 08:35:25AM +0200, Daniel Veillard wrote:
> > > To make it thread-safe we'll need to add a real
virGetThreadLastError()
> > > API, which is something on my todo list - with that new apps can just
> > > call thevirGetThreadLastError() exclusively and never need to know the
> > > distinction between global/connection errors which causes so much
> > > trouble. I'm fairly sure I can preseve existing semantics at same
> > > time with some suitable internal cleverness.
> >
> > I really wished we could avoid thread local storage mess, and in
> > general anything having to do with API exported global variables. In
> > general (I mean for the vast majority of the userland code dealing with
> > libvirt) there is always a domain or connection object where we can plug
> > the error, and provide it in-context. The only exception is the
> > connection creation, maybe this means we need to provide a better entry
> > point for this, but I would really prefer to not expose the notion of
> > thread at the API level.
>
> Well you see I want to be able to use the same virConnectPtr object
> from multiple threads. I've looked at how todo this and it actually
> won't be very hard once we have a way to report thread-local errors.
not very hard ??? You expect to put a global lock in the connection
The threading problem is a pandora box, once you provide the
functionality people will use it in ways you didn't expect. You will see
things like people closing the connection in one thread while another
thread is still using it. I don't think you can catch all scenarios
so i'm quite worried about suggesting it will be usable and then facing
undebuggable scenarios coming from lost users.
Well, ok so let me step back a bit because there's actually several
layers to this issue...
- The libvirtd is single threaded. This was reasonable at first, but
some of our APIs take a long time to complete, so we need to have
libvirtd parallelized.
- The QEMU/LXC drivers have state stored in the daemon, so even if
you merely allow libvirtd to parallelize by putting long running
connections in the 'background' you ned to have locking in the
QEMU/LXC drivers.
- Given that we need to have locking anyway, might as well just make
the libvirtd daemon properly multi-threaded, rather than trying to
have a multi-request queuing system.
- Once you've done all that, then solving the virError thread-local
issue is all that remains to allow virConnect to be accessed by
multiple threads
We need to do 1->3 in order to make libvirtd more scalable. At which
point 4 becomes 'not very hard' :-)
That said, there is one other option I've thought about instead of
doing step 4, we could add a 'virConnectDup' method. This would take
an existing connection object, and create an independant copy of it
that another thread could use. The key would be that the internal
driver would automagically pass authentication credentials somehow,
so you wouldn't have to re-authenticate the 2nd connection. For the
remote driver, this could be as simple as invoking an RPC call to
fetch a one-time key, and passing that to the 2nd connection to use
as their authentication credential.
> Re-using a single virConnectPtr object is important because when
talking
> to a remote libvirtd instance, you don't want to have to open multiple
> connections for each thread in your app, because that will require the
> user to re-authenticate multiple times (or for the app to store your
> credentials - not cool)
you keep one thread per connection. You will have to lock anyway,
it's not like you will gain anything by trying to parallelize on a
single connection. I really don't see what you're trying to do. Even
entry points without side effect will need some kind of locking.
The libvirt APIs are in fact very parallelizable. There are very few times
you need to access state on the virConnectPtr object - most of the time
we're dealing with virDomainPtr objects. So you merely need to lock the
connection, while you obtain a lock on the virDomainPtr object, then
you can release the connection lock. So the connection lock is held for
virtually no time at all. I reckon it'll parallelize very well. But its
probably not worth getting into this debate unless I've actually got
some proof of concept code to show, which is probably quite a long way
out timewise...
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
5:04 a.m.
On Thu, Aug 21, 2008 at 10:42:51AM +0100, Daniel P. Berrange wrote:
Well, ok so let me step back a bit because there's actually
several
layers to this issue...
- The libvirtd is single threaded. This was reasonable at first, but
some of our APIs take a long time to complete, so we need to have
libvirtd parallelized.
- The QEMU/LXC drivers have state stored in the daemon, so even if
you merely allow libvirtd to parallelize by putting long running
connections in the 'background' you ned to have locking in the
QEMU/LXC drivers.
- Given that we need to have locking anyway, might as well just make
the libvirtd daemon properly multi-threaded, rather than trying to
have a multi-request queuing system.
- Once you've done all that, then solving the virError thread-local
issue is all that remains to allow virConnect to be accessed by
multiple threads
We need to do 1->3 in order to make libvirtd more scalable. At which
point 4 becomes 'not very hard' :-)
That said, there is one other option I've thought about instead of
doing step 4, we could add a 'virConnectDup' method. This would take
an existing connection object, and create an independant copy of it
that another thread could use. The key would be that the internal
driver would automagically pass authentication credentials somehow,
so you wouldn't have to re-authenticate the 2nd connection. For the
remote driver, this could be as simple as invoking an RPC call to
fetch a one-time key, and passing that to the 2nd connection to use
as their authentication credential.
Actually the one-time key idea wouldn't work because that wouldn't let
us negotiate the session encryption, which is done by SASL at the time
we authenticate the client. Rich suggested perhaps sharing a single
TCP connection and multiplexing messages on it - we have a serial number
with every message/reply so this is doable in theory. It'd require some
fun code to hand-off replies between the virConnect objects sharing the
connection, but might be more viable in general.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
2:15 a.m.
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
The virConnectOpen method is unfortuantely rather special. While
there is
a virConnect object available, the current rule is that drivers must report
errors against the global error object, because upon failure no virConnect
object will be returned to the caller. Unforatunately, despite frequent
cleanups of code getting this wrong, I've just audited the remote driver
and found another 20 or so places where its wrong. This is clearly not a
sustainable approach to error reporting.
The guarenteed correct solution is actually rather simple
- Always report errors against the virConnect object, even in the driver
open method
- In the cleanup patch of do_open() in libvirt.c, if no global error is
set, copy the error from the virConnect object's virError, into the
global virError.
With this in place we can change all the drivers back to always report
against the error object, and thus cleanup some disgusting code like
__virRaiseError (in_open ? NULL : conn, ...
To just
__virRaiseError (conn, ...
Looks good, indeed.
ACK
Index: src/libvirt.c
===================================================================
...
+ /* If not global error was set, copy any error set
s/not/no/
5939
days inactive
5939
days old
9 comments
4 participants
participants (4)
-
Daniel P. Berrange -
Daniel Veillard -
Jim Meyering -
Richard W.M. Jones