Re: [PATCH 1/2] rust/qemu-api: Add initial logging support based on C API

[Bernhard Beschow <shentey@gmail.com>]

Am 31. März 2025 09:53:41 UTC schrieb Paolo Bonzini <pbonzini@redhat.com>:
>On 3/30/25 22:58, Bernhard Beschow wrote:
>> A qemu_log_mask!() macro is provided which expects similar arguments as the C
>> version. However, the formatting works as one would expect from Rust.
>> 
>> To maximize code reuse the macro is just a thin wrapper around qemu_log().
>> Also, just the bare minimum of logging masks is provided which should suffice
>> for the current use case of Rust in QEMU.
>
>It's probably better to use an enum for this.  One possibility is also to change the #defines to a C enum, and see which enum translation of the several allowed by bindgen is best.

Currently the #defines contain some holes for "private" mask bits. Turning these into an enum without exposing all publicly, and changing the type of qemu_loglevel for consistency, would result in undefined behavior. Or do you suggest to convert just the public #defines into an enum to expose them to Rust, and keep the rest of the C API including the type of qemu_loglevel as is?

There are surely several tradeoffs and/or cleanups possible here, but that's way beyond for what I wanted to achieve -- which is closing a gap between C and Rust. My main goal is just to get my feet wet with Rust.

>
>Also, while this is good for now, later on we probably want to reimplement logging at a lower level via the std::fmt::Write trait.  But that's just for efficiency and your macro is indeed good enough to define what the API would look like.

Can we live with an easy solution then for now? As you suggest below, further abstractions like log_guest_error! can be built on top which further insulates client code from implementation details such as the representation of the mask bits.

> Right now I have a project for GSoC that will look at that, and the student can look into it later on.

Whoops, I didn't intend to spoil the project.

>
>This means answering the following two questions:
>
>- the mapping the LOG_* constants into Rust
>
>- whether to keep the "qemu" prefix for the API (personal opinion: no)
>
>- whether it makes sense to add more macros such as log_guest_error! or log_unimp! for the most common LOG_* values
>
>> +#[macro_export]
>> +macro_rules! qemu_log_mask {
>> +    ($mask:expr, $fmt:expr $(, $args:expr)*) => {{
>
>Looking at https://doc.rust-lang.org/std/macro.write.html they just use $($arg:tt)* for what is passed to format_args! (or in your case format!), so we can do the same here too. The main advantage is that it allows giving a trailing comma to qemu_log_mask!.

Easy to fix. Before proceeding I'd like to see a solution for the topic above.

Best regards,
Bernhard

>
>Paolo
>
>> +        if unsafe {
>> +            (::qemu_api::bindings::qemu_loglevel & ($mask as std::os::raw::c_int)) != 0
>> +        } {
>> +            let formatted_string = format!($fmt, $($args),*);
>> +            let c_string = std::ffi::CString::new(formatted_string).unwrap();
>> +
>> +            unsafe {
>> +                ::qemu_api::bindings::qemu_log(c_string.as_ptr());
>> +            }
>> +        }
>> +    }};
>> +}
>