On Mon, Jul 15, 2019 at 12:14:00PM +0200, Sebastien Marie wrote:
> 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.
new diff with changes asked by claudio@. I also used pipe_destroy()
name.
I send a new diff as a little omission was present in pipe_create(): it
still returned 0 on success (instead of the newly allocated pointer).
bad semarie@ that didn't test its own diff after splitting it to make a
small diff.
--
Sebastien Marie
Index: kern/sys_pipe.c
===================================================================
RCS file: /cvs/src/sys/kern/sys_pipe.c,v
retrieving revision 1.94
diff -u -p -r1.94 sys_pipe.c
--- kern/sys_pipe.c 15 Jul 2019 09:05:46 -0000 1.94
+++ kern/sys_pipe.c 15 Jul 2019 16:08:42 -0000
@@ -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);
+void pipe_destroy(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_destroy(wpipe);
+ pipe_destroy(rpipe);
return (error);
}
@@ -247,22 +244,27 @@ pipe_buffer_realloc(struct pipe *cpipe,
/*
* initialize and allocate VM and memory for pipe
*/
-int
-pipe_create(struct pipe *cpipe)
+struct pipe *
+pipe_create(void)
{
+ struct pipe *cpipe;
int error;
- sigio_init(&cpipe->pipe_sigio);
+ 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;
cpipe->pipe_mtime = cpipe->pipe_ctime;
- return (0);
+ return (cpipe);
}
@@ -768,7 +770,7 @@ pipe_close(struct file *fp, struct proc
fp->f_ops = NULL;
fp->f_data = NULL;
KERNEL_LOCK();
- pipeclose(cpipe);
+ pipe_destroy(cpipe);
KERNEL_UNLOCK();
return (0);
}
@@ -799,44 +801,46 @@ pipe_buffer_free(struct pipe *cpipe)
}
/*
- * shutdown the pipe
+ * shutdown the pipe, and free resources.
*/
void
-pipeclose(struct pipe *cpipe)
+pipe_destroy(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
