[Buildroot] [PATCH] set simple network setup via the system configuration submenu

[Jeremy Rosen]

Hello Arnout

I've read all your comments and it boils down to the following philosophy

* don't plan for having more complicated stuff : one interface, simple 
cases only
* lo should always be enabled
* backward compatibility

I agree with this approch, see my remarks below for specific answers to
your comments, I'll respin the patch when it's ready.

----- Mail original -----
> On 08/09/14 14:13, Jérémy Rosen wrote:
> > This patch allows the setup of simple /etc/network/interfaces via
> > the
> > configuration menus instead of using an overlay
> 
>  I definitely like the idea of that!
> 
>  I think that this is where buildroot can still grow a little: simple
>  system
> configuration through the Kconfig interface.
> 
> > 
> > * supports the loopback interface
> > * supports one normal interface (renamable)
> > * supports manual ipv4 configuration
> > * supports dhcp configuration
> > 
> > Signed-off-by: Jérémy Rosen <jeremy.rosen at openwide.fr>
> > 
> > ---
> > 
> > This patch is here to avoid having to do an overlay for the most
> > common
> > cases (ipv4 with fixed IP or DHCP)
> > 
> > I can make it more complex (second network if, support ipv6)
> > depending on
> > what people want/need, but I want to keep it simple. The point is
> > to avoid
> > having to tweak overlays to change stuff that everybody needs to
> > change for
> > prototyping
> 
>  Anyway that could be done in later patches. Better first let this
>  functionality
> be tested for a while and then extend it.
> 
> > 
> > With regards to systemd, as far as I could figure it still uses
> > /etc/network/interfaces but with different names. AFAIU there is no
> > sane
> > default to replace the "eth0" name so I did not put a different
> > default
> > when systemd is used
> 
>  In the meantime you know better :-). If you don't implement the
> systemd-networkd support in the end, then I suggest you mention it in
> comments
> in generate-interfaces.sh
> 

I've looked into it, it's doable but complicated so i'll disable 
network setup when networkd is enabled and let people write their
own service for the time being. networkd enables lo when it is
not configured so all should be fine.

> 
> > ---
> >  Makefile                               |  1 +
> >  support/scripts/generate-interfaces.sh | 75
> >  ++++++++++++++++++++++++++++++++
> >  system/Config.in                       | 78
> >  ++++++++++++++++++++++++++++++++++
> >  3 files changed, 154 insertions(+)
> >  create mode 100755 support/scripts/generate-interfaces.sh
> > 
> > diff --git a/Makefile b/Makefile
> > index e788f1b..71cad7d 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -477,6 +477,7 @@ $(BUILD_DIR)/.root:
> >  	@ln -snf lib $(TARGET_DIR)/$(LIB_SYMLINK)
> >  	@mkdir -p $(TARGET_DIR)/usr
> >  	@ln -snf lib $(TARGET_DIR)/usr/$(LIB_SYMLINK)
> > +	./support/scripts/generate-interfaces.sh $(TARGET_DIR)
> 
>  +1 for moving it to TARGET_FINALIZE_HOOKS.
> 
>  But I think it should be conditional on BR2_SIMPLE_NETWORK=y.
> 

I moved it to TARGET_FINALIZE but did not make it conditional.
it is needed for enabling lo, even when ethO is not configured

