-
Notifications
You must be signed in to change notification settings - Fork 134
Fix : vendor_response_get_id should not exist #3325
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
base: main
Are you sure you want to change the base?
Conversation
f072069 to
b00d055
Compare
include/library/spdm_common_lib.h
Outdated
|
|
||
| /** | ||
| * Vendor Response Callback Function Pointer. | ||
| * The library invokes this callback once. The integrator should: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The library invokes this callback once.
Can just delete this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| #if LIBSPDM_ENABLE_VENDOR_DEFINED_MESSAGES | ||
|
|
||
| /* expected number of bytes for VENDOR MESSAGE HEADERS */ | ||
| #define SPDM_VENDOR_DEFINED_FIXED_HEADER_LEN 7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pre-existing issue, but I do not see a reason for this #define to exist. I will file a separate issue for it.
| /* length of spdm request/response header before payload start */ | ||
| header_length = sizeof(spdm_vendor_defined_response_msg_t) + spdm_request->len + | ||
| /* reserve max vendor ID in header */ | ||
| header_length = sizeof(spdm_vendor_defined_response_msg_t) + SPDM_MAX_VENDOR_ID_LENGTH + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ShitalJumbad We (mostly me) have been a little lazy in defining SPDM_MAX_VENDOR_ID_LENGTH as 255 bytes. Let me file an issue, resolve it with a pull request, and then you can rebase with the new value. This should simplify things so that you do not need to
compact payload after Vendor ID
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/DMTF/SPDM-WG/issues/4396 has complicated things. Let me resolve that first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After discussing this in the SPDM Working Group, for IANA CBOR the VendorId field is the CBOR-encoded tag number. As such its maximum size is nine bytes: 0xdb plus eight bytes for the tag number. I will update SPDM_MAX_VENDOR_ID_LENGTH to this more precise value.
include/library/spdm_common_lib.h
Outdated
| uint16_t *resp_standard_id, | ||
| uint8_t *resp_vendor_id_len, | ||
| void *resp_vendor_id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ShitalJumbad as discussed in the meeting today, please remove these parameters and have libspdm just copy the standard ID and vendor ID from the request to the response in libspdm_rsp_vendor_response.c.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
b00d055 to
bc2c5e6
Compare
Signed-off-by: Shital Jumbad <[email protected]>
bc2c5e6 to
6062a2d
Compare
fix #3045