diff mbox series

[RFC,1/6] scripts/simpletrace-rust: Add the basic cargo framework

Message ID 20240527081421.2258624-2-zhao1.liu@intel.com (mailing list archive)
State New, archived
Headers show
Series scripts: Rewrite simpletrace printer in Rust | expand

Commit Message

Zhao Liu May 27, 2024, 8:14 a.m. UTC 
Define the basic cargo framework to support compiling simpletrace-rust
via cargo, and add the Rust code style (with some nightly features)
check items to make Rust style as close to the QEMU C code as possible.

With the base cargo package, define the basic code framework for
simpletrace-rust, approximating the Python version, and also abstract
Analyzer operations for simpletrace-rust. Event and other future
trace-related structures are placed in the trace module.

Additionally, support basic command line parsing for simpletrace-rust as
a start.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 scripts/simpletrace-rust/.gitignore    |   1 +
 scripts/simpletrace-rust/.rustfmt.toml |   9 +
 scripts/simpletrace-rust/Cargo.lock    | 239 +++++++++++++++++++++++++
 scripts/simpletrace-rust/Cargo.toml    |  11 ++
 scripts/simpletrace-rust/src/main.rs   | 173 ++++++++++++++++++
 scripts/simpletrace-rust/src/trace.rs  |  11 ++
 6 files changed, 444 insertions(+)
 create mode 100644 scripts/simpletrace-rust/.gitignore
 create mode 100644 scripts/simpletrace-rust/.rustfmt.toml
 create mode 100644 scripts/simpletrace-rust/Cargo.lock
 create mode 100644 scripts/simpletrace-rust/Cargo.toml
 create mode 100644 scripts/simpletrace-rust/src/main.rs
 create mode 100644 scripts/simpletrace-rust/src/trace.rs

Comments

Stefan Hajnoczi May 27, 2024, 8:05 p.m. UTC | #1 
On Mon, May 27, 2024 at 04:14:16PM +0800, Zhao Liu wrote:
> Define the basic cargo framework to support compiling simpletrace-rust
> via cargo, and add the Rust code style (with some nightly features)
> check items to make Rust style as close to the QEMU C code as possible.
> 
> With the base cargo package, define the basic code framework for
> simpletrace-rust, approximating the Python version, and also abstract
> Analyzer operations for simpletrace-rust. Event and other future
> trace-related structures are placed in the trace module.
> 
> Additionally, support basic command line parsing for simpletrace-rust as
> a start.
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>  scripts/simpletrace-rust/.gitignore    |   1 +
>  scripts/simpletrace-rust/.rustfmt.toml |   9 +
>  scripts/simpletrace-rust/Cargo.lock    | 239 +++++++++++++++++++++++++
>  scripts/simpletrace-rust/Cargo.toml    |  11 ++
>  scripts/simpletrace-rust/src/main.rs   | 173 ++++++++++++++++++
>  scripts/simpletrace-rust/src/trace.rs  |  11 ++
>  6 files changed, 444 insertions(+)
>  create mode 100644 scripts/simpletrace-rust/.gitignore
>  create mode 100644 scripts/simpletrace-rust/.rustfmt.toml
>  create mode 100644 scripts/simpletrace-rust/Cargo.lock
>  create mode 100644 scripts/simpletrace-rust/Cargo.toml
>  create mode 100644 scripts/simpletrace-rust/src/main.rs
>  create mode 100644 scripts/simpletrace-rust/src/trace.rs
> 
> diff --git a/scripts/simpletrace-rust/.gitignore b/scripts/simpletrace-rust/.gitignore
> new file mode 100644
> index 000000000000..2f7896d1d136
> --- /dev/null
> +++ b/scripts/simpletrace-rust/.gitignore
> @@ -0,0 +1 @@
> +target/
> diff --git a/scripts/simpletrace-rust/.rustfmt.toml b/scripts/simpletrace-rust/.rustfmt.toml
> new file mode 100644
> index 000000000000..97a97c24ebfb
> --- /dev/null
> +++ b/scripts/simpletrace-rust/.rustfmt.toml
> @@ -0,0 +1,9 @@
> +brace_style = "AlwaysNextLine"
> +comment_width = 80
> +edition = "2021"
> +group_imports = "StdExternalCrate"
> +imports_granularity = "item"
> +max_width = 80
> +use_field_init_shorthand = true
> +use_try_shorthand = true
> +wrap_comments = true

There should be QEMU-wide policy. That said, why is it necessary to customize rustfmt?

