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

wants to merge 1 commit into from

Conversation

acatangiu
Copy link
Contributor

To enable Clone for the safe wrappers CpuId, Msrs and MsrList we need to implement Clone for kvm_cpuid2, kvm_msrs and respectively kvm_msr_list.

Fixed #12

To enable `Clone` for the safe wrappers `CpuId`, `Msrs` and
`MsrList` we need to implement `Clone` for `kvm_cpuid2`,
`kvm_msrs` and respectively `kvm_msr_list`.

Signed-off-by: Adrian Catangiu <[email protected]>
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

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.

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.

@acatangiu
Copy link
Contributor Author

__IncompleteArrayField are not safe to clone as @jiangliu also said in the previous commit. More details on this issue: rust-lang/rust-bindgen#1431.

Yes, having read that thread I agree the using the result of that clone() by itself is UB. However, the way we use it in FamStructWrapper is actually safe since we don't care about or touch that member, but just copy the memory values of the normal members.

Even so, I agree we should not expose an implementation of the Clone trait for the unsafe fam struct. External users of this crate might decide to use that directly instead of through the safe FamStructWrapper which can lead to UB.

I am closing this PR and attempting a fix directly in the Clone implementation of FamStructWrapper.

@acatangiu acatangiu closed this Nov 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FamStruct safe wrappers don't implement clone
4 participants