On Thu, 25 Aug 2022 09:14:50 -0700
John Fastabend <john.fastabend@xxxxxxxxx> 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.
