define wrapper for C FAM structs #4

serban300 · 2019-05-31T13:24:13Z

In C an array of unknown size may appear within a struct definition
as the last member (as long as there is at least one other named member).
This is a known as flexible array member.

Pre C99, the same behavior could be achieved using zero length arrays.

Flexible Array Members are the go-to choice for working with large
amounts of data prefixed by header values.

KVM uses this type of structures (e.g., kvm_cpuid2, kvm_msr_list, etc.)

FamStructWrapper provides a common abstraction for this type of
structures and helps in treating them similarly to Rust Vec.

Signed-off-by: Serban Iorga seriorga@amazon.com

jiangliu · 2019-05-31T14:10:00Z

src/fam_struct_wrapper.rs

+        let mut mem_allocator = Vec::with_capacity(required_mem_allocator_capacity);
+        for _ in 0..required_mem_allocator_capacity {
+            mem_allocator.push(T::default())
+        }


Should we initialize memory other than first element to zero instead of T::default()? T::default() may generate some non-zero content.

Good catch. Done.

Well, then we should revert to zero-initialized instead of default-initialized in the method doc comment above.

alxiord

FYI I can't get coverage with cargo kcov, could you?

test fam_struct_wrapper::tests::test_reserve ... ok
kcov: Process exited with signal 4 (SIGILL) at 0x7ffff6ce00e3

kcov: Illegal instructions are sometimes caused by some GCC versions
kcov: miscompiling C++ headers. If the problem is persistent, try running
kcov: with --verify. For more information, see
kcov: http://github.com/SimonKagstrom/kcov/issues/18

src/fam_struct_wrapper.rs

alxiord · 2019-05-31T13:53:37Z

src/fam_struct_wrapper.rs

+
+use std::mem::size_of;
+
+/// Errors associated with the FamStructWrapper struct.


Suggested change

/// Errors associated with the FamStructWrapper struct.

/// Errors associated with the [`FamStructWrapper`](struct.FamStructWrapper.html) struct.

alxiord · 2019-05-31T13:53:52Z

src/fam_struct_wrapper.rs

+use std::mem::size_of;
+
+/// Errors associated with the FamStructWrapper struct.
+#[derive(Debug, Clone)]


nit: order derives alphabetically

alxiord · 2019-05-31T13:54:43Z

src/fam_struct_wrapper.rs

+    type Entry: PartialEq + Copy;
+
+    /// Get the array length
+    ///


nit: I don't think the extra newline improves readability here.

alxiord · 2019-05-31T13:55:33Z

src/fam_struct_wrapper.rs

+    fn as_mut_slice(&mut self) -> &mut [Self::Entry];
+}
+
+/// A wrapper that helps in treating a `FamStruct` similarly to an actual `Vec`.


Suggested change

/// A wrapper that helps in treating a `FamStruct` similarly to an actual `Vec`.

/// A wrapper that helps in treating a [`FamStruct`](trait.FamStruct.html) similarly to an actual `Vec`.

alxiord · 2019-05-31T13:58:57Z

src/fam_struct_wrapper.rs

+    /// # Arguments
+    ///
+    /// * `num_elements` - The number of default-initialized elements of type
+    ///                    `FamStruct::Entry` in the initial `FamStructWrapper`


Suggested change

/// `FamStruct::Entry` in the initial `FamStructWrapper`

/// [`FamStruct::Entry`](trait.FamStruct.html#associatedtype.Entry) in the initial [`FamStructWrapper`](struct.FamStructWrapper.html)

alxiord · 2019-05-31T13:59:42Z

src/fam_struct_wrapper.rs

+        }
+    }
+
+    /// Creates a new `FamStructWrapper` structure based on a supplied slice


Suggested change

/// Creates a new `FamStructWrapper` structure based on a supplied slice

/// Creates a new [`FamStructWrapper`](struct.FamStructWrapper.html) structure based on a supplied slice

alxiord · 2019-05-31T14:00:20Z

src/fam_struct_wrapper.rs

+    }
+
+    /// Creates a new `FamStructWrapper` structure based on a supplied slice
+    /// of `FamStruct::Entry`.


Suggested change

/// of `FamStruct::Entry`.

/// of [`FamStruct::Entry`](trait.FamStruct.html#associatedtype.Entry).

alxiord · 2019-05-31T14:00:46Z

src/fam_struct_wrapper.rs

+    ///
+    /// # Arguments
+    ///
+    /// * `entries` - The vector of `FamStruct::Entry` entries.


Suggested change

/// * `entries` - The vector of `FamStruct::Entry` entries.

/// * `entries` - The vector of [`FamStruct::Entry`](trait.FamStruct.html#associatedtype.Entry) entries.

alxiord · 2019-05-31T14:01:27Z

src/fam_struct_wrapper.rs

