rust-for-linux.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* PointerWrapper Trait vs croe::ops::Deref
@ 2021-05-09 10:25 Finn Behrens
  2021-05-09 14:21 ` Miguel Ojeda
  0 siblings, 1 reply; 3+ messages in thread
From: Finn Behrens @ 2021-05-09 10:25 UTC (permalink / raw)
  To: Miguel Ojeda, wedsonaf; +Cc: rust-for-linux

Howdy,

As discussed in the meeting yesterday, here are my two ways I can think of to get the internals in a good way.

### 2 Traits
The first one is this, where there are 2 traits (we can rename them to also keep our old PointerWrapper).

```rs
const EFAULT: u8 = 14;

struct Internal {
    pub number: u16,
}

trait PointerWrapper {
    type Output;

    /// Returns the raw pointer.
    fn get_pointer(&self) -> *const Self::Output;

    /// Returns the instance back from the raw pointer.
    ///
    /// # Safety
    ///
    /// The passed pointer must come from a previous call to [`PointerWrapper::into_pointer()`].
    unsafe fn from_pointer(ptr: *const Self::Output) -> Self;
    
    /// Returns a reference to to the internal struct
    fn get_internal(&self) -> Result<&Self::Output, u8> {
        let ptr = self.get_pointer();
        
        unsafe { ptr.as_ref() }.ok_or(EFAULT)
    }
}

trait PointerWrapperMut: PointerWrapper {
    /// Returns a mutable referecne to the internal struct
    fn get_internal_mut(&mut self) -> Result<&mut Self::Output, u8> {
        let ptr = self.get_pointer() as *mut Self::Output;
        
        unsafe { ptr.as_mut() }.ok_or(EFAULT)
    }
} 

#[derive(Debug)]
struct Wrapper {
    ptr: *const Internal,
}

impl PointerWrapper for Wrapper {
    type Output = Internal;

    fn get_pointer(&self) -> *const Self::Output {
        self.ptr
    }
    
    unsafe fn from_pointer(ptr: *const Internal) -> Self {
        Self {
            ptr,
        }
    }
}

impl PointerWrapperMut for Wrapper { }

fn main() {
    let orig = Internal {
      number: 42,
    };
    
    let mut wrapper = unsafe { Wrapper::from_pointer(&orig as *const Internal) };
    
    println!("wrapper: {:?}", wrapper.get_internal().unwrap().number );
    
    wrapper.get_internal_mut().unwrap().number = 1337;
    
    println!("wrapper: {:?}", wrapper.get_internal().unwrap().number );
}
```

Link to playground: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=49a51f16e5202c84a05e4a8a56ee2fe3
This approach would of course not return `Result<&Self::Output, u8>` but instead an instance of `KernelResult`.
Maybe we could also remove the result, as it would possible be an invariant, and never happen as the pointer should already be valid.

### Using `core::ops::Deref{,mut}`

```rs
use core::ops::{Deref, DerefMut};

struct Internal {
    pub number: u16,
}

trait PointerWrapper<T> {
    /// Returns the raw pointer.
    fn get_pointer(&self) -> *const T;

    /// Returns the instance back from the raw pointer.
    ///
    /// # Safety
    ///
    /// The passed pointer must come from a previous call to [`PointerWrapper::into_pointer()`].
    unsafe fn from_pointer(ptr: *const T) -> Self;
}

#[derive(Debug)]
struct Wrapper {
    ptr: *const Internal,
}

impl Deref for Wrapper {
    type Target = Internal;
    
    fn deref(&self) -> &Self::Target {
        let ptr = self.get_pointer();
        
        unsafe { ptr.as_ref() }.unwrap()
    }
}

impl DerefMut for Wrapper {
    fn deref_mut(&mut self) -> &mut Self::Target {
        let ptr = self.get_pointer() as *mut Internal;
        
        unsafe { ptr.as_mut() }.unwrap()
    }
}

impl PointerWrapper<Internal> for Wrapper {
    fn get_pointer(&self) -> *const Internal {
        self.ptr
    }
    
     unsafe fn from_pointer(ptr: *const Internal) -> Self {
        Self {
            ptr,
        }
    }
}