> diff --git a/scripts/simpletrace-rust/Cargo.lock b/scripts/simpletrace-rust/Cargo.lock
> new file mode 100644
> index 000000000000..4a0ff8092dcb
> --- /dev/null
> +++ b/scripts/simpletrace-rust/Cargo.lock
> @@ -0,0 +1,239 @@
> +# This file is automatically @generated by Cargo.
> +# It is not intended for manual editing.
> +version = 3
> +
> +[[package]]
> +name = "anstream"
> +version = "0.6.14"
> +source = "registry+https://github.com/rust-lang/crates.io-index"
> +checksum = "418c75fa768af9c03be99d17643f93f79bbba589895012a80e3452a19ddda15b"
> +dependencies = [
> + "anstyle",
> + "anstyle-parse",
> + "anstyle-query",
> + "anstyle-wincon",
> + "colorchoice",
> + "is_terminal_polyfill",
> + "utf8parse",
> +]
> +
> +[[package]]
> +name = "anstyle"
> +version = "1.0.7"
> +source = "registry+https://github.com/rust-lang/crates.io-index"
> +checksum = "038dfcf04a5feb68e9c60b21c9625a54c2c0616e79b72b0fd87075a056ae1d1b"
> +
> +[[package]]
> +name = "anstyle-parse"
> +version = "0.2.4"
> +source = "registry+https://github.com/rust-lang/crates.io-index"
> +checksum = "c03a11a9034d92058ceb6ee011ce58af4a9bf61491aa7e1e59ecd24bd40d22d4"
> +dependencies = [
> + "utf8parse",
> +]
> +
> +[[package]]
> +name = "anstyle-query"
> +version = "1.0.3"
> +source = "registry+https://github.com/rust-lang/crates.io-index"
> +checksum = "a64c907d4e79225ac72e2a354c9ce84d50ebb4586dee56c82b3ee73004f537f5"
> +dependencies = [
> + "windows-sys",
> +]
> +
> +[[package]]
> +name = "anstyle-wincon"
> +version = "3.0.3"
> +source = "registry+https://github.com/rust-lang/crates.io-index"
> +checksum = "61a38449feb7068f52bb06c12759005cf459ee52bb4adc1d5a7c4322d716fb19"
> +dependencies = [
> + "anstyle",
> + "windows-sys",
> +]
> +
> +[[package]]
> +name = "clap"
> +version = "4.5.4"
> +source = "registry+https://github.com/rust-lang/crates.io-index"
> +checksum = "90bc066a67923782aa8515dbaea16946c5bcc5addbd668bb80af688e53e548a0"
> +dependencies = [
> + "clap_builder",
> +]
> +
> +[[package]]
> +name = "clap_builder"
> +version = "4.5.2"
> +source = "registry+https://github.com/rust-lang/crates.io-index"
> +checksum = "ae129e2e766ae0ec03484e609954119f123cc1fe650337e155d03b022f24f7b4"
> +dependencies = [
> + "anstream",
> + "anstyle",
> + "clap_lex",
> + "strsim",
> +]
> +
> +[[package]]
> +name = "clap_lex"
> +version = "0.7.0"
> +source = "registry+https://github.com/rust-lang/crates.io-index"
> +checksum = "98cc8fbded0c607b7ba9dd60cd98df59af97e84d24e49c8557331cfc26d301ce"
> +
> +[[package]]
> +name = "colorchoice"
> +version = "1.0.1"
> +source = "registry+https://github.com/rust-lang/crates.io-index"
> +checksum = "0b6a852b24ab71dffc585bcb46eaf7959d175cb865a7152e35b348d1b2960422"
> +
> +[[package]]
> +name = "is_terminal_polyfill"
> +version = "1.70.0"
> +source = "registry+https://github.com/rust-lang/crates.io-index"
> +checksum = "f8478577c03552c21db0e2724ffb8986a5ce7af88107e6be5d2ee6e158c12800"
> +
> +[[package]]
> +name = "proc-macro2"
> +version = "1.0.83"
> +source = "registry+https://github.com/rust-lang/crates.io-index"
> +checksum = "0b33eb56c327dec362a9e55b3ad14f9d2f0904fb5a5b03b513ab5465399e9f43"
> +dependencies = [
> + "unicode-ident",
> +]
> +
> +[[package]]
> +name = "quote"
> +version = "1.0.36"
> +source = "registry+https://github.com/rust-lang/crates.io-index"
> +checksum = "0fa76aaf39101c457836aec0ce2316dbdc3ab723cdda1c6bd4e6ad4208acaca7"
> +dependencies = [
> + "proc-macro2",
> +]
> +
> +[[package]]
> +name = "simpletrace-rust"
> +version = "0.1.0"
> +dependencies = [
> + "clap",
> + "thiserror",
> +]
> +
> +[[package]]
> +name = "strsim"
> +version = "0.11.1"
> +source = "registry+https://github.com/rust-lang/crates.io-index"
> +checksum = "7da8b5736845d9f2fcb837ea5d9e2628564b3b043a70948a3f0b778838c5fb4f"
> +
> +[[package]]
> +name = "syn"
> +version = "2.0.66"
> +source = "registry+https://github.com/rust-lang/crates.io-index"
> +checksum = "c42f3f41a2de00b01c0aaad383c5a45241efc8b2d1eda5661812fda5f3cdcff5"
> +dependencies = [
> + "proc-macro2",
> + "quote",
> + "unicode-ident",
> +]
> +
> +[[package]]
> +name = "thiserror"
> +version = "1.0.61"
> +source = "registry+https://github.com/rust-lang/crates.io-index"
> +checksum = "c546c80d6be4bc6a00c0f01730c08df82eaa7a7a61f11d656526506112cc1709"
> +dependencies = [
> + "thiserror-impl",
> +]
> +
> +[[package]]
> +name = "thiserror-impl"
> +version = "1.0.61"
> +source = "registry+https://github.com/rust-lang/crates.io-index"
> +checksum = "46c3384250002a6d5af4d114f2845d37b57521033f30d5c3f46c4d70e1197533"
> +dependencies = [
> + "proc-macro2",
> + "quote",
> + "syn",
> +]
> +
> +[[package]]
> +name = "unicode-ident"
> +version = "1.0.12"
> +source = "registry+https://github.com/rust-lang/crates.io-index"
> +checksum = "3354b9ac3fae1ff6755cb6db53683adb661634f67557942dea4facebec0fee4b"
> +
> +[[package]]
> +name = "utf8parse"
> +version = "0.2.1"
> +source = "registry+https://github.com/rust-lang/crates.io-index"
> +checksum = "711b9620af191e0cdc7468a8d14e709c3dcdb115b36f838e601583af800a370a"
> +
> +[[package]]
> +name = "windows-sys"
> +version = "0.52.0"
> +source = "registry+https://github.com/rust-lang/crates.io-index"
> +checksum = "282be5f36a8ce781fad8c8ae18fa3f9beff57ec1b52cb3de0789201425d9a33d"
> +dependencies = [
> + "windows-targets",
> +]
> +
> +[[package]]
> +name = "windows-targets"
> +version = "0.52.5"
> +source = "registry+https://github.com/rust-lang/crates.io-index"
> +checksum = "6f0713a46559409d202e70e28227288446bf7841d3211583a4b53e3f6d96e7eb"
> +dependencies = [
> + "windows_aarch64_gnullvm",
> + "windows_aarch64_msvc",
> + "windows_i686_gnu",
> + "windows_i686_gnullvm",
> + "windows_i686_msvc",
> + "windows_x86_64_gnu",
> + "windows_x86_64_gnullvm",
> + "windows_x86_64_msvc",
> +]
> +
> +[[package]]
> +name = "windows_aarch64_gnullvm"
> +version = "0.52.5"
> +source = "registry+https://github.com/rust-lang/crates.io-index"
> +checksum = "7088eed71e8b8dda258ecc8bac5fb1153c5cffaf2578fc8ff5d61e23578d3263"
> +
> +[[package]]
> +name = "windows_aarch64_msvc"
> +version = "0.52.5"
> +source = "registry+https://github.com/rust-lang/crates.io-index"
> +checksum = "9985fd1504e250c615ca5f281c3f7a6da76213ebd5ccc9561496568a2752afb6"
> +
> +[[package]]
> +name = "windows_i686_gnu"
> +version = "0.52.5"
> +source = "registry+https://github.com/rust-lang/crates.io-index"
> +checksum = "88ba073cf16d5372720ec942a8ccbf61626074c6d4dd2e745299726ce8b89670"
> +
> +[[package]]
> +name = "windows_i686_gnullvm"
> +version = "0.52.5"
> +source = "registry+https://github.com/rust-lang/crates.io-index"
> +checksum = "87f4261229030a858f36b459e748ae97545d6f1ec60e5e0d6a3d32e0dc232ee9"
> +
> +[[package]]
> +name = "windows_i686_msvc"
> +version = "0.52.5"
> +source = "registry+https://github.com/rust-lang/crates.io-index"
> +checksum = "db3c2bf3d13d5b658be73463284eaf12830ac9a26a90c717b7f771dfe97487bf"
> +
> +[[package]]
> +name = "windows_x86_64_gnu"
> +version = "0.52.5"
> +source = "registry+https://github.com/rust-lang/crates.io-index"
> +checksum = "4e4246f76bdeff09eb48875a0fd3e2af6aada79d409d33011886d3e1581517d9"
> +
> +[[package]]
> +name = "windows_x86_64_gnullvm"
> +version = "0.52.5"
> +source = "registry+https://github.com/rust-lang/crates.io-index"
> +checksum = "852298e482cd67c356ddd9570386e2862b5673c85bd5f88df9ab6802b334c596"
> +
> +[[package]]
> +name = "windows_x86_64_msvc"
> +version = "0.52.5"
> +source = "registry+https://github.com/rust-lang/crates.io-index"
> +checksum = "bec47e5bfd1bff0eeaf6d8b485cc1074891a197ab4225d504cb7a1ab88b02bf0"
> diff --git a/scripts/simpletrace-rust/Cargo.toml b/scripts/simpletrace-rust/Cargo.toml
> new file mode 100644
> index 000000000000..b44ba1569dad
> --- /dev/null
> +++ b/scripts/simpletrace-rust/Cargo.toml
> @@ -0,0 +1,11 @@
> +[package]
> +name = "simpletrace-rust"
> +description = "Pretty-printer for simple trace backend binary trace files (Rust version)"
> +version = "0.1.0"
> +edition = "2021"
> +authors = ["Zhao Liu <zhao1.liu@intel.com>"]
> +license = "GPL-2.0-or-later"
> +
> +[dependencies]
> +clap = "4.5.4"
> +thiserror = "1.0.20"
> diff --git a/scripts/simpletrace-rust/src/main.rs b/scripts/simpletrace-rust/src/main.rs
> new file mode 100644
> index 000000000000..2d2926b7658d
> --- /dev/null
> +++ b/scripts/simpletrace-rust/src/main.rs
> @@ -0,0 +1,173 @@
> +/*
> + * Pretty-printer for simple trace backend binary trace files (Rust version)
> + *
> + * Copyright (C) 2024 Intel Corporation.
> + *
> + * Authors: Zhao Liu <zhao1.liu@intel.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#![allow(dead_code)]
> +#![allow(unused_variables)]
> +
> +mod trace;
> +
> +use std::env;
> +
> +use clap::Arg;
> +use clap::Command;
> +use thiserror::Error;
> +use trace::Event;
> +
> +#[derive(Error, Debug)]
> +pub enum Error
> +{
> +    #[error("usage: {0} [--no-header] <trace-events> <trace-file>")]
> +    CliOptionUnmatch(String),
> +}
> +
> +pub type Result<T> = std::result::Result<T, Error>;
> +
> +pub struct EventArgPayload {}
> +
> +trait Analyzer
> +{

The Python version treats this as an API so that users can write trace
analysis scripts. I see below that you're using non-doc comments. That
suggests you don't see this as a public API that people can write trace
analysis scripts against?

> +    /* Called at the start of the trace. */
> +    fn begin(&self) {}
> +
> +    /* Called if no specific method for processing a trace event. */
> +    fn catchall(
> +        &mut self,
> +        rec_args: &[EventArgPayload],
> +        event: &Event,
> +        timestamp_ns: u64,
> +        pid: u32,
> +        event_id: u64,
> +    ) -> Result<String>;
> +
> +    /* Called at the end of the trace. */
> +    fn end(&self) {}
> +
> +    /*
> +     * TODO: Support "variable" parameters (i.e. variants of process_event()
> +     * with different parameters, like **kwargs in python), when we need a
> +     * simpletrace rust module.
> +     */
> +    fn process_event(
> +        &mut self,
> +        rec_args: &[EventArgPayload],
> +        event: &Event,
> +        event_id: u64,
> +        timestamp_ns: u64,
> +        pid: u32,
> +    ) -> Result<String>
> +    {
> +        self.catchall(rec_args, event, timestamp_ns, pid, event_id)
> +
> +        /*
> +         * TODO: Support custom function hooks (like getattr() in python),
> +         * when we need a simpletrace rust module.
> +         */
> +    }
> +}
> +
> +struct Formatter
> +{
> +    last_timestamp_ns: Option<u64>,
> +}
> +
> +impl Formatter
> +{
> +    fn new() -> Self
> +    {
> +        Formatter {
> +            last_timestamp_ns: None,
> +        }
> +    }
> +}
> +
> +impl Analyzer for Formatter
> +{
> +    fn catchall(
> +        &mut self,
> +        rec_args: &[EventArgPayload],
> +        event: &Event,
> +        timestamp_ns: u64,
> +        pid: u32,
> +        event_id: u64,
> +    ) -> Result<String>
> +    {
> +        let fmt_str = String::new();
> +
> +        Ok(fmt_str)
> +    }
> +}
> +
> +fn process(
> +    event_path: &str,
> +    trace_path: &str,
> +    analyzer: &mut Formatter,
> +    read_header: bool,
> +) -> Result<()>
> +{
> +    analyzer.begin();
> +    analyzer.end();
> +
> +    Ok(())
> +}
> +
> +/*
> + * Execute an analyzer on a trace file given on the command-line.
> + * This function is useful as a driver for simple analysis scripts.  More
> + * advanced scripts will want to call process() instead.
> + */
> +fn run(analyzer: &mut Formatter) -> Result<()>
> +{
> +    let matches = Command::new("simple trace")
> +        .arg(
> +            Arg::new("no-header")
> +                .required(false)
> +                .long("no-header")
> +                .action(clap::ArgAction::SetTrue)
> +                .help("Disable header parsing"),
> +        )
> +        .arg(
> +            Arg::new("trace-events")
> +                .required(true)
> +                .action(clap::ArgAction::Set)
> +                .help("Path to trace events file"),
> +        )
> +        .arg(
> +            Arg::new("trace-file")
> +                .required(true)
> +                .action(clap::ArgAction::Set)
> +                .help("Path to trace file"),
> +        )
> +        .try_get_matches()
> +        .map_err(|_| {
> +            Error::CliOptionUnmatch(
> +                env::current_exe()
> +                    .unwrap_or_else(|_| "simpletrace".into())
> +                    .to_string_lossy()
> +                    .to_string(),
> +            )
> +        })?;
> +
> +    let no_header = matches.get_flag("no-header");
> +    let event_path = matches.get_one::<String>("trace-events").unwrap();
> +    let trace_path = matches.get_one::<String>("trace-file").unwrap();
> +
> +    process(event_path, trace_path, analyzer, !no_header)?;
> +
> +    Ok(())
> +}
> +
> +fn main()
> +{
> +    let mut fmt = Formatter::new();
> +
> +    if let Err(e) = run(&mut fmt) {
> +        println!("{:?}", e.to_string());
> +    }
> +}
> diff --git a/scripts/simpletrace-rust/src/trace.rs b/scripts/simpletrace-rust/src/trace.rs
> new file mode 100644
> index 000000000000..3a4b06435b8b
> --- /dev/null
> +++ b/scripts/simpletrace-rust/src/trace.rs
> @@ -0,0 +1,11 @@
> +/*
> + * Machinery for generating tracing-related intermediate files (Rust version)
> + *
> + * Copyright (C) 2024 Intel Corporation.
> + *
> + * Authors: Zhao Liu <zhao1.liu@intel.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +pub struct Event {}
> -- 
> 2.34.1
>
Zhao Liu May 28, 2024, 7:53 a.m. UTC | #2 
Hi Stefan,

[snip]

> > diff --git a/scripts/simpletrace-rust/.rustfmt.toml b/scripts/simpletrace-rust/.rustfmt.toml
> > new file mode 100644
> > index 000000000000..97a97c24ebfb
> > --- /dev/null
> > +++ b/scripts/simpletrace-rust/.rustfmt.toml
> > @@ -0,0 +1,9 @@
> > +brace_style = "AlwaysNextLine"
> > +comment_width = 80
> > +edition = "2021"
> > +group_imports = "StdExternalCrate"
> > +imports_granularity = "item"
> > +max_width = 80
> > +use_field_init_shorthand = true
> > +use_try_shorthand = true
> > +wrap_comments = true
> 
> There should be QEMU-wide policy. That said, why is it necessary to customize rustfmt?

Indeed, but QEMU's style for Rust is currently undefined, so I'm trying
to add this to make it easier to check the style...I will separate it
out as a style policy proposal.

[snip]

> > +trait Analyzer
> > +{
> 
> The Python version treats this as an API so that users can write trace
> analysis scripts. I see below that you're using non-doc comments. That
> suggests you don't see this as a public API that people can write trace
> analysis scripts against?

Yes, for the initial version, I'm not exposing it for now. It could be
exposed later if needed and then we can support multiple script builds
by defining multiple workspaces in cargo.

Thanks,
Zhao

> > +    /* Called at the start of the trace. */
> > +    fn begin(&self) {}
> > +
> > +    /* Called if no specific method for processing a trace event. */
> > +    fn catchall(
> > +        &mut self,
> > +        rec_args: &[EventArgPayload],
> > +        event: &Event,
> > +        timestamp_ns: u64,
> > +        pid: u32,
> > +        event_id: u64,
> > +    ) -> Result<String>;
> > +
> > +    /* Called at the end of the trace. */
> > +    fn end(&self) {}
> > +
> > +    /*
> > +     * TODO: Support "variable" parameters (i.e. variants of process_event()
> > +     * with different parameters, like **kwargs in python), when we need a
> > +     * simpletrace rust module.
> > +     */
> > +    fn process_event(
> > +        &mut self,
> > +        rec_args: &[EventArgPayload],
> > +        event: &Event,
> > +        event_id: u64,
> > +        timestamp_ns: u64,
> > +        pid: u32,
> > +    ) -> Result<String>
> > +    {
> > +        self.catchall(rec_args, event, timestamp_ns, pid, event_id)
> > +
> > +        /*
> > +         * TODO: Support custom function hooks (like getattr() in python),
> > +         * when we need a simpletrace rust module.
> > +         */
> > +    }
> > +}
Stefan Hajnoczi May 28, 2024, 2:14 p.m. UTC | #3 
On Tue, May 28, 2024 at 03:53:55PM +0800, Zhao Liu wrote:
> Hi Stefan,
> 
> [snip]
> 
> > > diff --git a/scripts/simpletrace-rust/.rustfmt.toml b/scripts/simpletrace-rust/.rustfmt.toml
> > > new file mode 100644
> > > index 000000000000..97a97c24ebfb
> > > --- /dev/null
> > > +++ b/scripts/simpletrace-rust/.rustfmt.toml
> > > @@ -0,0 +1,9 @@
> > > +brace_style = "AlwaysNextLine"
> > > +comment_width = 80
> > > +edition = "2021"
> > > +group_imports = "StdExternalCrate"
> > > +imports_granularity = "item"
> > > +max_width = 80
> > > +use_field_init_shorthand = true
> > > +use_try_shorthand = true
> > > +wrap_comments = true
> > 
> > There should be QEMU-wide policy. That said, why is it necessary to customize rustfmt?
> 
> Indeed, but QEMU's style for Rust is currently undefined, so I'm trying
> to add this to make it easier to check the style...I will separate it
> out as a style policy proposal.

