Utilities for X11 image capture and acceptance testing #22
Reference in New Issue
Block a user
Delete Branch "trurl/wmaker:refactor/riir.headless-utils"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
These are put in their own crate so that we can link tests against libWINGs freely.
Once libWINGs is entirely rewritten in Rust, we should be able to move these tests into the wings-rs crate.
5be0550532tobbdcb672e6bbdcb672e6todf3cfb5b40df3cfb5b40to7e000be7657e000be765toa2f7db3553@@ -0,0 +26,4 @@impl Lock {pub(crate) fn new(display: u16, file: File) -> Result<Self, LockError> {match file.try_lock() {Ok(_) => return Ok(Lock { display, file }),I think the
returnstatements here are redundant: the match is the only expression in this function.Right you are. Thanks!
a2f7db3553to09c9243930@@ -0,0 +74,4 @@/// `/tmp/.X*-lock`. If such a file exists, its `DISPLAY` value will not be/// used.////// Note also that this approach is far from perfect (and prone to TOCTOUSo...A couple of things you can do to make this a bit more robust.
O_EXCL; there's some wrapper infs::createthat does this, and lets you see the moral equivalent of EEXISTS. O_EXCL is atomic on Unix, and preventsflockthe file once it's open, but that's probably overkill.Oh! You do the first. I got confused by searching for /tmp/.X{}-lock, but that's to try and avoid racing against another process starting an X server.
The big issue I'd worry about, I suppose, is either one of the local servers, or an
sshserver process providing X forwarding. Starting displays at 32, as you do, will almost certainly never collide with anything.As a point of reference, https://man.archlinux.org/man/xvfb-run.1.en starts at display number 99, so we could go higher. But I think 32 is fine.
RE checking/claiming a lockfile atomically: we both left longer comments further down, so I'll resolve this one.
@@ -0,0 +86,4 @@return None;}let next = prev + 1;if let Ok(exists) = fs::exists(Path::new(&format!("/tmp/.X{}-lock", next))) && exists {Given that you do Not want to collide with an actual X server process (or ssh daemon simulating one), why not use these names as the lock file directly? It's not just that you have your own namespace to protect colliding with an already-running server, but also that a server started subsequently to you running could collide with you (though since it involves binding a socket, that is unlikely). But an
O_EXCLcreate here would pretty much guarantee isolation between actual X servers and you.Done.
@@ -0,0 +42,4 @@fn drop(&mut self) {// `file` should be unlocked on drop, but we explicitly unlock it and// unwrap the result so that errors aren't dropped silently.self.file.unlock().expect("could not unlock file");So this gets into some of the weird semantics between POSIX locks and
flock, but do you want to lock against threads in your own process, or other processes? Or both? The use of Atomics suggests that the former is at least on your radar.I think what I would consider doing is having a "display allocator" that always starts counting from the lower bound, and attempts to allocate a "display" number by creating a temporary file, and keeps track of what numbers have been allocated (e.g., in a bit vector or something), all protected by a mutex. It doesn't have to be terribly general purpose: you won't have more than a handful running at a time; you could make the "allocator" a bitmap in a u128. The method would basically be,
/tmp/.X{n}-lock(if that fails, continue)The allocated display number is now bitno + 32.
And then the drop is,
/tmp/.X{dispno}-lock)(Ok, so: the default Asahi Linux config powers your system completely off if you hit the power button, and Apple put the power button right next to backspace. I'll do my best to rewrite everything I just lost in a reply here, but please forgive me if it's a little terse.)
I think we're on the same page. I should definitely write better rustdoc comments.
I am trying to guard against other processes and threads in the same process. (https://nexte.st/, for example, runs each test in its own process.)
This looks good! It doesn't look like
Xvfbcares if you have an empty/tmp/X{n}-lock, andOpenOptions::create_newshould enable atomically checking and claiming aDISPLAYvalue. This simplifies things substantially (with no need for locking). Thank you!Regarding the use of a bitfield: I don't think we need to reclaim
DISPLAYvalues within the same process. Tracking the likely next unallocatedDISPLAYvalue for the process should help to avoid having to scan a range of lockfiles that are in use each time a newDISPLAYvalue is needed. I created over 50 stale/tmp/.X{n}-lockfiles while developing this library, and failed test runs could also conceivably leave them around. Using an atomic reduces the cost of scanning a chunk ofDISPLAYvalue space redundantly each time we want to provide a new value. So I'm in favor of simply incrementing an atomic.09c9243930tobcbba911f7bcbba911f7tod0fcb85ccdThanks again for the review. I have given this more of a shakedown in downstream branches with tests and tweaked things a little (e.g., using
prctl(2)to ensure that child processes are killed if the test process gets killed). I think this is ready.d0fcb85ccdto830924e251830924e251to8fdd27758b8fdd27758btoec1761f4f3I still want to merge this so that other work can be unblocked, but I'll leave a note for posterity: when running with
cargo-nextest, a test failed because it produced 100% the wrong image... which should have shown up for a different test!cargo-nextestruns tests in separate processes, so it looks like something may be going wrong with lockfile-based display number claims. I'll have to deal with this separately.