Re: [PATCH 06/14] rust: qemu-api: add bindings to Error

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Provide an implementation of std::error::Error that bridges the Rust
> anyhow::Error and std::panic::Location types with QEMU's Error*.
>
> It also has several utility methods, analogous to error_propagate(),
> that convert a Result into a return value + Error** pair.  One important
> difference is that these propagation methods *panic* if *errp is NULL,
> unlike error_propagate() which eats subsequent errors[1].  The reason
> for this is that in C you have an error_set*() call at the site where
> the error is created, and calls to error_propagate() are relatively rare.
>
> In Rust instead, even though these functions do "propagate" a
> qemu_api::Error into a C Error**, there is no error_setg() anywhere that
> could check for non-NULL errp and call abort().  error_propagate()'s
> behavior of ignoring subsequent errors is generally considered weird,
> and there would be a bigger risk of triggering it from Rust code.
>
> [1] This is actually a violation of the preconditions of error_propagate(),
>     so it should not happen.  But you never know...
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

[...]

> diff --git a/rust/qemu-api/src/error.rs b/rust/qemu-api/src/error.rs
> new file mode 100644
> index 00000000000..80157f6ea1b
> --- /dev/null
> +++ b/rust/qemu-api/src/error.rs

[...]

> +    /// Equivalent of the C function `error_propagate`.  Fill `*errp`
> +    /// with the information container in `self` if `errp` is not NULL;
> +    /// then consume it.
> +    ///
> +    /// This is similar to the C API `error_propagate`, but it panics if
> +    /// `*errp` is not `NULL`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `errp` must be a valid argument to `error_propagate`; it can be
> +    /// `NULL` or it can point to any of:
> +    /// * `error_abort`
> +    /// * `error_fatal`
> +    /// * a local variable of (C) type `Error *`

This local variable must contain NULL.

> +    ///
> +    /// Typically `errp` is received from C code and need not be
> +    /// checked further at the Rust↔C boundary.
> +    pub unsafe fn propagate(self, errp: *mut *mut bindings::Error) {
> +        if errp.is_null() {
> +            return;
> +        }
> +
> +        // SAFETY: caller guarantees that errp and *errp are valid
> +        unsafe {
> +            assert_eq!(*errp, ptr::null_mut());
> +            bindings::error_propagate(errp, self.clone_to_foreign_ptr());
> +        }
> +    }

[...]

With the comment tightened:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

The commit message and comment improvements are lovely!