Re: [RFC 1/6] scripts/simpletrace-rust: Add the basic cargo framework
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 > > > 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 ..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 -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [RFC 1/6] scripts/simpletrace-rust: Add the basic cargo framework
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 > > 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 ..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 signature.asc Description: PGP signature
Re: [RFC 1/6] scripts/simpletrace-rust: Add the basic cargo framework
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 > 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 ..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.
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 ..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 signature.asc Description: PGP signature
Re: [RFC 1/6] scripts/simpletrace-rust: Add the basic cargo framework
Hi Stefan,
[snip]
> > diff --git a/scripts/simpletrace-rust/.rustfmt.toml
> > b/scripts/simpletrace-rust/.rustfmt.toml
> > new file mode 100644
> > index ..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() {}
> > +
> > +/* Called if no specific method for processing a trace event. */
> > +fn catchall(
> > + self,
> > +rec_args: &[EventArgPayload],
> > +event: ,
> > +timestamp_ns: u64,
> > +pid: u32,
> > +event_id: u64,
> > +) -> Result;
> > +
> > +/* Called at the end of the trace. */
> > +fn end() {}
> > +
> > +/*
> > + * 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(
> > + self,
> > +rec_args: &[EventArgPayload],
> > +event: ,
> > +event_id: u64,
> > +timestamp_ns: u64,
> > +pid: u32,
> > +) -> Result
> > +{
> > +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.
> > + */
> > +}
> > +}
Re: [RFC 1/6] scripts/simpletrace-rust: Add the basic cargo framework
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 > Signed-off-by: Zhao Liu > --- > 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 ..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 ..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 ..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"
[RFC 1/6] scripts/simpletrace-rust: Add the basic cargo framework
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 Signed-off-by: Zhao Liu --- 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 ..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 ..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 ..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"