Why is a config file necessary? QEMU should use the default Rust style.

Stefan
Zhao Liu May 29, 2024, 2:30 p.m. UTC | #4 
Hi Stefan,

On Tue, May 28, 2024 at 10:14:01AM -0400, Stefan Hajnoczi wrote:
> Date: Tue, 28 May 2024 10:14:01 -0400
> From: Stefan Hajnoczi <stefanha@redhat.com>
> Subject: Re: [RFC 1/6] scripts/simpletrace-rust: Add the basic cargo
>  framework
> 
> On Tue, May 28, 2024 at 03:53:55PM +0800, Zhao Liu wrote:
> > Hi Stefan,
> > 
> > [snip]
> > 
> > > > diff --git a/scripts/simpletrace-rust/.rustfmt.toml b/scripts/simpletrace-rust/.rustfmt.toml
> > > > new file mode 100644
> > > > index 000000000000..97a97c24ebfb
> > > > --- /dev/null
> > > > +++ b/scripts/simpletrace-rust/.rustfmt.toml
> > > > @@ -0,0 +1,9 @@
> > > > +brace_style = "AlwaysNextLine"
> > > > +comment_width = 80
> > > > +edition = "2021"
> > > > +group_imports = "StdExternalCrate"
> > > > +imports_granularity = "item"
> > > > +max_width = 80
> > > > +use_field_init_shorthand = true
> > > > +use_try_shorthand = true
> > > > +wrap_comments = true
> > > 
> > > There should be QEMU-wide policy. That said, why is it necessary to customize rustfmt?
> > 
> > Indeed, but QEMU's style for Rust is currently undefined, so I'm trying
> > to add this to make it easier to check the style...I will separate it
> > out as a style policy proposal.
> 
> Why is a config file necessary? QEMU should use the default Rust style.
> 

