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

Merge buffer allocation methods #747

Closed
wants to merge 0 commits into from
Closed

Conversation

HTRamsey
Copy link
Contributor

@HTRamsey HTRamsey commented Feb 27, 2023

Merge Buffer Allocations methods

Description

The two files had fallen out of sync but can share a significant amount of code. A config macro should be defined to determine which method to use.

Checklist:

  • I have tested my changes. No regression in existing tests.
  • I have modified and/or added unit-tests to cover the code changes in this Pull Request.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@HTRamsey HTRamsey requested a review from a team as a code owner February 27, 2023 16:33
@HTRamsey
Copy link
Contributor Author

/bot run uncrustify

@htibosch
Copy link
Contributor

Thank you @holden-zenith for this welcome initiative.

BufferAllocation_1.c indeed has much more safety checks. That is the module that is mostly used.

Two problems can emerge:

  • A leak of buffers: they get allocated but not freed.
  • Buffers get released twice.

In my private copy I have even added two hidden fields: the places where a buffer was allocated, and where it was released. I don't think that this is useful as a general feature.

Functions like bIsValidNetworkDescriptor() are very useful and it would be nice if such functions are available in both files.

Please ping me if you want to have the changes tested.

@HTRamsey
Copy link
Contributor Author

@htibosch if you would like to test it that would be great. I've run them both on my STM32F7 so far. If you have any suggestions or changes let me know.

@amazonKamath amazonKamath reopened this Mar 1, 2023
@amazonKamath
Copy link
Member

@holden-zenith we will review your PR next week and provide further feedback. Apologies for the delay, we are in the middle of pushing a release candidate on FreeRTOS:dev/IPv6_integration

@HTRamsey HTRamsey marked this pull request as draft March 18, 2023 21:21
@HTRamsey HTRamsey closed this Mar 21, 2023
@HTRamsey HTRamsey reopened this Mar 21, 2023
@HTRamsey HTRamsey changed the title Synchronize buffer allocation methods Merge buffer allocation methods Mar 21, 2023
@moninom1 moninom1 mentioned this pull request May 29, 2023
2 tasks
@HTRamsey HTRamsey closed this Jul 22, 2023
@HTRamsey HTRamsey force-pushed the main branch 2 times, most recently from 256694b to 3210021 Compare July 22, 2023 15:04
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

Successfully merging this pull request may close these issues.

3 participants