2024-02-19, 12:07:03 -0800, Jakub Kicinski wrote:
> On Thu, 15 Feb 2024 17:17:31 +0100 Sabrina Dubroca wrote:
> > @@ -1772,7 +1772,8 @@ static int process_rx_list(struct tls_sw_context_rx *ctx,
> > u8 *control,
> > size_t skip,
> > size_t len,
> > - bool is_peek)
> > + bool is_peek,
> > + bool *more)
> > {
> > struct sk_buff *skb = skb_peek(&ctx->rx_list);
> > struct tls_msg *tlm;
>
>
> > @@ -1844,6 +1845,10 @@ static int process_rx_list(struct tls_sw_context_rx *ctx,
> >
> > out:
> > return copied ? : err;
> > +more:
> > + if (more)
> > + *more = true;
> > + goto out;
>
> Patches look correct, one small nit here -
>
> I don't have great ideas how to avoid the 7th argument completely but
I hesitated between this patch and a variant combining is_peek and
more into a single u8 *flags, but that felt a bit messy (or does that
fall into what you describe as "not [having] great ideas"? :))
@@ -1772,9 +1777,10 @@ static int process_rx_list(struct tls_sw_context_rx *ctx,
u8 *control,
size_t skip,
size_t len,
- bool is_peek)
+ u8 *flags)
{
struct sk_buff *skb = skb_peek(&ctx->rx_list);
+ bool is_peek = *flags & RXLIST_PEEK;
struct tls_msg *tlm;
ssize_t copied = 0;
int err;
[...]
@@ -1844,6 +1850,9 @@ static int process_rx_list(struct tls_sw_context_rx *ctx,
out:
return copied ? : err;
+more:
+ *flags |= RXLIST_MORE;
+ goto out;
}
and then in tls_sw_recvmsg:
u8 rxlist_flags = is_peek ? RXLIST_PEEK : 0;
err = process_rx_list(ctx, msg, &control, 0, len, &rxlist_flags);
> I think it'd be a little cleaner if we either:
> - passed in err as an output argument (some datagram code does that
> IIRC), then function can always return copied directly, or
(yes, __skb_wait_for_more_packets, __skb_try_recv_datagram, and their
variants)
> - passed copied as an output argument, and then we can always return
> err?
Aren't those 2 options adding an 8th argument?
I tend to find ">= 0 on success, otherwise errno" more readable,
probably because that's a very common pattern (either for recvmsg
style of cases, or all the ERR_PTR type situations).
> I like the former a little better because we won't have to special case
> NULL for the "after async decryption" call sites.
We could also pass &rx_more every time and not check for NULL.
What do you want to clean up more specifically? The number of
arguments, the backwards goto, the NULL check before setting *more,
something else/all of the above?
--
Sabrina
