Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support multiple RPCs parsing in wireshark dissector for baidu_std protocol #2710

Merged
merged 1 commit into from
Jul 29, 2024

Conversation

AnDiXL
Copy link
Contributor

@AnDiXL AnDiXL commented Jul 25, 2024

…otocol

What problem does this PR solve?

Issue Number: resolve #2408

Problem Summary:

What is changed and the side effects?

原本 tools/wireshark_baidu_std.lua 的实现存在两个问题:

  1. 由于原本传入的解析长度是body_size - meta_size,这会导致解析时不止解析了BRPC Message,还会把Attachment也一同解析,出现一些令人困惑的解析结果;
  2. 原本的实现方式只支持解析包含单个RPC请求/响应的报文,但实际同一报文中是可能包含多个RPC请求/响应的。

Changed:
修复 #2408 引入的wireshark dissector无法解析包含多个baidu_std protocol请求的报文的问题。

Side effects:

  • Performance effects(性能影响):无

  • Breaking backward compatibility(向后兼容性): yes


Check List:

  • Please make sure your changes are compilable(请确保你的更改可以通过编译).
  • When providing us with a new feature, it is best to add related tests(如果你向我们增加一个新的功能, 请添加相关测试).
  • Please follow Contributor Covenant Code of Conduct.(请遵循贡献者准则).

@wwbmmm
Copy link
Contributor

wwbmmm commented Jul 26, 2024

@wasphin 有空看看

@wasphin
Copy link
Member

wasphin commented Jul 27, 2024

@AnDiXL 能不能帮传个多 RPC 请求响应的抓包看下?

Copy link
Member

@wasphin wasphin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

目前 wireshark protobuf 中提供了不同类型 field 的值列表,但是较难跟 proto 中的 field 对应上,后面可以再看看 wireshark 中能不能做些调整。

@@ -237,6 +275,8 @@ dissect_proto = function(tvbuf, pktinfo, root, offset)
method_name = protobuf_field_values[k].range:string(ENC_UTF8)
elseif v.value == "correlation_id" then
correlation_id = protobuf_field_values[k].range:uint64()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里的值也是错的, 只不过目前只是作为 key 来用,没影响。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里的值也是错的, 只不过目前只是作为 key 来用,没影响。

是的,这里其实跟attachment_size是一样的,如果需要拿到真实的correlation_id值,同样要经过le_uint & 反序列化的操作,只是当前用不到

@AnDiXL
Copy link
Contributor Author

AnDiXL commented Jul 29, 2024

@AnDiXL 能不能帮传个多 RPC 请求响应的抓包看下?

嗯,已经私发

@wasphin wasphin merged commit 64ce760 into apache:master Jul 29, 2024
20 checks passed
chenBright pushed a commit to chenBright/brpc that referenced this pull request Aug 1, 2024
…otocol (apache#2710)

Co-authored-by: xulei25 <xulei25@baidu.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants