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