ImagingDataCommons / libdicom

C library for reading DICOM files
https://libdicom.readthedocs.io
MIT License
16 stars 7 forks source link

Provide a way to retrieve error messages from the library #17

Closed bgilbert closed 1 year ago

bgilbert commented 1 year ago

If a library operation fails, the caller knows from the NULL return value that the failure occurred, but doesn't have a way to obtain any further information. libdicom can log additional information to stderr, but there's no way for the caller to capture and report it via its own error-reporting mechanism.

Please provide a way for the caller to obtain further information about the failure. Ideally this would be integrated into the API, perhaps via a mechanism similar to GError. Alternatively, the library could allow configuring a logging callback, allowing the application to capture log messages. In the latter case, note that libdicom might be called by another library instead of the top-level application. Since another library can't safely set a global error handler, there'd need to be a mechanism for setting and restoring a thread-local handler.

hackermd commented 1 year ago

A DcmError type with functionality similar to GError would be an elegant solution in my opinion. Since many developers are already familiar with the glib API, following its mechanism for error handling is probably a good idea.

hackermd commented 1 year ago

@jcupitt as an afterthought, I would have probably preferred DcmError error objects as last rather than first arguments for the following two reasons: a) GLib uses the same pattern for GError and many developers are thus probably familiar with it b) many of the libdicom functions operate on objects (opaque pointers for DcmDataSet, DcmElement, etc.) and it would be more intuitive in my opinion if these objects would be the first arguments (analogous to self in Python)

What were your design considerations for requiring the error object as first argument?

jcupitt commented 1 year ago

Yes, I hummed and haaahed about that choice.

Glib went for the final argument because they added GError to an existing API rather than designing it in. We don't really need to worry much about compatibility, so we get to choose.

libdicom has a few vararg API calls, so of course DcmError can't go last for those at least..

The DcmError is really part of the return value of the function, so eg.:

DCM_EXTERN
DcmElement *dcm_dataset_get_clone(DcmError **error,
                                  const DcmDataSet *dataset, uint32_t tag);

The DcmElement return type and the DcmError are next to each other, and both come back from the function.

You're right that the "this" pointer is usually first, but it's still the first of the non-return parameters.

hackermd commented 1 year ago

That's a really good point. Having a return parameter consistently at the first position makes a lot of sense.