> > +/*
> > + * 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
