client.drive_for_block() errors for drive-less block devices instead of returning None

[yurkobb]

Consider:

use anyhow::Result;
use pollster::FutureExt;
use udisks2::Client;

fn main() -> Result<()> {
    let client = Client::new().block_on().unwrap();
    let block = client
        .object("/org/freedesktop/UDisks2/block_devices/loop0") // has no Drive (the Drive property is '/')
        .unwrap()
        .block()
        .block_on() // or .await
        .expect("can not create BlockProxy for loop0 block");

    assert!(block.drive().block_on().unwrap().as_ref() == "/");

    client
        .drive_for_block(&block)
        .block_on()
        .expect("can not create Drive for loop0 block");
    Ok(())
}

Results in:

thread 'main' panicked at src/bin/udisks-test.rs:19:10:
can not create Drive for loop0 block: InterfaceNotFound
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

This is because in the drive_for_block() method block.drive() is not checked to return a "real" drive. Also, the method signature doesn't match its documentation (returns a Result of DriveProxy rather than a Result of Option of DriveProxy). Finally, the docs say block where it should be drive.

[FineFindus]

Thanks for the report, the documentation deviating from the function signature is definitely an issue. The function is a direct port of the udisks_client_get_drive_for_block C function, so I'm assuming that the function itself does not check the drive is the intended behavior.

[yurkobb]

The documentation for the C function says it will return NULL if there is no drive, which to me seems more comparable to an Option than returning an error in Rust. Of course there are no exceptions in C, but since a block device without a drive is quite normal, I think most users will have to be checking for this specific error most of the time when iterating over block devices and looking for their drives.

[FineFindus]

I see the argument and don't necessarily mint changing it. For a bit more context, here are the arguments why I decided to return an error, instead of an Option:

• it is consistent with similar methods that also return a result
• it is possible for the method to fail due to some other error, especially with the safety focus of Rust, a user may want to handle the error
• it is easier code-wise, since a lot of other things already return zbus::Result, also returning zbus::Result massively simplifies error handling
  • It is easy enough to use is_err() (or similar) to check if only the value is important.

[yurkobb]

Thanks for following up! I fully agree that Result is the appropriate thing to return. The Option, if any, would go inside Ok, indicating whether the block device has a drive or not.

But I think this dilemma may apply to other things as well, like the CryptoBackingDevice of a Block or CleartextDevice of Encrypted, which also seem to use / to indicate absence of a related object.

For the client.cleartext_block() currently the return type is Option. Though I think there could also be an error case where the block device is not encrypted at all (does not implement Encrypted), rather than being an encrypted-but-not-unlocked device, or any other error (the block device going away / not existing, etc.)

So, I'm all for Result, but then maybe Option inside (or maybe not), for all such cases.