Skip to content
This repository has been archived by the owner on Nov 6, 2024. It is now read-only.

Enable Clone for CpuId, Msrs and MsrList #13

Closed
wants to merge 1 commit into from
Closed

Enable Clone for CpuId, Msrs and MsrList #13

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions src/x86/fam_wrappers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,16 @@ generate_fam_struct_impl!(
KVM_MAX_CPUID_ENTRIES
);

impl Clone for kvm_cpuid2 {
fn clone(&self) -> Self {
kvm_cpuid2 {
nent: self.nent,
padding: self.padding,
entries: self.entries.clone(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Field entries is type of __IncompleteArrayField, is it safe to clone it?

Copy link
Contributor Author

@acatangiu acatangiu Nov 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is like phantomdata safe to clone. The cloning of actual data is done by the Clone implementation of FamStructWrapper.

The clone implementation added by this PR is not even actually used. But it is needed to satisfy the

impl<T: Default + FamStruct + Clone> Clone for FamStructWrapper<T>

requirement.

Another option would be to remove Clone from the FamStructWrapper requirements as it's not actually needed AFAICT.

CC @serban300 as the FamStructWrapper op.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you need Clone functionality for an instance of FamStructWrapper, T needs to implement Clone, otherwise the code won't build, because mem_allocator is a Vec<T>.

impl<T: Default + FamStruct + Clone> Clone for FamStructWrapper<T> {
    fn clone(&self) -> Self {
        FamStructWrapper {
            mem_allocator: self.mem_allocator.to_vec(),
        }
    }
}

I think it's ok to implement Clone for kvm_cpuid2, kvm_msrs and kvm_msr_list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise the code won't build, because mem_allocator is a Vec<T>

that's right.

Let's stick to the strategy implemented in this PR then.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__IncompleteArrayField are not safe to clone as @jiangliu also said in the previous commit. More details on this issue: rust-lang/rust-bindgen#1431. I am not sure what is the best way to move forward here honestly, but adding clone doesn't seem right to me.

}
}
}

/// Wrapper over the `kvm_cpuid2` structure.
///
/// The `kvm_cpuid2` structure contains a flexible array member. For details check the
Expand All @@ -49,6 +59,16 @@ generate_fam_struct_impl!(
KVM_MAX_MSR_ENTRIES
);

impl Clone for kvm_msrs {
fn clone(&self) -> Self {
kvm_msrs {
nmsrs: self.nmsrs,
pad: self.pad,
entries: self.entries.clone(),
}
}
}

/// Wrapper over the `kvm_msrs` structure.
///
/// The `kvm_msrs` structure contains a flexible array member. For details check the
Expand All @@ -61,6 +81,15 @@ pub type Msrs = FamStructWrapper<kvm_msrs>;
// Implement the FamStruct trait for kvm_msr_list.
generate_fam_struct_impl!(kvm_msr_list, u32, indices, u32, nmsrs, KVM_MAX_MSR_ENTRIES);

impl Clone for kvm_msr_list {
fn clone(&self) -> Self {
kvm_msr_list {
nmsrs: self.nmsrs,
indices: self.indices.clone(),
}
}
}

/// Wrapper over the `kvm_msr_list` structure.
///
/// The `kvm_msr_list` structure contains a flexible array member. For details check the
Expand Down