+        adapter
+    }
+
+    /// Get a reference to the actual `FamStruct` instance.


Suggested change

/// Get a reference to the actual `FamStruct` instance.

/// Get a reference to the actual [`FamStruct`](trait.FamStruct.html) instance.

This applies to all the following mentions too, I'll stop with the suggestions 😄

andreeaflorescu · 2019-05-31T15:34:38Z

@alexandruag I know you had some comments on this PR. Can you please add them here in case they weren't already fixed?

I will also check if the comments I had were resolved.

serban300 · 2019-05-31T15:36:09Z

FYI I can't get coverage with cargo kcov, could you?

test fam_struct_wrapper::tests::test_reserve ... ok
kcov: Process exited with signal 4 (SIGILL) at 0x7ffff6ce00e3

kcov: Illegal instructions are sometimes caused by some GCC versions
kcov: miscompiling C++ headers. If the problem is persistent, try running
kcov: with --verify. For more information, see
kcov: http://github.com/SimonKagstrom/kcov/issues/18

Yes. 98.9% coverage for fam_struct_wrapper.rs

alexandruag

Sorry about the large number of rename related comments. I think they can make the code easier to use/understand, but I realize this is also a subjective perspective. Looking forward to other ppl's opinion as well.

alexandruag · 2019-06-26T17:55:46Z

src/lib.rs

@@ -10,6 +10,7 @@ pub mod ioctl;

 pub mod errno;
 pub mod eventfd;
+pub mod fam_struct_wrapper;


