Modified generate_initiator_request - #12
Conversation
| /// Sets the MCTP message-type byte at `msg_buf[0]` and writes the PLDM | ||
| /// request starting at `msg_buf[1]`. | ||
| /// | ||
| /// Returns `Ok(0)` when no request is pending (nothing to send). |
There was a problem hiding this comment.
From fd_progress it seems, as if Ok(0) can have two meanings:
- The
fd_statespecific handlers returned aresult = 0. That means, that the handlers (fd_progress_download,pldm_fd_progress_verify,pldm_fd_progress_apply) returned a0. - If a response is not received in T1, cancel the update and transition to idle
In the second case, is a silent cancel the correct handling of a failed update?
There was a problem hiding this comment.
I haven't implemented T1 timeouts yet. Can we make this a github issue and track it in there?
There was a problem hiding this comment.
who should make the issue? can you link it here?
There was a problem hiding this comment.
Or actually, can we change the contract to something like:
Some(n) = transmit msg_buf[..n]
None = nothing due, poll again
Err(UpdateCancelled) = update died
There was a problem hiding this comment.
Or actually, can we change the contract to something like:
Some(n) = transmit msg_buf[..n] None = nothing due, poll again Err(UpdateCancelled) = update died
Aren't the errors handle by fd_progress(payload)? and .map_err(MsgHandlerError::Util)?? Do we need another match?
There was a problem hiding this comment.
No extra match needed — the problem is that the T1 cancel path isn't an error. It's already implemented (fd_context.rs:661-674) and returns Ok(0), so it becomes None, same as "verify still in progress" (fd_context.rs:948). The caller can't tell a running update from a dead one.
Keep #13 open for the T1/T2 timestamp work, but in this PR make the T1-cancel branch return an error (e.g. UpdateCancelled) so None has exactly one meaning: nothing due, poll again.
|
This is potentially an API breaking change. Do we already have any consumers (here or openprot) that would need updating? |
| /// Sets the MCTP message-type byte at `msg_buf[0]` and writes the PLDM | ||
| /// request starting at `msg_buf[1]`. | ||
| /// | ||
| /// Returns `Ok(0)` when no request is pending (nothing to send). |
There was a problem hiding this comment.
who should make the issue? can you link it here?
| /// request starting at `msg_buf[1]`. | ||
| /// | ||
| /// Returns `Ok(0)` when no request is pending (nothing to send). | ||
| /// Returns `Ok(n)` where `n` is the total number of bytes written to |
There was a problem hiding this comment.
Update doc (and examples and test code, if there are any)
There was a problem hiding this comment.
The only consumer is run_terminus.
There was a problem hiding this comment.
What needs updating here? Do you need me to mention errors returned on fd_progress or construct_mctp_pldm_msg? That is where errors are returned.
There was a problem hiding this comment.
The function returns Ok(Some(n)) now instead of Ok(n)
There was a problem hiding this comment.
The Returns section above still documents Ok(0)/Ok(n) — update it to describe Some(n)/None.
| /// Sets the MCTP message-type byte at `msg_buf[0]` and writes the PLDM | ||
| /// request starting at `msg_buf[1]`. | ||
| /// | ||
| /// Returns `Ok(0)` when no request is pending (nothing to send). |
There was a problem hiding this comment.
Or actually, can we change the contract to something like:
Some(n) = transmit msg_buf[..n]
None = nothing due, poll again
Err(UpdateCancelled) = update died
… use in Open-Prot
42eb8c6 to
f7a2bb3
Compare
| /// request starting at `msg_buf[1]`. | ||
| /// | ||
| /// Returns `Ok(0)` when no request is pending (nothing to send). | ||
| /// Returns `Ok(n)` where `n` is the total number of bytes written to |
There was a problem hiding this comment.
The function returns Ok(Some(n)) now instead of Ok(n)
| /// request starting at `msg_buf[1]`. | ||
| /// | ||
| /// Returns `Ok(0)` when no request is pending (nothing to send). | ||
| /// Returns `Ok(n)` where `n` is the total number of bytes written to |
There was a problem hiding this comment.
Is the doc consistent with the code? Is the total number of bytes written pldm_len or msg_buf length or so?
| /// Sets the MCTP message-type byte at `msg_buf[0]` and writes the PLDM | ||
| /// request starting at `msg_buf[1]`. | ||
| /// | ||
| /// Returns `Ok(0)` when no request is pending (nothing to send). |
There was a problem hiding this comment.
No extra match needed — the problem is that the T1 cancel path isn't an error. It's already implemented (fd_context.rs:661-674) and returns Ok(0), so it becomes None, same as "verify still in progress" (fd_context.rs:948). The caller can't tell a running update from a dead one.
Keep #13 open for the T1/T2 timestamp work, but in this PR make the T1-cancel branch return an error (e.g. UpdateCancelled) so None has exactly one meaning: nothing due, poll again.
3 participants
Creates a cleaner interface for use in Open-Prot.