fn main() {
    let orig = Internal {
      number: 42,
    };
    
    let mut wrapper = unsafe { Wrapper::from_pointer(&orig as *const Internal) };
    
    println!("wrapper: {:?}", wrapper.deref().number );
    
    wrapper.deref_mut().number = 1337;
    
    println!("wrapper: {:?}", wrapper.deref_mut().number );
}
```
https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=47944d314e293ab50093b2540fb3f9e5


Pleas tell me what version you would favour.

CU,
Finn


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: PointerWrapper Trait vs croe::ops::Deref
  2021-05-09 10:25 PointerWrapper Trait vs croe::ops::Deref Finn Behrens
@ 2021-05-09 14:21 ` Miguel Ojeda
  2021-05-10 14:04   ` Finn Behrens
  0 siblings, 1 reply; 3+ messages in thread
From: Miguel Ojeda @ 2021-05-09 14:21 UTC (permalink / raw)
  To: Finn Behrens; +Cc: Wedson Almeida Filho, rust-for-linux

On Sun, May 9, 2021 at 12:25 PM Finn Behrens <me@kloenk.de> wrote:
>
> Maybe we could also remove the result, as it would possible be an invariant, and never happen as the pointer should already be valid.

If it can fail, we cannot use `Deref` to begin with. So yeah, to make
the examples equivalent, the first one should not return a `Result`.

Now, assuming it cannot fail, what I mentioned in the meeting is that
with `Deref` all this compiles:

    wrapper.deref().number
    (*wrapper).number
    wrapper.number

The question boils down to: do we want this shortcut or not?

Having said that, I am not sure I understand the traits shown here.
Isn't `PointerWrapper` meant only to save a `Box<T>` and similar types
(which already implement `Deref` themselves etc.) as a pointer in a C
data structure? (which is why `into_pointer()` consumes the object).

In the examples shown, the new `get_pointer()` does not consume
`self`; and `main()` creates a `Wrapper` using `from_pointer()`. But
the idea is that you already have an object (constructed via other
means), transform it into a raw pointer, and then later on
re-construct the original object back from that pointer.

So `PointerWrapper` trait is like a `SavedAsPointer` trait, if you will.

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: PointerWrapper Trait vs croe::ops::Deref
  2021-05-09 14:21 ` Miguel Ojeda
@ 2021-05-10 14:04   ` Finn Behrens
  0 siblings, 0 replies; 3+ messages in thread
From: Finn Behrens @ 2021-05-10 14:04 UTC (permalink / raw)
  To: Miguel Ojeda; +Cc: Wedson Almeida Filho, rust-for-linux

Howdy,

> On 9. May 2021, at 16:21, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> 
> On Sun, May 9, 2021 at 12:25 PM Finn Behrens <me@kloenk.de> wrote:
>> 
>> Maybe we could also remove the result, as it would possible be an invariant, and never happen as the pointer should already be valid.
> 
> If it can fail, we cannot use `Deref` to begin with. So yeah, to make
> the examples equivalent, the first one should not return a `Result`.
> 
> Now, assuming it cannot fail, what I mentioned in the meeting is that
> with `Deref` all this compiles:
> 
> wrapper.deref().number
> (*wrapper).number
> wrapper.number
> 
> The question boils down to: do we want this shortcut or not?

I would not like to decide that on my own :-).
After thinking about it I think we don’t want that shortcut, as the other way give the same behaviour, but it is explicit.

> Having said that, I am not sure I understand the traits shown here.
> Isn't `PointerWrapper` meant only to save a `Box<T>` and similar types
> (which already implement `Deref` themselves etc.) as a pointer in a C
> data structure? (which is why `into_pointer()` consumes the object).
> 

All Pointers I’m wrapping are got from the C side. Either because there is a special malloc flag and some initialisation to the C struct, or as this is in a function call, and therefor the object is constructed out of my reach.
I usually do not consume the pointer, as I have to give the pointer to multiple function on the C side.

> In the examples shown, the new `get_pointer()` does not consume
> `self`; and `main()` creates a `Wrapper` using `from_pointer()`. But
> the idea is that you already have an object (constructed via other
> means), transform it into a raw pointer, and then later on
> re-construct the original object back from that pointer.
> 
> So `PointerWrapper` trait is like a `SavedAsPointer` trait, if you will.

Renaming to `SavedAsPointer` sounds great.

> 
> Cheers,
> Miguel

CU,
Finn


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-05-10 14:08 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-09 10:25 PointerWrapper Trait vs croe::ops::Deref Finn Behrens
2021-05-09 14:21 ` Miguel Ojeda
2021-05-10 14:04   ` Finn Behrens

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox