Hi,
Next move in revisiting pipe initialization.
After some discussion with mpi@, it seems better to have the whole
`struct pipe' allocation and initialization inside pipe_create()
function, aka moving pool_get() inside pipe_create() instead of
allocating the struct in dopipe() and initialize it in pipe_create().
It makes pipe_create() to return a `struct pipe *' instead of an error
code. The sole possible error was ENOMEM, so returning NULL in such case
is fine.
In the same move, I revisited the freeing of the struct pipe: the
pipeclose() function.
About pipeclose():
- I dislike having both pipe_close() and pipeclose() functions. So I
renamed pipeclose() to pipe_free(). The function does a bit more that
freeing resources, as it ensures all customers possibling in wait state
are aware of the pipe terminaison. but I don't have better name...
- I inversed the if-condition to return early, and move the whole
if-body in the function. It is more readable, and match what we usually
do in such case.
This way, pipe_create() and pipe_free() are symetric: allocation in one,
and free in another.
Comments or OK ?
--
Sebastien Marie
Index: sys/kern/sys_pipe.c
===================================================================
--- sys/kern/sys_pipe.c.orig 2019-07-15 11:06:05.015708914 +0200
+++ sys/kern/sys_pipe.c 2019-07-15 11:11:35.667317769 +0200
@@ -97,12 +97,12 @@ static unsigned int amountpipekva;
struct pool pipe_pool;
int dopipe(struct proc *, int *, int);
-void pipeclose(struct pipe *);
-int pipe_create(struct pipe *);
int pipelock(struct pipe *);
void pipeunlock(struct pipe *);
void pipeselwakeup(struct pipe *);
+struct pipe *pipe_create();
+void pipe_free(struct pipe *);
int pipe_buffer_realloc(struct pipe *, u_int);
void pipe_buffer_free(struct pipe *);
@@ -144,14 +144,11 @@ dopipe(struct proc *p, int *ufds, int fl
cloexec = (flags & O_CLOEXEC) ? UF_EXCLOSE : 0;
- rpipe = pool_get(&pipe_pool, PR_WAITOK | PR_ZERO);
- error = pipe_create(rpipe);
- if (error != 0)
- goto free1;
- wpipe = pool_get(&pipe_pool, PR_WAITOK | PR_ZERO);
- error = pipe_create(wpipe);
- if (error != 0)
+ if (((rpipe = pipe_create()) == NULL) ||
+ ((wpipe = pipe_create()) == NULL)) {
+ error = ENOMEM;
goto free1;
+ }
fdplock(fdp);
@@ -202,8 +199,8 @@ free3:
free2:
fdpunlock(fdp);
free1:
- pipeclose(wpipe);
- pipeclose(rpipe);
+ pipe_free(wpipe);
+ pipe_free(rpipe);
return (error);
}
@@ -247,16 +244,19 @@ pipe_buffer_realloc(struct pipe *cpipe,
/*
* initialize and allocate VM and memory for pipe
*/
-int
-pipe_create(struct pipe *cpipe)
+struct pipe *
+pipe_create()
{
int error;
-
- sigio_init(&cpipe->pipe_sigio);
+ struct pipe *cpipe = pool_get(&pipe_pool, PR_WAITOK | PR_ZERO);
error = pipe_buffer_realloc(cpipe, PIPE_SIZE);
- if (error != 0)
- return (error);
+ if (error != 0) {
+ pool_put(&pipe_pool, cpipe);
+ return (NULL);
+ }
+
+ sigio_init(&cpipe->pipe_sigio);
getnanotime(&cpipe->pipe_ctime);
cpipe->pipe_atime = cpipe->pipe_ctime;
@@ -768,7 +768,7 @@ pipe_close(struct file *fp, struct proc
fp->f_ops = NULL;
fp->f_data = NULL;
KERNEL_LOCK();
- pipeclose(cpipe);
+ pipe_free(cpipe);
KERNEL_UNLOCK();
return (0);
}
@@ -799,44 +799,46 @@ pipe_buffer_free(struct pipe *cpipe)
}
/*
- * shutdown the pipe
+ * shutdown the pipe, and free resources.
*/
void
-pipeclose(struct pipe *cpipe)
+pipe_free(struct pipe *cpipe)
{
struct pipe *ppipe;
- if (cpipe) {
- pipeselwakeup(cpipe);
- sigio_free(&cpipe->pipe_sigio);
- /*
- * If the other side is blocked, wake it up saying that
- * we want to close it down.
- */
- cpipe->pipe_state |= PIPE_EOF;
- while (cpipe->pipe_busy) {
- wakeup(cpipe);
- cpipe->pipe_state |= PIPE_WANTD;
- tsleep(cpipe, PRIBIO, "pipecl", 0);
- }
+ if (cpipe == NULL)
+ return;
- /*
- * Disconnect from peer
- */
- if ((ppipe = cpipe->pipe_peer) != NULL) {
- pipeselwakeup(ppipe);
+ pipeselwakeup(cpipe);
+ sigio_free(&cpipe->pipe_sigio);
- ppipe->pipe_state |= PIPE_EOF;
- wakeup(ppipe);
- ppipe->pipe_peer = NULL;
- }
+ /*
+ * If the other side is blocked, wake it up saying that
+ * we want to close it down.
+ */
+ cpipe->pipe_state |= PIPE_EOF;
+ while (cpipe->pipe_busy) {
+ wakeup(cpipe);
+ cpipe->pipe_state |= PIPE_WANTD;
+ tsleep(cpipe, PRIBIO, "pipecl", 0);
+ }
- /*
- * free resources
- */
- pipe_buffer_free(cpipe);
- pool_put(&pipe_pool, cpipe);
+ /*
+ * Disconnect from peer
+ */
+ if ((ppipe = cpipe->pipe_peer) != NULL) {
+ pipeselwakeup(ppipe);
+
+ ppipe->pipe_state |= PIPE_EOF;
+ wakeup(ppipe);
+ ppipe->pipe_peer = NULL;
}
+
+ /*
+ * free resources
+ */
+ pipe_buffer_free(cpipe);
+ pool_put(&pipe_pool, cpipe);
}
int
