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 used *_destroy() in similar cases but honestly I don't mind pipe_free().
> - 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();
Please make this a real void function:
struct pipe *pipe_create(void);
> +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()
again make this really not take any arguments by using
pipe_create(void)
> {
> int error;
> -
> - sigio_init(&cpipe->pipe_sigio);
> + struct pipe *cpipe = pool_get(&pipe_pool, PR_WAITOK | PR_ZERO);
In general complex functions should not be used in initializers.
So maybe make this:
struct pipe *cpipe;
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
>
Apart from that OK claudio@
--
:wq Claudio
