5:04 a.m.
These two functions are currently private to libvirt, hence not available to
consumers. Would it be possible to make them public? Without them, there's no
way to do any libvirt call without stomping on an existing error that you may
want to preserve.
I have multiple threads sharing a remote connection (to local libvirtd). I need
to be able to handle connection drops (e.g. libvirtd restart). Long story short,
the only approach I've found that actually works properly is that there's one
main conn object, each thread has a virConnectRef() to it, and whenever a thread
gets an error, we check in the error callback if !virConnectIsAlive(). If so, we
close the thread's conn, and potentially also clean up the main conn.
Then when the error gets returned from the relevant library call, we try to
re-connect.
However, virConnectIsAlive() stomps on the thread's error, meaning we lose the
context on a real, non-connection-related error.
So I need to be able to save and restore in the error handler.
Given how much these are used inside libvirt code already, it feels likely it
would be useful in other public cases too.
Thoughts?
thnaks
john
3:17 a.m.
On 12/13/23 12:04, John Levon wrote:
These two functions are currently private to libvirt, hence not available to
consumers. Would it be possible to make them public? Without them, there's no
way to do any libvirt call without stomping on an existing error that you may
want to preserve.
I have multiple threads sharing a remote connection (to local libvirtd). I need
to be able to handle connection drops (e.g. libvirtd restart). Long story short,
the only approach I've found that actually works properly is that there's one
main conn object, each thread has a virConnectRef() to it, and whenever a thread
gets an error, we check in the error callback if !virConnectIsAlive(). If so, we
close the thread's conn, and potentially also clean up the main conn.
Then when the error gets returned from the relevant library call, we try to
re-connect.
However, virConnectIsAlive() stomps on the thread's error, meaning we lose the
context on a real, non-connection-related error.
So I need to be able to save and restore in the error handler.
Given how much these are used inside libvirt code already, it feels likely it
would be useful in other public cases too.
Thoughts?
Yeah, it makes sense. I can see it used also in cleanup code (just like
we do in our code). For instance:
void func()
{
...
if (virDomainCreate(dom) < 0)
goto cleanup;
...
cleanup:
/* save error here */
if (dom)
virDomainDestroy(dom);
if (conn)
virConnectClose(conn);
...
/* restore error here */
}
Trouble is that every public API resets the error (by design). And while
the error struct is public (which means users can write functions to
create its copy), we already do have functions for that. So let's just
expose them.
Michal
5:18 a.m.
On Thu, Dec 14, 2023 at 10:17:01 +0100, Michal Prívozník wrote:
On 12/13/23 12:04, John Levon wrote:
>
> These two functions are currently private to libvirt, hence not available to
> consumers. Would it be possible to make them public? Without them, there's no
> way to do any libvirt call without stomping on an existing error that you may
> want to preserve.
>
> I have multiple threads sharing a remote connection (to local libvirtd). I need
> to be able to handle connection drops (e.g. libvirtd restart). Long story short,
> the only approach I've found that actually works properly is that there's
one
> main conn object, each thread has a virConnectRef() to it, and whenever a thread
> gets an error, we check in the error callback if !virConnectIsAlive(). If so, we
> close the thread's conn, and potentially also clean up the main conn.
>
> Then when the error gets returned from the relevant library call, we try to
> re-connect.
>
> However, virConnectIsAlive() stomps on the thread's error, meaning we lose the
> context on a real, non-connection-related error.
>
> So I need to be able to save and restore in the error handler.
>
> Given how much these are used inside libvirt code already, it feels likely it
> would be useful in other public cases too.
>
> Thoughts?
>
Yeah, it makes sense. I can see it used also in cleanup code (just like
we do in our code). For instance:
void func()
{
...
if (virDomainCreate(dom) < 0)
goto cleanup;
...
cleanup:
/* save error here */
if (dom)
virDomainDestroy(dom);
if (conn)
virConnectClose(conn);
...
/* restore error here */
}
Trouble is that every public API resets the error (by design). And while
the error struct is public (which means users can write functions to
create its copy), we already do have functions for that. So let's just
expose them.
While I have no problems with virErrorPreserveLast() as that's just
syntax sugar on top of existing public functions, I don't think that
it's a good idea to expose virErrorRestore() as that gives the access to
previously unexposed virSetError(). This gives the users possibility to
set arbitrary errors due to the error struct being public as noted
above.
Saving an error to a custom error object should be enough and I don't
really see a point in making libvirt carry it around with the
possibility of it being overwritten at any point by another library
call.
Apps can then pass the object instance around as needed with more
explicit logic behind it.
6:03 a.m.
On Thu, Dec 14, 2023 at 12:18:27PM +0100, Peter Krempa wrote:
While I have no problems with virErrorPreserveLast() as that's
just
syntax sugar on top of existing public functions, I don't think that
it's a good idea to expose virErrorRestore() as that gives the access to
previously unexposed virSetError(). This gives the users possibility to
set arbitrary errors due to the error struct being public as noted
above.
This sounds very much like a "don't do that then" scenario.
Saving an error to a custom error object should be enough and I
don't
really see a point in making libvirt carry it around with the
possibility of it being overwritten at any point by another library
call.
I'm not very clear on your suggestion. If I can't restore the libvirt
error in the error callback, how would the eventual API call return have
the right error?
Are you suggesting I have to have custom logic to save the error to some
thread-local location, and change every single API call handling to look there,
ignoring the API return value and error object?
Or do you have a suggestion I'm missing that is more elegant than this?
I think it's pretty illustrative that the library itself uses these calls
everywhere.
regards
john
10:55 a.m.
On Thu, Dec 14, 2023 at 12:03:55 +0000, John Levon wrote:
On Thu, Dec 14, 2023 at 12:18:27PM +0100, Peter Krempa wrote:
> While I have no problems with virErrorPreserveLast() as that's just
> syntax sugar on top of existing public functions, I don't think that
> it's a good idea to expose virErrorRestore() as that gives the access to
> previously unexposed virSetError(). This gives the users possibility to
> set arbitrary errors due to the error struct being public as noted
> above.
This sounds very much like a "don't do that then" scenario.
> Saving an error to a custom error object should be enough and I don't
> really see a point in making libvirt carry it around with the
> possibility of it being overwritten at any point by another library
> call.
Please note that virErrorRestore() is used (almost [1]) exclusively
inside libvirt's library code, thus *before* the error itself gets
raised to the caller of a public libvirt API.
Also TBH I'm not a fan of the passing of errors via the thread local
variable because, while it's convenient in the simple cases, for any
more complicated cases it just creates headaches (even internally).
As in your original problem statement it also makes problems when you
want to associate the error itself with the function call that caused
it.
I'm not very clear on your suggestion. If I can't restore the
libvirt
error in the error callback, how would the eventual API call return have
the right error?
Are you suggesting I have to have custom logic to save the error to some
thread-local location, and change every single API call handling to look there,
ignoring the API return value and error object?
If you need any form of more complex logic in your application for error
reporting (ab)using libvirt's thread local variable to store the error
for other functions in your application will not bring too much
simplification of the logic itself.
Additionally care would still have to be taken to use e.g. libvirt's
cleanup functions which would still reset the thread local error.
Applications which wish to do delayed error reporting have the means to
do so by having their own instance of virError, either per process (as
virsh does), or any other granularity, including a thread local one if
they wish to do so.
To use such error reporting you can use virSaveLastError(), which
retrieves the proper error object and then pass it via your own
structures with known lifetimes and without the possibly surprising
resetting of the error.
Using the global error handler function is not a very good idea in
multithreaded applications, so I'd really suggest to have a generic
handler function that you'll call after receiving a failure code from
any of the libvirt APIs you care about and that will preserve/raise the
error in your application.
Additionally for handling disconnects libvirt provides a connection
close callback registrable via virConnectRegisterCloseCallback(), which
can be registered multiple times with private data for each instance.
That might help you getting rid of the generic error handler's duty to
catch disconnects.
I understand that the error reporting situation is not ideal but I'm
still very sceptical that having the ability to reset the error would
help much.
Or do you have a suggestion I'm missing that is more elegant than
this?
I think it's pretty illustrative that the library itself uses these calls
everywhere.
[1] There is one use in virsh and one use in virt-admin code, which I'll
be addressing.
1:02 p.m.
On Thu, Dec 14, 2023 at 05:55:41PM +0100, Peter Krempa wrote:
If you need any form of more complex logic in your application for
error
reporting (ab)using libvirt's thread local variable to store the error
I'm a bit lost on the "abuse" claim here. Is this abuse in your eyes:
```
int saved_errno = errno;
close(fd);
errno = saved_errno;
```
since "errno" is libc's thread local variable that stores the error?
for other functions in your application will not bring too much
simplification of the logic itself.
Excuse the pseudocode:
```
int func() {
...
cleanup:
err = virErrorSaveLast(error);
virDomainFree(domain);
virErrorRestore(error);
return ret;
}
func2() {
ret = func();
fprintf(stderr, "%s\n", virGetLastErrorMessage());
```
versus:
```
int func() {
...
cleanup:
err = pthread_getspecific(global->pthread_key_libvirt_error);
*err = virSaveLastError();
virDomainFree(domain);
pthread_setspecific(global->pthread_key_libvirt_error, err);
return ret;
}
func2() {
ret = func();
err = pthread_getspecific(global->pthread_key_libvirt_error);
fprintf(stderr, "%s\n", err->message);
```
If your argument is that the second version is somehow better, I don't think
we're going to agree, and we'll just keep another local patch. Thanks for the
discussion!
Using the global error handler function is not a very good idea in
multithreaded applications
Side note: there's little point in me going into it, but been there, earned the
scars. Due to, for example, various connection failures showing up as
VIR_ERR_INTERNAL_ERROR, there's no reliable way to use the close callback with
multiple threads sharing a connection and correctly differentiating between a
need to reconnect and an actual API failure.
Although, it sounds like you know of a counter-example piece of code: is it
something you could share?
regards
john
3:32 a.m.
On Thu, Dec 14, 2023 at 19:02:56 +0000, John Levon wrote:
On Thu, Dec 14, 2023 at 05:55:41PM +0100, Peter Krempa wrote:
> If you need any form of more complex logic in your application for error
> reporting (ab)using libvirt's thread local variable to store the error
I'm a bit lost on the "abuse" claim here. Is this abuse in your eyes:
```
int saved_errno = errno;
close(fd);
errno = saved_errno;
```
since "errno" is libc's thread local variable that stores the error?
> for other functions in your application will not bring too much
> simplification of the logic itself.
Excuse the pseudocode:
```
int func() {
...
cleanup:
err = virErrorSaveLast(error);
virDomainFree(domain);
virErrorRestore(error);
return ret;
}
func2() {
ret = func();
fprintf(stderr, "%s\n", virGetLastErrorMessage());
```
The suggested approach for applications using libvirt is to have their
own virError instance, which is:
1) global (not suited for multithreaded apps, virsh uses this)
2) thread local
3) in a context object
This error instance is then updated only when an error is reported:
A) via the virSetErrorFunc callback (applicable to 1, 2)
B) via a local helper function to store the error after failed libvirt API call
This way, the resetting of the 'last error' object internal to libvirt
will not influence/stomp over the error saved inside the separate error
variable, thus removing the need to actually reset the error.
Option A) is a bit more automatic, as it doesn't require you to be
storing the error after each libvirt call. In case your application
needs to report original error and then invoke some APIs which may fail
to clean up this isn't ideal as you still have the same problem.
Graceful recovery using different code path can be done by simply
clearing the error.
Option B) requires a function call on each cleanup path from a failed
libvirt API but gives you the option to pick specific APIs to report
errors from.
In virsh we use 1) a global error object with A), errors being stored
inside the error callback:
You can have a look at:
vshErrorHandler, vshResetLibvirtError, and vshReportError
(You'll also find vshSaveLibvirtError, but that is for virsh's usage of
utility code from libvirt which doesn't raise the error via the error
handler. Those helpers are not available from the library)
versus:
```
int func() {
...
cleanup:
err = pthread_getspecific(global->pthread_key_libvirt_error);
*err = virSaveLastError();
virDomainFree(domain);
pthread_setspecific(global->pthread_key_libvirt_error, err);
return ret;
}
func2() {
ret = func();
err = pthread_getspecific(global->pthread_key_libvirt_error);
fprintf(stderr, "%s\n", err->message);
```
If your argument is that the second version is somehow better,
Putting the helpers into a function:
void
yourAppLibvirtErrorReset(void)
{
err = pthread_getspecific(global->pthread_key_libvirt_error);
if (*err)
virERrorFree(*err);
pthread_setspecific(global->pthread_key_libvirt_error, NULL);
}
void
yourAppLibvirtErrorStore(void)
{
if (virGetLastErrorCode() == VIR_ERR_OK)
return;
err = virSaveLastError();
yourAppLibvirtErrorReset();
pthread_setspecific(global->pthread_key_libvirt_error, err);
}
void
yourAppLibvirtErrorReport(void)
{
err = pthread_getspecific(global->pthread_key_libvirt_error);
fprintf(stderr, "%s\n", err->message);
yourAppLibvirtErrorReset();
}
your example then becomes:
```
int func() {
...
cleanup:
yourAppLibvirtErrorStore();
virDomainFree(domain);
return ret;
}
func2() {
ret = func();
yourAppLibvirtErrorReport();
```
Or, if you chose to call yourAppLibvirtErrorStore() from the error
callback:
```
int func() {
...
cleanup:
virDomainFree(domain);
return ret;
}
func2() {
ret = func();
yourAppLibvirtErrorReport();
```
I don't think
we're going to agree, and we'll just keep another local patch. Thanks for the
discussion!
> Using the global error handler function is not a very good idea in
> multithreaded applications
Side note: there's little point in me going into it, but been there, earned the
scars. Due to, for example, various connection failures showing up as
VIR_ERR_INTERNAL_ERROR, there's no reliable way to use the close callback with
multiple threads sharing a connection and correctly differentiating between a
need to reconnect and an actual API failure.
Could you please elaborate? This sounds like an actual bug we'd want to
fix. The connection close callback is supposed to be reliable because
e.g. migration recovery depends on it.
Although, it sounds like you know of a counter-example piece of code:
is it
something you could share?
See above for the pointers to virsh.
4:31 a.m.
On Fri, Dec 15, 2023 at 10:28:18 +0000, John Levon wrote:
On Fri, Dec 15, 2023 at 10:32:36AM +0100, Peter Krempa wrote:
> > Although, it sounds like you know of a counter-example piece of code: is it
> > something you could share?
>
> See above for the pointers to virsh.
virsh is single threaded.
Yup, I wrote that in the trimmed part of the reply which also shows
example which should work for multi-threaded application.
4:54 a.m.
On Thu, Dec 14, 2023 at 05:55:41PM +0100, Peter Krempa wrote:
On Thu, Dec 14, 2023 at 12:03:55 +0000, John Levon wrote:
Using the global error handler function is not a very good idea in
multithreaded applications, so I'd really suggest to have a generic
handler function that you'll call after receiving a failure code from
any of the libvirt APIs you care about and that will preserve/raise the
error in your application.
Right, both virConnSetErrorFunc & virSetErrorFunc are functions
from day 1, that we effectively considered to be redundant once
we retrofitted thread local error reporting into libvirt in the
distant past. Pretty much every all just calls virSetErrorFunc
to disable the silly built-in default print-to-stderr behaviour
we unfortunately have.
Additionally for handling disconnects libvirt provides a connection
close callback registrable via virConnectRegisterCloseCallback(), which
can be registered multiple times with private data for each instance.
That might help you getting rid of the generic error handler's duty to
catch disconnects.
OpenStack uses this callback to trigger reconnect when a connection
dies, and I'd consider that good practice. It also executes in clean
stack context direct from the event loop, as opposed to the error
callbacks which can be triggered from completely arbitrary application
code stack context.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
4:58 a.m.
On Wed, Dec 13, 2023 at 11:04:54AM +0000, John Levon wrote:
These two functions are currently private to libvirt, hence not available to
consumers. Would it be possible to make them public? Without them, there's no
way to do any libvirt call without stomping on an existing error that you may
want to preserve.
I have multiple threads sharing a remote connection (to local libvirtd). I need
to be able to handle connection drops (e.g. libvirtd restart). Long story short,
the only approach I've found that actually works properly is that there's one
main conn object, each thread has a virConnectRef() to it, and whenever a thread
gets an error, we check in the error callback if !virConnectIsAlive(). If so, we
close the thread's conn, and potentially also clean up the main conn.
Is there a real need to call virConnectIsAlive synchronously after every
API error scenario ? That is going to be checking for libvirtd restart
both too frequently, and at the same time, not frequently enough - ie you
don't detect the libvirtd restart until after the next time you make an API
call
If you use the virConnectRegisterCloseCallback() method, you should get
notified of the connection drop at precisely the right time, every time,
regardless of whether you happen to be making an API call at the time.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
5:08 a.m.
New subject: close callback and multiple threads
On Fri, Dec 15, 2023 at 10:58:29AM +0000, Daniel P. Berrangé wrote:
If you use the virConnectRegisterCloseCallback() method, you should
get
notified of the connection drop at precisely the right time, every time,
regardless of whether you happen to be making an API call at the time.
That only gets called once, not for every thread sharing the connection,
so there is no way to clean up all the other threads.
regards
john
5:20 a.m.
On Wed, Dec 13, 2023 at 11:04:54AM +0000, John Levon wrote:
These two functions are currently private to libvirt, hence not available to
consumers. Would it be possible to make them public? Without them, there's no
way to do any libvirt call without stomping on an existing error that you may
want to preserve.
I have multiple threads sharing a remote connection (to local libvirtd). I need
to be able to handle connection drops (e.g. libvirtd restart). Long story short,
the only approach I've found that actually works properly is that there's one
main conn object, each thread has a virConnectRef() to it, and whenever a thread
gets an error, we check in the error callback if !virConnectIsAlive(). If so, we
close the thread's conn, and potentially also clean up the main conn.
I wonder if we should take the position that this behaviour is a libvirt
bug.
We declare that on return from an API call, the last error object reflects
any error that occurred in this API call.
Libvirt is executing the error callback and work done in the error callback
is breaking our API guarantee.
This broken API guarantee is not the app's fault, it is libvirt's fault for
not protecting itself against things the callback might do.
IOW, should we fix the virDispatchError function, to do the save/restore
dance either side of invoking the callback. This would aviod exposing any
new APIs, and make the app should "just work".
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
5:25 a.m.
On Fri, Dec 15, 2023 at 11:20:10AM +0000, Daniel P. Berrangé wrote:
Libvirt is executing the error callback and work done in the error
callback
is breaking our API guarantee.
This broken API guarantee is not the app's fault, it is libvirt's fault for
not protecting itself against things the callback might do.
IOW, should we fix the virDispatchError function, to do the save/restore
dance either side of invoking the callback. This would aviod exposing any
new APIs, and make the app should "just work".
This would certainly fix my scenario, yes, thanks.
We still have to deal with the fact that e.g. virDomainFree() cannot be used in
cleanup paths, but the workarounds there are generally not so bad.
regards
john
344
days inactive
346
days old
13 comments
4 participants
participants (4)
-
Daniel P. Berrangé -
John Levon -
Michal Prívozník -
Peter Krempa