Skip to content

Commit 9c15cd5

Browse files
authored
Explicitly call Context::set_last_error in ffi (#4195)
This adds a result extension trait to explicitly set the last error, which *should* be the default for the FFI. Currently not touching all APIs since that's potentially disruptive and we're close to a release. The logging story is messy, as described in the doc comment. We should further clean this up and tidy up these APIs so it's more obvious to people how to do the right thing.
1 parent 8302d22 commit 9c15cd5

File tree

1 file changed

+62
-1
lines changed

1 file changed

+62
-1
lines changed

deltachat-ffi/src/lib.rs

Lines changed: 62 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4166,6 +4166,8 @@ pub unsafe extern "C" fn dc_backup_provider_new(
41664166
})
41674167
.map(|ffi_provider| Box::into_raw(Box::new(ffi_provider)))
41684168
.log_err(ctx, "BackupProvider failed")
4169+
.context("BackupProvider failed")
4170+
.set_last_error(ctx)
41694171
.unwrap_or(ptr::null_mut())
41704172
}
41714173

@@ -4178,7 +4180,11 @@ pub unsafe extern "C" fn dc_backup_provider_get_qr(
41784180
return "".strdup();
41794181
}
41804182
let ffi_provider = &*provider;
4183+
let ctx = &*ffi_provider.context;
41814184
deltachat::qr::format_backup(&ffi_provider.provider.qr())
4185+
.log_err(ctx, "BackupProvider get_qr failed")
4186+
.context("BackupProvider get_qr failed")
4187+
.set_last_error(ctx)
41824188
.unwrap_or_default()
41834189
.strdup()
41844190
}
@@ -4195,6 +4201,9 @@ pub unsafe extern "C" fn dc_backup_provider_get_qr_svg(
41954201
let ctx = &*ffi_provider.context;
41964202
let provider = &ffi_provider.provider;
41974203
block_on(generate_backup_qr(ctx, &provider.qr()))
4204+
.log_err(ctx, "BackupProvider get_qr_svg failed")
4205+
.context("BackupProvider get_qr_svg failed")
4206+
.set_last_error(ctx)
41984207
.unwrap_or_default()
41994208
.strdup()
42004209
}
@@ -4209,7 +4218,9 @@ pub unsafe extern "C" fn dc_backup_provider_wait(provider: *mut dc_backup_provid
42094218
let ctx = &*ffi_provider.context;
42104219
let provider = &mut ffi_provider.provider;
42114220
block_on(provider)
4212-
.log_err(ctx, "Failed to join provider")
4221+
.log_err(ctx, "Failed to await BackupProvider")
4222+
.context("Failed to await BackupProvider")
4223+
.set_last_error(ctx)
42134224
.ok();
42144225
}
42154226

@@ -4262,6 +4273,56 @@ impl<T: Default, E: std::fmt::Display> ResultExt<T, E> for Result<T, E> {
42624273
}
42634274
}
42644275

4276+
trait ResultLastError<T, E>
4277+
where
4278+
E: std::fmt::Display,
4279+
{
4280+
/// Sets this `Err` value using [`Context::set_last_error`].
4281+
///
4282+
/// Normally each FFI-API *should* call this if it handles an error from the Rust API:
4283+
/// errors which need to be reported to users in response to an API call need to be
4284+
/// propagated up the Rust API and at the FFI boundary need to be stored into the "last
4285+
/// error" so the FFI users can retrieve an appropriate error message on failure. Often
4286+
/// you will want to combine this with a call to [`LogExt::log_err`].
4287+
///
4288+
/// Since historically calls to the `deltachat::log::error!()` macro were (and sometimes
4289+
/// still are) shown as error toasts to the user, this macro also calls
4290+
/// [`Context::set_last_error`]. It is preferable however to rely on normal error
4291+
/// propagation in Rust code however and only use this `ResultExt::set_last_error` call
4292+
/// in the FFI layer.
4293+
///
4294+
/// # Example
4295+
///
4296+
/// Fully handling an error in the FFI code looks like this currently:
4297+
///
4298+
/// ```no_compile
4299+
/// some_dc_rust_api_call_returning_result()
4300+
/// .log_err(&context, "My API call failed")
4301+
/// .context("My API call failed")
4302+
/// .set_last_error(&context)
4303+
/// .unwrap_or_default()
4304+
/// ```
4305+
///
4306+
/// As shows it is a shame the `.log_err()` call currently needs a message instead of
4307+
/// relying on an implicit call to the [`anyhow::Context`] call if needed. This stems
4308+
/// from a time before we fully embraced anyhow. Some day we'll also fix that.
4309+
///
4310+
/// [`Context::set_last_error`]: context::Context::set_last_error
4311+
fn set_last_error(self, context: &context::Context) -> Result<T, E>;
4312+
}
4313+
4314+
impl<T, E> ResultLastError<T, E> for Result<T, E>
4315+
where
4316+
E: std::fmt::Display,
4317+
{
4318+
fn set_last_error(self, context: &context::Context) -> Result<T, E> {
4319+
if let Err(ref err) = self {
4320+
context.set_last_error(&format!("{err:#}"));
4321+
}
4322+
self
4323+
}
4324+
}
4325+
42654326
trait ResultNullableExt<T> {
42664327
fn into_raw(self) -> *mut T;
42674328
}

0 commit comments

Comments
 (0)