Re: [pve-devel] [PATCH v3 proxmox 02/66] notify: preparation for the first endpoint plugin

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

Not an in-depth review, just an explanation:

On Tue, Jul 18, 2023 at 09:19:54AM +0200, Lukas Wagner wrote:
> Thanks for the review!
> 
> On 7/17/23 17:48, Maximiliano Sandoval wrote:
> > 
> > Lukas Wagner <l.wagner@proxmox.com> writes:
> > 
(...)
> > > +#[derive(Debug)]
> > > +pub enum Error {
> > > +    ConfigSerialization(Box<dyn StdError + Send + Sync + 'static>),

FYI you can leave out the `'static` in type definitions like this. It's
the only possible choice and therefor automatic ;-)

^ Here we have `Box<dyn StdError>`

...

> > > +    ConfigDeserialization(Box<dyn StdError + Send + Sync + 'static>),
> > > +    NotifyFailed(String, Box<dyn StdError + Send + Sync + 'static>),
> > > +    TargetDoesNotExist(String),
> > > +}
> > > +
> > > +impl Display for Error {
> > > +    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
> > > +        match self {
> > > +            Error::ConfigSerialization(err) => {
> > > +                write!(f, "could not serialize configuration: {err}")
> > > +            }
> > > +            Error::ConfigDeserialization(err) => {
> > > +                write!(f, "could not deserialize configuration: {err}")
> > > +            }
> > > +            Error::NotifyFailed(endpoint, err) => {
> > > +                write!(f, "could not notify via endpoint(s): {endpoint}: {err}")
> > > +            }
> > > +            Error::TargetDoesNotExist(target) => {
> > > +                write!(f, "notification target '{target}' does not exist")
> > > +            }
> > > +        }
> > > +    }
> > > +}
> > > +
> > > +impl StdError for Error {
> > > +    fn source(&self) -> Option<&(dyn StdError + 'static)> {
> > > +        match self {

^ Here, `self` is `&Self`

> > > +            Error::ConfigSerialization(err) => Some(&**err),
> > 
> > Does this really need the double deref?
> 
> Yeah, does not seem to work any way. Though I'm not sure I can fully explain why.
> Copied the approach from (I think) the TFA crate.

Since, as noted above, `self` is `&Self`, `err` here will be `&T`.
`T` is `Box<dyn StdError + Send + Sync>`.
So we have `&Box<dyn StdError + Send + Sync>`

The first deref goes to `Box<dyn StdError + Send + Sync>` via an immutable ref.
We cannot return this as this would be a moving expression and rust does
not implicitly dereference multiple times automatically.

The second deref calls `<Box as Deref>::deref` and implicitly
dereferences it (since 'Deref' is kind of "transparent" to the '*'
operator), from `&(dyn StdError + Send + Sync)` to `(dyn StdError + Send
+ Sync)`.

Finally we borrow -> `&**` :-)