Re: [PATCH v5 bpf-next 1/4] bpf: Add 'bpf_dynptr_get_data' helper

[Shmulik Ladkani <shmulik@metanetworks.com>]

On Thu, 25 Aug 2022 09:14:50 -0700
John Fastabend <john.fastabend@gmail.com> wrote:

> As a bit of an addmitedly nitpick I just wonder if having the avail_bytes
> passed through like this is much use anymore? For example,
> 
> +BPF_CALL_3(bpf_skb_set_tunnel_opt_dynptr, struct sk_buff *, skb,
> +	   struct bpf_dynptr_kern *, ptr, u32, len)
> +{
> +	const u8 *from;
> +	u32 avail;
> +
> -       if (!ptr->data)
> -		return -EFAULT;
> -       avail = bpf_dynptr_get_size(ptr)
> +	from = bpf_dynptr_get_data(ptr, &avail);
> +	if (unlikely(len > avail))
> +		return -EINVAL;
> +	return __bpf_skb_set_tunopt(skb, from, len);
> +}
> +
> 
> seems just about as compact to me and then drop the null check from the
> helper so we have a bpf_dynptr_get_data(*ptr) that just does the
> data+offset arithmatic. Then it could also be used in a few other
> spots where that calculation seems common.
> 
> I find it easier to read at least and think the helper would get
> more use, also land it in one of the .h files. And avoids bouncing
> avail around.
> 
> Bit of a gripe but what do you think?

Sure, I don't mind. 

My rationale extracting 'avail' was due to the fact the combo of 
"if !ptr->data + bpf_dynptr_get_size + bpf_dynptr_get_data" will be
repeated in future locations that need to access the stored data.
Therefore encapsulating this looked beneficial.

BTW, note we'll need to make bpf_dynptr_get_size public (currently it's
static).

Let me know your preference, I'm fine either way.