> >  	touch $@
> >  
> >  $(TARGET_DIR): $(BUILD_DIR)/.root
> > diff --git a/support/scripts/generate-interfaces.sh
> > b/support/scripts/generate-interfaces.sh
> > new file mode 100755
> > index 0000000..b685669
> > --- /dev/null
> > +++ b/support/scripts/generate-interfaces.sh
> > @@ -0,0 +1,75 @@
> > +#!/bin/sh
> > +
> > +
> > +#extract  our parameters from the config file
> > +for PARAM in \
> > +	BR2_SIMPLE_NETWORK_LO_ENABLE \
> > +	BR2_SIMPLE_NETWORK_LO_AUTO \
> > +	\
> > +	BR2_SIMPLE_NETWORK_1_ENABLE \
> > +	BR2_SIMPLE_NETWORK_1_AUTO \
> > +	BR2_SIMPLE_NETWORK_1_IPV4_DHCP \
> > +	BR2_SIMPLE_NETWORK_1_IPV4_MANUAL \
> > +	; do
> > +TMP=$(sed -r -e "/^$PARAM=(.*)$/!d;" -e 's//\1/;' $BR2_CONFIG)
> > +	export $PARAM=$TMP
> > +done
> > +
> > +for PARAM in \
> > +	BR2_SIMPLE_NETWORK_1_NAME \
> > +	BR2_SIMPLE_NETWORK_1_IPV4_ADDRESS \
> > +	BR2_SIMPLE_NETWORK_1_IPV4_NETMASK \
> > +	BR2_SIMPLE_NETWORK_1_IPV4_BROADCAST \
> > +	BR2_SIMPLE_NETWORK_1_IPV4_GATEWAY \
> > +	; do
> > +TMP=$(sed -r -e "/^$PARAM=\"(.*)\"$/!d;" -e 's//\1/;' $BR2_CONFIG)
> > +	export $PARAM=$TMP
> > +done
> > +
> 
> 
>  You can just source the config, that's a lot simpler than all this
>  magic. If
> you really want you could grep out the BR2_SIMPLE_NETWORK part but
> really I
> don't see the point.
> 

unfortunately that doesnt work because of lines like the following

BR2_HOST_DIR="$(BASE_DIR)/host"

I've copied the logic from mkusers and yeah... it's a bit ugly.


> > +
> > +
> > +IFACE_FILE=$TARGET_DIR/etc/network/interfaces
> > +echo -n > $IFACE_FILE # empty the file
> 
>  Perhaps, just to be sure, an mkdir -p of /etc/network ?
> 

will do

>  Just a minor nit, but wouldn't it be simpler to put all of the below
>  in a
> function which you then call with
> 
> do_generate_interfaces > $TARGET_DIR/etc/network/interfaces
> 
> ? Then you don't have to redirect all the time. Or you could also
> just do
> 
> exec > $TARGET_DIR/etc/network/interfaces
> 

will do

> > +
> > +if [ $BR2_SIMPLE_NETWORK_LO_ENABLE ] ; then
> 
>  I didn't know that this worked... I always thought you had to put
>  quotes to
> protect against an empty string, but it turns out that [ ] works
> perfectly.
> Thanks for teaching me!
> 
> > +	if [ $BR2_SIMPLE_NETWORK_LO_AUTO ] ; then
> > +		echo "auto lo">> $IFACE_FILE
> > +	fi
> > +	echo "iface lo inet loopback">> $IFACE_FILE
> > +	echo >>$IFACE_FILE
> > +fi
> > +
> > +if [ $BR2_SIMPLE_NETWORK_1_ENABLE ] ; then
> > +	if [ -z $BR2_SIMPLE_NETWORK_1_NAME ] ; then
> > +		echo ERROR no name specified for first network interface
> 
>  If you follow my suggestion of redirecting once, then this should of
>  course be
> 1>&2 (which would be a good idea anyway).
> 

indeed, will do

>  I would actually prefer to check things like this in the .mk file,
>  so you see
> the error earlier. However, in this case, that would make things way
> to
> complicated I'm afraid.
> 

my Makefile-fu is even worse than my shell-fu, I'm not sure I could 
handle this properly...

> > +		exit 1
> > +	fi
> > +	if [ $BR2_SIMPLE_NETWORK_1_AUTO ] ; then
> > +		echo "auto  $BR2_SIMPLE_NETWORK_1_NAME">> $IFACE_FILE
> > +	fi
> > +	if [ $BR2_SIMPLE_NETWORK_1_IPV4_DHCP ] ; then
> > +		echo "iface $BR2_SIMPLE_NETWORK_1_NAME inet dhcp">> $IFACE_FILE
> > +	elif [ $BR2_SIMPLE_NETWORK_1_IPV4_MANUAL ] ; then
> > +		for PARAM in \
> > +			BR2_SIMPLE_NETWORK_1_IPV4_ADDRESS \
> > +			BR2_SIMPLE_NETWORK_1_IPV4_NETMASK \
> > +			BR2_SIMPLE_NETWORK_1_IPV4_BROADCAST \
> > +			BR2_SIMPLE_NETWORK_1_IPV4_GATEWAY \
> 
>  Only address is really required. ifconfig will derive the netmask
>  from the
> address (though in practice that usually doesn't work anymore
> nowadays, because
> of CIDR and because we use network-local IP addresses with a much
> smaller range
> than the default 192.168.0.0/16).

