Comments on my code for Rust discid?

I am pretty new to Rust. As an exercise I wrote a small binding for using the MusicBrainz libdiscid library in Rust. libdiscid is a C library to read the table of contents (TOC) from an audio CD and calculate an ID for it, which can be used to lookup metadata for this audio CD on MusicBrainz or FreeDB.

The code is available at https://github.com/phw/rust-discid

Since this is my very first public Rust code I would like to get some feedback on anything I might have done wrong or could do better. I tried to make this code be a good Rust citizen and to follow what I perceived as best practices, but I likely missed things. Please be ruthless with the code :wink:

Thanks a lot!

use libc::{c_int, c_uint, size_t};

You can find these types in std::os::raw except size_t,
but there is no size_t in real C API:

typedef void *DiscId;
DiscId *discid_new()

So there is no need for libc dependency.

Also for some reason you don’t check discid_new result.

And Rust way will be split crate into two crates: something-sys and something,
something-sys gives access to raw/unsafe C API, something provides safe wrapper.
And you can use https://github.com/rust-lang/rust-bindgen inside something-sys to make safe wrappers around C API.

1 Like

Thanks a lot for this review. bindgen is great, so I used it to generate https://github.com/phw/rust-discid-sys and migrated the code to make use of this separate crate.

Also removed libc dependency and added error handling for DiscId::new.

One question about some functions like e.g. DiscId::track_length() which accept an index for a CD track to return it’s length. Currently this just calls the C method, and that asserts that the index is inside the bounds.

I wonder whether I should perform these checks in Rust and either panic or actually allow returning with an error (I think panic is better in this case, I’d consider out of bound access a hard failure). But this somewhat would duplicate the checks already in place in C code.

Another approach would be to get rid of these functions entirely and instead provide kind of an iterator over the tracks to get the data. I did something like that for the Ruby bindings.

disc: *mut discid_sys::DiscId,

You can use https://doc.rust-lang.org/std/ptr/struct.NonNull.html here,
after null checking of course.

IMHO this is rust way to do things.

Plus, check your code with clippy, if you haven’t yet use it.
There are some false positive, but often it find not optimal usage of std api.

1 Like

Thanks again for commenting. I have updated the code to allow a higher level access to track data which makes the API much clearer. I’m actually pretty happy with the current state and have published it to crates.io.

I just did this and was very happy that it had nothing to report :slight_smile: It did have some minor comments about generated code for constant definitions in discid-sys containing unnecessary lifetime, but this is just how bindgen generates the code.