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.
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.
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 back into the wings-rs crate.
So...A couple of things you can do to make this a bit more robust.
Use your own subdirectory under /tmp for your lock files, to avoid conflicts against an actual X server.
Open the file with O_EXCL; there's some wrapper in fs::create that does this, and lets you see the moral equivalent of EEXISTS. O_EXCL is atomic on Unix, and prevents
You could also flock the file once it's open, but that's probably overkill.
So...A couple of things you can do to make this a bit more robust.
1. Use your own subdirectory under /tmp for your lock files, to avoid conflicts against an actual X server.
2. Open the file with `O_EXCL`; there's some wrapper in `fs::create` that does this, and lets you see the moral equivalent of EEXISTS. O_EXCL is atomic on Unix, and prevents
3. You could also `flock` the 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 ssh server process providing X forwarding. Starting displays at 32, as you do, will almost certainly never collide with anything.
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 `ssh` server process providing X forwarding. Starting displays at 32, as you do, will almost certainly never collide with anything.
RE checking/claiming a lockfile atomically: we both left longer comments further down, so I'll resolve this one.
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.
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_EXCL create here would pretty much guarantee isolation between actual X servers and you.
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_EXCL` create here would pretty much guarantee isolation between actual X servers and you.
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,
lock the mutex
find the position of the first unset bit in the allocator variable,
If all bits are allocated, panic!().
Set the display candidate number to the bit position + 32.
Attempt to create a lock file as /tmp/.X{n}-lock (if that fails, continue)
Set the bit
Unlock the mutex
The allocated display number is now bitno + 32.
And then the drop is,
Lock the mutex
Clear the bit (display number - 32) in the allocator variable
Close and unlink the lock file (/tmp/.X{dispno}-lock)
Unlock the mutex
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,
- lock the mutex
- find the position of the first unset bit in the allocator variable,
- If all bits are allocated, panic!().
- Set the display candidate number to the bit position + 32.
- Attempt to create a lock file as `/tmp/.X{n}-lock` (if that fails, continue)
- Set the bit
- Unlock the mutex
The allocated display number is now bitno + 32.
And then the drop is,
- Lock the mutex
- Clear the bit (display number - 32) in the allocator variable
- Close and unlink the lock file (`/tmp/.X{dispno}-lock`)
- Unlock the mutex
-
(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.)
Attempt to create a lock file as /tmp/.X{n}-lock (if that fails, continue)
This looks good! It doesn't look like Xvfb cares if you have an empty /tmp/X{n}-lock, and OpenOptions::create_new should enable atomically checking and claiming a DISPLAY value. 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 DISPLAY values within the same process. Tracking the likely next unallocated DISPLAY value for the process should help to avoid having to scan a range of lockfiles that are in use each time a new DISPLAY value is needed. I created over 50 stale /tmp/.X{n}-lock files while developing this library, and failed test runs could also conceivably leave them around. Using an atomic reduces the cost of scanning a chunk of DISPLAY value space redundantly each time we want to provide a new value. So I'm in favor of simply incrementing an atomic.
(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.)
> Attempt to create a lock file as `/tmp/.X{n}-lock` (if that fails, continue)
This looks good! It doesn't look like `Xvfb` cares if you have an empty `/tmp/X{n}-lock`, and `OpenOptions::create_new` should enable atomically checking and claiming a `DISPLAY` value. 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 `DISPLAY` values within the same process. Tracking the likely next unallocated `DISPLAY` value for the process should help to avoid having to scan a range of lockfiles that are in use each time a new `DISPLAY` value is needed. I created over 50 stale `/tmp/.X{n}-lock` files while developing this library, and failed test runs could also conceivably leave them around. Using an atomic reduces the cost of scanning a chunk of `DISPLAY` value space redundantly each time we want to provide a new value. So I'm in favor of simply incrementing an atomic.
Thanks 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.
Thanks 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.
I 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-nextest runs 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.
I 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-nextest` runs 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.
trurl
merged commit ec1761f4f3 into refactor/riir2026-03-26 18:47:37 -04:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
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.