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

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

Open
dcrowell77 opened this issue Nov 11, 2024 · 3 comments
Open

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

dcrowell77 opened this issue Nov 11, 2024 · 3 comments

Comments

@dcrowell77
Copy link

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
Copy link
Member

amboar commented Nov 11, 2024

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
Copy link
Contributor

@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, openbmc/pldm@8b97665 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
Copy link
Author

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 0585de9). 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.

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

No branches or pull requests

3 participants