Re: [RFC 2/6] scripts/simpletrace-rust: Support Event & Arguments in trace module

[Zhao Liu <zhao1.liu@intel.com>]

> > +/*
> > + * Refer to the description of ALLOWED_TYPES in
> > + * scripts/tracetool/__init__.py.
> 
> Please don't reference the Python implementation because this will not
> age well. It may bitrot if the Python code changes or if the Python
> implementation is deprecated then the source file will go away
> altogether. Make the Rust implementation self-contained. If there are
> common file format concerns shared by implementations, then move that
> information to a separate document in docs/interop/ (i.e. a simpletrace
> file format specification).

Thanks for your guidance, will do.

> > + */
> > +const ALLOWED_TYPES: [&str; 20] = [
> > +    "int",
> > +    "long",
> > +    "short",
> > +    "char",
> > +    "bool",
> > +    "unsigned",
> > +    "signed",
> > +    "int8_t",
> > +    "uint8_t",
> > +    "int16_t",
> > +    "uint16_t",
> > +    "int32_t",
> > +    "uint32_t",
> > +    "int64_t",
> > +    "uint64_t",
> > +    "void",
> > +    "size_t",
> > +    "ssize_t",
> > +    "uintptr_t",
> > +    "ptrdiff_t",
> > +];
> > +
> > +const STRING_TYPES: [&str; 4] =
> > +    ["const char*", "char*", "const char *", "char *"];
> > +
> > +/* TODO: Support 'vcpu' property. */
> 
> The vcpu property was removed in commit d9a6bad542cd ("docs: remove
> references to TCG tracing"). Is this comment outdated or are you
> planning to bring it back?

Thanks! I have no plan for this, I just follow _VALID_PROPS[] in
scripts/tracetool/__init__.py. As you commented above, I think I should
just ignore it. ;-)

> > +const VALID_PROPS: [&str; 1] = ["disable"];

[snip]

> > +    pub fn build(arg_str: &str) -> Result<Arguments>
> > +    {
> > +        let mut args = Arguments::new();
> > +        for arg in arg_str.split(',').map(|s| s.trim()) {
> > +            if arg.is_empty() {
> > +                return Err(Error::EmptyArg);
> > +            }
> > +
> > +            if arg == "void" {
> > +                continue;
> > +            }
> > +
> > +            let (arg_type, identifier) = if arg.contains('*') {
> > +                /* FIXME: Implement rsplit_inclusive(). */
> > +                let p = arg.rfind('*').unwrap();
> > +                (
> > +                    /* Safe because arg contains "*" and p exists. */
> > +                    unsafe { arg.get_unchecked(..p + 1) },
> > +                    /* Safe because arg contains "*" and p exists. */
> > +                    unsafe { arg.get_unchecked(p + 1..) },
> > +                )
> > +            } else {
> > +                arg.rsplit_once(' ').unwrap()
> > +            };
> 
> Can you write this without unsafe? Maybe rsplit_once(' ') followed by a
> check for (_, '*identifier'). If the identifier starts with '*', then
> arg_type += ' *' and identifier = identifier[1:].

Clever idea! It should work, will try this way.

> > +
> > +            validate_c_type(arg_type)?;
> > +            args.props.push(ArgProperty::new(arg_type, identifier));
> > +        }
> > +        Ok(args)
> > +    }
> > +}
> > +

[snip]

> > +    pub fn build(line_str: &str, lineno: u32, filename: &str) -> Result<Event>
> > +    {
> > +        static RE: Lazy<Regex> = Lazy::new(|| {
> > +            Regex::new(
> > +                r#"(?x)
> > +                ((?P<props>[\w\s]+)\s+)?
> > +                (?P<name>\w+)
> > +                \((?P<args>[^)]*)\)
> > +                \s*
> > +                (?:(?:(?P<fmt_trans>".+),)?\s*(?P<fmt>".+))?
> 
> What is the purpose of fmt_trans?
> 
> > +                \s*"#,
> > +            )
> > +            .unwrap()
> 
> I wonder if regular expressions help here. It's not easy to read this
> regex and there is a bunch of logic that takes apart the matches
> afterwards. It might even be clearer to use string methods to split
> fields.

Yes, regular matching is a burden here (it's a "lazy simplification" on
my part), and I'll think if it's possible to avoid regular matching with
string methods.

> Please add a comment showing the format that's being parsed:
> 
>  // [disable] <name>(<type1> <arg1>[, <type2> <arg2>] ...) "<format-string>"
> 

OK.

> > +        });
> > +
> > +        let caps_res = RE.captures(line_str);
> > +        if caps_res.is_none() {
> > +            return Err(Error::UnknownEvent(line_str.to_owned()));
> > +        }
> > +        let caps = caps_res.unwrap();
> > +        let name = caps.name("name").map_or("", |m| m.as_str());
> > +        let props: Vec<String> = if caps.name("props").is_some() {
> > +            caps.name("props")
> > +                .unwrap()
> > +                .as_str()
> > +                .split_whitespace()
> > +                .map(|s| s.to_string())
> > +                .collect()
> > +        } else {
> > +            Vec::new()
> > +        };
> > +        let fmt: String =
> > +            caps.name("fmt").map_or("", |m| m.as_str()).to_string();
> > +        let fmt_trans: String = caps
> > +            .name("fmt_trans")
> > +            .map_or("", |m| m.as_str())
> > +            .to_string();
> > +
> > +        if fmt.contains("%m") || fmt_trans.contains("%m") {
> > +            return Err(Error::InvalidFormat(
> > +                "Event format '%m' is forbidden, pass the error 
> > +                as an explicit trace argument"
> > +                    .to_string(),
> > +            ));
> > +        }
> 
> I'm not sure simpletrace needs to check this. That's a job for tracetool
> the build-time tool that generates code from trace-events files.

Thanks for the clarification, this item has bothered me before, I also
noticed that simpletrace doesn't use it, but don't feel confident about
deleting it completely, I'll clean it up!

> > +        if fmt.ends_with(r"\n") {
> > +            return Err(Error::InvalidFormat(
> > +                "Event format must not end with a newline 
> > +                character"
> > +                    .to_string(),
> > +            ));
> > +        }

Thanks,
Zhao