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

[Bernhard Beschow <shentey@gmail.com>]

Am 1. April 2025 10:51:06 UTC schrieb 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.

Ping

>
>> 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());
>>> +            }
>>> +        }
>>> +    }};
>>> +}
>>