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

Add & use ipc_sync.h and ipc_sync.c #517

Merged
merged 4 commits into from
Jul 24, 2024
Merged

Add & use ipc_sync.h and ipc_sync.c #517

merged 4 commits into from
Jul 24, 2024

Conversation

gperciva
Copy link
Member

No description provided.

@gperciva
Copy link
Member Author

This replaces #514.

The API is slightly different: it now includes _wait_prep() and _signal_prep(), which are optional functions.


/* close(), but looping upon EINTR. */
static inline int
ipc_sync_close(int fd)
Copy link
Member Author

Choose a reason for hiding this comment

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

Two notable things about this function:

  1. we will replace it once we have noeintr_close().
  2. this function retains the previous behaviour of daemonize.c, namely that if close() has EINTR, this function calls close() again. This behaviour is required for HP-UX, but is explicitly wrong for the glibc implementation of close(): https://www.man7.org/linux/man-pages/man2/close.2.html

I propose that we keep the same behaviour as before (which is broken on linux), but I thought that I should bring it to your attention.


/**
* ipc_sync_wait_prep(IS):
* Perform any operation(s) that would speed up an upcoming call to
Copy link
Member Author

Choose a reason for hiding this comment

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

This is somewhat vaguely-worded because a different implementation of synchronization might have no "pre-wait" operations. For example, if we used a sem_t on some platforms (since it's not portable everywhere), the _prep() functions would be a no-op.

@gperciva gperciva force-pushed the ipc-sync-c branch 2 times, most recently from 2038282 to 900f8b5 Compare June 27, 2024 17:18
util/daemonize.c Outdated
* explicitly closing it for the benefit of leak checkers.
* Free resources associated with ${IS}. These would be
* released by the following _exit(), but we're explicitly
* closing it for the benefit of leak checkers.
Copy link
Member

Choose a reason for hiding this comment

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

s/closing it/releasing them/

util/ipc_sync.h Outdated
* 4) both processes: ipc_sync_done()
*
* If there's a need to make _wait() or _signal() as fast as possible
* (i.e. for a benchmark or a performance bottleneck), call the
Copy link
Member

Choose a reason for hiding this comment

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

s/i.e./e.g./

@cperciva
Copy link
Member

Ok aside from very minor comment nits. Please fix and then I'll merge.

@gperciva gperciva force-pushed the ipc-sync-c branch 3 times, most recently from 03129db to 0a85a39 Compare July 23, 2024 23:23
@cperciva cperciva merged commit af0ca13 into master Jul 24, 2024
2 checks passed
@gperciva gperciva deleted the ipc-sync-c branch July 24, 2024 15:51
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