Skip to content

standalone-enc: move fd_drain() to a separate file#414

Merged
cperciva merged 4 commits intomasterfrom
standalone-fd-drain
Jan 18, 2025
Merged

standalone-enc: move fd_drain() to a separate file#414
cperciva merged 4 commits intomasterfrom
standalone-fd-drain

Conversation

@gperciva
Copy link
Member

No description provided.

@gperciva
Copy link
Member Author

It's useful to share fd_drain() -- or rather, fd_drain_fork() -- between multiple tests.

At the moment it's only used in _transfer_noencrypt.c and _pipe_socketpair_one.c, but we'll be adding _pipe_socketpair_two.c soon.

* Drain bytes from the file descriptor ${*fd_p} -- passed as an (int *) -- as
* quickly as possible.
*/
int fd_drain(void *);
Copy link
Member Author

Choose a reason for hiding this comment

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

My original intent was only to expose fd_drain_fork(), but standalone_transfer_noencrypt.c uses threads rather than processes, so it needs to call fd_drain() directly.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of the "pass a pointer to the file descriptor, cast to void *" API here. How about int fd_drain(int) and static int fd_drain_internal(void *) which gets called by both of the externally-visible functions?

@gperciva
Copy link
Member Author

This has no impact on the perftest timings.

When I first glanced at it with N=5 it looked like it did, but repeating the test with N=11 and then N=31 showed it was a fluke.

@gperciva gperciva force-pushed the standalone-fd-drain branch 2 times, most recently from 2133364 to d79c58e Compare January 15, 2025 18:02
/* Loop until we hit EOF. */
do {
/*
* This will almost always read 1060 bytes (size of an
Copy link
Member

Choose a reason for hiding this comment

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

This comment seems like it made sense in the original context of where this code was used, but doesn't belong any more once fd_drain is in a separate file since it could end up being used by completely different callers.


#include <sys/types.h>

/* The smallest this can be is PCRYPT_ESZ (which is 1060). */
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if this comment is necessary or even correct any more?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I guess that if it's something tiny like 10 bytes, that would just mean read gets called more often.

... yeah, I'll delete it.

@cperciva
Copy link
Member

Flagged one more comment. Aside from that, looks good; please tidy up the history.

- rename pipe_output() to fd_drain_internal()
- simplify loop in fd_drain_internal()
- rename MAXOUTSIZE to FD_DRAIN_MAX_SIZE
- use (int *) instead of (struct pipeinfo)
- add fd_drain_fork_start()
- remove sanity check for FD_DRAIN_MAX_SIZE
@gperciva gperciva force-pushed the standalone-fd-drain branch from d8a1d36 to 63936be Compare January 17, 2025 22:40
@gperciva
Copy link
Member Author

Rebased, ready for merge.

@cperciva cperciva merged commit 7ec5520 into master Jan 18, 2025
2 checks passed
@gperciva gperciva deleted the standalone-fd-drain branch January 18, 2025 04:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants