1
\$\begingroup\$

I am currently implementing a native Rust API to X11, especially around the XDisplay for a related project. I chose an object-oriented approach:

use crate::util::discard_const_1;
use std::ffi::{c_char, CString};
use x11::xlib::{
    self, Window, XActivateScreenSaver, XAddExtension, XAddHost, XAddHosts, XAddToSaveSet,
    XDefaultRootWindow, XExtCodes, XHostAddress, XOpenDisplay, XSync,
};

#[cfg(feature = "xfixes")]
use x11::xfixes::XFixesHideCursor;

pub struct Display<'a> {
    display: &'a mut xlib::Display,
}

impl<'a> Display<'a> {
    pub fn open(name: Option<impl Into<String>>) -> Option<Self> {
        match name {
            Some(name) => match CString::new(name.into()) {
                Ok(name) => Self::open_raw(name.as_ptr()),
                Err(_) => None,
            },
            None => Self::open_raw(&0),
        }
    }

    fn open_raw(display: *const c_char) -> Option<Self> {
        unsafe { XOpenDisplay(display).as_mut() }.map(|display| Self { display })
    }

    pub fn activate_screen_saver(&mut self) {
        discard_const_1(
            unsafe { XActivateScreenSaver(self.display) },
            "XActivateScreenSaver",
        )
    }

    pub fn add_extension(&mut self) -> XExtCodes {
        unsafe { *XAddExtension(self.display) }
    }

    pub fn add_host(&mut self, address: &mut XHostAddress) {
        discard_const_1(unsafe { XAddHost(self.display, address) }, "XAddHost")
    }

    pub fn add_hosts(&mut self, address: &mut XHostAddress, n: i32) {
        discard_const_1(unsafe { XAddHosts(self.display, address, n) }, "XAddHosts")
    }

    pub fn default_root_window(&mut self) -> Window {
        unsafe { XDefaultRootWindow(self.display) }
    }

    pub fn add_to_save_set(&mut self, window: Window) {
        discard_const_1(
            unsafe { XAddToSaveSet(self.display, window) },
            "XAddToSaveSet",
        )
    }

    // TODO: implement all xlib functions that take a display as first argument as methods.

    pub fn sync(&mut self, discard: bool) {
        discard_const_1(unsafe { XSync(self.display, discard as i32) }, "XSync")
    }
}

#[cfg(feature = "xfixes")]
impl<'a> Display<'a> {
    pub fn hide_cursor(&mut self, window: Window) {
        unsafe { XFixesHideCursor(self.display, window) }
    }
}

Many of the X* functions always return 1, which the caller does not need. Therefore I chose to return () from my functions in this case instead. In order to not write the same code all over again, I implemented the following function:

#[inline]
pub(crate) fn discard_const_1(retval: i32, name: &str) {
    match retval {
        1 => (),
        _ => unreachable!("{} always returns 1.", name),
    }
}

Is this a good / acceptable solution? I am still pretty new to Rust and somehow managed to avoid writing macros up until now, but I have a gut feeling, that a macro might do a better job, than an inline function in my use case. Is there a better way to convert from the constant 1s to () in my library functions?

NB: The implementation of the API does not yet cover all of xlib's functionality, but the API works and is being used by a program.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Please edit your question so that the title describes the purpose of the code, rather than its mechanism. We really need to understand the motivational context to give good reviews. Thanks! \$\endgroup\$ Nov 25 at 13:15

2 Answers 2

1
\$\begingroup\$
pub struct Display<'a> {
    display: &'a mut xlib::Display,
}

This bit is problematic. A borrow makes sense if your struct is holding references to data which is borrowed from somewhere else. But that's not what's going on. It seems rather that your Display struct owns the pointer to xlib::Display. You should really hold a raw pointer not a reference. You might want to use std::ptr::NonNull to signal that the pointer is not null. Additionally, you should implement a drop function to call XCloseDisplay when your struct is dropped.

pub fn open(name: Option<impl Into<String>>) -> Option<Self> {

If I understand correctly, if you get NULL back from XOpenDisplay its an error. This function should probably return a Result to indicate that failure is an error.

#[inline]
pub(crate) fn discard_const_1(retval: i32, name: &str) {
    match retval {
        1 => (),
        _ => unreachable!("{} always returns 1.", name),
    }
}

The other response suggest that you should return Result for errors instead of panicing. Generally speaking, I'd agree with them. However, it seems that xlib doesn't provide useful error handling mechanics. Most errors will come back asynchronously and by default will terminate the application. In light of that, I think panicing isn't a bad strategy.

However I'd:

  1. Call it something like check_status
  2. Check for 0, since the documentation states that errors return 0
  3. Use a generic panic, not unreachable. Its panicing because it got an error, not because its unreachable
  4. Not put the name in the error message. This complicates all your calls because you have to pass the name, and it should very rarely matter (since these functions don't practically return errors). And if you need to know where the error comes from, you want to look at a stack trace anyways which will tell you.
\$\endgroup\$
1
\$\begingroup\$

First of all, you must distinguish between return values that represent a Status to those that return something else. This may not be immediately obvious from the function declaration, unfortunately. Anyway, you should make sure that the returned value is a Status somehow, possibly by looking at the source code.

Note that you cannot determine from the code if the value will be returned differently in future versions of the software. All future versions have to do is to keep to the contract, and nothing prevents a future developer to change the way that a function operates internally.

From the X documentation:

Some functions return Status, an integer error indication. If the function fails, it returns a zero. If the function returns a status of zero, it has not updated the return arguments.

This also shows other errors:

  • it is not that the function returns 1, it returns a value different from zero. So you should match zero (for false / an error) and otherwise continue.
  • discard_const_1 is a bad function name, it should say something like handle_status: currently it describes syntax rather than semantics.

Now for the implementation: you perform an unreachable which indicates that the code should be unreachable. But this is incorrect: the code is reachable, depending on the status. That would be akin to saying that the C-code of X.org can never fail. Obviously it can, otherwise there would be no reason to ever return a Status code.

Furthermore, unreachable will result in a panic. That's not good for code that - for instance - activates a screen saver. If it fails than this is bad, but it is certainly not a good reason to create an unrecoverable error. Instead you could return a Result.

As this seems to be a relatively low level wrapper library, I would try and find a way to propagate the Status codes (in case of success) and possibly an error description on failure. Reading through the X docs, it seems you may have to supply your own error handling routine for retrieving the error description.

For a higher level library you might want to return a lower amount of information.


Note that I'm not a Rust expert, so I will not propose a specific format of Result to return. The given review is largely agnostic to the PL.

\$\endgroup\$
2
  • \$\begingroup\$ Thank you for your review. But the functions in question actually always return zero. I actually did check the source code for each of the functions, where I discard a constant return value of one. E.g.: github.com/freedesktop/xorg-libX11/blob/…. Hence all other return values actually are unreachable in those cases. \$\endgroup\$ yesterday
  • 1
    \$\begingroup\$ This may sound strange to you, but this is not enough. You're programming against an API that has functions that return a value. Your code would only be correct assuming that no errors are returned at any place in the code, now and in the future. To me, the whole idea that functions can never error - especially when it comes to things as launching a screen saver - is unbelievable anyway (this is why I prefer languages that include exceptions). \$\endgroup\$ yesterday

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.