xwayland: take advantage of RAII for X11 lockfile

src/xwayland/x11_sockets.rs

@@ -7,19 +7,15 @@ use nix::errno::Errno;
 use nix::sys::socket;
 
 /// Find a free X11 display slot and setup
-pub(crate) fn prepare_x11_sockets() -> Result<(u32, [UnixStream; 2]), ()> {
+pub(crate) fn prepare_x11_sockets() -> Result<(X11Lock, [UnixStream; 2]), ()> {
     for d in 0..33 {
         // if fails, try the next one
-        if let Err(()) = grab_lockfile(d) {
+        if let Ok(lock) = X11Lock::grab(d) {
-            continue;
+            // we got a lockfile, try and create the socket
+            if let Ok(sockets) = open_x11_sockets_for_display(d) {
+                return Ok((lock, sockets));
+            }
         }
-        // we got a lockfile, try and create the socket
-        if let Ok(fds) = open_x11_sockets_for_display(d) {
-            return Ok((d, fds));
-        }
-        // creating the sockets failed, for some readon ?
-        // release the lockfile and try with the next
-        release_lockfile(d);
     }
     // If we reach here, all values from 0 to 32 failed
     // we need to stop trying at some point
@@ -27,63 +23,72 @@ pub(crate) fn prepare_x11_sockets() -> Result<(u32, [UnixStream; 2]), ()> {
 }
 
 /// Remove the X11 sockets for a given display number
-pub(crate) fn cleanup_x11_sockets(display: u32) {
+pub(crate) fn cleanup_x11_sockets(display: u32) {}
-    let _ = ::std::fs::remove_file(format!("/tmp/.X11-unix/X{}", display));
+
-    let _ = ::std::fs::remove_file(format!("/tmp/.X{}-lock", display));
+pub(crate) struct X11Lock {
+    display: u32,
 }
 
-/// Try to grab a lockfile for given X display number
+impl X11Lock {
-fn grab_lockfile(display: u32) -> Result<(), ()> {
+    /// Try to grab a lockfile for given X display number
-    let filename = format!("/tmp/.X{}-lock", display);
+    fn grab(display: u32) -> Result<X11Lock, ()> {
-    let lockfile = ::std::fs::OpenOptions::new()
+        let filename = format!("/tmp/.X{}-lock", display);
-        .write(true)
+        let lockfile = ::std::fs::OpenOptions::new()
-        .create_new(true)
+            .write(true)
-        .open(&filename);
+            .create_new(true)
-    match lockfile {
+            .open(&filename);
-        Ok(mut file) => {
+        match lockfile {
-            // we got it, write our PID in it and we're good
+            Ok(mut file) => {
-            let ret = file.write_fmt(format_args!("{:>10}", ::nix::unistd::Pid::this()));
+                // we got it, write our PID in it and we're good
-            if let Err(_) = ret {
+                let ret = file.write_fmt(format_args!("{:>10}", ::nix::unistd::Pid::this()));
-                // write to the file failed ? we abandon
+                if let Err(_) = ret {
+                    // write to the file failed ? we abandon
+                    ::std::mem::drop(file);
+                    let _ = ::std::fs::remove_file(&filename);
+                    return Err(());
+                } else {
+                    // we got the lockfile and wrote our pid to it, all is good
+                    return Ok(X11Lock { display });
+                }
+            }
+            Err(_) => {
+                // we could not open the file, now we try to read it
+                // and if it contains the pid of a process that no longer
+                // exist (so if a previous x server claimed it and did not
+                // exit gracefully and remove it), we claim it
+                // if we can't open it, give up
+                let mut file = ::std::fs::File::open(&filename).map_err(|_| ())?;
+                let mut spid = [0u8; 11];
+                file.read_exact(&mut spid).map_err(|_| ())?;
                 ::std::mem::drop(file);
-                let _ = ::std::fs::remove_file(&filename);
+                let pid = ::nix::unistd::Pid::from_raw(::std::str::from_utf8(&spid)
+                    .map_err(|_| ())?
+                    .trim()
+                    .parse::<i32>()
+                    .map_err(|_| ())?);
+                if let Err(NixError::Sys(Errno::ESRCH)) = ::nix::sys::signal::kill(pid, None) {
+                    // no process whose pid equals the contents of the lockfile exists
+                    // remove the lockfile and try grabbing it again
+                    let _ = ::std::fs::remove_file(filename);
+                    return X11Lock::grab(display);
+                }
+                // if we reach here, this lockfile exists and is probably in use, give up
                 return Err(());
-            } else {
-                // we got the lockfile and wrote our pid to it, all is good
-                return Ok(());
             }
         }
-        Err(_) => {
+    }
-            // we could not open the file, now we try to read it
+
-            // and if it contains the pid of a process that no longer
+    pub(crate) fn display(&self) -> u32 {
-            // exist (so if a previous x server claimed it and did not
+        self.display
-            // exit gracefully and remove it), we claim it
-            // if we can't open it, give up
-            let mut file = ::std::fs::File::open(&filename).map_err(|_| ())?;
-            let mut spid = [0u8; 11];
-            file.read_exact(&mut spid).map_err(|_| ())?;
-            ::std::mem::drop(file);
-            let pid = ::nix::unistd::Pid::from_raw(::std::str::from_utf8(&spid)
-                .map_err(|_| ())?
-                .trim()
-                .parse::<i32>()
-                .map_err(|_| ())?);
-            if let Err(NixError::Sys(Errno::ESRCH)) = ::nix::sys::signal::kill(pid, None) {
-                // no process whose pid equals the contents of the lockfile exists
-                // remove the lockfile and try grabbing it again
-                let _ = ::std::fs::remove_file(filename);
-                return grab_lockfile(display);
-            }
-            // if we reach here, this lockfile exists and is probably in use, give up
-            return Err(());
-        }
     }
 }
 
-/// Release an X11 lockfile
+impl Drop for X11Lock {
-fn release_lockfile(display: u32) {
+    fn drop(&mut self) {
-    let filename = format!("/tmp/.X{}-lock", display);
+        // Cleanup all the X11 files
-    let _ = ::std::fs::remove_file(filename);
+        let _ = ::std::fs::remove_file(format!("/tmp/.X11-unix/X{}", self.display));
+        let _ = ::std::fs::remove_file(format!("/tmp/.X{}-lock", self.display));
+    }
 }
 
 /// Open the two unix sockets an X server listens on

src/xwayland/xserver.rs

@@ -28,18 +28,18 @@ use std::cell::RefCell;
 use std::rc::Rc;
 use std::env;
 use std::ffi::CString;
-use std::os::unix::io::{RawFd, AsRawFd, IntoRawFd};
+use std::os::unix::io::{AsRawFd, IntoRawFd};
 use std::os::unix::net::UnixStream;
 
 use nix::{Error as NixError, Result as NixResult};
 use nix::errno::Errno;
-use nix::unistd::{close, fork, ForkResult, Pid};
+use nix::unistd::{fork, ForkResult, Pid};
 use nix::sys::signal;
 
 use wayland_server::{Client, Display, LoopToken};
 use wayland_server::sources::{SignalEvent, Source};
 
-use super::x11_sockets::{cleanup_x11_sockets, prepare_x11_sockets};
+use super::x11_sockets::{X11Lock, prepare_x11_sockets};
 
 /// The XWayland handle
 pub struct XWayland<WM: XWindowManager> {
@@ -85,7 +85,7 @@ impl<WM: XWindowManager> Drop for XWayland<WM> {
 }
 
 struct XWaylandInstance {
-    display: u32,
+    display_lock: X11Lock,
     wayland_client: Client,
     sigusr1_handler: Option<Source<SignalEvent>>,
     wm_fd: Option<UnixStream>,
@@ -113,7 +113,7 @@ fn launch<WM: XWindowManager + 'static>(inner: &Rc<RefCell<Inner<WM>>>) -> Resul
     let (x_wm_x11, x_wm_me) = UnixStream::pair().map_err(|_| ())?;
     let (wl_x11, wl_me) = UnixStream::pair().map_err(|_| ())?;
 
-    let (display, x_fds) = prepare_x11_sockets()?;
+    let (lock, x_fds) = prepare_x11_sockets()?;
 
     // we have now created all the required sockets
 
@@ -121,23 +121,23 @@ fn launch<WM: XWindowManager + 'static>(inner: &Rc<RefCell<Inner<WM>>>) -> Resul
     let creation_time = ::std::time::Instant::now();
 
     // create the wayland client for XWayland
-    let client = unsafe { guard.wayland_display.borrow_mut().create_client(wl_me.into_raw_fd()) };
+    let client = unsafe {
+        guard
+            .wayland_display
+            .borrow_mut()
+            .create_client(wl_me.into_raw_fd())
+    };
     client.set_user_data(Rc::into_raw(inner.clone()) as *const () as *mut ());
     client.set_destructor(client_destroy::<WM>);
 
     // setup the SIGUSR1 handler
     let my_inner = inner.clone();
-    let sigusr1_handler = match guard
+    let sigusr1_handler = guard
         .token
         .add_signal_event_source(signal::Signal::SIGUSR1, move |_, ()| {
             xwayland_ready(&my_inner)
-        }) {
+        })
-        Ok(v) => v,
+        .map_err(|_| ())?;
-        Err(_) => {
-            cleanup_x11_sockets(display);
-            return Err(());
-        }
-    };
 
     // all is ready, we can do the fork dance
     let child_pid = match fork() {
@@ -170,7 +170,7 @@ fn launch<WM: XWindowManager + 'static>(inner: &Rc<RefCell<Inner<WM>>>) -> Resul
                 }
                 Ok(ForkResult::Child) => {
                     // we are the second child, we exec xwayland
-                    match exec_xwayland(display, wl_x11, x_wm_x11, &x_fds) {
+                    match exec_xwayland(lock.display(), wl_x11, x_wm_x11, &x_fds) {
                         Ok(x) => match x {},
                         Err(e) => {
                             // well, what can we do ?
@@ -188,14 +188,12 @@ fn launch<WM: XWindowManager + 'static>(inner: &Rc<RefCell<Inner<WM>>>) -> Resul
         }
         Err(e) => {
             eprintln!("[smithay] XWayland first fork failed: {:?}", e);
-            // fork failed ? cleanup
-            cleanup_x11_sockets(display);
             return Err(());
         }
     };
 
     guard.instance = Some(XWaylandInstance {
-        display,
+        display_lock: lock,
         wayland_client: client,
         sigusr1_handler: Some(sigusr1_handler),
         wm_fd: Some(x_wm_me),
@@ -218,12 +216,9 @@ impl<WM: XWindowManager> Inner<WM> {
             if let Some(s) = instance.sigusr1_handler.take() {
                 s.remove();
             }
-            // close the WM fd if it is still there
+            // All connexions and lockfiles are cleaned by their destructors
-            if let Some(s) = instance.wm_fd.take() {
+
-                let _ = s.shutdown(::std::net::Shutdown::Both);
+            // Remove DISPLAY from the env
-            }
-            // cleanup the X11 sockets
-            cleanup_x11_sockets(instance.display);
             ::std::env::remove_var("DISPLAY");
             // We do like wlroots:
             // > We do not kill the Xwayland process, it dies to broken pipe
@@ -282,11 +277,11 @@ fn xwayland_ready<WM: XWindowManager>(inner: &Rc<RefCell<Inner<WM>>>) {
         // signal the WM
         wm.xwayland_ready(
             instance.wm_fd.take().unwrap(), // This is a bug if None
-            instance.wayland_client.clone()
+            instance.wayland_client.clone(),
         );
 
         // setup the environemnt
-        ::std::env::set_var("DISPLAY", format!(":{}", instance.display));
+        ::std::env::set_var("DISPLAY", format!(":{}", instance.display_lock.display()));
     }
 
     // in all cases, cleanup