Merge blocked
欢迎来到 openUBMC 社区
Hey @Byo_wupengfei , 感谢你对社区的贡献.
机器人使用手册
有关指令的使用,可以点击 说明文档 或 论坛 查看详情。开发人员可以在每个PR或Issue下方评论特定指令来触发机器人任务。您可以自助配置此仓库的 PR 合并规则,更多详情,请参阅机器人自助配置使用手册。
联系指引
有疑问可以联系 SIG: sig-component-drivers ,
维护者是: @dongyushan, @net_swift_wangchen, @sam_h1973, @u012924942, @weixin_37569011, @xiaoxinglxs ,
优秀贡献者是: @hzy1010, @leessential .


/ai review


代码审查
审查结论
- [建议]
src/lualib-src/libmctp/upper/pldm_protocol.cpp:81fix: PLDM 响应发送路径仍固定使用请求头,缺少完成码 - [建议]
include/mctp_lib/init.lua:430fix: register_mctp_cmd 用 reg_ret == false 判失败会漏掉 nil,err 返回 - [建议]
src/lualib-src/libmctp/transport/mctp_protocol.cpp:302fix: 上层无协议受理的合法报文被计入 incomplete_msg 计数 - [建议]
gen/mctpd/client.lua:50fix: 新增 MctpCmd 枚举接口的末位实参与同类调用不一致 - [建议]
src/lualib-src/libmctp/transport/mctp_protocol.h:111fix: make_rx_key 将 uint32_t medium_type 截断为 uint8_t,可能把不同接口的重组上下文合并 - [建议]
gen/mctpd/json_types/MctpCmd.lua:109fix: 生成代码中 Context 为 nil 时 new(nil) 会直接抛 setmetatable 错误 - [建议]
src/lualib/mctp_cmd_dispatcher.lua:243fix: 在途名额回收未做异常保护,调用抛错会导致永久忙码 - [建议]
src/lualib/mctp_mdb_mgmt.lua:519fix: 确认 log:warn 在 mc.logging 中存在,否则重复登记路径会抛错 - [建议]
src/lualib/mctp_engine.lua:489fix: rx_drop_no_endpoint_count 只写不读,丢弃计数无任何消费方 - [建议]
src/lualib/mctp_endpoints/ep_pldm.lua:21fix: is_smbus_medium 未防 medium_name 查表为 nil(新增 copy_ctxs 调用点使其进入收包主路径) - [建议]
src/lualib/mctp_mdb_mgmt.lua:552fix: 命令对象登记失败后无重试,属性缺失/顺序问题会永久漏注册 - [建议]
test/unit/test_mctp_mdb_mgmt.lua:215fix: 不在共享 engine 上临时替换 cmd_registry 而不恢复 - [建议]
test/unit/test_libmctp/test_mctp_tx.lua:347fix: 重置 setup 中的 smbus_eid,避免跨用例状态泄漏 - [建议]
test/unit/test_mctp_engine.lua:22fix: 新引擎测试用例构造对象前补 c_tasks stub - [建议]
test/unit/test_mctp_lib.lua:73test: 注册参数校验用例无法区分各分支,弱断言 - [建议]
test/unit/test_ep_response_e2e.lua:291test: 入站响应用例无法区分“未分发”与“更早层丢弃” - [建议]
test/unit/test_ep_response_e2e.lua:30test: 报文打印默认开启,污染单测结构化输出 - [建议]
test/unit/test_ep_response_e2e.lua:203test: inject 对入站响应帧硬性要求 pop 返回 0,易误判失败
| 类型 | 数量 |
|---|---|
| 🔴 阻塞 | 0 |
| 🟡 建议 | 18 |
💬 仅评论


CI已开始运行,查看流水线详情:流水线链接


| 任务名称 | 结果 |
|---|---|
| pre-commit检查 | ✅ SUCCESS |
| PR描述检查 | ✅ SUCCESS |
| 防投毒检查 | ✅ SUCCESS |
| 开源合规检查 | ✅ SUCCESS |
| 编译构建 | ❌ FAILED |
| UT测试 | ❌ FAILED |
| IT测试 | ✅ SUCCESS |
| 代码覆盖率 | ❌ FAILED |
| 修改行数检查 | ❌ FAILED |
| 头文件检查 | ❌ FAILED |
| AI Review | ✅ SUCCESS |
| ARM UT测试 | ❌ FAILED |


