fix fam issues #118

lauralt · 2021-02-17T13:13:57Z

lauralt · 2021-02-17T14:33:40Z

@jiangliu was there any other concern regarding #104 besides the Clone and PartialEq implementations?
I've noticed in vfio_ioctls the following structure:

struct vfio_region_info_with_cap {
    region_info: vfio_region_info,
    cap_info: __IncompleteArrayField<u8>,
}

which can be somehow seen as a fam struct even though the entries length is based on the region_info fields.

generate_fam_struct_impl indeed won't work for a structure that doesn't contain its own len field, but I don't think fixing this should be part of #104 since that macro is somehow of a helper (and I guess FamStruct can be implemented for the above structure as well). What do you think?

jiangliu · 2021-02-17T15:58:49Z

src/fam.rs

+        // The number of entries (self.as_slice().len()) can't be > T::max_len() since `self` is a
+        // valid `FamStructWrapper`.
+        let required_mem_allocator_capacity =
+            FamStructWrapper::<T>::mem_allocator_len(self.as_slice().len());


Should we clone the source FamStruct exactly by using "self.capacipy" instead of "self.as_slice().len()"?

After having a chat with @alexandruag it turned out it would be better to avoid this kind of confusion and he suggested to add the Clone trait bound to FamStruct; this way we would simplify things a lot by simply cloning the mem_allocator. Does anyone have anything against this or see a potential problem with it?

Turns out that right now bindgen will not automatically derive/generate Clone a implementation for structures that contain incomplete array fields (which is a bit odd). I've added a bullet point about this to #120.

Regarding the first question, it looks like using self.capacity instead of self.as_slice().len() would cause us to push more elements than necessary below. If I'm not mistaken, I think clone is not necessarily supposed to preserve to capacity of containers (it doesn't for Vec IIRC).

jiangliu · 2021-02-17T16:02:17Z

src/fam.rs

+        // aligned `T`s.
+        let (fam_struct, other_fam_struct): (T, T) = unsafe {
+            (
+                std::ptr::read(self.as_fam_struct_ref()),


why do we need to std::ptr::read() instead of comparing self.as_fam_struct_ref() and other.as_fam_struct_ref() directly?

no need, sorry, updated

acatangiu · 2021-02-17T16:02:28Z

src/fam.rs

+        foo.as_mut_fam_struct().length = 3;
+        foo.push(3).unwrap();
+        foo.push(14).unwrap();


if length is 3 shouldn't we push 3 elements?

Well, this was confusing for me as well, when you use push() it appends the element at the end of the entries collection which is considered as already having 3 elements (because of the above line). Anyway, this is not something that was added in this PR and I should've probably kept the tests simpler. Also, the whole fam module needs better documentation :D.

acatangiu · 2021-02-17T16:07:09Z

src/fam.rs

+        assert_eq!(foo.as_slice().len(), 3 + 2);
+        assert_eq!(foo.as_slice()[3 + 0], 3);
+        assert_eq!(foo.as_slice()[3 + 1], 14);


why is this 5? as slice should only provide the list of entries &[T::entry], right? I was expecting len to be 2 since we started off with 0 and pushed only 2.

Documentation for as_slice() says it gets the elements/entries. In your tests it seems it also comes back with contents of FAM struct packed as elements.

I am now confused 😄

Hmm, if the 5 is about the 3 + 2, that's because the original length is 3 (which are auto-pushed as zero-initialized elements as defined by the original interface) and we pushed 2. as_slice seems to work as intended here (i.e. we get the entries that we pushed earlier).

acatangiu · 2021-02-17T16:10:15Z

src/fam.rs

+            foo2.as_mut_fam_struct().flags
+        );
+        assert_eq!(foo.as_slice(), foo2.as_slice());
+        assert_eq!(foo2.as_slice().len(), foo2.as_mut_fam_struct().length as usize);


This also confuses me..

foo.as_slice().len() was checked to be 3 + 2, then here foo2.as_slice().len() is checked to be 3 (foo2.as_mut_fam_struct().length is 3).

src/fam.rs

serban300 · 2021-02-18T11:12:42Z

src/fam.rs

+        // safety can't be violated by the ownership of `fam_struct`). It is also safe because we're
+        // trying to read a T from a `&T` that is pointing to a properly initialized and aligned T.
+        unsafe {
+            let fam_struct: T = std::ptr::read(self.as_fam_struct_ref());


what if the FAM header contains a pointer ? Can it happen ?

Good question! As discussed offline, this might be possible, but T must be Default and from what I understand, implementing Default for a pointer becomes really awkward. Also, if a pointer is Copy, I wonder if ptr::read is actually unsafe :-?.
@jiangliu @alexandruag do you have some input on this? Thanks!

FamStruct is an unsafe trait, which requires following conditions for safety

/// Trait for accessing properties of C defined FAM structures. /// /// This is unsafe due to the number of constraints that aren't checked: /// * the implementer should be a POD /// * the implementor should contain a flexible array member of elements of type `Entry` /// * `Entry` should be a POD

So it's illegal for an FamStruct implementor to host pointer fields.

I was under the same impression, but after some googling/asking around, looks like a pointer is a POD. So isn't this actually right? :-?

I like this definition from https://stackoverflow.com/questions/45634083/is-there-a-concept-of-pod-types-in-rust:
POD: Types whose values can be duplicated simply by copying bits.

Sorry, my bad. Now I remember that the entire class was written with the assumption that T is memcopyable. It's just very strange that in C++ the following code prints true:

struct Pod { int *a; }; int main() { cout<<is_pod<Pod>::value<<endl; return 0; }

Hi! Don't know what authoritative source is best to quote here, but POD structs can contain pointers, which are effectively just integers meant to be interpreted as addresses. A pointer holding a value does not come with any guarantees by itself, and all safety concerns are passed on to whomever actually uses that pointer for a memory operation later (since all fields of a POD struct are always accessible and can be modified, any method operating on them must take into account that every possible value can be encountered). So, even if the POD struct contains pointers (which is unlikely), that doesn't seem to break any of the current assumptions.

I think the place to go would be the C++ standard. I don't own the hard copy, but the latest snapshot from the official repo loosely defines POD types through the template @serban300 used -

is_pod<T> is a Cpp17UnaryTypeTrait (20.15.2) with a base characteristic of true_type if T is a POD type, and false_type otherwise. A POD class is a class that is both a trivial class and a standard-layout class, and has no non-static data members of type non-POD class (or array thereof). A POD type is a scalar type, a POD class, an array of such a type, or a cv-qualified version of one of these types. [Note 1 : It is unspecified whether a closure type (7.5.5.2) is a POD type. — end note]

So my understanding here is in line with @alexandruag in the sense that scalar types are inherently POD, and since a pointer is basically just a number, it falls into this category.

I also found an unofficial definition that (more helpfully IMO) defines POD types as anything without a vtable.

Thanks for the clarifications. I agree, I don't have any concern here.

As a rule of thumb anything in C++ is POD that can be used from C. So any C struct is almost by definition POD.

Personally, I find cppreference easier to read than the standard, but it's not authoritative.

Fixes: rust-vmm#99. Signed-off-by: Laura Loghin <[email protected]>

All the fields from the FamStruct shuld be cloned instead of just the len field and the entries slice. Fixes: rust-vmm#85. Signed-off-by: Laura Loghin <[email protected]>

PartialEq should compare all the fields from the FamStruct. Fixes: rust-vmm#104. Signed-off-by: Laura Loghin <[email protected]>

jiangliu · 2021-02-20T02:01:04Z

@jiangliu was there any other concern regarding #104 besides the Clone and PartialEq implementations?
I've noticed in vfio_ioctls the following structure:
struct vfio_region_info_with_cap {
    region_info: vfio_region_info,
    cap_info: __IncompleteArrayField<u8>,
}
which can be somehow seen as a fam struct even though the entries length is based on the region_info fields.

generate_fam_struct_impl indeed won't work for a structure that doesn't contain its own len field, but I don't think fixing this should be part of #104 since that macro is somehow of a helper (and I guess FamStruct can be implemented for the above structure as well). What do you think?

When I was working on vfio-ioctls, I investigated the way to use generate_fam_struct_impl. But seems it's a little complex, so maybe it's ok to keep the hand-writing version for vfio-ioctls.

Signed-off-by: Andreea Florescu <[email protected]>

alexandruag · 2021-02-24T08:34:09Z

src/fam.rs

+        // The number of entries (self.as_slice().len()) can't be > T::max_len() since `self` is a
+        // valid `FamStructWrapper`.
+        let required_mem_allocator_capacity =
+            FamStructWrapper::<T>::mem_allocator_len(self.as_slice().len());


Turns out that right now bindgen will not automatically derive/generate Clone a implementation for structures that contain incomplete array fields (which is a bit odd). I've added a bullet point about this to #120.

Regarding the first question, it looks like using self.capacity instead of self.as_slice().len() would cause us to push more elements than necessary below. If I'm not mistaken, I think clone is not necessarily supposed to preserve to capacity of containers (it doesn't for Vec IIRC).

alexandruag · 2021-02-24T09:08:14Z

src/fam.rs

+        assert_eq!(foo.as_slice().len(), 3 + 2);
+        assert_eq!(foo.as_slice()[3 + 0], 3);
+        assert_eq!(foo.as_slice()[3 + 1], 14);


Hmm, if the 5 is about the 3 + 2, that's because the original length is 3 (which are auto-pushed as zero-initialized elements as defined by the original interface) and we pushed 2. as_slice seems to work as intended here (i.e. we get the entries that we pushed earlier).

lauralt requested review from andreeaflorescu, jiangliu and sameo as code owners February 17, 2021 13:13

lauralt force-pushed the max_len branch 2 times, most recently from 4b34117 to a2e43d0 Compare February 17, 2021 15:29

jiangliu reviewed Feb 17, 2021

View reviewed changes

acatangiu reviewed Feb 17, 2021

View reviewed changes

serban300 reviewed Feb 18, 2021

View reviewed changes

src/fam.rs Show resolved Hide resolved

serban300 reviewed Feb 18, 2021

View reviewed changes

src/fam.rs Show resolved Hide resolved

serban300 reviewed Feb 18, 2021

View reviewed changes

lauralt added 3 commits February 19, 2021 17:54

fam: validate max capacity when doing new

38fa681

Fixes: rust-vmm#99. Signed-off-by: Laura Loghin <[email protected]>

fix Clone implementation

b36dfd9

All the fields from the FamStruct shuld be cloned instead of just the len field and the entries slice. Fixes: rust-vmm#85. Signed-off-by: Laura Loghin <[email protected]>

fam: fix PartialEq

e977514

PartialEq should compare all the fields from the FamStruct. Fixes: rust-vmm#104. Signed-off-by: Laura Loghin <[email protected]>

lauralt force-pushed the max_len branch from a2e43d0 to e977514 Compare February 19, 2021 15:54

update coverage

13562e9

Signed-off-by: Andreea Florescu <[email protected]>

jiangliu approved these changes Feb 22, 2021

View reviewed changes

alexandruag approved these changes Feb 24, 2021

View reviewed changes

andreeaflorescu merged commit 1f060f9 into rust-vmm:master Feb 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix fam issues #118

fix fam issues #118

lauralt commented Feb 17, 2021

lauralt commented Feb 17, 2021

jiangliu Feb 17, 2021

lauralt Feb 19, 2021

alexandruag Feb 24, 2021

jiangliu Feb 17, 2021

lauralt Feb 19, 2021

acatangiu Feb 17, 2021

lauralt Feb 17, 2021

acatangiu Feb 17, 2021

alexandruag Feb 24, 2021

acatangiu Feb 17, 2021

serban300 Feb 18, 2021

lauralt Feb 18, 2021

jiangliu Feb 18, 2021

lauralt Feb 18, 2021

jiangliu Feb 18, 2021

serban300 Feb 18, 2021

alexandruag Feb 19, 2021

alxiord Feb 19, 2021

serban300 Feb 19, 2021 •

edited

Loading

blitz Feb 21, 2021

jiangliu commented Feb 20, 2021

alexandruag Feb 24, 2021

alexandruag Feb 24, 2021

fix fam issues #118

fix fam issues #118

Conversation

lauralt commented Feb 17, 2021

lauralt commented Feb 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serban300 Feb 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiangliu commented Feb 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serban300 Feb 19, 2021 •

edited

Loading