On Apr 25 10:19, Corey Minyard wrote: > On Tue, Apr 25, 2023 at 08:35:38AM +0200, Klaus Jensen wrote: > > From: Klaus Jensen <k.jen...@samsung.com> > > > > Add an abstract MCTP over I2C endpoint model. This implements MCTP > > control message handling as well as handling the actual I2C transport > > (packetization). > > > > Devices are intended to derive from this and implement the class > > methods. > > > > Parts of this implementation is inspired by code[1] previously posted by > > Jonathan Cameron. > > All in all this looks good. Two comments: > > I would like to see the buffer handling consolidated into one function > and the length checked, even for (especially for) the outside users of > this code, like the nvme code. Best to avoid future issues with buffer > overruns. This will require reworking the get_message_types function, > unfortunately. >
Right now the implementations (i.e. hw/nvme/nmi-i2c.c) writes directly into the mctp core buffer for get_message_bytes(). The contract is that it must not write more than the `maxlen` parameter. Is that bad? Would it be better that get_message_bytes() returned a pointer to its own buffer that hw/mctp can then copy from? > You have one trace function on a bad receive message check, but lots of > other bad receive message checks with no trace. Just a suggestion, but > it might be nice for tracking down issues to trace all the reasons a > message is dropped. > Sounds reasonable! :) Thanks for the review!
signature.asc
Description: PGP signature
