[gPXE-devel] [gPXE] PATCH: Allow "dhcp" command to iterate over multiple interfaces like autoboot()

[Stefan Hajnoczi]

On Sat, Jul 24, 2010 at 07:04:43PM +0100, Stefan Hajnoczi wrote:
> CCed to the gpxe-devel at etherboot.org mailing list.  The original mail went to
> gpxe at etherboot.org.
> 
> Here is the dhcp multiple interface patch from Lars Kellogg-Stedman
> <lars at oddbit.com> inline for easy reviewing:

Please use gPXE coding style, see net/netdevice.c for examples.
Whitespace is used like this:

static void foo ( int arg ) {
	if ( arg > 2 )
		bar(); /* no spaces when function has no arguments */
}

> 
> diff --git a/src/hci/commands/dhcp_cmd.c b/src/hci/commands/dhcp_cmd.c
> index 96aac8d..a494068 100644
> --- a/src/hci/commands/dhcp_cmd.c
> +++ b/src/hci/commands/dhcp_cmd.c
> @@ -31,6 +31,7 @@ FILE_LICENCE ( GPL2_OR_LATER );
>  #include <gpxe/in.h>
>  #include <gpxe/command.h>
>  #include <usr/dhcpmgmt.h>
> +#include <usr/ifmgmt.h>
>  
>  /** @file
>   *
> @@ -45,10 +46,87 @@ FILE_LICENCE ( GPL2_OR_LATER );
>   */
>  static void dhcp_syntax ( char **argv ) {
>  	printf ( "Usage:\n"
> -		 "  %s <interface>\n"
> +		 "  %s [-c] <interface> [<interface> ...]\n"
> +		 "  %s any\n"
>  		 "\n"
>  		 "Configure a network interface using DHCP\n",
> -		 argv[0] );
> +		 argv[0], argv[0] );
> +}
> +
> +/**
> + * Given a device name, attempt to configure that device using dhcp.
> + *
> + * @netdev_name		Name of the network device
> + */
> +static int dhcp_one_device_name (char *netdev_name) {
> +	int rc;
> +	struct net_device *netdev;
> +
> +	netdev = find_netdev ( netdev_name );
> +
> +	if ( ! netdev ) {
> +		printf ( "No such interface: %s\n", netdev_name );
> +		return 2;
> +	}
> +
> +	/* Perform DHCP */
> +	if ( ( rc = dhcp ( netdev ) ) != 0 ) {
> +		printf ( "Could not configure %s: %s\n", netdev->name,
> +				strerror ( rc ) );

If DHCP fails the device should be closed.  The dhcp() function will
automatically open the netdev.  Having multiple netdevs open
simultaneously can cause out-of-memory situations because gPXE only
claims a small chunk of memory to use as its heap.

Previously the command didn't close single netdevs.  gPXE aborts script
execution if a command fails, so I don't think existing users will
be able to detect this change.

> +		return 1;
> +	} 
> +
> +	return 0;
> +}
> +
> +/**
> + * Call dhcp_one_device_name() for each name in argv.
> + *
> + * @argc		Number of devices
> + * @argv		List of device names
> + * @keep_going		If TRUE, ignore errors caused by unknown device names.
> + */
> +static int dhcp_each_device_name (int argc, char *argv[], int keep_going) {
> +	int i;
> +
> +	for (i=0; i<argc; i++) {
> +		switch ( dhcp_one_device_name ( argv[i] ) ) {
> +			case 0:
> +				return 0;
> +			case 1:
> +				break;
> +			case 2:
> +				if (! keep_going)
> +					return 1;
> +		}
> +	}
> +
> +	printf( "Could not configure any interface.\n" );
> +	return 1;
> +}
> +
> +/**
> + * Call dhcp_one_device_name() for each device in net_devices.
> + */
> +static int dhcp_each_device () {

Please use a void argument list to mark a function with no arguments.
In C giving no argument list means the compiler doesn't type-check
calls, you could get away with using the function like this:

dhcp_each_device ( "hello", 123 );

> +	struct net_device *netdev;
> +
> +	for_each_netdev ( netdev ) {
> +		switch ( dhcp_one_device_name ( netdev->name ) ) {
> +			case 0:
> +				return 0;
> +			case 1:
> +				break;
> +			case 2:
> +				/* we should never get here, since we're working
> +				 * from a list of known device names.
> +				 */
> +				return 1;

If dhcp_one_device_name() doesn't do find_netdev(), then this switch
statement can turn into a simple if statement:

if ( ! dhcp_one_device_name ( netdev ) )
	return 0;

And the change to dhcp_each_device_name() would make the code nicer too,
I think.

> +		}
> +	}
> +
> +	printf( "Could not configure any interface.\n" );
> +	return 1;
>  }
>  
>  /**
> @@ -61,16 +139,18 @@ static void dhcp_syntax ( char **argv ) {
>  static int dhcp_exec ( int argc, char **argv ) {
>  	static struct option longopts[] = {
>  		{ "help", 0, NULL, 'h' },
> +		{ "continue", 0, NULL, 'c' },
>  		{ NULL, 0, NULL, 0 },
>  	};
> -	const char *netdev_txt;
> -	struct net_device *netdev;
>  	int c;
> -	int rc;
> +	int keep_going = 0;
>  
>  	/* Parse options */
> -	while ( ( c = getopt_long ( argc, argv, "h", longopts, NULL ) ) >= 0 ){
> +	while ( ( c = getopt_long ( argc, argv, "ch", longopts, NULL ) ) >= 0 ){
>  		switch ( c ) {
> +		case 'c':
> +			keep_going = 1;
> +			break;
>  		case 'h':
>  			/* Display help text */
>  		default:
> @@ -81,27 +161,19 @@ static int dhcp_exec ( int argc, char **argv ) {
>  	}
>  
>  	/* Need exactly one interface name remaining after the options */
> -	if ( optind != ( argc - 1 ) ) {
> +	if ( ( argc - optind ) < 1 ) {
>  		dhcp_syntax ( argv );
>  		return 1;
>  	}
> -	netdev_txt = argv[optind];
>  
> -	/* Parse arguments */
> -	netdev = find_netdev ( netdev_txt );
> -	if ( ! netdev ) {
> -		printf ( "No such interface: %s\n", netdev_txt );
> -		return 1;
> +	if (strcmp(argv[optind], "any") == 0) {
> +		return dhcp_each_device ();
> +	} else {
> +		return dhcp_each_device_name (argc-optind, argv+optind, keep_going);
>  	}

I'd move dhcp_each_device_name() outside an "else"...

>  
> -	/* Perform DHCP */
> -	if ( ( rc = dhcp ( netdev ) ) != 0 ) {
> -		printf ( "Could not configure %s: %s\n", netdev->name,
> -			 strerror ( rc ) );
> -		return 1;
> -	}
> -
> -	return 0;
> +	/* we should never get here. */
> +	return 1;

...and drop these two lines of dead code.

>  }
>  
>  /**