There are some that may be overdone, but I think some basic may still
be necessary, like "comment_width = 80", "max_width = 80",
"wrap_comments". Is it necessary to specify the width? As C.

And, "group_imports" and "imports_granularity" (refered from crosvm),
can also be used to standardize including styles and improve
readability, since importing can be done in many different styles.

This fmt config is something like ./script/check_patch.pl for QEMU/linux.
Different programs have different practices, so I feel like that's an
open too!

This certainly also depends on the maintainer's/your preferences, ;-)
in what way looks more comfortable/convenient that is the best,
completely according to the default is also good.
Stefan Hajnoczi May 29, 2024, 6:41 p.m. UTC | #5 
On Wed, May 29, 2024 at 10:30:13PM +0800, Zhao Liu wrote:
> Hi Stefan,
> 
> On Tue, May 28, 2024 at 10:14:01AM -0400, Stefan Hajnoczi wrote:
> > Date: Tue, 28 May 2024 10:14:01 -0400
> > From: Stefan Hajnoczi <stefanha@redhat.com>
> > Subject: Re: [RFC 1/6] scripts/simpletrace-rust: Add the basic cargo
> >  framework
> > 
> > On Tue, May 28, 2024 at 03:53:55PM +0800, Zhao Liu wrote:
> > > Hi Stefan,
> > > 
> > > [snip]
> > > 
> > > > > diff --git a/scripts/simpletrace-rust/.rustfmt.toml b/scripts/simpletrace-rust/.rustfmt.toml
> > > > > new file mode 100644
> > > > > index 000000000000..97a97c24ebfb
> > > > > --- /dev/null
> > > > > +++ b/scripts/simpletrace-rust/.rustfmt.toml
> > > > > @@ -0,0 +1,9 @@
> > > > > +brace_style = "AlwaysNextLine"
> > > > > +comment_width = 80
> > > > > +edition = "2021"
> > > > > +group_imports = "StdExternalCrate"
> > > > > +imports_granularity = "item"
> > > > > +max_width = 80
> > > > > +use_field_init_shorthand = true
> > > > > +use_try_shorthand = true
> > > > > +wrap_comments = true
> > > > 
> > > > There should be QEMU-wide policy. That said, why is it necessary to customize rustfmt?
> > > 
> > > Indeed, but QEMU's style for Rust is currently undefined, so I'm trying
> > > to add this to make it easier to check the style...I will separate it
> > > out as a style policy proposal.
> > 
> > Why is a config file necessary? QEMU should use the default Rust style.
> > 
> 
> There are some that may be overdone, but I think some basic may still
> be necessary, like "comment_width = 80", "max_width = 80",
> "wrap_comments". Is it necessary to specify the width? As C.

