fix(server): Don't send a copy of the PDU instead of applying the response to the original response PDU #36
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pulsastrix
added
bug
Workflow Status ReportGenerated for commit 1fa997b on Thu Nov 28 15:25:15 UTC 2024.
Linting ReportClippy check result: success Refer to the "Files Changed" tab for identified issues. Code Coverage ReportDiff against main
Results for commit: 8586afb Coverage target is |
pulsastrix
force-pushed
the
fix_observe
branch
from
8db7082 to
42f78f0
Compare
…ponse to the original response PDU This will require application-side modifications to server-side request handlers. Namely, the resource handler must no longer call coap_send for the response PDU. This change was made to conform to libcoap's API contract and to enable CoAP Observe support. Additionally, a small workaround for proper CoAP Observe support was added that prevents duplicate addition of the Observe option in server-side resource handlers. This workaround should be removed once #21 has been tackled and the API for message handling was reworked.
Explicitly freeing the endpoints before calling coap_free_context() may cause a SIGABRT if sessions are still referenced somewhere (e.g., due to the session still being observed). This change decomposes the CoapEndpoint instances into their raw variant without calling coap_free_endpoint() on dropping the context so that libcoap can perform its own cleanup before freeing the endpoints itself in coap_free_context().
…anics in callbacks
pulsastrix
force-pushed
the
fix_observe
branch
from
42f78f0 to
8586afb
Compare
pulsastrix
changed the title
JKRhb
reviewed
Dec 2, 2024
Comment on lines
353
to
354
| // Send remaining packets until we can cleanly shutdown. | ||
| // SAFETY: Provided context is always valid as an invariant of this struct. |
There was a problem hiding this comment.
Should this comment be moved above the while loop?
Comment on lines
+424
to
-432
| pub fn into_raw_pdu<'a, S: CoapSessionCommon<'a>+?Sized>( | ||
| mut self, | ||
| session: &S, | ||
| ) -> Result<*mut coap_pdu_t, MessageConversionError> { | ||
| let message = self.as_message_mut(); | ||
|
|
There was a problem hiding this comment.
If cargo fmt allows it, I would probably rather keep the formatting here.
| @@ -466,7 +462,7 @@ impl CoapMessage { | |||
| /// | |||
| /// # Safety | |||
| /// raw_pdu must point to a valid mutable instance of coap_pdu_t. | |||
| pub(crate) unsafe fn apply_to_raw_pdu<'a, S: CoapSessionCommon<'a> + ?Sized>( | |||
| pub(crate) unsafe fn apply_to_raw_pdu<'a, S: CoapSessionCommon<'a>+?Sized>( | |||
Comment on lines
+493
to
+516
| // TODO this is a really ugly workaround, remove this as soon as we have builder-like | ||
| // interfaces for PDUs. | ||
| // Do not duplicate Observe option as reapplying to the same PDU would cause the option | ||
| // to be added twice otherwise. | ||
| if let CoapOption::Observe(option) = option { | ||
| // Check if Observe has already been set. | ||
| let mut scratch: MaybeUninit<coap_opt_iterator_t> = MaybeUninit::uninit(); | ||
| let cur_option = unsafe { coap_check_option(raw_pdu, optnum, scratch.as_mut_ptr()) }; | ||
| if !cur_option.is_null() { | ||
| // Observe has already been set, get value and compare with the one set for this message. | ||
| let cur_value = unsafe { | ||
| core::slice::from_raw_parts(coap_opt_value(cur_option), coap_opt_length(cur_option) as usize) | ||
| }; | ||
| let cur_value = decode_var_len_u32(cur_value); | ||
| // If Observe option values differ, return an appropriate error. | ||
| if option != cur_value { | ||
| return Err(MessageConversionError::NonRepeatableOptionRepeated( | ||
| CoapOptionType::Observe, | ||
| )); | ||
| } | ||
| // Otherwise, skip adding the option value to the optlist to avoid a duplicate. | ||
| continue; | ||
| } | ||
| } |
There was a problem hiding this comment.
Should this (or at least parts of it) be factored out into its own function, maybe?
| @@ -50,7 +46,7 @@ use crate::session::CoapSessionCommon; | |||
| macro_rules! resource_handler { | |||
| ($f:ident, $t:path) => {{ | |||
| #[allow(clippy::unnecessary_mut_passed)] // We don't know whether the function needs a mutable reference or not. | |||
| unsafe extern "C" fn _coap_method_handler_wrapper<D: Any + ?Sized + Debug>( | |||
| unsafe extern "C" fn _coap_method_handler_wrapper<D: Any+?Sized+Debug>( | |||
| @@ -101,7 +101,7 @@ pub unsafe fn prepare_resource_handler_data<'a, D: Any + ?Sized + Debug>( | |||
| } | |||
|
|
|||
| /// Trait with functions relating to [CoapResource]s with an unknown data type. | |||
| pub trait UntypedCoapResource: Any + Debug { | |||
| pub trait UntypedCoapResource: Any+Debug { | |||
| /// Decomposes this endpoint into its raw version. | ||
| /// | ||
| /// Note that the caller is now responsible for freeing the endpoint (either by calling | ||
| /// coap_free_endpoint() or calling coap_free_context() on the associated context). |
There was a problem hiding this comment.
Should the documentation use [coap_free_endpoint()] here?
| * dtls_client_server_test.rs - Tests for UDP clients+servers. | ||
| * This file is part of the libcoap-rs crate, see the README and LICENSE files for | ||
| * more information and terms of use. | ||
| * Copyright © 2021-2023 The NAMIB Project Developers, all rights reserved. |
Comment on lines
+481
to
+487
| // Update token if necessary. | ||
| if message.token.is_some() && cur_token != message.token.as_deref() { | ||
| let token = message.token.as_ref().unwrap(); | ||
| if coap_add_token(raw_pdu, token.len(), token.as_ptr()) == 0 { | ||
| return Err(MessageConversionError::Unknown); | ||
| } | ||
| } |
There was a problem hiding this comment.
I was wondering here if you could just write the token regardless of the comparison result, as you would need to iterate over the buffers twice when they do not match.
Comment on lines
+10
to
-15
| use crate::{ | ||
| error::{MessageConversionError, MessageTypeError, OptionValueError}, | ||
| message::{construct_path_string, construct_query_string, CoapMessage, CoapMessageCommon, CoapOption}, | ||
| protocol::{ | ||
| CoapMessageCode, CoapMessageType, CoapOptionType, CoapResponseCode, ContentFormat, ETag, Echo, MaxAge, Observe, | ||
| }, | ||
| types::CoapUri, | ||
| }; | ||
| use crate::types::CoapUri; |
There was a problem hiding this comment.
Maybe the (unrelated) reformatting could be factored out of this PR in general to keep the diff a bit cleaner. But that is not strictly necessary.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change will require application-side modifications to server-side request handlers. Namely, the resource handler must no longer call coap_send for the response PDU.
This change was made to conform to libcoap's API contract and to enable CoAP Observe support.
Additionally, a small workaround for proper CoAP Observe support was added that prevents duplicate addition of the Observe option in server-side resource handlers.
This workaround should be removed once #21 has been tackled and the API for message handling was reworked.
Lastly, there was an issue when calling
CoapContext::dropon a server while a resource was observed by at least one client because libcoap keeps the server session referenced in this case.As we tried to explicitly free the endpoints before calling
coap_free_context(), this caused a SIGABRT as the sessions prevent freeing the associated endpoint.Closes #14