Comments on my code for Rust discid?

help
1

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 GitHub - phw/rust-discid: Rust bindings for MusicBrainz libdiscid

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!

2

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 GitHub - rust-lang/rust-bindgen: Automatically generates Rust FFI bindings to C (and some C++) libraries. inside something-sys to make safe wrappers around C API.

1 Like
3

Thanks a lot for this review. bindgen is great, so I used it to generate GitHub - phw/rust-discid-sys: Unsafe Rust FFI bindings for MusicBrainz libdiscid 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.

4 
disc: *mut discid_sys::DiscId,

You can use NonNull in std::ptr - Rust 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
5

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.

6

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.