rust-vmm / vmm-sys-util

Helpers and utilities used by multiple rust-vmm components and VMMs
BSD 3-Clause "New" or "Revised" License
78 stars 64 forks source link

[TempFile] Revert to using `mkstemp` on unix #181

Closed roypat closed 1 year ago

roypat commented 1 year ago

Summary of the PR

The name generation in TempFile is done by appending a randomly generated string of 6 alphanumeric characters to a prefix-path. This PR changes that implementation back ot using mkstemp on unix systems, to prevent name collisions. This additionally brings it inline with the TempDir implementation in the unix::tempdir module.

It seems that on windows something similar is possible via GetTempFileName, but I am too unfamiliar with the win32 api (and also have no windows machine available for testing) to be confident in implementing that change.

Reason

We encountered spurious failures in tests due to multiple parallel running tests generating the same random file name.

Open Questions

Requirements

Before submitting your PR, please make sure you addressed the following requirements:

roypat commented 1 year ago

@andreeaflorescu Do you want to me address your comments by amending the commit that git revert generated, or do you want them in a follow up commit? 😅

andreeaflorescu commented 1 year ago

@andreeaflorescu Do you want to me address your comments by amending the commit that git revert generated, or do you want them in a follow up commit? 😅

I missed the commits history actually. Why do you want to handle this as a revert? It looks like we want to keep some of the functionality from the initial commit (i.e. the part where it works on Windows), so a revert might not be the best path forward in this case. I am opened to being changed my mind 😆 but for me right now it does not make sense to have this as a revert.

roypat commented 1 year ago

@andreeaflorescu Do you want to me address your comments by amending the commit that git revert generated, or do you want them in a follow up commit? 😅

I missed the commits history actually. Why do you want to handle this as a revert? It looks like we want to keep some of the functionality from the initial commit (i.e. the part where it works on Windows), so a revert might not be the best path forward in this case. I am opened to being changed my mind 😆 but for me right now it does not make sense to have this as a revert.

In my head I treated it as a "revert the unix behavior change introduced by commit xxxxxx", but am happy to just treat it as a normal commit that cfgs the current impl to windows and reintroduces the impl from pre- 4e950be27262c19ea90855ec8ec127271aa9a522 on unix. Since it's not a clean revert anyway, that'd result in a cleaner commit history, too.