On 10/07/2019 23:47, Sabrina Dubroca wrote:
> 2019-07-10, 16:07:43 +0100, Edward Cree wrote:
>> On 10/07/2019 14:52, Sabrina Dubroca wrote:
>>> -static int __netif_receive_skb_core(struct sk_buff *skb, bool pfmemalloc,
>>> +static int __netif_receive_skb_core(struct sk_buff **pskb, bool pfmemalloc,
>>> struct packet_type **ppt_prev)
>>> {
>>> struct packet_type *ptype, *pt_prev;
>>> rx_handler_func_t *rx_handler;
>>> + struct sk_buff *skb = *pskb;
>> Would it not be simpler just to change all users of skb to *pskb?
>> Then you avoid having to keep doing "*pskb = skb;" whenever skb changes
>> (with concomitant risk of bugs if one gets missed).
> Yes, that would be less risky. I wrote a version of the patch that did
> exactly that, but found it really too ugly (both the patch and the
> resulting code).
If you've still got that version (or can dig it out of your reflog), can
you post it so we can see just how ugly it turns out?
> We have more than 50 occurences of skb, including
> things like:
>
> atomic_long_inc(&skb->dev->rx_dropped);
Ooh, yes, I can see why that ends up looking funny...
Here's a thought, how about switching round the return-vs-pass-by-pointer
and writing:
static struct sk_buff *__netif_receive_skb_core(struct sk_buff *skb, bool
pfmemalloc,
struct packet_type **ppt_prev,
int *ret)
?
(Although, you might want to rename 'ret' in that case.)
Does that make things any less ugly?
-Ed
