On Sat, Jul 24, 2010 at 07:04:43PM +0100, Stefan Hajnoczi wrote:
> CCed to the gpxe-devel@etherboot.org mailing list. The original mail went to
> g...@etherboot.org.
>
> Here is the dhcp multiple interface patch from Lars Kellogg-Stedman
> <l...@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.
> }
>
> /**
_______________________________________________
gPXE-devel mailing list
gPXE-devel@etherboot.org
http://etherboot.org/mailman/listinfo/gpxe-devel
