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.
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
