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.

Sign up for GitHub

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

Jump to bottom

More packet memory stuff #141

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

More packet memory stuff #141

wants to merge 5 commits into from

Conversation

pallas
Copy link
Collaborator

@pallas pallas commented Nov 15, 2018

No description provided.

Derrick Pallas added 4 commits November 14, 2018 20:05
click/etherswitch: bail out on OOM
a4caeb1 
Change-Id: Iedfff2d0be5fb8686a28fd8acb775c94f2fd7e63
Reviewed-on: https://gerrit.ikarem.io/74140
Static-Analysis: Jenkins Build System <[email protected]>
Tested-by: Jenkins Build System <[email protected]>
Reviewed-by: Peter Hurley <[email protected]>
Signed-off-by: Derrick Pallas <[email protected]>
click/fromfile: plug mmap leak in error path
1868fae 
If mmap is successful but Packet::make fails to create a new SKB, we need to
execute the destructor we passed to Packet::make.  It actually might make
sense to do this in Packet::make instead instead of the caller, not sure
which is better.

Change-Id: If97d221daf3443003f40097cc36f7410bb544669
Reviewed-on: https://gerrit.ikarem.io/74137
Static-Analysis: Jenkins Build System <[email protected]>
Tested-by: Jenkins Build System <[email protected]>
Reviewed-by: Peter Hurley <[email protected]>
Signed-off-by: Derrick Pallas <[email protected]>
click/hashtable: assert that operator[] does not return a NULL reference
7aec8b8 
There is no such thing as a NULL reference, so there is no way to tell if
find_insert failed when invoked by operator[].  Some old code attempted to
check !&x for the reference, but the compiler can optimize it out since it
is "impossible" according to the ARM.  Therefore, we should just assert that
find_insert succeeds.

Change-Id: I1d34e19033e77dd6aebfdba76d553e927711a62c
Signed-off-by: Derrick Pallas <[email protected]>
click/icmp: bail out on OOM
f2a3821 
Change-Id: Ia7b9f3c4a4e7547d17251edac67c75f967507b3f
Reviewed-on: https://gerrit.ikarem.io/74139
Static-Analysis: Jenkins Build System <[email protected]>
Tested-by: Jenkins Build System <[email protected]>
Reviewed-by: Peter Hurley <[email protected]>
Signed-off-by: Derrick Pallas <[email protected]>
@pallas
Copy link
Collaborator Author

pallas commented Nov 15, 2018

As an aside, InfiniteSource is broken but the fastclick copy is different enough now from my copy that the patch is not clean.

@tbarbette
Copy link
Owner

Should be fixed.This file was never modified in FastClick, I wonder why on your side the "q" got renamed in "p" :p

I just wonder about the assert. Personally, I never disable them, so it will cost a little. Is it really necessary? If we don't add "null" to the hashtable, we'll never get null, right?

@tbarbette
Copy link
Owner

I'll let you rebase, and remove what's unneeded. And for the assert?

@bcronje
Copy link
Collaborator

bcronje commented Nov 15, 2018

I just wonder about the assert. Personally, I never disable them, so it will cost a little. Is it really necessary? If we don't add "null" to the hashtable, we'll never get null, right?

i.get() here refers to the hashtable element, not the value. So I think it could be that OOM can cause this assert to fail. However we also dont disable asserts and operator[] could possibly be used very frequently so I'm not sure this is a good idea. How about using click_hash_assert as defined in hashcontainer.hh where CLICK_DEBUG_HASHMAP should be enable for the assert to take affect?

@tbarbette tbarbette added the wait-for-op Additional information from the OP are needed label Dec 3, 2018
@tbarbette tbarbette force-pushed the master branch 2 times, most recently from b9c1853 to b9f3521 Compare December 9, 2020 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
wait-for-op Additional information from the OP are needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pallas @tbarbette @bcronje