the busybox ifup seems to need both address and netmask to bring an
interface up (I havn't tested with the real net-utils) so i'll have
to make both mandatory.

I have settled to a default address of 0.0.0.0 and a netmask of
255.255.255.255  which is accepted by ifconfig to bring the 
interface up with no IP address (so not dangerous on a 
network) and should allow the autobuilder to correctly work.

Broadcast and gateway are indeed optional and can default to
empty

I am still open to suggestions on exactly what default to 
provide, but I think this is the best we can do without
a way to tell the autobuilder to only generate for DHCP or
something like that...

> Broadcast is derived from the
> netmask, and the
> gateway is only needed if you want non-local access.
> 
> > +			; do
> > +			eval VALUE=\$$PARAM
> > +			if [ -z $VALUE ] ; then
> > +				echo ERROR $PARAM not set
> > +				exit 1
> > +			fi
> > +		done
> > +		echo "iface $BR2_SIMPLE_NETWORK_1_NAME inet static">>
> > $IFACE_FILE
> > +		echo "	address $BR2_SIMPLE_NETWORK_1_IPV4_ADDRESS">> $IFACE_FILE
> > +		echo "	netmask $BR2_SIMPLE_NETWORK_1_IPV4_NETMASK">> $IFACE_FILE
> > +		echo "	broadcast $BR2_SIMPLE_NETWORK_1_IPV4_BROADCAST">>
> > $IFACE_FILE
> > +		echo "	gateway $BR2_SIMPLE_NETWORK_1_IPV4_GATEWAY">> $IFACE_FILE
> 
>  So the last three should be conditional on whether it is defined.
> 

will fix all that

> > +	else
> > +		echo Incorrect buildroot configuration
> > +		exit 1
> > +	fi
> > +	echo >>$IFACE_FILE
> > +fi
> > diff --git a/system/Config.in b/system/Config.in
> > index e7e146a..d5711bc 100644
> > --- a/system/Config.in
> > +++ b/system/Config.in
> > @@ -389,4 +389,82 @@ config BR2_ROOTFS_POST_SCRIPT_ARGS
> >  	  directory / images directory. The arguments in this option will
> >  	  be
> >  	  passed *after* those.
> >  
> > +menuconfig BR2_SIMPLE_NETWORK
> > +	bool "Generate simple network configuration"
> > +	default n
> 
>  Since we currently do generate this, I think it should default to y.
> 

I slightly changed the logic, the option is only about eth0, so should
default to n for backward compatibility, the script is always called
to generate the lo part. 

>  Also, I think this menu should go before the rootfs overlay option,
>  so it
> corresponds to the order in which we do things. Maybe even just after
> the
> remount option.
> 

makes sense, will do

> > +	help
> > +	  Use buildroot to set network configuration during the build
> > process
> 
>  Suggested more complete help text:
> 
> 	  Let buildroot create the network configuration file during the
> 	  build
> 	  process. This is /etc/network/interfaces when using ifupdown, and
> 	  /etc/systemd/network/<iface>.network when using systemd-networkd.
> 
> 	  The simple network configuration only supports the loopback
> 	  interface
> 	  and a single other network interface using a static or
> 	  DHCP-allocated
> 	  IPv4 address. If you need something more complicate, create your
> 	  own
> 	  configuration file in the BR2_ROOTFS_OVERLAY.
> 

much better, I will use that (slight reword because the option is not 
about generating the file, but including a default eth0 in the file)

> > +
> > +if BR2_SIMPLE_NETWORK
> > +menuconfig BR2_SIMPLE_NETWORK_LO_ENABLE
> > +	bool "enable loopback device"
> > +	default y
> > +	help
> > +	   Enables the loopback interface at startup
> > +
> > +if BR2_SIMPLE_NETWORK_LO_ENABLE
> > +config BR2_SIMPLE_NETWORK_LO_AUTO
> > +	bool "enable loopback interface at startup"
> > +	default y
> > +	help
> > +	   Should the loopback inteface be brought up automatically at
> > startup
> 
>  I wonder if these auto things really make sense. Why would you ever
>  want it
> non-auto? I mean, I can imagine a lot of situations, but then you
> anyway don't
> want to use the simple network management.
> 

makes sense, option removed

>  Actually, I even doubt it makes sense to leave out the loopback
>  interface.
> Without loopback, things tend to be pretty fragile, so I wouldn't put
> that
> option in a _simple_ network configuration. Too much ammo to shoot
> yourself in
> the foot, and you can make your own /etc/network/interfaces if you
> really want to.
> 

agreed

> > +
> > +endif
> > +
> > +menuconfig BR2_SIMPLE_NETWORK_1_ENABLE
> > +	bool "enable first network interface"
> > +	default y
> > +	help
> > +	   Enable the first network interface
> 
>  As long as we support only one interface, calling it the first one
>  doesn't make
> much sense.
> 

ok, will rename the options

>  Also, if the loopback option is removed, then nesting it another
>  level doesn't
> make much sense. And even if the loopback stays, I would do the
> second-level
> nesting with indentation (i.e. config instead of menuconfig and rely
> on the
> condition to indent it).
> 

will do


> > +
> > +if BR2_SIMPLE_NETWORK_1_ENABLE
> > +config BR2_SIMPLE_NETWORK_1_AUTO
> > +	bool "enable first network interface at startup"
> > +	default y
> > +	help
> > +	   Should the first network inteface be brought up automatically
> > at startup
> 
>  Again, I don't think it's useful to have this option.
> 
> > +
> > +config BR2_SIMPLE_NETWORK_1_NAME
> > +	string "name of the first physical network interface"
> 
>  Since this is already in the 'first network interface' menu, there
>  is no point
> in repeating 'first physical network interface' everywhere. Same
> story if it is
> indented.
> 

ok

> 
> > +	default "eth0"
> > +	help
> > +	   The name used to recognise the first network interface as
> > reported by the kernel
> > +
> > +choice
> > +	prompt "Configuration type"
> > +	default BR2_SIMPLE_NETWORK_1_DHCP
> > +	help
> > +	   The type of configuration to use for the first physical
> > interface
> > +
> > +config BR2_SIMPLE_NETWORK_1_IPV4_DHCP
> > +	bool "IPv4 with DHCP"
> > +	help
> > +	   Use DHCP to configure this interface
> > +	   using the IPv4 protocol
> > +
> > +config BR2_SIMPLE_NETWORK_1_IPV4_MANUAL
> > +	bool "IPv4 with parameters manually specified"
> 
>  Call this "IPv4 with static IP address"
> 

better indeed

> > +	help
> > +	   Configure IPv4 by specifying each parameter separately
> > +endchoice
> > +
> > +if BR2_SIMPLE_NETWORK_1_IPV4_MANUAL
> > +config BR2_SIMPLE_NETWORK_1_IPV4_ADDRESS
> > +	string "IP Address of the first network interface"
> > +
> > +config BR2_SIMPLE_NETWORK_1_IPV4_NETMASK
> > +	string "Netmask of the first network interface"
> > +
> > +config BR2_SIMPLE_NETWORK_1_IPV4_BROADCAST
> > +	string "Broadcast Address of the first network interface"
> 
>  Actually, does it make sense to specify a broadcast address? When is
>  it ever
> different from default?
> 

I don't know how to guess it, pretty standard, easy to add and 
can be left empty. I think it makes sense to leave it as is...

> 
>  Bottom line: I'd limit the configurability to the minimum useful
>  set, and leave
> the rest for a rootfs overlay.
> 
>  Regards,
>  Arnout
> 

Thx for the review i'll respin this soon

> > +
> > +config BR2_SIMPLE_NETWORK_1_IPV4_GATEWAY
> > +	string "Address of the gateway for the first network interface"
> > +endif
> > +
> > +endif
> > +
> > +endif
> > +
> > +
> >  endmenu
> > 
> 
> 
> --
> Arnout Vandecappelle                          arnout at mind be
> Senior Embedded Software Architect            +32-16-286500
> Essensium/Mind                                http://www.mind.be
> G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR
> Leuven
> LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
> GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F
>