search

openbmc / libpldm

Apache License 2.0
9 stars 12 forks source link

Why the change from < to != for response decode? #14

Open dcrowell77 opened 1 week ago

dcrowell77 commented 1 week ago

Why were all of the decoder functions changed to require exact sizes instead of "big enough" data buffers? For example from src/oem/ibm/file_io.c

if (payload_length != PLDM_RW_FILE_MEM_RESP_BYTES) {
    return PLDM_ERROR_INVALID_LENGTH;
}

This breaks some of the abstraction that we've been using because it requires the callers to explicitly understand the response structures when they didn't before.

(Note - I would normally use git blame but the "libpldm: Migrate to subproject" commit pretty much broke that without doing some bisecting so asking here.)

amboar commented 1 week ago

If it's going back past the end of the libpldm.git history I'll have to defer to @manojkiraneda as such changes are before my time.

I agree that those checks sound quite unhelpful.

I expect we can apply patches if you can push them for review @dcrowell77

manojkiraneda commented 1 week ago

@dcrowell77 seems like the change goes way back to 2019 when pldm stack was just written - even i was not involved with pldm at that time, https://github.com/openbmc/pldm/commit/8b9766599f09b7dc5c2f1def8ff3218e95990b55 commit seems those add those changes ? is it ? its very unfortunate that the commit message seems to tell nothing useful on the changes that were being done in the commit. Would be great if you can push the changes, happy to review !

dcrowell77 commented 1 week ago

I did a little digging and it seems that we've been wiping out the != check since almost the beginning. We have a script that updates our subtree that pulls in libpldm while preserving any local changes. It looks like that script confused me a bit. I can see that we picked up the != change in decode_rw_file_by_type_resp() when we did a fresh pull of the entire repo back in early 2023 (commit 0585de95306d3e4c04f89dbed2bc413a9dee8cd6). We then did a local patch on top to make it <, and we've maintained that locally the whole time. I think that the previous code was likely some pre-release version.

Net - We can push up a PR with the change. I don't have any way of testing the openbmc implementation so I'll rely on others (or some kind of CI?) to handle that.