What do you think about calling the new module simply fam? fam_struct_wrapper looks a bit ugly to me :(

Sounds good. Done.

src/fam_struct_wrapper.rs

alexandruag · 2019-06-26T18:21:50Z

src/fam_struct_wrapper.rs

+/// ```
+///
+#[allow(clippy::len_without_is_empty)]
+pub trait FamStruct {


What do ppl think about renaming this to Fam and the wrapper struct to FamWrapper? The shorter names seem a lot nicer to me :(

Using Fam here wouldn't be completely accurate.

FAM = Flexible array member. This is the field in the structure. Not the structure itself.

I would keep it as it is.

alexandruag · 2019-07-01T22:14:36Z

src/fam_struct_wrapper.rs

+    // which must be contiguous. Since the entries are of type `FamStruct::Entry` we must
+    // be careful to convert the desired capacity of the `FamStructWrapper`
+    // from `FamStruct::Entry` to `T` when reserving or releasing memory.
+    mem_allocator: Vec<T>,


How about renaming this to simply allocator? (applying this to other mem_allocator strings/substrings in the rest of the file as well)

I would keep it as it is. I don't see a great benefit in doing this. It would decrease the length of some method names but it would also make the names less suggestive.

alexandruag · 2019-07-01T22:15:59Z

src/fam_struct_wrapper.rs

+    /// Get the capacity required by mem_allocator in order to hold
+    /// the provided number of [`FamStruct::Entry`](trait.FamStruct.html#associatedtype.Entry)
+    ///
+    fn mem_allocator_len(wrapper_len: usize) -> usize {


I think reading this function becomes easier if we change the name of the parameter from wrapper_len to something like num_entries.

I renamed it to fam_len

alexandruag · 2019-07-01T22:35:04Z

src/fam_struct_wrapper.rs

+    /// [`FamStruct::Entry`](trait.FamStruct.html#associatedtype.Entry) elements.
+    /// If the capacity is already reserved, this method doesn't do anything.
+    ///
+    fn reserve(&mut self, additional: usize) {


It might be worth noting in the docs that calls to reserve (and other methods based on it, for example set_let) eventually lead to Vec::reserve being called, which may end up doing a realloc of the underlying buffer, and that invalidates all previous pointers obtained via methods such as as_ptr, etc.

Ok. I added comments related to this.

alexandruag · 2019-07-01T22:43:57Z

src/fam_struct_wrapper.rs

+    /// This is needed after `self.as_ptr()` is supplied as param to an FFI call
+    /// which may change it's state.
+    ///
+    pub fn sync(&mut self) -> Result<(), Error> {


Does this function make sense when new_len > self.capacity? It looks like that should be an error condition, because it means someone effectively wrote past the end of our allocator.

True. Fixed.

alexandruag · 2019-07-01T23:25:29Z

src/fam_struct_wrapper.rs

+
+impl<T: Default + FamStruct> PartialEq for FamStructWrapper<T> {
+    fn eq(&self, other: &FamStructWrapper<T>) -> bool {
+        self.len == other.len && self.as_slice() == other.as_slice()


Sorry I know I asked this regarding the old PR as well, but I don't remember the conclusion :( Is it a valid occurence to have self.as_slice() == other.as_slice(), but self.len != other.len?

Yes, if sync() isn't called.

It's possible to have for example:

cpuid_1 = CpuId::new(100); cpuid_2 = CpuId::new(110); // here the inner fam_struct of cpuid_1 will be modified by kvm. Let's say that it will have len = 50 call_to_kvm(cpuid_1.as_mut_fam_struct_ptr()); // here the inner fam_struct of cpuid_2 will be modified by kvm. Let's say that it will have len = 50 call_to_kvm(cpuid_2.as_mut_fam_struct_ptr()); After this cpuid_1.as_slice() == cpuid_2.as_slice() . But cpuid_1.len() = 100 while cpuid_2.len = 110.

alexandruag · 2019-07-01T23:28:47Z

src/fam_struct_wrapper.rs

+    ///
+    /// # Error: When len is greater than the max possible len it returns Error::SizeLimitExceeded
+    ///
+    fn set_len(&mut self, len: usize) -> Result<(), Error> {


It seems that when set_len causes the length to increase, the newly added elements are based on basically random memory contents (I don't think reserving capacity also initializes the memory to a particular value). If that's true, is this a part of the set_len method contract, or we should try to zero-initialize the new elements like we do for new?

For the moment set_len is used to increase the lenght of the array only when pushing. So it should be safe as it is. I don't know if in the future there will be any case where we would need to zero-initialize the memory.

alexandruag · 2019-07-01T23:33:10Z

src/fam_struct_wrapper.rs

+/// ```
+///
+#[allow(clippy::len_without_is_empty)]
+pub trait FamStruct {


Also, I think we should mark this trait as unsafe, just so the user gives an explicit sign-off that the implementation adheres to the conventions established by the trait, including that any type implementing this must be valid to initialize from any random bits. Without this assumption,we can't be sure that zero-initializing the memory in new and using Vec::set_len are actually safe to do in the implementation of FamStructWrapper. What do you ppl think?

Makes sense. Done.

src/fam.rs

andreeaflorescu · 2019-07-11T11:38:13Z

src/fam.rs

+    ///
+    /// This method might trigger reallocations of the underlying buffer.
+    ///
+    /// # Error: When len is greater than the max possible len it returns Error::SizeLimitExceeded


This will look a bit weird in markdown. You basically have a heading title that is very very long.

Please use as heading title Errors as this is the standard defined in the Rust documentation guidelines (even if the function can return just on type of error). The explanation for the error needs to be in a paragraph.

src/fam.rs

jiangliu · 2019-08-03T12:53:03Z

What's the plan with this PR? I saw this pattern has been repeated in several crates, so it may help to reduce common code.

serban300 · 2019-08-05T11:03:09Z

What's the plan with this PR? I saw this pattern has been repeated in several crates, so it may help to reduce common code.

We have to discuss the final details related to this comment. I'll try to sync with @alexandruag about this as soon as possible.

alexandruag · 2019-08-06T08:40:30Z

We have to discuss the final details related to this comment. I'll try to sync with @alexandruag about this as soon as possible.

I've had another look and for now there are no other pressing concerns from my point of view. I was worried before there were dangerous corner cases when the size of Entry is not a multiple of its alignment, but it looks like the compiler ensures that's never the case via padding bytes when necessary.

liujing2 · 2019-08-06T12:19:42Z

src/fam.rs

+/// ```
+///
+#[allow(clippy::len_without_is_empty)]
+pub unsafe trait FamStruct {


Could you please add some documentation on this trait? Some references here

vmm-sys-util/src/terminal.rs

Line 72 in 9014b7a

pub unsafe trait Terminal {

I added just a couple of lines. I couldn't think of any other details that would need to be specified. Please let me know if anything is not clear.

liujing2 · 2019-08-06T12:21:27Z

src/fam.rs

+
+/// A wrapper that helps in treating a [`FamStruct`](trait.FamStruct.html) similarly to
+/// an actual `Vec`.
+///


Blank line is useless if no other description.

Could you please kindly separate the documentation into a short summary line of its functionality, and more explanation in a new paragraph? Ditto for others below. EventFd structure in eventfd.rs is a reference for you.

liujing2 · 2019-08-06T12:28:05Z

src/fam.rs

+    /// * `num_elements` - The number of zero-initialized elements of type
+    ///                    [`FamStruct::Entry`](trait.FamStruct.html#associatedtype.Entry)
+    ///                    in the initial [`FamStructWrapper`](struct.FamStructWrapper.html)
+    ///


Yep, good to see the detailed #Arguments description. :)

liujing2 · 2019-08-06T12:36:07Z

src/fam.rs

+        desired_len = 5;
+        assert!(adapter.set_len(desired_len).is_ok());
+        assert_eq!(adapter.len(), desired_len);
+        // check that the capacity


Useless comment?

liujing2

Some comments as mentioned.

In C an array of unknown size may appear within a struct definition as the last member (as long as there is at least one other named member). This is a known as flexible array member. Pre C99, the same behavior could be achieved using zero length arrays. Flexible Array Members are the go-to choice for working with large amounts of data prefixed by header values. KVM uses this type of structures (e.g., kvm_cpuid2, kvm_msr_list, etc.) FamStructWrapper provides a common abstraction for this type of structures and helps in treating them similarly to Rust Vec. Signed-off-by: Serban Iorga <seriorga@amazon.com>

jiangliu · 2019-08-06T17:15:48Z

src/fam.rs

+///     pub unsafe fn as_mut_slice(&mut self, len: usize) -> &mut [T] {
+///         ::std::slice::from_raw_parts_mut(self.as_mut_ptr(), len)
+///     }
+/// }


Could the __IncompleteArrayField struct be exported as a pub generic type, seems it only contains generic code.

This struct is generated automatically by bindgen. You can find it for example in the kvm-bindings project. I needed to copy it just for the unit tests and the example. I don't know if it adds any value exporting it.

serban300 mentioned this pull request May 31, 2019

implement KvmVec struct rust-vmm/kvm#6

Closed

jiangliu reviewed May 31, 2019

View reviewed changes

alxiord reviewed May 31, 2019

View reviewed changes

andreeaflorescu requested review from alexandruag and andreeaflorescu May 31, 2019 15:33

serban300 mentioned this pull request May 31, 2019

Scope use_host_cpuid_function refactoring firecracker-microvm/firecracker#1019

Closed

jiangliu previously approved these changes Jun 5, 2019

View reviewed changes

alexandruag reviewed Jul 1, 2019

View reviewed changes

serban300 dismissed jiangliu’s stale review via f7f1367 July 3, 2019 15:10

alexandruag reviewed Jul 10, 2019

View reviewed changes

src/fam.rs Show resolved Hide resolved

jiangliu previously approved these changes Jul 11, 2019

View reviewed changes

andreeaflorescu reviewed Jul 11, 2019

View reviewed changes

serban300 dismissed jiangliu’s stale review via cfc4f09 July 18, 2019 18:17

serban300 requested review from liujing2 and sameo as code owners August 6, 2019 08:59

liujing2 reviewed Aug 6, 2019

View reviewed changes

liujing2 requested changes Aug 6, 2019

View reviewed changes

jiangliu reviewed Aug 6, 2019

View reviewed changes

jiangliu approved these changes Aug 7, 2019

View reviewed changes

liujing2 approved these changes Aug 7, 2019

View reviewed changes

liujing2 merged commit 54e256b into rust-vmm:master Aug 7, 2019


		use std::mem::size_of;

		/// Errors associated with the FamStructWrapper struct.

	/// Errors associated with the FamStructWrapper struct.
	/// Errors associated with the [`FamStructWrapper`](struct.FamStructWrapper.html) struct.

	/// A wrapper that helps in treating a `FamStruct` similarly to an actual `Vec`.
	/// A wrapper that helps in treating a [`FamStruct`](trait.FamStruct.html) similarly to an actual `Vec`.

	/// `FamStruct::Entry` in the initial `FamStructWrapper`
	/// [`FamStruct::Entry`](trait.FamStruct.html#associatedtype.Entry) in the initial [`FamStructWrapper`](struct.FamStructWrapper.html)

	/// Creates a new `FamStructWrapper` structure based on a supplied slice
	/// Creates a new [`FamStructWrapper`](struct.FamStructWrapper.html) structure based on a supplied slice

	/// of `FamStruct::Entry`.
	/// of [`FamStruct::Entry`](trait.FamStruct.html#associatedtype.Entry).

	/// * `entries` - The vector of `FamStruct::Entry` entries.
	/// * `entries` - The vector of [`FamStruct::Entry`](trait.FamStruct.html#associatedtype.Entry) entries.

	/// Get a reference to the actual `FamStruct` instance.
	/// Get a reference to the actual [`FamStruct`](trait.FamStruct.html) instance.

define wrapper for C FAM structs #4

define wrapper for C FAM structs #4

Conversation

serban300 commented May 31, 2019

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

alxiord left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreeaflorescu commented May 31, 2019

serban300 commented May 31, 2019 • edited Loading

alexandruag left a 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

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

jiangliu commented Aug 3, 2019

serban300 commented Aug 5, 2019

alexandruag commented Aug 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liujing2 Aug 6, 2019 • edited Loading

Choose a reason for hiding this comment

liujing2 Aug 6, 2019 • edited Loading

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

liujing2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serban300 commented May 31, 2019 •

edited

Loading

liujing2 Aug 6, 2019 •

edited

Loading

liujing2 Aug 6, 2019 •

edited

Loading