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

ng_pipe: Various improvements #1181

Closed
wants to merge 4 commits into from
Closed

ng_pipe: Various improvements #1181

wants to merge 4 commits into from

Conversation

gartens
Copy link

@gartens gartens commented Apr 17, 2024

Various small improvements to ng_pipe(4):

  • Fix various whitespace issues
  • Replace deprecated random(9) with arc4random(9)
  • Remove the node when all hooks are disconnected. This is the behavior described in the man page
  • Do not panic when memory allocations fail

Martin Vahlensieck added 2 commits April 17, 2024 21:21
ng_pipe: Fix whitespace
a75976d 
Signed-off-by: Martin Vahlensieck <git@academicsolutions.ch>
ng_pipe: Remove node when all hooks are disconnected
699b48a 
This is the behavior described in the man page.

Signed-off-by: Martin Vahlensieck <git@academicsolutions.ch>
@bsdimp
Copy link
Member

bsdimp commented Apr 17, 2024

Authorized ci

markjdb
markjdb reviewed Apr 17, 2024
View reviewed changes
sys/netgraph/ng_pipe.c Outdated
@@ -771,7 +771,7 @@ pipe_dequeue(struct hookinfo *hinfo, struct timeval *now) {
* the original packet...
*/
if (hinfo->cfg.duplicate &&
random() % 100 <= hinfo->cfg.duplicate) {
arc4random() % 100 <= hinfo->cfg.duplicate) {
Copy link
Member

Choose a reason for hiding this comment

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

Why arc4random() instead of prng32() or prng32_bounded()? The former is a cryptographic PRNG, which doesn't look to be what you want.

Copy link
Author

@gartens gartens Apr 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Changed, as well as the other random(9) replacement.

markjdb
markjdb reviewed Apr 17, 2024
View reviewed changes
sys/netgraph/ng_pipe.c Outdated
@@ -814,7 +828,7 @@ pipe_dequeue(struct hookinfo *hinfo, struct timeval *now) {
/* Randomly discard the frame, according to BER setting */
if (hinfo->cfg.ber) {
oldrand = rand;
rand = random();
rand = prng32() & 0x7fffffff;
Copy link
Member

Choose a reason for hiding this comment

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

This calls for prng32_bounded() as well.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

sys/netgraph/ng_pipe.c Outdated
@@ -976,7 +998,7 @@ ngp_modevent(module_t mod, int type, void *unused)
sizeof (struct ngp_fifo)), NULL, NULL, NULL, NULL,
UMA_ALIGN_PTR, 0);
if (ngp_zone == NULL)
panic("ng_pipe: couldn't allocate descriptor zone");
error = ENOMEM;
Copy link
Member

Choose a reason for hiding this comment

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

This check can simply be removed: uma_zcreate() never fails.

Copy link
Author

Choose a reason for hiding this comment

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

A quick grep shows that there are various places where the result of uma_zcreate() is checked (e.g. here or here). How are they different?

Copy link
Member

Choose a reason for hiding this comment

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

They aren't different, that code doesn't need to check either. It's just cargo-culting.

Copy link
Author

Choose a reason for hiding this comment

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

Removed

sys/netgraph/ng_pipe.c
@@ -658,7 +660,8 @@ ngp_rcvdata(hook_p hook, item_p item)
break;
if (ngp_f == NULL) {
ngp_f = uma_zalloc(ngp_zone, M_NOWAIT);
KASSERT(ngp_h != NULL, ("ngp_h zalloc failed (2)"));
if (ngp_f == NULL)
return (ENOMEM);
Copy link
Member

Choose a reason for hiding this comment

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

Here you're leaking ngp_h (and maybe other things).

Copy link
Author

Choose a reason for hiding this comment

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

Good catch!

ng_pipe: Replace deprecated random() with prng32_bounded()
222f7b8 
Signed-off-by: Martin Vahlensieck <git@academicsolutions.ch>
ng_pipe: Do not panic when memory allocations fail
8b180e5 
Signed-off-by: Martin Vahlensieck <git@academicsolutions.ch>
@markjdb
Copy link
Member

markjdb commented Apr 19, 2024

This looks good to me. Maybe @glebius could comment on the functional change (destroying the node when hooks are removed)?

@glebius
Copy link
Member

glebius commented Apr 23, 2024

Given this behavior is documented and also it is typical for many other nodes, I believe the patch does the right thing.

@bsdimp bsdimp self-assigned this Apr 23, 2024
@bsdimp bsdimp added ready and removed needs-review labels Apr 23, 2024
@markjdb markjdb added merged and removed ready labels Apr 24, 2024
@markjdb
Copy link
Member

markjdb commented Apr 24, 2024

Landed, thanks.

@markjdb markjdb closed this Apr 24, 2024
freebsd-git pushed a commit that referenced this pull request Apr 24, 2024
@markjdb
ng_pipe: Fix whitespace
8512311 
Signed-off-by: Martin Vahlensieck <git@academicsolutions.ch>

Reviewed by:	markj
MFC after:	2 weeks
Pull Request:	#1181
freebsd-git pushed a commit that referenced this pull request Apr 24, 2024
@markjdb
ng_pipe: Remove node when all hooks are disconnected
bb2ab7a 
This is the behavior described in the man page.

Signed-off-by: Martin Vahlensieck <git@academicsolutions.ch>

Discussed with:	glebius
Reviewed by:	markj
MFC after:	2 weeks
Pull Request:	#1181
freebsd-git pushed a commit that referenced this pull request Apr 24, 2024
@markjdb
ng_pipe: Replace deprecated random() with prng32_bounded()
a3ecf8c 
Signed-off-by: Martin Vahlensieck <git@academicsolutions.ch>

MFC after:	2 weeks
Reviewed by:	markj
Pull Request:	#1181
freebsd-git pushed a commit that referenced this pull request Apr 24, 2024
@markjdb
ng_pipe: Do not panic when memory allocations fail
d44c780 
Signed-off-by: Martin Vahlensieck <git@academicsolutions.ch>

Reviewed by:	markj
MFC after:	2 weeks
Pull Request:	#1181
@gartens
Copy link
Author

gartens commented Apr 26, 2024

Thank you for your help!

freebsd-git pushed a commit that referenced this pull request May 8, 2024
@markjdb
ng_pipe: Fix whitespace
a709ac6 
Signed-off-by: Martin Vahlensieck <git@academicsolutions.ch>

Reviewed by:	markj
MFC after:	2 weeks
Pull Request:	#1181

(cherry picked from commit 8512311)
freebsd-git pushed a commit that referenced this pull request May 8, 2024
@markjdb
ng_pipe: Remove node when all hooks are disconnected
a3ca254 
This is the behavior described in the man page.

Signed-off-by: Martin Vahlensieck <git@academicsolutions.ch>

Discussed with:	glebius
Reviewed by:	markj
MFC after:	2 weeks
Pull Request:	#1181

(cherry picked from commit bb2ab7a)
freebsd-git pushed a commit that referenced this pull request May 8, 2024
@markjdb
ng_pipe: Replace deprecated random() with prng32_bounded()
f4ab464 
Signed-off-by: Martin Vahlensieck <git@academicsolutions.ch>

MFC after:	2 weeks
Reviewed by:	markj
Pull Request:	#1181

(cherry picked from commit a3ecf8c)
freebsd-git pushed a commit that referenced this pull request May 8, 2024
@markjdb
ng_pipe: Do not panic when memory allocations fail
50af25e 
Signed-off-by: Martin Vahlensieck <git@academicsolutions.ch>

Reviewed by:	markj
MFC after:	2 weeks
Pull Request:	#1181

(cherry picked from commit d44c780)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markjdb markjdb markjdb left review comments

Assignees

@bsdimp bsdimp

Labels
merged
Projects
None yet
Milestone
No milestone
4 participants
@gartens @bsdimp @markjdb @glebius