ACK/cmnt: [SRU][Trusty][PATCH v2 0/4] bridge: Provide IPv6 refragmentation for bridge conntrack

[Stefan Bader]

On 19.02.2016 21:17, Jay Vosburgh wrote:
> Stefan Bader <stefan.bader at canonical.com> wrote:
> 
>> On 17.02.2016 01:41, Jay Vosburgh wrote:
>>> BugLink: https://bugs.launchpad.net/nova/+bug/1463911
>>>
>>> 	This is a respin of
>>>
>>> https://lists.ubuntu.com/archives/kernel-team/2015-November/065772.html
>>> https://lists.ubuntu.com/archives/kernel-team/2015-November/065773.html
>>> https://lists.ubuntu.com/archives/kernel-team/2015-November/065774.html
>>>
>>> 	to break out the consolidated patches into their individual parts.
>>>
>>> 	This patch series provides the ability to re-fragment IPv6
>>> datagrams being forwarded by the bridge when conntrack is enabled.
>>> Conntrack will defragment any datagrams it processes, and presently the
>>> bridge cannot refragment them on egress (resulting in a drop of affected
>>> datagrams).
>>>
>>> 	Patches tested according to the recipe provided in patch 3 of the
>>> series.
>>
>> ACK more based on testing results. Especially #3 is hard to verify without being
>> accustomed to the network code.
>>
>> Minor thought on #1: The commit messages talks about renaming a data structure
>> which was not actually done. Not sure there is already an established method
>> here but maybe quick note under the backported ...
> 
> 	That comment in the commit log refers to the following change in
> the upstream patch:
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 36f3f43c0117..f66a089afc41 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -169,7 +169,7 @@ struct nf_bridge_info {
>         unsigned int            mask;
>         struct net_device       *physindev;
>         struct net_device       *physoutdev;
> -       unsigned long           data[32 / sizeof(unsigned long)];
> +       char                    neigh_header[8];
>  };
>  #endif
>  
> 	I left that particular change out because, on the kernels at
> issue here (3.13, 3.16 and 3.19), the assertion in the commit log that
> the "only user left is neigh resolution" is not true.  Including this
> change would require a cascade of other changes (because the extant
> usage exceeds the new neigh_header[8] capacity).
> 
> 	That said, I should have explained it better somewhere; I can
> respin the patches with higher verbosity in the commit log if that would
> be preferred.

Was more a remark than a requirement. As I wrote I am not even sure there is
anything preferred way established. When reviewing I find it helpful to ignore
obvious differences and when applied it may (or may not) help to quickly figure
out how a patch may be different from the upstream version.
But even if this is thought of as improvement it can be added when applying the
set. So no need for a re-spin. And in fact its moot as that has already happened. :)

-Stefan
> 
> 	-J
> 
> ---
> 	-Jay Vosburgh, jay.vosburgh at canonical.com
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20160222/87bc664d/attachment.sig>