[image] Simplify image management commands and internal API

Remove the name, cmdline, and action parameters from imgdownload() and
imgdownload_string().  These functions now simply download and return
an image.

Add the function imgacquire(), which will interpret a "name or URI
string" parameter and return either an existing image or a newly
downloaded image.

Use imgacquire() to merge similar image-management commands that
currently differ only by whether they take the name of an existing
image or the URI of a new image to download.  For example, "chain" and
"imgexec" can now be merged.

Extend imgstat and imgfree commands to take an optional list of
images.

Remove the arbitrary restriction on the length of image names.

Signed-off-by: Michael Brown <mcb30@ipxe.org>

src/arch/i386/core/runtime.c

@@ -189,14 +189,18 @@ static int initrd_init ( void ) {
 	       initrd_phys, ( initrd_phys + initrd_len ) );
 
 	/* Allocate image */
-	image = alloc_image();
+	image = alloc_image ( NULL );
 	if ( ! image ) {
 		DBGC ( colour, "RUNTIME could not allocate image for "
 		       "initrd\n" );
 		rc = -ENOMEM;
 		goto err_alloc_image;
 	}
-	image_set_name ( image, "<INITRD>" );
+	if ( ( rc = image_set_name ( image, "<INITRD>" ) ) != 0 ) {
+		DBGC ( colour, "RUNTIME could not set image name: %s\n",
+		       strerror ( rc ) );
+		goto err_set_name;
+	}
 
 	/* Allocate and copy initrd content */
 	image->data = umalloc ( initrd_len );
@@ -227,6 +231,7 @@ static int initrd_init ( void ) {
 
  err_register_image:
  err_umalloc:
+ err_set_name:
 	image_put ( image );
  err_alloc_image:
 	return rc;

src/arch/i386/interface/syslinux/comboot_call.c

@@ -166,6 +166,8 @@ void comboot_force_text_mode ( void ) {
  * Fetch kernel and optional initrd
  */
 static int comboot_fetch_kernel ( char *kernel_file, char *cmdline ) {
+	struct image *kernel;
+	struct image *initrd;
 	char *initrd_file;
 	int rc;
 
@@ -184,8 +186,7 @@ static int comboot_fetch_kernel ( char *kernel_file, char *cmdline ) {
 		DBG ( "COMBOOT: fetching initrd '%s'\n", initrd_file );
 
 		/* Fetch initrd */
-		if ( ( rc = imgdownload_string ( initrd_file, NULL, NULL,
-						 NULL ) ) != 0 ) {
+		if ( ( rc = imgdownload_string ( initrd_file, &initrd ) ) != 0){
 			DBG ( "COMBOOT: could not fetch initrd: %s\n",
 			      strerror ( rc ) );
 			return rc;
@@ -198,14 +199,20 @@ static int comboot_fetch_kernel ( char *kernel_file, char *cmdline ) {
 
 	DBG ( "COMBOOT: fetching kernel '%s'\n", kernel_file );
 
-	/* Allocate and fetch kernel */
-	if ( ( rc = imgdownload_string ( kernel_file, NULL, cmdline,
-					 image_replace ) ) != 0 ) {
+	/* Fetch kernel */
+	if ( ( rc = imgdownload_string ( kernel_file, &kernel ) ) != 0 ) {
 		DBG ( "COMBOOT: could not fetch kernel: %s\n",
 		      strerror ( rc ) );
 		return rc;
 	}
 
+	/* Replace comboot image with kernel */
+	if ( ( rc = image_replace ( kernel ) ) != 0 ) {
+		DBG ( "COMBOOT: could not replace with kernel: %s\n",
+		      strerror ( rc ) );
+		return rc;
+	}
+
 	return 0;
 }
 

src/core/image.c

@@ -66,6 +66,7 @@ static int require_trusted_images_permanent = 0;
 static void free_image ( struct refcnt *refcnt ) {
 	struct image *image = container_of ( refcnt, struct image, refcnt );
 
+	free ( image->name );
 	free ( image->cmdline );
 	uri_put ( image->uri );
 	ufree ( image->data );
@@ -77,37 +78,56 @@ static void free_image ( struct refcnt *refcnt ) {
 /**
  * Allocate executable image
  *
+ * @v uri		URI, or NULL
  * @ret image		Executable image
  */
-struct image * alloc_image ( void ) {
+struct image * alloc_image ( struct uri *uri ) {
+	const char *name;
 	struct image *image;
+	int rc;
 
+	/* Allocate image */
 	image = zalloc ( sizeof ( *image ) );
-	if ( image ) {
-		ref_init ( &image->refcnt, free_image );
+	if ( ! image )
+		goto err_alloc;
+
+	/* Initialise image */
+	ref_init ( &image->refcnt, free_image );
+	if ( uri ) {
+		image->uri = uri_get ( uri );
+		name = basename ( ( char * ) uri->path );
+		if ( ( rc = image_set_name ( image, name ) ) != 0 )
+			goto err_set_name;
 	}
+
 	return image;
+
+ err_set_name:
+	image_put ( image );
+ err_alloc:
+	return NULL;
 }
 
 /**
- * Set image URI
+ * Set image name
  *
  * @v image		Image
- * @v URI		New image URI
- *
- * If no name is set, the name will be updated to the base name of the
- * URI path (if any).
+ * @v name		New image name
+ * @ret rc		Return status code
  */
-void image_set_uri ( struct image *image, struct uri *uri ) {
-	const char *path = uri->path;
+int image_set_name ( struct image *image, const char *name ) {
+	char *name_copy;
 
-	/* Replace URI reference */
-	uri_put ( image->uri );
-	image->uri = uri_get ( uri );
+	/* Duplicate name */
+	name_copy = strdup ( name );
+	if ( ! name_copy )
+		return -ENOMEM;
+
+	/* Replace existing name */
+	free ( image->name );
+	image->name = name_copy;
 
-	/* Set name if none already specified */
-	if ( path && ( ! image->name[0] ) )
-		image_set_name ( image, basename ( ( char * ) path ) );
+	return 0;
 }
 
 /**
@@ -137,11 +157,14 @@ int image_set_cmdline ( struct image *image, const char *cmdline ) {
  */
 int register_image ( struct image *image ) {
 	static unsigned int imgindex = 0;
+	char name[8]; /* "imgXXXX" */
+	int rc;
 
 	/* Create image name if it doesn't already have one */
-	if ( ! image->name[0] ) {
-		snprintf ( image->name, sizeof ( image->name ), "img%d",
-			   imgindex++ );
+	if ( ! image->name ) {
+		snprintf ( name, sizeof ( name ), "img%d", imgindex++ );
+		if ( ( rc = image_set_name ( image, name ) ) != 0 )
+			return rc;
 	}
 
 	/* Avoid ending up with multiple "selected" images on

src/core/parseopt.c

@@ -114,28 +114,6 @@ int parse_netdev ( const char *text, struct net_device **netdev ) {
 	return 0;
 }
 
-/**
- * Parse image name
- *
- * @v text		Text
- * @ret image		Image
- * @ret rc		Return status code
- */
-int parse_image ( const char *text, struct image **image ) {
-
-	/* Sanity check */
-	assert ( text != NULL );
-
-	/* Find network device */
-	*image = find_image ( text );
-	if ( ! *image ) {
-		printf ( "\"%s\": no such image\n", text );
-		return -ENOENT;
-	}
-
-	return 0;
-}
-
 /**
  * Parse flag
  *

src/hci/commands/digest_cmd.c

@@ -28,6 +28,7 @@ FILE_LICENCE ( GPL2_OR_LATER );
 #include <ipxe/crypto.h>
 #include <ipxe/md5.h>
 #include <ipxe/sha1.h>
+#include <usr/imgmgmt.h>
 
 /** @file
  *
@@ -74,8 +75,8 @@ static int digest_exec ( int argc, char **argv,
 
 	for ( i = optind ; i < argc ; i++ ) {
 
-		/* find image */
-		if ( ( rc = parse_image ( argv[i], &image ) ) != 0 )
+		/* Acquire image */
+		if ( ( rc = imgacquire ( argv[i], &image ) ) != 0 )
 			continue;
 		offset = 0;
 		len = image->len;

src/hci/commands/image_cmd.c

@@ -34,164 +34,201 @@ FILE_LICENCE ( GPL2_OR_LATER );
  *
  */
 
-/** "imgfetch" options */
-struct imgfetch_options {
+/** "img{single}" options */
+struct imgsingle_options {
 	/** Image name */
 	const char *name;
 };
 
-/** "imgfetch" option list */
-static struct option_descriptor imgfetch_opts[] = {
+/** "img{single}" option list */
+static struct option_descriptor imgsingle_opts[] = {
 	OPTION_DESC ( "name", 'n', required_argument,
-		      struct imgfetch_options, name, parse_string ),
+		      struct imgsingle_options, name, parse_string ),
 };
 
-/** "imgfetch" command descriptor */
-static struct command_descriptor imgfetch_cmd =
-	COMMAND_DESC ( struct imgfetch_options, imgfetch_opts, 1, MAX_ARGUMENTS,
-		       "[--name <name>] <uri> [<arguments>...]" );
+/** "img{single}" command descriptor */
+static struct command_descriptor imgsingle_cmd =
+	COMMAND_DESC ( struct imgsingle_options, imgsingle_opts,
+		       1, MAX_ARGUMENTS,
+		       "[--name <name>] <uri|image> [<arguments>...]" );
+
+/** An "img{single}" family command descriptor */
+struct imgsingle_descriptor {
+	/** Command descriptor */
+	struct command_descriptor *cmd;
+	/** Function to use to acquire the image */
+	int ( * acquire ) ( const char *name, struct image **image );
+	/** Pre-action to take upon image, or NULL */
+	void ( * preaction ) ( struct image *image );
+	/** Action to take upon image, or NULL */
+	int ( * action ) ( struct image *image );
+	/** Verb to describe action */
+	const char *verb;
+};
 
 /**
- * The "imgfetch" and friends command body
+ * The "img{single}" family of commands
  *
  * @v argc		Argument count
  * @v argv		Argument list
- * @v cmd		Command descriptor
+ * @v desc		"img{single}" command descriptor
  * @v action_name	Action name (for error messages)
- * @v action		Action to take upon a successful download
+ * @v action		Action to take upon image
  * @ret rc		Return status code
  */
-static int imgfetch_core_exec ( int argc, char **argv,
-				const char *action_name,
-				int ( * action ) ( struct image *image ) ) {
-	struct imgfetch_options opts;
-	char *uri_string;
+static int imgsingle_exec ( int argc, char **argv,
+			    struct imgsingle_descriptor *desc ) {
+	struct imgsingle_options opts;
+	char *name_uri = NULL;
 	char *cmdline = NULL;
+	struct image *image;
 	int rc;
 
 	/* Parse options */
-	if ( ( rc = parse_options ( argc, argv, &imgfetch_cmd, &opts ) ) != 0 )
+	if ( ( rc = parse_options ( argc, argv, desc->cmd, &opts ) ) != 0 )
 		goto err_parse_options;
 
-	/* Parse URI string */
-	uri_string = argv[optind];
+	/* Parse name/URI string and command line, if present */
+	if ( optind < argc ) {
+		name_uri = argv[optind];
+		if ( argv[ optind + 1 ] != NULL ) {
+			cmdline = concat_args ( &argv[ optind + 1 ] );
+			if ( ! cmdline ) {
+				rc = -ENOMEM;
+				goto err_parse_cmdline;
+			}
+		}
+	}
 
-	/* Parse command line */
-	if ( argv[ optind + 1 ] != NULL ) {
-		cmdline = concat_args ( &argv[ optind + 1 ] );
-		if ( ! cmdline ) {
-			rc = -ENOMEM;
-			goto err_cmdline;
+	/* Acquire the image */
+	if ( name_uri ) {
+		if ( ( rc = desc->acquire ( name_uri, &image ) ) != 0 )
+			goto err_acquire;
+	} else {
+		image = image_find_selected();
+		if ( ! image ) {
+			printf ( "No image selected\n" );
+			goto err_acquire;
 		}
 	}
 
-	/* Fetch the image */
-	if ( ( rc = imgdownload_string ( uri_string, opts.name, cmdline,
-					 action ) ) != 0 ) {
-		printf ( "Could not %s %s: %s\n",
-			 action_name, uri_string, strerror ( rc ) );
-		goto err_imgdownload;
+	/* Carry out command pre-action, if applicable */
+	if ( desc->preaction )
+		desc->preaction ( image );
+
+	/* Set the image name, if applicable */
+	if ( opts.name ) {
+		if ( ( rc = image_set_name ( image, opts.name ) ) != 0 ) {
+			printf ( "Could not name image: %s\n",
+				 strerror ( rc ) );
+			goto err_set_name;
+		}
 	}
 
-	/* Free command line */
-	free ( cmdline );
+	/* Set the command-line arguments, if applicable */
+	if ( cmdline ) {
+		if ( ( rc = image_set_cmdline ( image, cmdline ) ) != 0 ) {
+			printf ( "Could not set arguments: %s\n",
+				 strerror ( rc ) );
+			goto err_set_cmdline;
+		}
+	}
 
-	return 0;
+	/* Carry out command action, if applicable */
+	if ( desc->action ) {
+		if ( ( rc = desc->action ( image ) ) != 0 ) {
+			printf ( "Could not %s: %s\n",
+				 desc->verb, strerror ( rc ) );
+			goto err_action;
+		}
+	}
 
- err_imgdownload:
+	/* Success */
+	rc = 0;
+
+ err_action:
+ err_set_cmdline:
+ err_set_name:
+ err_acquire:
 	free ( cmdline );
- err_cmdline:
+ err_parse_cmdline:
  err_parse_options:
 	return rc;
 }
 
-/**
- * The "imgfetch"/"module" command
- *
- * @v argc		Argument count
- * @v argv		Argument list
- * @ret rc		Return status code
- */
-static int imgfetch_exec ( int argc, char **argv ) {
+/** "imgfetch" command descriptor */
+static struct command_descriptor imgfetch_cmd =
+	COMMAND_DESC ( struct imgsingle_options, imgsingle_opts,
+		       1, MAX_ARGUMENTS,
+		       "[--name <name>] <uri> [<arguments>...]" );
 
-	return imgfetch_core_exec ( argc, argv, "fetch", NULL );
-}
+/** "imgfetch" family command descriptor */
+struct imgsingle_descriptor imgfetch_desc = {
+	.cmd = &imgfetch_cmd,
+	.acquire = imgdownload_string,
+};
 
 /**
- * The "kernel" command
+ * The "imgfetch" command
  *
  * @v argc		Argument count
  * @v argv		Argument list
  * @ret rc		Return status code
  */
-static int kernel_exec ( int argc, char **argv ) {
-
-	return imgfetch_core_exec ( argc, argv, "select", image_select );
+static int imgfetch_exec ( int argc, char **argv ) {
+	return imgsingle_exec ( argc, argv, &imgfetch_desc );
 }
 
+/** "imgselect" family command descriptor */
+struct imgsingle_descriptor imgselect_desc = {
+	.cmd = &imgsingle_cmd,
+	.acquire = imgacquire,
+	.action = image_select,
+	.verb = "select",
+};
+
 /**
- * The "chain" command
+ * The "imgselect" command
  *
  * @v argc		Argument count
  * @v argv		Argument list
  * @ret rc		Return status code
  */
-static int chain_exec ( int argc, char **argv) {
-
-	return imgfetch_core_exec ( argc, argv, "boot", image_exec );
+static int imgselect_exec ( int argc, char **argv ) {
+	return imgsingle_exec ( argc, argv, &imgselect_desc );
 }
 
-/** "imgselect" options */
-struct imgselect_options {};
-
-/** "imgselect" option list */
-static struct option_descriptor imgselect_opts[] = {};
-
-/** "imgselect" command descriptor */
-static struct command_descriptor imgselect_cmd =
-	COMMAND_DESC ( struct imgselect_options, imgselect_opts, 1, 1,
-		       "<image>" );
+/** "imgexec" command descriptor */
+static struct command_descriptor imgexec_cmd =
+	COMMAND_DESC ( struct imgsingle_options, imgsingle_opts,
+		       0, MAX_ARGUMENTS,
+		       "[--name <name>] [<uri|image> [<arguments>...]]" );
+
+/** "imgexec" family command descriptor */
+struct imgsingle_descriptor imgexec_desc = {
+	.cmd = &imgexec_cmd,
+	.acquire = imgacquire,
+	.action = image_exec,
+	.verb = "boot",
+};
 
 /**
- * The "imgselect" command
+ * The "imgexec" command
  *
  * @v argc		Argument count
  * @v argv		Argument list
  * @ret rc		Return status code
  */
-static int imgselect_exec ( int argc, char **argv ) {
-	struct imgselect_options opts;
-	struct image *image;
-	int rc;
-
-	/* Parse options */
-	if ( ( rc = parse_options ( argc, argv, &imgselect_cmd, &opts ) ) != 0 )
-		return rc;
-
-	/* Parse image name */
-	if ( ( rc = parse_image ( argv[optind], &image ) ) != 0 )
-		return rc;
-
-	/* Load image */
-	if ( ( rc = imgselect ( image ) ) != 0 ) {
-		printf ( "Could not select %s: %s\n",
-			 image->name, strerror ( rc ) );
-		return rc;
-	}
-
-	return 0;
+static int imgexec_exec ( int argc, char **argv) {
+	return imgsingle_exec ( argc, argv, &imgexec_desc );
 }
 
-/** "imgargs" options */
-struct imgargs_options {};
-
-/** "imgargs" option list */
-static struct option_descriptor imgargs_opts[] = {};
-
-/** "imgargs" command descriptor */
-static struct command_descriptor imgargs_cmd =
-	COMMAND_DESC ( struct imgargs_options, imgargs_opts, 1, MAX_ARGUMENTS,
-		       "<image> [<arguments>...]" );
+/** "imgargs" family command descriptor */
+struct imgsingle_descriptor imgargs_desc = {
+	.cmd = &imgsingle_cmd,
+	.acquire = imgacquire,
+	.preaction = image_clear_cmdline,
+};
 
 /**
  * The "imgargs" command body
@@ -201,105 +238,60 @@ static struct command_descriptor imgargs_cmd =
  * @ret rc		Return status code
  */
 static int imgargs_exec ( int argc, char **argv ) {
-	struct imgargs_options opts;
-	struct image *image;
-	char *cmdline = NULL;
-	int rc;
-
-	/* Parse options */
-	if ( ( rc = parse_options ( argc, argv, &imgargs_cmd, &opts ) ) != 0 )
-		goto err_parse_options;
-
-	/* Parse image name */
-	if ( ( rc = parse_image ( argv[optind], &image ) ) != 0 )
-		goto err_parse_image;
-
-	/* Parse command line */
-	if ( argv[ optind + 1 ] != NULL ) {
-		cmdline = concat_args ( &argv[ optind + 1 ] );
-		if ( ! cmdline ) {
-			rc = -ENOMEM;
-			goto err_cmdline;
-		}
-	}
-
-	/* Set command line */
-	if ( ( rc = image_set_cmdline ( image, cmdline ) ) != 0 )
-		goto err_set_cmdline;
-
-	/* Free command line */
-	free ( cmdline );
-
-	return 0;
-
- err_set_cmdline:
-	free ( cmdline );
- err_cmdline:
- err_parse_image:
- err_parse_options:
-	return rc;
+	return imgsingle_exec ( argc, argv, &imgargs_desc );
 }
 
-/** "imgexec" options */
-struct imgexec_options {};
+/** "img{multi}" options */
+struct imgmulti_options {};
 
-/** "imgexec" option list */
-static struct option_descriptor imgexec_opts[] = {};
+/** "img{multi}" option list */
+static struct option_descriptor imgmulti_opts[] = {};
 
-/** "imgexec" command descriptor */
-static struct command_descriptor imgexec_cmd =
-	COMMAND_DESC ( struct imgexec_options, imgexec_opts, 0, 1,
-		       "[<image>]" );
+/** "img{multi}" command descriptor */
+static struct command_descriptor imgmulti_cmd =
+	COMMAND_DESC ( struct imgmulti_options, imgmulti_opts, 0, MAX_ARGUMENTS,
+		       "[<image>...]" );
 
 /**
- * The "imgexec" command
+ * The "img{multi}" family of commands
  *
  * @v argc		Argument count
  * @v argv		Argument list
+ * @v payload		Function to execute on each image
  * @ret rc		Return status code
  */
-static int imgexec_exec ( int argc, char **argv ) {
-	struct imgexec_options opts;
+static int imgmulti_exec ( int argc, char **argv,
+			   void ( * payload ) ( struct image *image ) ) {
+	struct imgmulti_options opts;
 	struct image *image;
+	struct image *tmp;
+	int i;
 	int rc;
 
 	/* Parse options */
-	if ( ( rc = parse_options ( argc, argv, &imgexec_cmd, &opts ) ) != 0 )
+	if ( ( rc = parse_options ( argc, argv, &imgmulti_cmd, &opts ) ) != 0 )
 		return rc;
 
-	/* Parse image name */
-	if ( optind < argc ) {
-		if ( ( rc = parse_image ( argv[optind], &image ) ) != 0 )
-			return rc;
-	} else {
-		image = imgautoselect();
-		if ( ! image ) {
-			rc = -ENOTTY;
-			printf ( "No image selected: %s\n", strerror ( rc ) );
-			return rc;
-		}
+	/* If no images are explicitly specified, process all images */
+	if ( optind == argc ) {
+		for_each_image_safe ( image, tmp )
+			payload ( image );
+		return 0;
 	}
 
-	/* Execute image */
-	if ( ( rc = imgexec ( image ) ) != 0 ) {
-		printf ( "Could not execute %s: %s\n",
-			 image->name, strerror ( rc ) );
-		return rc;
+	/* Otherwise, process specified images */
+	for ( i = optind ; i < argc ; i++ ) {
+		image = find_image ( argv[i] );
+		if ( ! image ) {
+			printf ( "\"%s\": no such image\n", argv[i] );
+			return -ENOENT;
+		}
+		payload ( image );
 	}
 
 	return 0;
 }
 
-/** "imgstat" options */
-struct imgstat_options {};
-
-/** "imgstat" option list */
-static struct option_descriptor imgstat_opts[] = {};
-
-/** "imgstat" command descriptor */
-static struct command_descriptor imgstat_cmd =
-	COMMAND_DESC ( struct imgstat_options, imgstat_opts, 0, 0, "" );
-
 /**
  * The "imgstat" command
  *
@@ -308,33 +300,9 @@ static struct command_descriptor imgstat_cmd =
  * @ret rc		Return status code
  */
 static int imgstat_exec ( int argc, char **argv ) {
-	struct imgstat_options opts;
-	struct image *image;
-	int rc;
-
-	/* Parse options */
-	if ( ( rc = parse_options ( argc, argv, &imgstat_cmd, &opts ) ) != 0 )
-		return rc;
-
-	/* Show status of all images */
-	for_each_image ( image ) {
-		imgstat ( image );
-	}
-
-	return 0;
+	return imgmulti_exec ( argc, argv, imgstat );
 }
 
-/** "imgfree" options */
-struct imgfree_options {};
-
-/** "imgfree" option list */
-static struct option_descriptor imgfree_opts[] = {};
-
-/** "imgfree" command descriptor */
-static struct command_descriptor imgfree_cmd =
-	COMMAND_DESC ( struct imgfree_options, imgfree_opts, 0, 1,
-		       "[<image>]" );
-
 /**
  * The "imgfree" command
  *
@@ -343,28 +311,7 @@ static struct command_descriptor imgfree_cmd =
  * @ret rc		Return status code
  */
 static int imgfree_exec ( int argc, char **argv ) {
-	struct imgfree_options opts;
-	struct image *image;
-	struct image *tmp;
-	int rc;
-
-	/* Parse options */
-	if ( ( rc = parse_options ( argc, argv, &imgfree_cmd, &opts ) ) != 0 )
-		return rc;
-
-	if ( optind < argc ) {
-		/* Free specified image */
-		if ( ( rc = parse_image ( argv[optind], &image ) ) != 0 )
-			return rc;
-		imgfree ( image );
-	} else {
-		/* Free all images */
-		list_for_each_entry_safe ( image, tmp, &images, list ) {
-			imgfree ( image );
-		}
-	}
-
-	return 0;
+	return imgmulti_exec ( argc, argv, unregister_image );
 }
 
 /** Image management commands */
@@ -383,19 +330,19 @@ struct command image_commands[] __command = {
 	},
 	{
 		.name = "kernel",
-		.exec = kernel_exec,
+		.exec = imgselect_exec, /* synonym for "imgselect" */
 	},
 	{
 		.name = "chain",
-		.exec = chain_exec,
+		.exec = imgexec_exec, /* synonym for "imgexec" */
 	},
 	{
 		.name = "imgselect",
 		.exec = imgselect_exec,
 	},
 	{
-		.name = "imgload", /* synonym for "imgselect" */
-		.exec = imgselect_exec,
+		.name = "imgload",
+		.exec = imgselect_exec, /* synonym for "imgselect" */
 	},
 	{
 		.name = "imgargs",

src/include/ipxe/image.h

@@ -29,7 +29,7 @@ struct image {
 	/** URI of image */
 	struct uri *uri;
 	/** Name */
-	char name[16];
+	char *name;
 	/** Flags */
 	unsigned int flags;
 
@@ -122,6 +122,10 @@ extern struct image *current_image;
 #define for_each_image( image ) \
 	list_for_each_entry ( (image), &images, list )
 
+/** Iterate over all registered images, safe against deletion */
+#define for_each_image_safe( image, tmp ) \
+	list_for_each_entry_safe ( (image), (tmp), &images, list )
+
 /**
  * Test for existence of images
  *
@@ -140,8 +144,8 @@ static inline struct image * first_image ( void ) {
 	return list_first_entry ( &images, struct image, list );
 }
 
-extern struct image * alloc_image ( void );
-extern void image_set_uri ( struct image *image, struct uri *uri );
+extern struct image * alloc_image ( struct uri *uri );
+extern int image_set_name ( struct image *image, const char *name );
 extern int image_set_cmdline ( struct image *image, const char *cmdline );
 extern int register_image ( struct image *image );
 extern void unregister_image ( struct image *image );
@@ -174,15 +178,12 @@ static inline void image_put ( struct image *image ) {
 }
 
 /**
- * Set image name
+ * Clear image command line
  *
  * @v image		Image
- * @v name		New image name
- * @ret rc		Return status code
  */
-static inline int image_set_name ( struct image *image, const char *name ) {
-	strncpy ( image->name, name, ( sizeof ( image->name ) - 1 ) );
-	return 0;
+static inline void image_clear_cmdline ( struct image *image ) {
+	image_set_cmdline ( image, NULL );
 }
 
 /**

src/include/ipxe/parseopt.h

@@ -13,7 +13,6 @@ FILE_LICENCE ( GPL2_OR_LATER );
 #include <stddef.h>
 
 struct net_device;
-struct image;
 
 /** A command-line option descriptor */
 struct option_descriptor {
@@ -117,7 +116,6 @@ struct command_descriptor {
 extern int parse_string ( const char *text, const char **value );
 extern int parse_integer ( const char *text, unsigned int *value );
 extern int parse_netdev ( const char *text, struct net_device **netdev );
-extern int parse_image ( const char *text, struct image **image );
 extern int parse_flag ( const char *text __unused, int *flag );
 extern void print_usage ( struct command_descriptor *cmd, char **argv );
 extern int reparse_options ( int argc, char **argv,

src/include/usr/imgmgmt.h

@@ -11,41 +11,9 @@ FILE_LICENCE ( GPL2_OR_LATER );
 
 #include <ipxe/image.h>
 
-extern int imgdownload ( struct uri *uri, const char *name, const char *cmdline,
-			 int ( * action ) ( struct image *image ) );
-extern int imgdownload_string ( const char *uri_string, const char *name,
-				const char *cmdline,
-				int ( * action ) ( struct image *image ) );
+extern int imgdownload ( struct uri *uri, struct image **image );
+extern int imgdownload_string ( const char *uri_string, struct image **image );
+extern int imgacquire ( const char *name, struct image **image );
 extern void imgstat ( struct image *image );
-extern void imgfree ( struct image *image );
-
-/**
- * Select an image for execution
- *
- * @v image		Image
- * @ret rc		Return status code
- */
-static inline int imgselect ( struct image *image ) {
-	return image_select ( image );
-}
-
-/**
- * Find the previously-selected image
- *
- * @ret image		Image, or NULL
- */
-static inline struct image * imgautoselect ( void ) {
-	return image_find_selected();
-}
-
-/**
- * Execute an image
- *
- * @v image		Image
- * @ret rc		Return status code
- */
-static inline int imgexec ( struct image *image ) {
-	return image_exec ( image );
-}
 
 #endif /* _USR_IMGMGMT_H */

src/tests/test.c

@@ -149,12 +149,8 @@ static struct image_type test_image_type = {
 	.exec = test_image_exec,
 };
 
-static void test_image_free ( struct refcnt *refcnt __unused ) {
-	/* Do nothing */
-}
-
 static struct image test_image = {
-	.refcnt = REF_INIT ( test_image_free ),
+	.refcnt = REF_INIT ( ref_no_free ),
 	.name = "<TESTS>",
 	.type = &test_image_type,
 };

src/usr/autoboot.c

@@ -131,6 +131,7 @@ struct setting skip_san_boot_setting __setting ( SETTING_SANBOOT_EXTRA ) = {
  */
 int uriboot ( struct uri *filename, struct uri *root_path, int drive,
 	      unsigned int flags ) {
+	struct image *image;
 	int rc;
 
 	/* Hook SAN device, if applicable */
@@ -157,9 +158,10 @@ int uriboot ( struct uri *filename, struct uri *root_path, int drive,
 
 	/* Attempt filename boot if applicable */
 	if ( filename ) {
-		if ( ( rc = imgdownload ( filename, NULL, NULL,
-					  image_exec ) ) != 0 ) {
-			printf ( "\nCould not chain image: %s\n",
+		if ( ( rc = imgdownload ( filename, &image ) ) != 0 )
+			goto err_download;
+		if ( ( rc = image_exec ( image ) ) != 0 ) {
+			printf ( "Could not boot image: %s\n",
 				 strerror ( rc ) );
 			/* Fall through to (possibly) attempt a SAN boot
 			 * as a fallback.  If no SAN boot is attempted,
@@ -190,6 +192,7 @@ int uriboot ( struct uri *filename, struct uri *root_path, int drive,
 		}
 	}
 
+ err_download:
  err_san_describe:
 	/* Unhook SAN device, if applicable */
 	if ( ( drive >= 0 ) && ! ( flags & URIBOOT_NO_SAN_UNHOOK ) ) {

src/usr/imgmgmt.c

@@ -36,39 +36,25 @@ FILE_LICENCE ( GPL2_OR_LATER );
  */
 
 /**
- * Download an image
+ * Download a new image
  *
  * @v uri		URI
- * @v name		Image name, or NULL to use default
- * @v cmdline		Command line, or NULL for no command line
- * @v action		Action to take upon a successful download, or NULL
+ * @v image		Image to fill in
  * @ret rc		Return status code
  */
-int imgdownload ( struct uri *uri, const char *name, const char *cmdline,
-		  int ( * action ) ( struct image *image ) ) {
-	struct image *image;
+int imgdownload ( struct uri *uri, struct image **image ) {
 	size_t len = ( unparse_uri ( NULL, 0, uri, URI_ALL ) + 1 );
 	char uri_string_redacted[len];
 	const char *password;
 	int rc;
 
 	/* Allocate image */
-	image = alloc_image();
-	if ( ! image ) {
+	*image = alloc_image ( uri );
+	if ( ! *image ) {
 		rc = -ENOMEM;
 		goto err_alloc_image;
 	}
 
-	/* Set image name */
-	if ( name )
-		image_set_name ( image, name );
-
-	/* Set image URI */
-	image_set_uri ( image, uri );
-
-	/* Set image command line */
-	image_set_cmdline ( image, cmdline );
-
 	/* Redact password portion of URI, if necessary */
 	password = uri->password;
 	if ( password )
@@ -78,8 +64,9 @@ int imgdownload ( struct uri *uri, const char *name, const char *cmdline,
 	uri->password = password;
 
 	/* Create downloader */
-	if ( ( rc = create_downloader ( &monojob, image, LOCATION_URI,
+	if ( ( rc = create_downloader ( &monojob, *image, LOCATION_URI,
 					uri ) ) != 0 ) {
+		printf ( "Could not start download: %s\n", strerror ( rc ) );
 		goto err_create_downloader;
 	}
 
@@ -88,49 +75,64 @@ int imgdownload ( struct uri *uri, const char *name, const char *cmdline,
 		goto err_monojob_wait;
 
 	/* Register image */
-	if ( ( rc = register_image ( image ) ) != 0 )
+	if ( ( rc = register_image ( *image ) ) != 0 ) {
+		printf ( "Could not register image: %s\n", strerror ( rc ) );
 		goto err_register_image;
+	}
 
 	/* Drop local reference to image.  Image is guaranteed to
 	 * remain in scope since it is registered.
 	 */
-	image_put ( image );
+	image_put ( *image );
 
-	/* Carry out specified post-download action, if applicable */
-	return ( action ? action ( image ) : 0 );
+	return 0;
 
  err_register_image:
  err_monojob_wait:
  err_create_downloader:
-	image_put ( image );
+	image_put ( *image );
  err_alloc_image:
 	return rc;
 }
 
 /**
- * Download an image
+ * Download a new image
  *
- * @v uri_string	URI as a string (e.g. "http://www.nowhere.com/vmlinuz")
- * @v name		Image name, or NULL to use default
- * @v cmdline		Command line, or NULL for no command line
- * @v action		Action to take upon a successful download
+ * @v uri_string	URI string
+ * @v image		Image to fill in
  * @ret rc		Return status code
  */
-int imgdownload_string ( const char *uri_string, const char *name,
-			 const char *cmdline,
-			 int ( * action ) ( struct image *image ) ) {
+int imgdownload_string ( const char *uri_string, struct image **image ) {
 	struct uri *uri;
 	int rc;
 
 	if ( ! ( uri = parse_uri ( uri_string ) ) )
 		return -ENOMEM;
 
-	rc = imgdownload ( uri, name, cmdline, action );
+	rc = imgdownload ( uri, image );
 
 	uri_put ( uri );
 	return rc;
 }
 
+/**
+ * Acquire an image
+ *
+ * @v name_uri		Name or URI string
+ * @v image		Image to fill in
+ * @ret rc		Return status code
+ */
+int imgacquire ( const char *name_uri, struct image **image ) {
+
+	/* If we already have an image with the specified name, use it */
+	*image = find_image ( name_uri );
+	if ( *image )
+		return 0;
+
+	/* Otherwise, download a new image */
+	return imgdownload_string ( name_uri, image );
+}
+
 /**
  * Display status of an image
  *
@@ -148,12 +150,3 @@ void imgstat ( struct image *image ) {
 		printf ( " \"%s\"", image->cmdline );
 	printf ( "\n" );
 }
-
-/**
- * Free an image
- *
- * @v image		Executable/loadable image
- */
-void imgfree ( struct image *image ) {
-	unregister_image ( image );
-}