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

chg: Use static memory buffer for MAC address conversion #39

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

BenBE
Copy link
Contributor

@BenBE BenBE commented Oct 2, 2018

This avoids memory allocations when trying to output the hardware MAC address in various situations.

Multiple buffers are provided to allow for multiple MAC addresses to be processed before copies need to be created.

@sargon
Copy link
Owner

sargon commented Jan 7, 2019

I currently see no use case for more than one buffer.
We only use this function in debug output.

@BenBE
Copy link
Contributor Author

BenBE commented Jan 11, 2019

Use case is avoiding dynamic memory allocation. Also this simplifies use in code a bit as the return value can be used directly by the caller.

tools.c Outdated
@@ -79,15 +79,16 @@ dhcp_option* parse_option() {
return option;
}

char* hwaddr2c(uint8_t* hwaddr) {
char* str = calloc(18, sizeof(char));
#define hwaddr_strcount 16u
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please comment in code on the intention of this constant.
Since we currently only need one of these buffers set it to one.

This patch uses several buffers in a round-robin fashion to avoid overwriting a just-returned result.
The number of buffers internally used is choosen at 16 to allow for enough spare buffers even
when more complicated situations need to be processed.
@christf
Copy link
Contributor

christf commented Sep 8, 2019

using multiple buffers is required when logging more than one address on the same line. I am using a similar mechanism for debug output in l3roamd, allowing either 4 small buffers or one large one.
It is easy to adjust. And I would defenitely keep the functionality to use multiple buffers.

It should be easy enough to increase when demand arises.

OTOH: Does the memory overhad of those 18 Bytes really justify not merging the patch set as it is?

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