Let's agree to follow the Rust coding style from the start, then the
problem is solved. My view is that deviating from the standard Rust
coding style in order to make QEMU Rust code resemble QEMU C code is
less helpful than following Rust conventions so our Rust code looks like
Rust.

> 
> And, "group_imports" and "imports_granularity" (refered from crosvm),
> can also be used to standardize including styles and improve
> readability, since importing can be done in many different styles.
> 
> This fmt config is something like ./script/check_patch.pl for QEMU/linux.
> Different programs have different practices, so I feel like that's an
> open too!

In languages like Rust that have a standard, let's follow the standard
instead of inventing our own way of formatting code.

> This certainly also depends on the maintainer's/your preferences, ;-)
> in what way looks more comfortable/convenient that is the best,
> completely according to the default is also good.

This will probably affect all Rust code in QEMU so everyone's opinion
counts.

Stefan
Daniel P. Berrangé May 31, 2024, 12:22 p.m. UTC | #6 
On Wed, May 29, 2024 at 02:41:03PM -0400, Stefan Hajnoczi wrote:
> On Wed, May 29, 2024 at 10:30:13PM +0800, Zhao Liu wrote:
> > Hi Stefan,
> > 
> > On Tue, May 28, 2024 at 10:14:01AM -0400, Stefan Hajnoczi wrote:
> > > Date: Tue, 28 May 2024 10:14:01 -0400
> > > From: Stefan Hajnoczi <stefanha@redhat.com>
> > > Subject: Re: [RFC 1/6] scripts/simpletrace-rust: Add the basic cargo
> > >  framework
> > > 
> > > On Tue, May 28, 2024 at 03:53:55PM +0800, Zhao Liu wrote:
> > > > Hi Stefan,
> > > > 
> > > > [snip]
> > > > 
> > > > > > diff --git a/scripts/simpletrace-rust/.rustfmt.toml b/scripts/simpletrace-rust/.rustfmt.toml
> > > > > > new file mode 100644
> > > > > > index 000000000000..97a97c24ebfb
> > > > > > --- /dev/null
> > > > > > +++ b/scripts/simpletrace-rust/.rustfmt.toml
> > > > > > @@ -0,0 +1,9 @@
> > > > > > +brace_style = "AlwaysNextLine"
> > > > > > +comment_width = 80
> > > > > > +edition = "2021"
> > > > > > +group_imports = "StdExternalCrate"
> > > > > > +imports_granularity = "item"
> > > > > > +max_width = 80
> > > > > > +use_field_init_shorthand = true
> > > > > > +use_try_shorthand = true
> > > > > > +wrap_comments = true
> > > > > 
> > > > > There should be QEMU-wide policy. That said, why is it necessary to customize rustfmt?
> > > > 
> > > > Indeed, but QEMU's style for Rust is currently undefined, so I'm trying
> > > > to add this to make it easier to check the style...I will separate it
> > > > out as a style policy proposal.
> > > 
> > > Why is a config file necessary? QEMU should use the default Rust style.
> > > 
> > 
> > There are some that may be overdone, but I think some basic may still
> > be necessary, like "comment_width = 80", "max_width = 80",
> > "wrap_comments". Is it necessary to specify the width? As C.
> 
> Let's agree to follow the Rust coding style from the start, then the
> problem is solved. My view is that deviating from the standard Rust
> coding style in order to make QEMU Rust code resemble QEMU C code is
> less helpful than following Rust conventions so our Rust code looks like
> Rust.

Agreed. The value of a language wide standard is undermined if apps
diverge from it.

All code style rules come down to bike shedding, and by simply adopting
the Rust community defaults, we avoid endless debates as to what style
is best, and stay aligned with the rest of the Rust community who mostly
won't override defaults for rustfmt.


With regards,
Daniel
diff mbox series

Patch

diff --git a/scripts/simpletrace-rust/.gitignore b/scripts/simpletrace-rust/.gitignore
new file mode 100644
index 000000000000..2f7896d1d136
--- /dev/null
+++ b/scripts/simpletrace-rust/.gitignore
@@ -0,0 +1 @@ 
+target/
diff --git a/scripts/simpletrace-rust/.rustfmt.toml b/scripts/simpletrace-rust/.rustfmt.toml
new file mode 100644
index 000000000000..97a97c24ebfb
--- /dev/null
+++ b/scripts/simpletrace-rust/.rustfmt.toml
@@ -0,0 +1,9 @@ 
+brace_style = "AlwaysNextLine"
+comment_width = 80
+edition = "2021"
+group_imports = "StdExternalCrate"
+imports_granularity = "item"
+max_width = 80
+use_field_init_shorthand = true
+use_try_shorthand = true
+wrap_comments = true
diff --git a/scripts/simpletrace-rust/Cargo.lock b/scripts/simpletrace-rust/Cargo.lock
new file mode 100644
index 000000000000..4a0ff8092dcb
--- /dev/null
+++ b/scripts/simpletrace-rust/Cargo.lock
@@ -0,0 +1,239 @@ 
+# This file is automatically @generated by Cargo.
+# It is not intended for manual editing.
+version = 3
+
+[[package]]
+name = "anstream"
+version = "0.6.14"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "418c75fa768af9c03be99d17643f93f79bbba589895012a80e3452a19ddda15b"
+dependencies = [
+ "anstyle",
+ "anstyle-parse",
+ "anstyle-query",
+ "anstyle-wincon",
+ "colorchoice",
+ "is_terminal_polyfill",
+ "utf8parse",
+]
+
+[[package]]
+name = "anstyle"
+version = "1.0.7"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "038dfcf04a5feb68e9c60b21c9625a54c2c0616e79b72b0fd87075a056ae1d1b"
+
+[[package]]
+name = "anstyle-parse"
+version = "0.2.4"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "c03a11a9034d92058ceb6ee011ce58af4a9bf61491aa7e1e59ecd24bd40d22d4"
+dependencies = [
+ "utf8parse",
+]
+
+[[package]]
+name = "anstyle-query"
+version = "1.0.3"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "a64c907d4e79225ac72e2a354c9ce84d50ebb4586dee56c82b3ee73004f537f5"
+dependencies = [
+ "windows-sys",
+]
+
+[[package]]
+name = "anstyle-wincon"
+version = "3.0.3"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "61a38449feb7068f52bb06c12759005cf459ee52bb4adc1d5a7c4322d716fb19"
+dependencies = [
+ "anstyle",
+ "windows-sys",
+]
+
+[[package]]
+name = "clap"
+version = "4.5.4"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "90bc066a67923782aa8515dbaea16946c5bcc5addbd668bb80af688e53e548a0"
+dependencies = [
+ "clap_builder",
+]
+
+[[package]]
+name = "clap_builder"
+version = "4.5.2"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "ae129e2e766ae0ec03484e609954119f123cc1fe650337e155d03b022f24f7b4"
+dependencies = [
+ "anstream",
+ "anstyle",
+ "clap_lex",
+ "strsim",
+]
+
+[[package]]
+name = "clap_lex"
+version = "0.7.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "98cc8fbded0c607b7ba9dd60cd98df59af97e84d24e49c8557331cfc26d301ce"
+
+[[package]]
+name = "colorchoice"
+version = "1.0.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "0b6a852b24ab71dffc585bcb46eaf7959d175cb865a7152e35b348d1b2960422"
+
+[[package]]
+name = "is_terminal_polyfill"
+version = "1.70.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "f8478577c03552c21db0e2724ffb8986a5ce7af88107e6be5d2ee6e158c12800"
+
+[[package]]
+name = "proc-macro2"
+version = "1.0.83"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "0b33eb56c327dec362a9e55b3ad14f9d2f0904fb5a5b03b513ab5465399e9f43"
+dependencies = [
+ "unicode-ident",
+]
+
+[[package]]
+name = "quote"
+version = "1.0.36"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "0fa76aaf39101c457836aec0ce2316dbdc3ab723cdda1c6bd4e6ad4208acaca7"
+dependencies = [
+ "proc-macro2",
+]
+
+[[package]]
+name = "simpletrace-rust"
+version = "0.1.0"
+dependencies = [
+ "clap",
+ "thiserror",
+]
+
+[[package]]
+name = "strsim"
+version = "0.11.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "7da8b5736845d9f2fcb837ea5d9e2628564b3b043a70948a3f0b778838c5fb4f"
+
+[[package]]
+name = "syn"
+version = "2.0.66"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "c42f3f41a2de00b01c0aaad383c5a45241efc8b2d1eda5661812fda5f3cdcff5"
+dependencies = [
+ "proc-macro2",
+ "quote",
+ "unicode-ident",
+]
+
+[[package]]
+name = "thiserror"
+version = "1.0.61"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "c546c80d6be4bc6a00c0f01730c08df82eaa7a7a61f11d656526506112cc1709"
+dependencies = [
+ "thiserror-impl",
+]
+
+[[package]]
+name = "thiserror-impl"
+version = "1.0.61"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "46c3384250002a6d5af4d114f2845d37b57521033f30d5c3f46c4d70e1197533"
+dependencies = [
+ "proc-macro2",
+ "quote",
+ "syn",
+]
+
+[[package]]
+name = "unicode-ident"
+version = "1.0.12"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "3354b9ac3fae1ff6755cb6db53683adb661634f67557942dea4facebec0fee4b"
+
+[[package]]
+name = "utf8parse"
+version = "0.2.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "711b9620af191e0cdc7468a8d14e709c3dcdb115b36f838e601583af800a370a"
+
+[[package]]
+name = "windows-sys"
+version = "0.52.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "282be5f36a8ce781fad8c8ae18fa3f9beff57ec1b52cb3de0789201425d9a33d"
+dependencies = [
+ "windows-targets",
+]
+
+[[package]]
+name = "windows-targets"
+version = "0.52.5"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "6f0713a46559409d202e70e28227288446bf7841d3211583a4b53e3f6d96e7eb"
+dependencies = [
+ "windows_aarch64_gnullvm",
+ "windows_aarch64_msvc",
+ "windows_i686_gnu",
+ "windows_i686_gnullvm",
+ "windows_i686_msvc",
+ "windows_x86_64_gnu",
+ "windows_x86_64_gnullvm",
+ "windows_x86_64_msvc",
+]
+
+[[package]]
+name = "windows_aarch64_gnullvm"
+version = "0.52.5"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "7088eed71e8b8dda258ecc8bac5fb1153c5cffaf2578fc8ff5d61e23578d3263"
+
+[[package]]
+name = "windows_aarch64_msvc"
+version = "0.52.5"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "9985fd1504e250c615ca5f281c3f7a6da76213ebd5ccc9561496568a2752afb6"
+
+[[package]]
+name = "windows_i686_gnu"
+version = "0.52.5"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "88ba073cf16d5372720ec942a8ccbf61626074c6d4dd2e745299726ce8b89670"
+
+[[package]]
+name = "windows_i686_gnullvm"
+version = "0.52.5"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "87f4261229030a858f36b459e748ae97545d6f1ec60e5e0d6a3d32e0dc232ee9"
+
+[[package]]
+name = "windows_i686_msvc"
+version = "0.52.5"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "db3c2bf3d13d5b658be73463284eaf12830ac9a26a90c717b7f771dfe97487bf"
+
+[[package]]
+name = "windows_x86_64_gnu"
+version = "0.52.5"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "4e4246f76bdeff09eb48875a0fd3e2af6aada79d409d33011886d3e1581517d9"
+
+[[package]]
+name = "windows_x86_64_gnullvm"
+version = "0.52.5"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "852298e482cd67c356ddd9570386e2862b5673c85bd5f88df9ab6802b334c596"
+
+[[package]]
+name = "windows_x86_64_msvc"
+version = "0.52.5"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "bec47e5bfd1bff0eeaf6d8b485cc1074891a197ab4225d504cb7a1ab88b02bf0"
diff --git a/scripts/simpletrace-rust/Cargo.toml b/scripts/simpletrace-rust/Cargo.toml
new file mode 100644
index 000000000000..b44ba1569dad
--- /dev/null
+++ b/scripts/simpletrace-rust/Cargo.toml
@@ -0,0 +1,11 @@ 
+[package]
+name = "simpletrace-rust"
+description = "Pretty-printer for simple trace backend binary trace files (Rust version)"
+version = "0.1.0"
+edition = "2021"
+authors = ["Zhao Liu <zhao1.liu@intel.com>"]
+license = "GPL-2.0-or-later"
+
+[dependencies]
+clap = "4.5.4"
+thiserror = "1.0.20"
diff --git a/scripts/simpletrace-rust/src/main.rs b/scripts/simpletrace-rust/src/main.rs
new file mode 100644
index 000000000000..2d2926b7658d
--- /dev/null
+++ b/scripts/simpletrace-rust/src/main.rs
@@ -0,0 +1,173 @@ 
+/*
+ * Pretty-printer for simple trace backend binary trace files (Rust version)
+ *
+ * Copyright (C) 2024 Intel Corporation.
+ *
+ * Authors: Zhao Liu <zhao1.liu@intel.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#![allow(dead_code)]
+#![allow(unused_variables)]
+
+mod trace;
+
+use std::env;
+
+use clap::Arg;
+use clap::Command;
+use thiserror::Error;
+use trace::Event;
+
+#[derive(Error, Debug)]
+pub enum Error
+{
+    #[error("usage: {0} [--no-header] <trace-events> <trace-file>")]
+    CliOptionUnmatch(String),
+}
+
+pub type Result<T> = std::result::Result<T, Error>;
+
+pub struct EventArgPayload {}
+
+trait Analyzer
+{
+    /* Called at the start of the trace. */
+    fn begin(&self) {}
+
+    /* Called if no specific method for processing a trace event. */
+    fn catchall(
+        &mut self,
+        rec_args: &[EventArgPayload],
+        event: &Event,
+        timestamp_ns: u64,
+        pid: u32,
+        event_id: u64,
+    ) -> Result<String>;
+
+    /* Called at the end of the trace. */
+    fn end(&self) {}
+
+    /*
+     * TODO: Support "variable" parameters (i.e. variants of process_event()
+     * with different parameters, like **kwargs in python), when we need a
+     * simpletrace rust module.
+     */
+    fn process_event(
+        &mut self,
+        rec_args: &[EventArgPayload],
+        event: &Event,
+        event_id: u64,
+        timestamp_ns: u64,
+        pid: u32,
+    ) -> Result<String>
+    {
+        self.catchall(rec_args, event, timestamp_ns, pid, event_id)
+
+        /*
+         * TODO: Support custom function hooks (like getattr() in python),
+         * when we need a simpletrace rust module.
+         */
+    }
+}
+
+struct Formatter
+{
+    last_timestamp_ns: Option<u64>,
+}
+
+impl Formatter
+{
+    fn new() -> Self
+    {
+        Formatter {
+            last_timestamp_ns: None,
+        }
+    }
+}
+
+impl Analyzer for Formatter
+{
+    fn catchall(
+        &mut self,
+        rec_args: &[EventArgPayload],
+        event: &Event,
+        timestamp_ns: u64,
+        pid: u32,
+        event_id: u64,
+    ) -> Result<String>
+    {
+        let fmt_str = String::new();
+
+        Ok(fmt_str)
+    }
+}
+
+fn process(
+    event_path: &str,
+    trace_path: &str,
+    analyzer: &mut Formatter,
+    read_header: bool,
+) -> Result<()>
+{
+    analyzer.begin();
+    analyzer.end();
+
+    Ok(())
+}
+
+/*
+ * Execute an analyzer on a trace file given on the command-line.
+ * This function is useful as a driver for simple analysis scripts.  More
+ * advanced scripts will want to call process() instead.
+ */
+fn run(analyzer: &mut Formatter) -> Result<()>
+{
+    let matches = Command::new("simple trace")
+        .arg(
+            Arg::new("no-header")
+                .required(false)
+                .long("no-header")
+                .action(clap::ArgAction::SetTrue)
+                .help("Disable header parsing"),
+        )
+        .arg(
+            Arg::new("trace-events")
+                .required(true)
+                .action(clap::ArgAction::Set)
+                .help("Path to trace events file"),
+        )
+        .arg(
+            Arg::new("trace-file")
+                .required(true)
+                .action(clap::ArgAction::Set)
+                .help("Path to trace file"),
+        )
+        .try_get_matches()
+        .map_err(|_| {
+            Error::CliOptionUnmatch(
+                env::current_exe()
+                    .unwrap_or_else(|_| "simpletrace".into())
+                    .to_string_lossy()
+                    .to_string(),
+            )
+        })?;
+
+    let no_header = matches.get_flag("no-header");
+    let event_path = matches.get_one::<String>("trace-events").unwrap();
+    let trace_path = matches.get_one::<String>("trace-file").unwrap();
+
+    process(event_path, trace_path, analyzer, !no_header)?;
+
+    Ok(())
+}
+
+fn main()
+{
+    let mut fmt = Formatter::new();
+
+    if let Err(e) = run(&mut fmt) {
+        println!("{:?}", e.to_string());
+    }
+}
diff --git a/scripts/simpletrace-rust/src/trace.rs b/scripts/simpletrace-rust/src/trace.rs
new file mode 100644
index 000000000000..3a4b06435b8b
--- /dev/null
+++ b/scripts/simpletrace-rust/src/trace.rs
@@ -0,0 +1,11 @@ 
+/*
+ * Machinery for generating tracing-related intermediate files (Rust version)
+ *
+ * Copyright (C) 2024 Intel Corporation.
+ *
+ * Authors: Zhao Liu <zhao1.liu@intel.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+pub struct Event {}