[Darwin] Add a MdnsContexts::Print method for debugging purposes #36763
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
|
Review changes with |
|
PR #36763: Size comparison from af8d173 to 807964f Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
andy31415
reviewed
Dec 9, 2024
| @@ -259,6 +259,36 @@ CHIP_ERROR MdnsContexts::Has(GenericContext * context) | |||
| return CHIP_ERROR_KEY_NOT_FOUND; | |||
| } | |||
|
|
|||
| #ifdef DEBUG | |||
There was a problem hiding this comment.
nit: within chip we prefer #if instead of ifdef, so we guarantee the existence of the condition.
Would it make sense to maybe have a config for mdns debug instead of a global debug flag?
There was a problem hiding this comment.
| Would it make sense to maybe have a config for mdns debug instead of a global debug flag?
If you’re thinking of something similar to lwip_debug, I agree it could be useful.
However, I’d generally prefer a solution like ChipLogDebug(Discovery, ...) that can be toggled on or off easily, such as through an environment variable or a build-time define.
This approach would be less intrusive than adding #if ... #endif directives throughout the code and aligns better with our current practices.
Followup ?
There was a problem hiding this comment.
Or did you mean just for src/platform/Darwin ?
There was a problem hiding this comment.
I would be thinking of a gn flag inside platform/Darwin that controls these things. You should also likely use a build_header instead of using defines, since defines leak throughout all calls and DEBUG sounds like a very generic define to use.
My recommendation would be to have some config and have a constant in there for MDNS_VERBOSE_CONTEXT_PRINT (or some other name that seems intuitive)
andy31415
reviewed
Dec 9, 2024
| break; | ||
| } | ||
|
|
||
| ChipLogDetail(Discovery, "\t%p: %s", *iter, contextType); |
There was a problem hiding this comment.
what is the value of printing pointer values here?
There was a problem hiding this comment.
We can have multiple resolves in flight simultaneously. When debugging, printing the pointer serves as a unique identifier, making it easier to track which resolve operation is performing which actions.
There was a problem hiding this comment.
Fair, let's add that as a comment.
bzbarsky-apple
approved these changes
Dec 10, 2024
vivien-apple
force-pushed
the
Darwin_DebugMdnsContextsWithPrint
branch
from
807964f to
947391d
Compare
|
PR #36763: Size comparison from 2c11741 to 947391d Full report (10 builds for cc32xx, nrfconnect, qpg, stm32, tizen)
|
vivien-apple
force-pushed
the
Darwin_DebugMdnsContextsWithPrint
branch
from
947391d to
84c7a82
Compare
|
PR #36763: Size comparison from 2c11741 to 84c7a82 Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
andy31415
reviewed
Dec 13, 2024
There was a problem hiding this comment.
@vivien-apple please add a Testing section on how this was tested.
It seems to me this is "manual" just by looking at logs. We need to meet a higher (i.e. more annoying to type everything out) bar for manual so that we do not have most PRs marked as manual because it is more convenient than writing tests. Please add justification why automated tests are imposssible (seems like this is platform dnssd triggering and logging for debug only, so maybe impractical or of little user value maybe) and a description on how you tested and ensured that your manual tests had good coverage of the added code.
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.
Problem
This PR adds a
MdnsContexts::Printmethod for listing the different contexts currently registered. That is for debugging purposes.