Re: [PATCH 5/6] net/core: support compiling out splice

[Josh Triplett <josh@joshtriplett.org>]

On Sat, Nov 22, 2014 at 10:00:00PM +0100, Pieter Smith wrote:
> Compile out splice support from networking core when the splice-family of
> syscalls is not supported by the system (i.e. CONFIG_SYSCALL_SPLICE is
> undefined).

Please explain in the commit message why this particular bit of splice
support needs compiling out when most other bits do not.

Also, a couple of style comments below.

> Signed-off-by: Pieter Smith <pieter@boesman.nl>
> ---
>  include/linux/skbuff.h | 9 +++++++++
>  net/core/skbuff.c      | 9 ++++++---
>  2 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index a59d934..8c28524 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -2640,9 +2640,18 @@ int skb_copy_bits(const struct sk_buff *skb, int offset, void *to, int len);
>  int skb_store_bits(struct sk_buff *skb, int offset, const void *from, int len);
>  __wsum skb_copy_and_csum_bits(const struct sk_buff *skb, int offset, u8 *to,
>  			      int len, __wsum csum);
> +#ifdef CONFIG_SYSCALL_SPLICE
>  int skb_splice_bits(struct sk_buff *skb, unsigned int offset,
>  		    struct pipe_inode_info *pipe, unsigned int len,
>  		    unsigned int flags);
> +#else /* #ifdef CONFIG_SYSCALL_SPLICE */
> +static inline int skb_splice_bits(struct sk_buff *skb, unsigned int offset,
> +		    struct pipe_inode_info *pipe, unsigned int len,
> +		    unsigned int flags)
> +{
> +	return -EPERM;
> +}
> +#endif /* #ifdef CONFIG_SYSCALL_SPLICE */

These comments, and the one added below in the corresponding .c file,
don't match the prevailing style.  These are so short (fitting on one
screen) that they hardly seem worth the comments at all, but if you want
to include them, s/#ifdef// in the comment.

>  void skb_copy_and_csum_dev(const struct sk_buff *skb, u8 *to);
>  unsigned int skb_zerocopy_headlen(const struct sk_buff *from);
>  int skb_zerocopy(struct sk_buff *to, struct sk_buff *from,
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 61059a0..74fad8a 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -1781,9 +1781,9 @@ static bool __splice_segment(struct page *page, unsigned int poff,
>   * Map linear and fragment data from the skb to spd. It reports true if the
>   * pipe is full or if we already spliced the requested length.
>   */
> -static bool __skb_splice_bits(struct sk_buff *skb, struct pipe_inode_info *pipe,
> -			      unsigned int *offset, unsigned int *len,
> -			      struct splice_pipe_desc *spd, struct sock *sk)
> +static bool __maybe_unused __skb_splice_bits(struct sk_buff *skb, struct pipe_inode_info *pipe,
> +					     unsigned int *offset, unsigned int *len,
> +					     struct splice_pipe_desc *spd, struct sock *sk)
>  {
>  	int seg;
>  
> @@ -1821,6 +1821,7 @@ static bool __skb_splice_bits(struct sk_buff *skb, struct pipe_inode_info *pipe,
>   * the frag list, if such a thing exists. We'd probably need to recurse to
>   * handle that cleanly.
>   */
> +#ifdef CONFIG_SYSCALL_SPLICE
>  int skb_splice_bits(struct sk_buff *skb, unsigned int offset,
>  		    struct pipe_inode_info *pipe, unsigned int tlen,
>  		    unsigned int flags)
> @@ -1876,6 +1877,8 @@ done:
>  
>  	return ret;
>  }
> +#endif /* #ifdef CONFIG_SYSCALL_SPLICE */
> +

This extra blank line shouldn't be here.

>  /**
>   *	skb_store_bits - store bits from kernel buffer to skb
> -- 
> 1.9.1
>