Hi Thomas, More on this topic. (1) as related to my below email. I tried to do a ‘-U interface[/port]’, but I have problem at the place such as: if (!(length = add_relay_agent_options(ip, packet, length, ip->addresses[0]))) return; … for (sp = servers; sp; sp = sp->next) { if (send_packet((fallback_interface ? fallback_interface : interfaces), ULL, packet, length, ip->addresses[0], &sp->to, NULL) < 0) { Since the ‘relay-port’ is only related to upstream interface, while the interface_info in that ‘add_relay_agent_options()’ is a downstream interface. Although we can require users to put a port number at each of the downstream interfaces, but that does not make sense and it can be too many places. We need to insert the RAI_RELAY_PORT sub-option when we receives traffic from the client, and we have not got to the outbound (upstream) interface yet. This is again why I think this global ‘-rp’ makes sense for this relay-port feature. (2) on your comment of “.. the interface discovery code has been modified to only register on upstream and one downstream interface.” I have to apologize that I didn’t include much comments in this patch. If you look at this code in discover_interfaces(), it has these lines: #if defined(DHCPv6) /* Only register the first interface for V6, since * servers and relays all use the same socket. * XXX: This has some messy side effects if we start * dynamically adding and removing interfaces, but * we're well beyond that point in terms of mess. */ if (((state == DISCOVER_SERVER) || (state == DISCOVER_RELAY)) && (local_family == AF_INET6)) break; #endif My understanding of the above code is that, for IPv6 only one socket is needed so as long as we found only one interface, we breakout here. This will break this 'relay-port' feature, since the 'relay-port' is only related to the upstream devices (server or upstream relay agent), if this code just happen to find a 'downstream' interface and breakout, we have no place to do the 'relay-port' change and relay-port option for the upstream socket. Thus in the patch if this 'relay port' is configured, we at least need to have tow sockets, one to receive from DHCP client side with the normal UDP port number, the other interfafce from the server side to receive message with the user defined UDP port. Thus the code is added actually not to break out until we at least find two intfs, one upstream and one downstream. This is done the same way for IPv4 with the bpf filters, and we need to make sure we receive packets from both up and down streams with two different port numbers. (3) on your comment of “relay/dhcrelay.c:process_up6(), Relocate the logic to add the D60_RELAY_PORT to inside the upstream send loop….” this is another place my patch needs to have good comments. the purpose of the diff in process_up6() is not with finding and replacing the existing option, this relay agent never replaces someone else’s relay options because IPv6 relay options are encapsulated. This is to figure out if THIS 'relay agent' needs to format and send our own D60_RELAY_PORT option. There are two cases we need to send: (a) this relay-agent uses a non-dhcp standard UDP port for upstream, this is the "relay_port" globally defined in dhcrelay.c; and (b) if our downstream relay-agent uses a non-standard UDP port. For that, we need to check if inbound port has the != 547 port in packet->client_port, and record it in 'relay_client_port'. If either one of the above condition is true, we'll put in this D60_RELAY_PORT option in our relay option. (4) if you agree to the patch using the global configuration as ‘-rp’, I’ll break the patch into 5 smaller patches as you described, and make good comments to the places the diff may not be obvious. Then I’ll send to you and for further review. Or any other suggestions you may have. Best Regards, - Naiming On May 3, 2017, at 4:43 PM, Naiming Shen (naiming) > wrote: Hi Thomas, One thing I noticed when starting to make changes is that, the per-interface defintion of relay-port may have some issues. The current patch with this ‘-rp’ knob is modeling after the existing ‘-p’ usage, which means if we are changing the DHCP relay port number, it makes sense to change it for the global context. Since a relay agent device may have many interfaces. The DHCP server may be multiple hops away. From a relay-agent admin point of view (s)he does not exactly know which interface(s) it should use. Even (s)he knows for now, the routing may change later, and the device may also have too many interfaces to define each of them explicitly. The use case we explicitly target for this relay-port feature is because multiple relay instances sharing the same physical device with the same source IP address, so it can not use the same UDP relay port among different relay-agent processes. For this feature I doubt there is a need for one relay-agent trying to use different UDP ports for different interfaces. Best Regards, - Naiming On Feb 27, 2017, at 6:09 AM, Thomas Markwalder via RT > wrote: Hello Naiming: First, I apologize for the delay in getting our comments back to you regarding the patch. In order to preserve formatting I have attached our review as a text file. In general, the patch mechanics are fine, but we would rather see the the relay port specification done per interface rather than as global parameter. Details on this are included in the attachment. To set expectations, these changes will not be included in our upcoming maintenance release, 4.3.6, due out in May '17. Rather they would go into our feature release, 4.4.0, which is due out later in the year, date is TBD. Thank you for your efforts thus far. If you have any comments or questions please let us know. Sincerely, Thomas Markwalder ISC Software Engineering We would like to the patch split as follows: 1. V4 Server support 2. V4 Relay support 3. V6 Server support 4. V6 Relay 5. 4o6 Support 2. The BPF filter changes seem fine. They allows an interface to accept messages either from the standard port or the alternate port which matches the RFC draft. For interfaces that are both up and down, though it would mean that the upstream could only receive on the alternate port as the first port in the filter would be the standard downstream port. This is likely acceptable versus more complicated filter logic. 3. The patch implements a global command line parameter, "-rp", used by dhcrelay to specify an alternate port value. When this parameter is specified the interface discovery code has been modified to only register one upstream and one downstream interface. It is unclear if this was done for simplicity or based upon a perceived most likely use-case. There is a more flexible approach, that supports multiple interfaces, and has the additional benefit of being more seamless: Add an optional port value to the v4's "-i,-iu,-U" and v6's "-u" command arguments: -iu interface[/port] -u [address%]interface[/port] This makes the alternate port use specific to an upstream interface. If users want only a single upstream or downstream interfaces they explicitly configure it that way, if they want more than one they can still do that. The steps to implement this are outlined below: Rather than a single global relay_port variable, add a member for it to struct interface_info: struct interface_info { : unit16_t relay_port; } relay/dhcrealy.c:request_v4_interface() Modify to look for the port value and populate it in the structure, only allowed if the flag parameter includes INTERFACE_UPSTREAM. relay/dhcrealy.c:parse_upstream() Modify to look for the port value and populate it in the structure. discover.c::discover_interfaces() Would not require the changes made by the patch, it would revert to as is. Several places would need look at the structure port value, instead of the global value: common/discover.c::lpf_gen_filter_setup(): if ((udp.uh_dport != local_port) && ((interface->relay_port == 0) || (udp.uh_dport != interface->relay_port))) common/packet.c:assemble_udp_ip_header(): udp.uh_sport = (interface->relay_port ? interface->relay_port : local_port); common/packet.c:decode_udp_ip_header(): if ((udp.uh_dport != local_port) && ((interface->relay_port == 0) || (udp.uh_dport != interface->relay_port))) common/raw.c:if_register_send(): name.sin_port = (info->relay_port ? info->relay_port: local_port; common/socket.c:if_register_socket(): addr6->sin6_port = (info->relay_port ? info->relay_port: local_port; The rest of the changes the patch makes to this function are unnecessary. relay/dhcrelay.c:process_up6() Relocate the logic to add the D6O_RELAY_PORT to inside the upstream send loop. Here the logic would need to be a little smarter: /* Send it to all upstreams. */ int has_relay_option = 0; for (up = upstreams; up; up = up->next) { if (has_relay_option) { /* Previous upstream added it, this either shouldn't * have it or needs to replace it, so let's remove it */ delete_option(&dhcpv6_universe, opts, D6O_RELAY_PORT); has_relay_option = 0; } if (upstream->relay_port || relay_client_port) { if (!save_option_buffer(&dhcpv6_universe, opts, NULL, (unsigned char *) &relay_client_port, sizeof(u_int16_t), D6O_RELAY_PORT, 0)) { log_error("Can't save relay-port."); option_state_dereference(&opts, MDL); return; } has_relay_option = 1; } send_packet6(up->ifp, (unsigned char *) forw_data, (size_t) cursor, &up->link); } Note these changes may not have covered everything necessary, and of course have not been been compile so there may syntax errors. However they should suffice to guide the actual code work. 4. The 4o6 patch seems fine. It adds room in the IPC header to accommodate the alternate port the V6 server received the message from so the v4 server can echo it back. This allows the V6 server to send the response back on the alternate port.