|
Thanks for your review!
I've made most of the changes that you've asked for, just a couple
quick questions before I submit v2.
> This feature looks good to me.
>
> Such change should go first in net-next tree, after the change
> for skb->encapsulation (net tree). So, probably, after some -rc
> we can add this patchset.
>
SGTM
> So, to summarize our plan:
>
> - we have sockopt and netlink interface
>
> - IPv6 is supported only on netlink interface
>
> - the sockopt structs can not be changed, eg.
> struct ip_vs_dest_user in include/uapi/linux/ip_vs.h.
>
> - ipvsadm should allow different family (svc and dest) only for TUN
> method
>
> - we can also encode somehow the tunnel mode in sync message, eg.
> around the STYPE_F_INET6 value. May be this octet can become
> tunnel/header type. This can be done later in another patchset.
>
> Comments for your patchset:
>
> Patch 1:
> - please fix the dest->af = udest.af in __ip_vs_update_dest(),
> so that this patch can compile and as result this fix will
> disappear from patch 2:
> - dest->af = udest.af;
> + dest->af = udest->af;
Ah, embarrassing. I thought I checked that this all compiled. Will do.
> - __ip_vs_get_dest_entries() needs to skip dests with
> dest->af != svc->af, we can not exports such dests to
> sockopt users
Yeah seems reasonable, I'll include a short comment as well
> - too long comments, use scripts/checkpatch.pl
> --strict /tmp/p1.diff
Another amateur move. Thanks for the epointer
> - everything else looks ok
>
> Patch 2:
> - avoid error from patch 1 in __ip_vs_update_dest
>
> - errors from scripts/checkpatch.pl --strict /tmp/p2.diff
>
> Patch 3:
> - in the "Destination %u/%s:%u still in trash, " message
> we should use dest->af, not dest_af.
>
> Patch 4:
> - we have to introduce u16 cp->daf, just below cp->protocol
yeah makes sense
> - as result, cp->af is for caddr and vaddr, cp->daf is for
> cp->daddr, this is needed because sync protocol can create connections
> with cp->dest = NULL, cp binds to dest later
Agree that we should just create daf, but if we keep heterogeneous pools
and sync distinct can we still run into this? (Just curious)
> - ip_vs_ftp.c uses ip_vs_conn_new, now it does not compile
It appears that these are all IPv4 only, so I'll just use AF_INET and add a
comment.
> - Another file net/netfilter/xt_ipvs.c depends on IPVS
> definitions but I think this patchset does not break it
>
Yeah, this still builds
> Patch 5:
> - note that dest address can not be changed, so if you see
> reason this patch to exist the check in __ip_vs_update_dest
> should be: if (add && udest->af != svc->af)
>
Well that definitely simplifies things. My plan is to add a
BUG_ON(!add && udest->af != dest->af) -- more as documentation than
anything.
> - errors from scripts/checkpatch.pl --strict /tmp/p5.diff
>
> Patch 6:
> - comment and code do not match
What do you mean? This just removes a temporary "return -EINVAL" to keep
the code sane.
>
> Patch 7:
> - patch looks based on skb_checksum_help patch, the plan
> is to base it after the offload change we are about to
> send soon
Yeah, nothing significant about that, I was planning to rebase it the patch
when it lands.
> - ip_vs_bind_xmit*: sync protocol does not guarantee that
> cp->dest exists in all cases, we can not use cp->dest->af
> because cp->dest can be NULL, we have to use cp->daf (from
> patch 4)
Fixed this
>
> - for ip_vs_prepare_tunneled_skb():
> - usage of 'version' can be changed to 'af'
I'm a little bit confused by this. In this case, we aren't passing in af.
Since the caller doesn't really know (or care) what the type of the packet
is it made sense to figure it out from the ipheader version. Do you mean
that there's another function I should use to grab the address family from
the skb or is it possible to get it from somewhere else?
> - __ip_vs_get_out_rt_v6 and __ip_vs_get_out_rt should
> be changed to account header from both families for MTU check,
> these checks happen for IP_VS_RT_MODE_TUNNEL
>
Yes this looks fairly significant. Unless you'll object I'll introduce
this as a separate patch.
> - errors from scripts/checkpatch.pl --strict /tmp/p7.diff
>
> - looking at cp, set_tcp_state() can not know what family is used for
> cp->daddr, we have to use cp->daf. We have to check the
> cp->af usage everywhere.
>
> - I didn't fixed ip_vs_core.c logs yet
>
> I'm attaching some patches for your patchset. If it is
> difficult for you to maintain them in the patchset, we can post
> them later after your patchset, as separate patchset. For now you
> can use them as reference. Or may be they should be inserted
> at some point in your patchset, so that your patchset looks
> correct if applied up to some patch.
I'll add them to my patch set. Thank you for doing them!
> Let me know if you plan ipvsadm changes based on the
> ipvsadm tree:
>
I have made the appropriate changes for this. It's a relatively
straightforward patch. I can submit it at any time.
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe lvs-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
|