🔵 Low Priority
建议:把 rx_key::medium_type 声明为 uint32_t,make_rx_key 直接赋值 medium_type(不截断),使重组键的接口标识与协议栈其他处的 medium_type 语义一致。
|
114 | + inline rx_key make_rx_key(const mctp_hdr &hdr, const uint32_t medium_type) |
|
115 | + { |
|
116 | + return rx_key{medium_type, hdr.u.src, hdr.u.dest, hdr.u.msg_tag, static_cast<bool>(hdr.u.to)}; |
| 114
117 | } |


🔵 Low Priority
gen/mctpd/client.lua:51、55 新增的 GetMctpCmdObjects / ForeachMctpCmdObjects 传 false 作为 get/foreach_non_virtual_interface_objects 的第三个实参,而同一文件内所有其它非虚拟接口调用(第 35、39、43、47、59、63、67、71、116、121 行的 Pmu/PCIeTransport/BlockIO/Env/SmBios)统一传 true。
影响:该布尔实参控制接口对象枚举的路径匹配方式(是否按带参数路径/子路径展开)。若这里本应为 true,GetMctpCmdObjects() 将无法枚举到通过 mctp_lib.register_mctp_cmd 动态注册在 /bmc/kepler/Systems/<id>/Mctp/Cmds/<MT>/<ST>/<CC> 下的对象,业务侧「查询已注册命令」的能力静默失效(注册成功但查不到)。由于该文件为 mako 生成,也可能是生成器依据接口定义输出的正确结果,故仅作核对提示。
触发条件:业务组件调用 mctpd_client:GetMctpCmdObjects() 或 ForeachMctpCmdObjects() 枚举命令对象。
建议:核对 mako 模板/接口模型中对 bmc.kepler.Systems.Mctp.MctpCmd 的路径参数定义,确认末位实参应为 true 还是 false,与 mds/service.json 中 "path": "*" 及 create_mctp_cmd_path 生成的多级路径保持一致。


🟡 Medium Priority
改动点(pldm_protocol.cpp:81-98)把 pop 的报文头长度按 rq 区分:请求 4 字节、响应 5 字节(响应多 1 字节 completion_code),并在响应分支解析 completion_code。但同文件的发送侧 pldm_protocol::push(第 101-122 行,未随本次改动调整)仍然固定 m_tx_buf.resize(sizeof(pldm_req_payload_hdr))(4 字节)并只填 rq/iid/hdr_ver/pldm_type/command_code,从不写 local_ctx.completion_code。
后果:本 PR 新增的 responder 能力需要向对端回送 PLDM 响应,经 push 发出的帧是「rq=0 但只有 4 字节头且无完成码」的畸形响应。对端(以及本端新逻辑第 83-85 行)在 is_request=false 时要求 data.size() >= 5,空负载响应会被直接丢弃;若有负载,对端会把首个负载字节误读成 completion_code,命令数据整体错位。触发条件:任意 PLDM 命令(如 Issue #9/#10 的 GetTID/GetDateTime)的响应经统一 TX 返回时必然发生。
即本次单方面修改了「头长度由 rq 决定」这一契约,兄弟代码 push 仍保持旧形式,两侧不一致。
建议:同步修改 pldm_protocol::push:按 local_ctx.rq 选择 pldm_req_payload_hdr/pldm_rsp_payload_hdr,响应分支把 local_ctx.completion_code(缺省 0)写入包头,保证收发两侧头长度契约一致。


🟡 Medium Priority
改动点 include/mctp_lib/init.lua:430-434 与 440-446 用 local reg_ok, reg_ret, reg_err = pcall(properties_obj.register, properties_obj, bus) 后以 if not reg_ok or reg_ret == false 判定失败。
问题:pcall 只在被调函数抛错时返回 ok=false;若 register 以「返回 nil, err」的方式失败(同文件 create_transport_obj 第 148/155 行就完全没有检查返回值,说明该 API 的失败不一定抛错),此时 reg_ok=true, reg_ret=nil, reg_err="...",reg_ret == false 不成立,判定为成功。后果:对象实际未挂到总线,但函数继续执行并返回 handle,调用方(业务组件)误以为注册成功,之后对端请求不会到达该对象的 GetResponse,形成静默失效;且 reg_err 在该分支永远为 nil,tostring(reg_err or reg_ret) 也拿不到真实原因,与注释声称的「保持 nil,err 契约」不符。
触发条件:mdb 对象注册因路径冲突/服务未就绪等原因返回 nil,err 而非抛错。
建议:把成功判定改为显式校验返回值:if not reg_ok or reg_ret == nil or reg_ret == false then(两处同样修改),必要时同时确认 mdb register 的返回契约;失败回滚逻辑保持不变。


🔵 Low Priority
改动点 mctp_protocol.cpp:302-306 在 protocol::pop 返回 false 时累加 m_incomplete_msg_cnt,注释解释为「消息体与消息类型不匹配或长度不足:上层无法解析」。
但 protocol::pop 的 false 语义更宽:protocol.h:183-186 的实现是遍历子协议 for (auto &child : m_childrens) { if (child->pop_data(...)) return true; },全部不受理即返回 false。因此发给本机、格式完全合法但上层未注册协议的消息(例如 NCSI 0x02、SPDM、VD-PCI 等未挂载子协议的消息类型),以及请求在 ENDPOINT 下但对应上层未注册的情况,都会被算成「不完整报文」。
后果:header 中对该计数的定义是「对端消息不完整或校验失败…丢弃且不产生响应」,属于可用于告警/诊断的指标(Issue #7/#5 要求可诊断)。上述误计入会让正常的多协议流量持续抬高 incomplete 计数,运维据此误判链路异常,也会掩盖真正的分片校验失败。
触发条件:对端发送本端未注册上层协议类型的合法 MCTP 报文。
建议:区分「消息类型无上层协议受理」与「报文不完整/长度不足」:可在计数前先判断是否为本端支持的消息类型(或让上层 pop 返回三态),仅对长度/格式失败累加 m_incomplete_msg_cnt,另设 unsupported 计数。


🔵 Low Priority
变更行:MctpCmd.lua:112 与 MctpCmd.lua:165 在 validate 中无条件执行 MctpCmd.Context.new(self.Context):validate(...),而 TContext.new/ TContext_from_obj(第 28-34 行)内部是 setmetatable(obj, TContext)。失效模式:当业务侧构造 MctpCmd.GetResponseRsp.new(data)(未传第二个参数 Context)或反序列化得到不含 Context 的报文时,self.Context 为 nil,setmetatable(nil, ...) 抛 “bad argument #1 to 'setmetatable' (table expected, got nil)”,请求/响应处理直接中断而不是返回字段校验错误(对应 Data 字段用 validate.Optional/ OptionalArray 做了可空处理,Context 却没有)。注意本文件是 message.lua.mako 生成的“DO NOT EDIT”文件,修复应落在接口模型/模板侧(把 Context 声明为可选并生成 nil 保护),再重新生成。此为可选改进,非必须修复。
建议:在模型/模板层修正后重新生成:validate 中对 Context 做 nil 判断(if self.Context ~= nil then MctpCmd.Context.new(self.Context):validate(prefix, errs, need_convert) end),或把 Context 声明为可选字段;请求侧(第 165 行)同样处理。不要手工修改此生成文件。


🔵 Low Priority
建议:将 rx_drop_no_endpoint_count 纳入已有诊断/属性输出,或直接移除该字段与两处自增。


🟡 Medium Priority
建议:核实 mc.logging 是否提供 warn 级别;若不存在则改用 log:notice/log:warning,避免在最高频的重复登记分支抛出 nil 方法调用。


🔵 Low Priority
建议:先取 medium_name 再判空,避免未知介质时 string.match 抛错,与仓库其它位置 medium_name[x] or 'UNKNOWN' 的写法对齐。
|
23 | + local function is_smbus_medium(medium_type) |
|
24 | + local name = mctp_def.medium_name[medium_type] |
|
25 | + return name ~= nil and mctp_def.s_match(name, 'SMBus') ~= nil |
| 23
26 | end |


🟡 Medium Priority
变更行(src/lualib/mctp_cmd_dispatcher.lua:243-247)把 self.inflight 加一放在 run_async 之前、减一放在协程体内 call_business 返回之后,且减一没有放在 pcall/受保护块中。
调用链:dispatch_business → run_async(skynet.fork 协程) → self:call_business(...)。call_business 内部只用 pcall 包住了 mdb.get_object(...) 与 obj:GetResponse(...),而 make_call_context(context.new())与 context.with_context(dctx, ...) 本身在 pcall 之外。一旦 context.with_context/context.new 抛错(上下文栈损坏、dctx 非法、libmc 抛错),或 skynet.fork 协程因其它未捕获错误终止,第 247 行的 self.inflight = self.inflight - 1 不会执行,名额永久泄漏。
失败模式:MCTP_CMD_MAX_INFLIGHT = 10(mctp_def.lua:87),max_inflight 检查在 dispatch_business 入口(第 233 行)。泄漏累计到 10 次后,此后所有入站 PLDM 请求都会命中 self.inflight >= max_inflight 分支,永久返回 NODE_BUSY_CODE(0xC0),端点再也不会被业务应答,且没有计数器/定时器可以自愈(超时回调只置 done,不释放名额)。同样的泄漏也会在“业务协程永不返回且调用硬超时未生效”时发生——名额回收完全依赖调用返回。
建议:把第 245 行的 call_business 调用改为 pcall 包裹(pcall(self.call_business, self, entry, data, { IID = iid })),异常时按调用失败处理,确保第 247 行的名额释放一定执行。


🔵 Low Priority
建议:订阅 mdb 属性变更(PropertiesChanged)或在登记失败时做一次延迟重试,避免属性后置/registry 未就绪导致命令永久漏注册。


🔵 Low Priority
变更行:test/unit/test_mctp_mdb_mgmt.lua:215 与 :279(self.mctp_engine.cmd_registry = c_mctp_cmd_registry.new(1))。
受影响契约:self.mctp_engine 在 setupClass(第 44 行)创建一次,是本测试类内所有用例共享的实例;c_mctp_mdb_mgmt.mctp_engine 也被赋成同一实例。失败模式:两个新增用例把共享引擎内置的 cmd_registry 整体替换为空的新注册表,用例结束既不恢复也不在 setup/teardown 重建;luaunit 同类内用例执行顺序不保证,若 test_register_cmd_object 先于 test_rpc_method/test_add_objects/test_find_endpoint 执行,后续用例看到的是测试临时构造(甚至已被 unregister 清空)的命令反查表,一旦被测路径读取 cmd_registry 就会出现顺序相关的偶发失败或误通过。修复:在 setup/teardown 中替换与恢复,或为每个用例重建 engine。
建议:在 setup() 中保存并替换 cmd_registry、在 teardown() 中恢复,或每个用例重建 engine,避免污染共享实例的既有序状态。


🔵 Low Priority
变更行:test/unit/test_mctp_engine.lua:22-24(新增文件 setupClass 中直接 c_mctp_engine.new(nil, 1))。
受影响契约:单测运行在 skynet mock 环境,skynet.fork 同步执行、skynet.sleep 为空实现(见本次改动 test/unit/test_mctp_mdb_mgmt.lua:19-31 新增的 stub 与注释,以及 test/unit/test_libmctp/test_mctp_engine.lua 中"new discovery 之前先 hook c_tasks"的既有约定)。失败模式:新文件在构造引擎前没有任何 c_tasks 钩子,若引擎构造或其 rx/mctp_control_rx 触发发现流程启动 mc.orm.tasks 的 loop,在同步 fork + 空 sleep 下会变成同步死循环,使整个单测进程挂死而非给出失败信息。修复:按兄弟用例做法在 setupClass 先替换 c_tasks.get_instance 为假 task,teardownClass 恢复。
建议:setupClass 中先保存并替换 c_tasks.get_instance(loop/set_timeout_ms/stop 均为空实现的假 task)再创建引擎,teardownClass 恢复原函数。


🔵 Low Priority
变更点:test_register_mctp_cmd_invalid_opts 遍历 8 个 mutator,每个只断言 `handle == nil` 与 `type(err) == 'string'`(第 73-78 行)。
行为/契约:这 8 个用例分别覆盖“非表 opts”“缺 message_type”“sub_type 越界”“command_code 为负/非整数/超 16 位”“on_request 非函数”“禁止注册 MCTP Control”等互相独立的校验分支。
失效模式:断言只要求“返回字符串错误”,因此只要实现里存在任意一个粗粒度校验(例如只校验 opts 是 table,或统一返回 "invalid opts"),8 个用例会全部通过,而 sub_type 越界、command_code 非整数/超界、on_request 类型、Control 类型禁止注册这些真正的边界校验即使缺失或写错(如少判 `0x100`、把 `<=` 写成 `<`)也无法被发现。这正是该 PR 新增注册接口的输入边界,属于高风险分支。
建议:把 mutators 改为 { mutate = fn, expect_err = '片段' } 形式,对每个分支断言可区分的错误信息,避免所有分支共用一个粗略校验也能全部通过。


🔵 Low Priority
变更行:test/unit/test_libmctp/test_mctp_tx.lua:347(新增用例 test_different_medium_independent 中 self.mctp_proto.smbus_eid = 10)。
受影响契约:self.mctp_proto 来自 setupClass(第 43 行),是整个测试类共享的单例;setup()(第 54-60 行,本 diff 也改动了这里)只重置 pcie_eid、m_wrong_seq_cnt、m_incomplete_msg_cnt、m_unassoc_msg_cnt,并不重置 smbus_eid。
失败模式:该用例把共享协议的 smbus_eid 从默认值改成 10 后不恢复。luaunit 遍历测试方法的顺序不保证(pairs 顺序),若该用例先于其它用例执行,后续"目的 EID 不是本端"类用例(如 test_wrong_dest_eid_silently_dropped、test_drop_unassoc_counted 中的目的过滤断言)在 SMBus 相关判定上会看到被动过的绑定参数,出现用例顺序相关的偶发失败/误通过。修复:在 setup() 中把 smbus_eid 一并复位(或在该用例末尾恢复原值)。
建议:在 TestMctpSeqValidation:setup() 中补一行 self.mctp_proto.smbus_eid = nil(或保存/恢复原值),使该用例对共享协议的修改不外溢;也可在该用例末行恢复原 smbus_eid。


🔵 Low Priority
变更点:新增用例 test_inbound_response_not_dispatched 只断言 `#business_calls == 0` 与 `#tx == 0`。
行为/契约:该用例要验证的是“入站响应(Rq=0)被识别为响应、因此不进入命令分发且不产生应答”。
失效模式:这两个断言只检查“最终没有副作用”。只要报文在 mctp/pldm 层、端点查找阶段或任何更早环节被丢弃(例如 endpoint 未创建、pldm 层因无在途事务拒绝解析),用例同样会通过,从而对“响应误入分发并回包”这一真正回归不敏感。这也是本 PR 的核心需求(responder 只处理 Rq=1)所依赖的关键判定,缺少能证明报文确实到达 PLDM/端点层的断言。
建议:在该用例中补充能证明报文到达 PLDM/端点层的断言(例如用 new_decoder 解析注入帧并断言 ctx.rq == 0),而不仅仅断言无业务回调、无回包。


🔵 Low Priority
变更点:`local DUMP = os.getenv('MCTP_E2E_DUMP') ~= '0'`(第 32 行)使报文打印默认开启,dump_frame 在 inject/teardown 中对每个用例打印线上字节与解析字段。
行为/契约:单元测试本应由 `test/unit/test.lua` 中的 `lu.LuaUnit.run()` 输出结构化结果。
失效模式:默认开启后,每次单测运行都会向 stdout 混入若干 `[MCTP-E2E] ...` 行。若 CI 以机器可读格式(如 luaunit 的 junit/TAP 输出)收集结果,这些额外行会破坏输出解析,导致结果解析失败或被误判;即使使用文本模式,也会显著放大日志量、淹没真正的失败信息。留证需求可以通过显式开启环境变量满足,不需要默认打开。
建议:把默认行为改为关闭:DUMP 仅在显式设置 MCTP_E2E_DUMP=1 时开启,避免单测输出被默认污染。
| 32
| - local DUMP = os.getenv('MCTP_E2E_DUMP') |
|
32 | + local DUMP = os.getenv('MCTP_E2E_DUMP') == '1' |


🔵 Low Priority
变更点:inject() 对每一次注入都调用 `lu.assertEquals(self.engine.hw_pcie_proto:pop(frame, nil, mctp_def.PHY_MEDIUM_PCI), 0)`,把“报文被接收链接受”等同于“返回 0”。
行为/契约:当被注入的是对端主动发来的响应帧(rq=false,见第 295 行 `build_frame({ rq = false })`)时,协议栈没有任何在途事务可匹配,实现对这类无主响应返回非 0(例如“无匹配事务”错误码)是完全合理的语义。
失效模式:一旦实现以非 0 返回表示“该帧被丢弃/无匹配事务”,test_inbound_response_not_dispatched 会在 inject 的这行断言上失败,失败原因与被测行为无关(是测试前置过强),造成误报失败或迫使实现为了通过测试而返回 0,弱化“丢弃无主响应”的表达。
建议:让 inject 接受一个可选参数表示期望的 pop 返回值(请求方向断言 0;响应方向允许 0 或“无匹配事务”错误码),避免测试前置与被测语义耦合。


[P2] 回调完成时应验证注册条目的身份/代际,而不只是path。current.path ~= entry.path无法识别同路径注销后重新注册;命令path由system/MT/ST/CC确定,替换同一命令仍用同一路径。触发顺序:旧entry的GetResponse在途→unregister(path)→同三元组同path注册新entry→旧GetResponse返回。检查通过并把旧业务状态成功发给对端,cmd_inflight_unregistered仍为0。已直接加载本HEAD原dispatcher与registry,用协程暂停旧请求、注销/重注册后恢复,得到cc=0、body=OLD_OBJECT_RESULT、计数0。建议比较current ~= entry或固化每次注册generation,并增加同path替换测试;现有测试只覆盖注销后不重建。该根因在前一PR !19的199107926意见中也已指出,当前新PR仍保留。


CLA 签署引导
@Byo_wupengfei , 感谢您的代码贡献。
以下 Commit(s) 没有签署 Contributor License Agreement (CLA)。
| Commit | 原因 |
|---|---|
| 4ae8ed0c Add new feature | 这条 Commit 的邮箱未签署 CLA !请检查下邮箱是否正确配置。 |
如果您需要签署 CLA,您可以点击此链接去签署 CLA 。
如果需要检查邮箱是否配置正确, 可以点击 FAQs 去查看使用手册 。
在完成 CLA 签署或者更新完邮箱后, 需再次评论 /check-cla 来检查 CLA 的签署状态。


###注意:
当前PR代码已经发生变化(代码更新或者PR关闭),所以移除以下标签: limit_1k_failed.


CI已开始运行,查看流水线详情:流水线链接


| 任务名称 | 结果 |
|---|---|
| pre-commit检查 | ✅ SUCCESS |
| PR描述检查 | ✅ SUCCESS |
| 防投毒检查 | ✅ SUCCESS |
| 开源合规检查 | ✅ SUCCESS |
| 编译构建 | ❌ FAILED |
| UT测试 | ✅ SUCCESS |
| IT测试 | ✅ SUCCESS |
| 代码覆盖率 | ❌ FAILED |
| 修改行数检查 | ❌ FAILED |
| 头文件检查 | ✅ SUCCESS |
| AI Review | ✅ SUCCESS |
| ARM UT测试 | ✅ SUCCESS |


当前PR是否有AI参与:
[x] 否
[ ] 是
__1. AI Agent 平台:
__2. AI 模型:
__3. Prompt上下文 :
PR功能描述 / 为什么需要这个合入:
添加支持通用EP端响应mctp命令功能
修改方案:
新增BMC支持EP端响应mctp pldm命令
验证方案:
在仿真环境下用工具模拟验证
测试建议:
模拟网卡发送报文验证BMC回包情况
自验证报告:
待后续结果出来之后补充
该PR关联的issue
fixes #10
fixes #9
fixes #8
fixes #7
fixes #6
fixes #5
fixes #4
希望检视人员了解: