Re: [PATCH] pipe_fs_i.h: add pipe_buf_init()

[Randy Dunlap]

Hi--

On 9/19/23 01:07, Max Kellermann wrote:
> Adds one central function which shall be used to initialize a newly
> allocated struct pipe_buffer.  This shall make the pipe code more
> robust for the next time the pipe_buffer struct gets modified, to
> avoid leaving new members uninitialized.  Instead, adding new members
> should also add a new pipe_buf_init() parameter, which causes
> compile-time errors in call sites that were not adapted.
> 
> This commit doesn't refactor fs/fuse/dev.c because this code looks
> obscure to me; it initializes pipe_buffers incrementally through a
> variety of functions, too complicated for me to understand.
> 
> Signed-off-by: Max Kellermann <max.kellermann@xxxxxxxxx>
> ---
>  fs/pipe.c                 |  9 +++------
>  fs/splice.c               |  9 ++++-----
>  include/linux/pipe_fs_i.h | 20 ++++++++++++++++++++
>  kernel/watch_queue.c      |  8 +++-----
>  mm/filemap.c              |  8 ++------
>  mm/shmem.c                |  9 +++------
>  6 files changed, 35 insertions(+), 28 deletions(-)
> 

> diff --git a/include/linux/pipe_fs_i.h b/include/linux/pipe_fs_i.h
> index 608a9eb86bff..2ef2bb218641 100644
> --- a/include/linux/pipe_fs_i.h
> +++ b/include/linux/pipe_fs_i.h
> @@ -176,6 +176,26 @@ static inline struct pipe_buffer *pipe_head_buf(const struct pipe_inode_info *pi
>  	return pipe_buf(pipe, pipe->head);
>  }
>  
> +/**
> + * Initialize a struct pipe_buffer.
> + */

That's not a kernel-doc comment so don't begin it with /**.
Just use /* instead.
Thanks.

> +static inline void pipe_buf_init(struct pipe_buffer *buf,
> +				 struct page *page,
> +				 unsigned int offset, unsigned int len,
> +				 const struct pipe_buf_operations *ops,
> +				 unsigned int flags)
> +{
> +	buf->page = page;
> +	buf->offset = offset;
> +	buf->len = len;
> +	buf->ops = ops;
> +	buf->flags = flags;
> +
> +	/* not initializing the "private" member because it is only
> +	   used by pipe_buf_operations which inject it via struct
> +	   partial_page / struct splice_pipe_desc */
> +}
> +
>  /**
>   * pipe_buf_get - get a reference to a pipe_buffer
>   * @pipe:	the pipe that the buffer belongs to


-- 
~Randy