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

[Zhao Liu <zhao1.liu@intel.com>]

> > +    /// Equivalent of the C function `error_propagate`.  Fill `*errp`
> 
> Uh, is it?  Let's see...
> 
> > +    /// with the information container in `self` if `errp` is not NULL;
> > +    /// then consume it.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// `errp` must be a valid argument to `error_propagate`;
> 
> Reminder for later: the valid @errp arguments for C error_propagate()
> are
> 
> * NULL
> 
> * &error_abort
> 
> * &error_fatal
> 
> * Address of some Error * variable containing NULL
> 
> * Address of some Error * variable containing non-NULL
> 
> The last one is *not* valid with error_setg().
> 
> > +    /// typically it 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) {
> 
> Reminder, just to avoid confusion: C error_propagate() has the arguments
> in the opposite order.
> 
> > +        if errp.is_null() {
> > +            return;
> > +        }
> > +
> > +        let err = self.clone_to_foreign_ptr();
> > +
> > +        // SAFETY: caller guarantees errp is valid
> > +        unsafe {
> > +            errp.write(err);
> > +        }
> > +    }
> 
> In C, we have two subtly different ways to store into some Error **errp
> argument: error_setg() and error_propagate().
> 
> Their obvious difference is that error_setg() creates the Error object
> to store, while error_propagate() stores an existing Error object if
> any, else does nothing.
> 
> Their unobvious difference is behavior when the destination already
> contains an Error.  With error_setg(), this must not happen.
> error_propagate() instead throws away the new error.  This permits
> "first one wins" error accumulation.  Design mistake if you ask me.
> 
> Your Rust propagate() also stores an existing bindings::Error.  Note
> that "else does nothing" doesn't apply, because we always have an
> existing error object here, namely @self.  In the error_propagate() camp
> so far.
> 
> Let's examine the other aspect: how exactly "storing" behaves.
> 
> error_setg() according to its contract:
> 
>     If @errp is NULL, the error is ignored.  [...]
> 
>     If @errp is &error_abort, print a suitable message and abort().
> 
>     If @errp is &error_fatal, print a suitable message and exit(1).
> 
>     If @errp is anything else, *@errp must be NULL.
> 
> error_propagate() according to its contract:
> 
>     [...] if @dst_errp is NULL, errors are being ignored.  Free the
>     error object.
> 
>     Else, if @dst_errp is &error_abort, print a suitable message and
>     abort().
> 
>     Else, if @dst_errp is &error_fatal, print a suitable message and
>     exit(1).
> 
>     Else, if @dst_errp already contains an error, ignore this one: free
>     the error object.
> 
>     Else, move the error object from @local_err to *@dst_errp.
> 
> The second to last clause is where its storing differs from
> error_setg().
> 
> What does errp.write(err) do?  I *guess* it simply stores @err in @errp.
> Matches neither behavior.
> 
> If that's true, then passing &error_abort or &error_fatal to Rust does
> not work, and neither does error accumulation.  Not equivalent of C
> error_propagate().

I did some simple tests. yes, &error_abort or &error_fatal doesn't work.
Current @errp of realize() can work because @errp points to @local_err
in device_set_realized().

> Is "propagate" semantics what you want here?
> 
> If not, use another name.

I guess here we should call C version's error_propagate() instead of
write():

diff --git a/rust/qemu-api/src/error.rs b/rust/qemu-api/src/error.rs
index a91ce6fefaf4..56622065ad22 100644
--- a/rust/qemu-api/src/error.rs
+++ b/rust/qemu-api/src/error.rs
@@ -205,7 +205,7 @@ pub unsafe fn propagate(self, errp: *mut *mut bindings::Error) {

         // SAFETY: caller guarantees errp is valid
         unsafe {
-            errp.write(err);
+            bindings::error_propagate(errp, err);
         }
     }

---

Then Rust's propagate has the same behavior as C (Of course, here Rust
is actually using C's error_propagate, so the two are equivalent.)

Regards,
Zhao