Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FamStructWrapper: incorrect Clone implementation #85

Closed
lauralt opened this issue Apr 15, 2020 · 2 comments · Fixed by firecracker-microvm/versionize#28
Closed

FamStructWrapper: incorrect Clone implementation #85

lauralt opened this issue Apr 15, 2020 · 2 comments · Fixed by firecracker-microvm/versionize#28

Comments

@lauralt
Copy link
Contributor

lauralt commented Apr 15, 2020

Clone implementation for FamStructWrapper keeps only the number of entries and the entries array members. This implementation should be either removed or modified so that all fields are cloned.

@Twister915
Copy link
Contributor

Twister915 commented Jan 28, 2021

I'm trying to understand this issue, please correct me if I'm wrong. A FAM struct, in the typical case, just has length and a bunch of entries. Therefore, in this case, the Clone implementation is correct. However, FAM isn't strictly length + entries, but can instead be something like: ... other fields ... + length + ... other fields ... + entries, and this is the case that Clone does not currently handle correctly. If that's correct, then I think if #104 is addressed first, then this should be pretty straight-forward.

@andreeaflorescu
Copy link
Member

@Twister915 I find it to be the other way around. We already support other fields in FAM Struct, but the clone is implemented such that only the entries and length are cloned. This makes the Clone implementation wrong, and as a consequence, the FamStructWrapper implementation also wrong (in the scenarios where clone is needed). Once we fix the clone, and add a few tests with FAM structs that have more fields, we can close both issues.

lauralt added a commit to lauralt/vmm-sys-util that referenced this issue Feb 17, 2021
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]>
@lauralt lauralt mentioned this issue Feb 17, 2021
lauralt added a commit to lauralt/vmm-sys-util that referenced this issue Feb 17, 2021
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]>
lauralt added a commit to lauralt/vmm-sys-util that referenced this issue Feb 19, 2021
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants