On Mon Dec 5 17:11:11 2022 +0000, eric pouech wrote:
looking a bit deeper into it:
- for the FAM field 'protocols': is there a good coverage of BindNAck
packets in tests/? from perusing the code, this change would move the protocols[] array from header to payload. Header + payload should be correctly aggregated afterwards, but I would be better off if we could test it. I don't seem to find any test for that in regression tests. Since, the packets crafted locally only fill one protocol, using simply size of struct for allocation would do for now. I'm a bit afraid of changing this right now.
- by 'getting rid of RpcPktHdr' you mean completely dropping the union?
something like that?[rpcrt4-nounion.patch](/uploads/dda6c2f3fee7748d0933dcb4b0bf3f2b/rpcrt4-nounion.patch)
- If so that's not a small change:
<<diffstat>> dlls/rpcrt4/rpc_assoc.c | 20 +-- dlls/rpcrt4/rpc_binding.h | 6 +- dlls/rpcrt4/rpc_defs.h | 24 ++-- dlls/rpcrt4/rpc_message.c | 312 +++++++++++++++++++++++--------------------- dlls/rpcrt4/rpc_message.h | 36 ++--- dlls/rpcrt4/rpc_server.c | 32 ++--- dlls/rpcrt4/rpc_transport.c | 73 ++++++----- 7 files changed, 259 insertions(+), 244 deletions(-)and it adds more casts as the previous patch as we need to cast down from CommonHdr to another Hdr. I tried to hide them thanks to rpc_castdown (ugly ;-) macro (could rename RpcPktCommonHdr into RpcPktHdr to safe a few changes, but still). There's still some uglyness in rpcrt4_http_prepare_out_pipe where getting rid of the union doesn't allow to predefine size of incoming packet... setting the type of pkt_from_server to HttpHdr hides the dust under the carpet, but IMO this should rather be dynamically allocated (like what's done in RPCRT4_default_receive_fragment) / (attached patch is wrong has it will write outside of local variable) so I'd rather keep in simple for now (no FAM, keep the union(s)!!)
Ok, let's stick with using the union.