Re: [PATCH] xen/netback: fix build warning

From: Juergen Gross
Date: Wed Dec 07 2022 - 06:29:51 EST

On 07.12.22 11:26, Jan Beulich wrote:
On 07.12.2022 11:18, Juergen Gross wrote:
On 07.12.22 10:25, Jan Beulich wrote:
On 07.12.2022 08:23, Juergen Gross wrote:
Commit ad7f402ae4f4 ("xen/netback: Ensure protocol headers don't fall in
the non-linear area") introduced a (valid) build warning.

Fix it.

Fixes: ad7f402ae4f4 ("xen/netback: Ensure protocol headers don't fall in the non-linear area")
Signed-off-by: Juergen Gross <jgross@xxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -530,7 +530,7 @@ static int xenvif_tx_check_gop(struct xenvif_queue *queue,
const bool sharedslot = nr_frags &&
frag_get_pending_idx(&shinfo->frags[0]) ==
copy_pending_idx(skb, copy_count(skb) - 1);
- int i, err;
+ int i, err = 0;
for (i = 0; i < copy_count(skb); i++) {
int newerr;

I'm afraid other logic (below here) is now slightly wrong as well, in
particular

/* If the mapping of the first frag was OK, but
* the header's copy failed, and they are
* sharing a slot, send an error
*/
if (i == 0 && !first_shinfo && sharedslot)
xenvif_idx_release(queue, pending_idx,
XEN_NETIF_RSP_ERROR);
else
xenvif_idx_release(queue, pending_idx,
XEN_NETIF_RSP_OKAY);

which looks to be intended to deal with _only_ failure of the one shared
part of the header, whereas "err" now can indicate an error on any earlier
part as well.

The comment at the end of that loop seems to imply this is the desired
behavior:

/* Remember the error: invalidate all subsequent fragments. */
err = newerr;
}

This says "subsequent", whereas I was describing a situation where e.g.
the first piece of header copying failed, the 2nd (shared part) succeeded,
and the mapping of the rest of the shared part also succeeded. At the
very least the comment in the code fragment I did quote then has become
stale. Furthermore, if "all subsequent" really meant all, then in the
new first loop this isn't followed either - an error response is sent
only for the pieces where copying failed.

Having stared at the code for quite some time now, I think there is some
room for confusion: "invalidating" the frags seems not to be the same as
setting the related idx to have an error.

XEN_NETIF_RSP_ERROR seems to be set only for the idx which really had an
error, while if any of them had one, all idx-es must be unmapped, have a
status set, and returned to the frontend.

And I think the code is doing this quite fine.

The comments _could_ need some improvements, though.

And some more restructuring could help, too (at least I think that the
"goto check_frags" is a rather clumsy construct - I'd prefer splitting
xenvif_tx_check_gop() into some helper functions and a rather small
body calling those with e.g. different shinfo values).


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature