Bug#433568: [PATCH] Add vlan support, based on YunQiang Su patch and Phillip Kern's rewrite, with own additions. Closes: #433568
Hi,
On Tue, Apr 05, 2016 at 10:57:07PM +0100, Dimitri John Ledkov wrote:
> Squished all the patches, and hopefully did the multi-sided merge of
> Yun's, Pkern's, mine and extra debug stuff. This starts to look
> reasonable. Successfully tested this on s390x, and I'm glad I did it
> there, as I had to fix up the s390x specific code path too.
>
> I do believe it should be possible to interractively set
> vlan. Currently I am asking this question a "medium" priority, but I
> am open to changing to "low". E.g. I find it acceptable that a user
> needs to return to the main menu and change debconf priority.
>
> Or for example, should we show it in the static-config by default,
> and offer "configure vlan" as an option when automatic network
> detection fails? E.g. alongside the retry-dhcp,
> retry-dhcp-with-hostname, do-wifi, etc. options?
That doesn't sound totally crazy I guess.
> Preseeding should be trivial - simply preseed netcfg/vlan_id=2654 and
> that's it.
>
> Updated template texts a bit. Given that the state-machine, templates
> and all of this patch is still under development, I have not sent
> these off to l10n-en for review.
I'd still like this to be reviewed by them and Christian for sanity,
though. Sublevels, understandability, ... ;-)
> Any further comments about this update patch?
>
> Makefile | 2 +-
> debian/changelog | 7 +++
> debian/netcfg-common.templates | 28 +++++++++++-
> dhcp.c | 2 +-
> netcfg-common.c | 32 ++++++++++++--
> netcfg.c | 20 ++++++---
> netcfg.h | 11 +++++
> static.c | 12 +++---
> vlan.c | 97 ++++++++++++++++++++++++++++++++++++++++++
> wireless.c | 4 +-
> write_interface.c | 19 ++++++++-
> 11 files changed, 213 insertions(+), 21 deletions(-)
> create mode 100644 vlan.c
>
> diff --git a/Makefile b/Makefile
> index a15d476..03343c9 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -7,7 +7,7 @@ TARGETS ?= netcfg-static netcfg
>
> LDOPTS = -ldebconfclient -ldebian-installer
> CFLAGS = -W -Wall -Werror -DNDEBUG -DNETCFG_VERSION="\"$(NETCFG_VERSION)\"" -I.
> -COMMON_OBJS = netcfg-common.o wireless.o write_interface.o ipv6.o
> +COMMON_OBJS = netcfg-common.o wireless.o write_interface.o ipv6.o vlan.o
> NETCFG_O = netcfg.o dhcp.o static.o ethtool-lite.o wpa.o wpa_ctrl.o rdnssd.o autoconfig.o
> NETCFG_STATIC_O = netcfg-static.o static.o ethtool-lite.o
>
> diff --git a/debian/changelog b/debian/changelog
> index f3dd40c..c2dae40 100644
> --- a/debian/changelog
> +++ b/debian/changelog
> @@ -1,3 +1,10 @@
> +netcfg (1.139) UNRELEASED; urgency=medium
> +
> + * Add vlan support, based on YunQiang Su patch and Phillip Kern's
Philipp ;-)
> + rewrite, with own additions. Closes: #433568
> +
> + -- Dimitri John Ledkov <xnox@ubuntu.com> Mon, 04 Apr 2016 12:18:08 +0100
> +
> netcfg (1.138) unstable; urgency=medium
>
> [ Hendrik Brueckner ]
> diff --git a/debian/netcfg-common.templates b/debian/netcfg-common.templates
> index 2b77936..bffded9 100644
> --- a/debian/netcfg-common.templates
> +++ b/debian/netcfg-common.templates
> @@ -26,6 +26,33 @@ _Description: Domain name:
> If you are setting up a home network, you can make something up, but make
> sure you use the same domain name on all your computers.
>
> +Template: netcfg/use_vlan
> +Type: boolean
> +Default: false
> +# :sl6:
> +_Description: Are you connected to an IEEE 802.1Q VLAN trunk port?
> + Virtual LAN (VLAN) is a concept of partitioning a physical network to create
> + distinct broadcast domains. Packets can be marked for different IDs by
> + tagging, so that a single interconnect (trunk) may be used to transport
> + data for various VLANs.
> + .
> + If your network interface is directly connected to a VLAN trunk port,
> + specifying a VLAN ID may be necessary to get a working connection.
> +
> +Template: netcfg/vlan_id
> +Type: string
> +# :sl6:
> +_Description: VLAN ID (1-4094):
> + If your network interface is directly connected to a VLAN trunk port,
> + specifying a VLAN ID may be necessary to get a working connection.
> +
> +Template: netcfg/vlan_failed
> +Type: error
> +# :sl6:
> +_Description: Error setting up VLAN
> + The command used to set up VLAN during the installation returned an
> + error, please go back and try again.
I sort of fear that "go back and try again" would not be helpful. Check
the logs is probably more sensible. \-:
> +
> Template: netcfg/get_nameservers
> Type: string
> # :sl1:
> @@ -371,4 +398,3 @@ _Choices: ${essid_list} Enter ESSID manually
> # :sl1:
> _Description: Wireless network:
> Select the wireless network to use during the installation process.
> -
> diff --git a/dhcp.c b/dhcp.c
> index fe06950..9476bac 100644
> --- a/dhcp.c
> +++ b/dhcp.c
> @@ -459,7 +459,7 @@ int netcfg_activate_dhcp (struct debconfclient *client, struct netcfg_interface
> kill_dhcp_client();
> loop_setup();
>
> - interface_up(interface->name);
> + netcfg_interface_up(interface);
>
> for (;;) {
> di_debug("State is now %i", state);
> diff --git a/netcfg-common.c b/netcfg-common.c
> index c6d1d8d..ac2c6df 100644
> --- a/netcfg-common.c
> +++ b/netcfg-common.c
> @@ -267,7 +267,7 @@ int is_raw_80211(const char *iface)
>
> #if defined(__s390__)
> // Layer 3 qeth on s390(x) cannot do arping to test gateway reachability.
> -int is_layer3_qeth(const char *iface)
> +int is_layer3_qeth(const struct netcfg_interface *interface)
> {
> const int bufsize = 1024;
> int retval = 0;
> @@ -277,6 +277,12 @@ int is_layer3_qeth(const char *iface)
> ssize_t slen;
> char* driver;
> int fd;
> + char* iface;
> +
> + if (interface->parentif)
> + iface = interface->parentif;
> + else
> + iface = interface->name;
>
> // This is sufficient for both /driver and /layer2.
> len = strlen(SYSCLASSNET) + strlen(iface) + strlen("/device/driver") + 1;
> @@ -329,7 +335,7 @@ out:
> return retval;
> }
> #else
> -int is_layer3_qeth(const char *iface __attribute__((unused)))
> +int is_layer3_qeth(const struct netcfg_interface *interface __attribute__((unused)))
> {
> return 0;
> }
> @@ -1338,6 +1344,24 @@ void interface_down (const char *if_name)
> }
> }
>
> +void netcfg_interface_up (const struct netcfg_interface *iface)
> +{
> + if (iface->parentif)
> + interface_up (iface->parentif);
> +
> + if (iface->name)
> + interface_up (iface->name);
> +}
> +
> +void netcfg_interface_down (const struct netcfg_interface *iface)
> +{
> + if (iface->name)
> + interface_down (iface->name);
> +
> + if (iface->parentif)
> + interface_down (iface->parentif);
> +}
> +
> void parse_args (int argc, char ** argv)
> {
> if (argc == 2) {
> @@ -1528,7 +1552,7 @@ int netcfg_detect_link(struct debconfclient *client, const struct netcfg_interfa
> if (ethtool_lite(if_name) == 1) /* ethtool-lite's CONNECTED */ {
> di_info("Found link on %s", if_name);
>
> - if (!empty_str(gateway) && !is_wireless_iface(if_name) && !is_layer3_qeth(if_name)) {
> + if (!empty_str(gateway) && !is_wireless_iface(if_name) && !is_layer3_qeth(interface)) {
> for (count = 0; count < gw_tries; count++) {
> if (di_exec_shell_log(arping) == 0)
> break;
> @@ -1564,6 +1588,8 @@ void netcfg_interface_init(struct netcfg_interface *iface)
> iface->v6_stateless_config = -1;
> iface->loopback = -1;
> iface->mode = MANAGED;
> + iface->parentif = NULL;
> + iface->vlanid = -1;
> }
>
> /* Parse an IP address (v4 or v6), with optional CIDR netmask, into
> diff --git a/netcfg.c b/netcfg.c
> index 195681b..c918641 100644
> --- a/netcfg.c
> +++ b/netcfg.c
> @@ -67,6 +67,7 @@ int main(int argc, char *argv[])
> GET_METHOD,
> GET_DHCP,
> GET_STATIC,
> + GET_VLAN,
> WCONFIG,
> WCONFIG_ESSID,
> WCONFIG_SECURITY_TYPE,
> @@ -216,13 +217,19 @@ int main(int argc, char *argv[])
> state = BACKUP;
> else if (! interface.name || ! num_interfaces)
> state = GET_HOSTNAME_ONLY;
> - else {
> - if (is_wireless_iface (interface.name))
> - state = WCONFIG;
> - else
> - state = GET_METHOD;
> - }
> + else if (is_wireless_iface (interface.name))
> + state = WCONFIG;
> + else
> + state = GET_VLAN;
> + break;
> +
> + case GET_VLAN:
> + if (netcfg_set_vlan(client, &interface) == GO_BACK)
> + state = BACKUP;
> + else
> + state = GET_METHOD;
> break;
> +
> case GET_HOSTNAME_ONLY:
> if(netcfg_get_hostname(client, "netcfg/get_hostname", hostname, 0))
> state = BACKUP;
> @@ -231,6 +238,7 @@ int main(int argc, char *argv[])
> state = QUIT;
> }
> break;
> +
> case GET_METHOD:
> if ((res = netcfg_get_method(client)) == GO_BACK)
> state = (num_interfaces == 1) ? BACKUP : GET_INTERFACE;
> diff --git a/netcfg.h b/netcfg.h
> index 00a2cea..8b45776 100644
> --- a/netcfg.h
> +++ b/netcfg.h
> @@ -151,6 +151,11 @@ struct netcfg_interface {
> /* WPA */
> wpa_t wpa_supplicant_status;
> char *passphrase;
> +
> + /* VLAN */
> + char *parentif;
> + int vlanid;
> +
> };
>
> /* Somewhere we can store both in_addr and in6_addr; convenient for all those
> @@ -221,6 +226,9 @@ extern void deconfigure_network(struct netcfg_interface *iface);
> extern void interface_up (const char *if_name);
> extern void interface_down (const char *if_name);
>
> +extern void netcfg_interface_up (const struct netcfg_interface *iface);
> +extern void netcfg_interface_down (const struct netcfg_interface *iface);
> +
> extern void loop_setup(void);
> extern int get_hostname_from_dns(const struct netcfg_interface *interface, char *hostname, const size_t max_hostname_len);
>
> @@ -270,4 +278,7 @@ extern void cleanup_dhcpv6_client(void);
> extern int start_dhcpv6_client(struct debconfclient *client, const struct netcfg_interface *interface);
> extern int netcfg_autoconfig(struct debconfclient *client, struct netcfg_interface *interface);
>
> +/* vlan.c */
> +extern int netcfg_set_vlan(struct debconfclient *client, struct netcfg_interface *interface);
> +
> #endif /* _NETCFG_H_ */
> diff --git a/static.c b/static.c
> index ea12fba..ba03931 100644
> --- a/static.c
> +++ b/static.c
> @@ -310,7 +310,7 @@ static int netcfg_activate_static_ipv4(struct debconfclient *client,
> deconfigure_network(NULL);
>
> loop_setup();
> - interface_up(interface->name);
> + netcfg_interface_up(interface);
>
> /* Flush all previous addresses, routes */
> snprintf(buf, sizeof(buf), "ifconfig %s inet 0 down", interface->name);
> @@ -345,7 +345,7 @@ static int netcfg_activate_static_ipv4(struct debconfclient *client,
> deconfigure_network(NULL);
>
> loop_setup();
> - interface_up(interface->name);
> + netcfg_interface_up(interface);
>
> /* Flush all previous addresses, routes */
> snprintf(buf, sizeof(buf), "ip -f inet addr flush dev %s", interface->name);
> @@ -426,7 +426,7 @@ static int netcfg_activate_static_ipv6(struct debconfclient *client,
> deconfigure_network(NULL);
>
> loop_setup();
> - interface_up(interface->name);
> + netcfg_interface_up(interface);
>
> /* Flush all previous addresses, routes */
> snprintf(buf, sizeof(buf), "ifconfig %s inet 0 down", interface->name);
> @@ -449,7 +449,7 @@ static int netcfg_activate_static_ipv6(struct debconfclient *client,
> deconfigure_network(NULL);
>
> loop_setup();
> - interface_up(interface->name);
> + netcfg_interface_up(interface);
>
> /* Flush all previous addresses, routes */
> snprintf(buf, sizeof(buf), "ip -f inet6 addr flush dev %s", interface->name);
> @@ -461,8 +461,8 @@ static int netcfg_activate_static_ipv6(struct debconfclient *client,
> /* Now down and up the interface, to get LL and SLAAC addresses back,
> * since flushing the addresses and routes gets rid of all that
> * sort of thing. */
> - interface_down(interface->name);
> - interface_up(interface->name);
> + netcfg_interface_down(interface);
> + netcfg_interface_up(interface);
>
> /* Add the new IP address and netmask */
> snprintf(buf, sizeof(buf), "ip addr add %s/%d dev %s",
> diff --git a/vlan.c b/vlan.c
> new file mode 100644
> index 0000000..d3aa227
> --- /dev/null
> +++ b/vlan.c
> @@ -0,0 +1,97 @@
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <cdebconf/debconfclient.h>
> +#include <debian-installer.h>
> +#include "netcfg.h"
> +
> +static char* get_vlan_command(const char* parentif, const char* vlanif, int vlanid) {
> +#if defined(__linux__)
> + const char* vlan_command = "ip link add link %s name %s type vlan id %d";
> + int len = strlen(vlan_command) + strlen(parentif) + strlen(vlanif) + 4 + 1;
> + char* buf = malloc(len);
> + snprintf(buf, len, vlan_command, parentif, vlanif, vlanid);
> + return buf;
> +#elif defined(__FreeBSD_kernel__)
> + const char* vlan_command = "ifconfig %s vlan %d vlandev %s";
> + int len = strlen(vlan_command) + strlen(parentif) + strlen(vlanif) + 4 + 1;
> + char* buf = malloc(len);
> + snprintf(buf, len, vlan_command, vlanif, vlanid, parentif);
> + return buf;
> +#endif
> +}
> +
> +/* Create a new VLAN interface attached to the currently selected
> + * network interface.
> + */
> +int netcfg_set_vlan (struct debconfclient *client, struct netcfg_interface *interface) {
> +#if defined(__linux__) || defined(__FreeBSD_kernel__)
> + int vlanid;
> +
> + debconf_get(client, "netcfg/vlan_id");
> + /* Empty string: no VLAN preseeded, ask if we should configure VLAN */
> + if (strlen(client->value) == 0) {
> + debconf_input (client, "medium", "netcfg/use_vlan");
> + if (debconf_go(client) == GO_BACK) {
> + return GO_BACK;
> + }
> +
> + debconf_get(client, "netcfg/use_vlan");
> +
> + if (!strcmp(client->value, "false")) {
> + return 0;
> + }
> +
> + debconf_input(client, "critical", "netcfg/vlan_id");
> + if (debconf_go(client) == GO_BACK) {
> + return GO_BACK;
> + }
> + debconf_get(client, "netcfg/vlan_id");
> + }
> +
> + for (;;) {
> + vlanid = strtol(client->value, NULL, 10);
> + /* Valid range: 1-4094 (0 and 4095 are reserved.)
> + * 0 will be returned by strtol if the value cannot be parsed.
> + */
> + if (vlanid < 1 || vlanid > 4094) {
> + di_error("VLAN ID \"%s\" is invalid.", client->value);
> + debconf_subst(client, "netcfg/vlan_id_invalid", "vlan_id", client->value);
> + debconf_input(client, "critical", "netcfg/invalid_vlan");
> + debconf_capb(client);
> + debconf_go(client);
> +
> + debconf_capb(client, "backup");
> + debconf_input(client, "critical", "netcfg/vlan_id");
> + if (debconf_go(client) == GO_BACK) {
> + return GO_BACK;
> + }
> + } else {
> + break;
> + }
> + }
> +
> + int vlaniflen = strlen(interface->name) + 1 + 4 + 1;
> + char* vlanif = malloc(vlaniflen);
> + snprintf(vlanif, vlaniflen, "%s.%d", interface->name, vlanid);
> +
> + char *vlan_command = get_vlan_command(interface->name, vlanif, vlanid);
> + int rc = di_exec_shell_log(vlan_command);
> + if (rc != 0) {
> + di_error("\"%s\" failed to create VLAN interface; return code = %d", vlan_command, rc);
> + free(vlan_command);
> + debconf_input(client, "critical", "netcfg/vlan_failed");
> + debconf_go(client);
> + return GO_BACK;
> + }
> + free(vlan_command);
> +
> + interface->parentif = interface->name;
> + interface->name = vlanif;
> + interface->vlanid = vlanid;
> + di_exec_shell_log("apt-install vlan");
> +#else
> + /* This platform does not support VLANs. */
I somewhat feel that it should fail if vlan_id has been set. But on the
other side it also shouldn't prompt for it. Maybe debconf_get it and
return an error so that the error screen is shown and log the concrete
ENOIMPL to syslog?
> +#endif
> + return 0;
> +}
> diff --git a/wireless.c b/wireless.c
> index 566b032..83b1d1d 100644
> --- a/wireless.c
> +++ b/wireless.c
> @@ -121,7 +121,7 @@ int netcfg_wireless_show_essids(struct debconfclient *client, struct netcfg_inte
> int essid_list_len = 1;
>
> iw_get_basic_config (wfd, interface->name, &wconf);
> - interface_up(interface->name);
> + netcfg_interface_up(interface);
>
> if (iw_scan(wfd, interface->name, iw_get_kernel_we_version(),
> &network_list) >= 0 ) {
> @@ -205,7 +205,7 @@ int netcfg_wireless_show_essids(struct debconfclient *client, struct netcfg_inte
> }
>
> iw_set_basic_config(wfd, interface->name, &wconf);
> - interface_down(interface->name);
> + netcfg_interface_down(interface);
>
> di_info("Network chosen: %s. Proceeding to connect.", interface->essid);
>
> diff --git a/write_interface.c b/write_interface.c
> index 2ab1a34..e768002 100644
> --- a/write_interface.c
> +++ b/write_interface.c
> @@ -44,6 +44,20 @@ static int nc_wi_loopback(const struct netcfg_interface *interface, FILE *fd)
> return 1;
> }
>
> +/* Write VLAN settings, such as: vlan_raw_device eth0
> +*/
> +static int nc_wi_vlan(const struct netcfg_interface *interface, FILE *fd)
> +{
> + int rv;
> + rv = 1;
> + if (interface && interface->parentif &&
> + (fprintf(fd, "\tvlan_raw_device %s\n", interface->parentif) < 0)) {
> + rv = 0;
> + }
> + return rv;
> +}
Would it make sense to have network-manager support as well? It should
be fairly simple and along these lines:
[vlan]
parent=%parentif%
id=%vlanid%
> +
> +
> static int nc_wi_wireless_options(const struct netcfg_interface *interface, FILE *fd)
> {
> /*
> @@ -271,7 +285,10 @@ int netcfg_write_interface(const struct netcfg_interface *interface)
> di_debug("Writing static IPv6 stanza for %s", interface->name);
> rv = nc_wi_static_ipv6(interface, fd);
> }
> -
> + if (rv && interface && interface->parentif) {
> + di_debug("Writing VLAN: %s", interface->name);
> + rv = nc_wi_vlan(interface, fd);
> + }
> if (rv && interface && is_wireless_iface(interface->name)) {
> di_debug("Writing wireless options for %s", interface->name);
> rv = nc_wi_wireless_options(interface, fd);
> --
> 2.7.4
>
Otherwise this looks pretty good to me at this point. Much thanks for
doing the effort.
Kind regards
Philipp Kern
Reply to: