Missing try/catch in some C API wrappers

[vsergeev]

The C API wrappers SoapySDRDevice_make() and SoapySDRDevice_makeStrArgs() are missing try/catch wrappers and can leak C++ exceptions to the calling C program, in cases when no compatible module is found or when a module constructor raises an exception. This results in abrupt termination of the calling C program.

This can be observed with the stock C API example:

$ ./example
terminate called after throwing an instance of 'std::runtime_error'
  what():  RTL-SDR device not found.
[1]    11386 abort (core dumped)  ./example
$

It looks like DeviceC.cpp addresses many of these cases with the __SOAPY_SDR_C_TRY and __SOAPY_SDR_C_CATCH macros, but these macros probably need to be applied more indiscriminately, as some seemingly benign module methods (like the RTL-SDR module's getGain()) can still raise an exception that would leak through.

If I get a chance this week, I'll try to make and PR the changes.

[guruofquality]

Thats a good point. The SoapySDRDevice_make() should definitely return null and set the error message in this case. Its a bit late, butSoapySDRDevice_setupStream() should probably work similarly by retuning a pointer which can be null. :-/

The try/catch macros arent exactly indiscriminate, its just that they are only found for setters, not getter calls. Most getter calls don't throw under regular operation. But its pretty easy to have an underlying layer throw when something disconnects, or perhaps a wrong name or key is used.

Without obliterating the API, some getters can be changed to return null on error. But many just return a bool, double, range... theres no good way to change that. It would be possible to put try/catches on all calls that simply log + set the error message when thrown, but dont actually indicate any error to the user through the arguments. In this case, the user could check the error message or null, or we could provide something like errno.

[vsergeev]

Yeah, sorry, by applying them indiscriminately, I meant that they should be everywhere, including getters and some other methods that don't have them yet (direct buffer access API, stream API, etc.), so there's no opportunity for an exception getting across the boundary.

But as you pointed out, this does present an issue for the getters. An errno-style solution would be ABI backwards compatible, but it still technically changes the usage for C programs moving forward to fully adopt it, so one could argue it's a different API. Personally, I would prefer if the direct getters got deprecated for a consistent return code API (like the setters use), rather than an errno-style one. It might be a little more verbose (and in some cases awkward with char ***), but it's bullet proof and more predictable than having both styles. Just my 2️⃣ cents.

[guruofquality]

c_error_work

Created a work branch to address some of this. This branch has an extra macro to deal with the catch with arbitrary return. The idea is to get this out as a fix that’s non breaking for 0.5 series (then break the API for 0.6!). There is no reason not to get try/catches on all of the calls that throw.

• Looks like Device.h still has some void setting calls that should have error codes as well.

noexcept
Some of the library calls do not throw by design and should be declared noexcept. Like module loading, device enumeration, device unmake. All of the stream calls that operate on an active stream object should be noexcept and already do return codes (this does not include setupStream). It was actually a design intention to make the stream calls based around error codes and to keep exceptions limited to configuration setter/getter calls. However, I'm afraid to add noexcept here because that breaks a lot of module inheritance code that would also have to add noexcept to match.

In terms of the C wrapper, this basically means that the noexcept calls dont need try/catches. But the others definitely should get try/catch.

API changes

An errno-style solution would be ABI backwards compatible, but it still technically changes the usage for C programs moving forward to fully adopt it, so one could argue it's a different API.

I guess the point is that the changes would be API backwards compatible as well, so its safe to do without breaking :-P Code that wasn't checking for errors, can continue to not check for errors.

It might be a little more verbose (and in some cases awkward with char ***), but it's bullet proof and more predictable than having both styles. Just my 2️⃣ cents.

Your right, that would be totally consistent. I also have this worry of making the code unwieldy at the same time.

• Setters return error codes (easy, done!)
• Calls that return pointers can set NULL on error (very C like, other APIs do this)
• Getters that return PODs are the issue. Treating them differently than pointer-return-calls creates this inconsistency. If this becomes status-return and pointer-for-output, its not going to be as ugly as char *** at least. But I was also thinking of how some of these calls can return an error through the POD:
  • bool - change to int, 0=false,1=true,-1=error
  • size_t - npos is error
  • double - use NaN for error
  • Range - use [NaN, NaN](ugly solution) (effects 2 calls)
  • Kwargs - npos length for error (effects 2 calls)
  • Totally not consistent, just thinking out loud...

[vsergeev]

Noice! Thanks for the quick turnaround. In the meanwhile, I've got the SoapySDRSource and SoapySDRSink via the C API working well in LuaRadio now, which should bring a lot more SDR support to the LuaRadio project.

I still think return codes (and NULL on pointers) on getters is better than sharing the error path with the values and checking for NaNs and -1 and members of structs and whatnot, but you've heard my vote already :-). Perhaps in a future version. Thanks for being receptive nonetheless.

[guruofquality]

Noice! Thanks for the quick turnaround. In the meanwhile, I've got the SoapySDRSource and SoapySDRSink via the C API working well in LuaRadio now, which should bring a lot more SDR support to the LuaRadio project.

Awesome news!

For an update:

The maint branch (pre 0.5.3 release) will have try/catches for all of the device calls, even the ones that should not throw. Some of the calls cant return a status, so there is an added SoapySDRDevice_lastStatus() that is cleared on entry to a device call and will be set on error. That's part of the reason for the try/catch macros on everything Device related, because they also clear the status. Think of lastStatus as akin to something like WSAGetLastError() for the device calls.

For the master branch, we have some minor API changes so far. Many of the remaining void return calls now return a integer status when possible. Some breaking API changes are planned for the next major release: removed deprecations, renames for some of the more obscure C calls, and general additions here and there.

I still think return codes (and NULL on pointers) on getters is better than sharing the error path with the values and checking for NaNs and -1 and members of structs and whatnot, but you've heard my vote already :-). Perhaps in a future version. Thanks for being receptive nonetheless.

Asking the user to actually check for NANs and other stuff like that is messy and basically doesnt work. I consider this the value that is set on return during a failure. But I'm hoping that in this case, the user can call lastStatus() in these cases if they care to follow through with the error checking safety.

I appreciate the feedback. I think I do need to break the API a little more :-) Just want to consider the trade-offs. Function overloads and memory management make it easier to not break things in C++, but I think some C apps might have to ifdef some code during the transition.

[vsergeev]

Thanks Josh! 👍