[image] Simplify use of imgdownload()

Allow imgdownload() to be called without first having to allocate (and
so keep track of) an image.

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

src/arch/i386/image/com32.c

@@ -132,10 +132,9 @@ static int com32_exec_loop ( struct image *image ) {
 		break;
 
 	case COMBOOT_EXIT_RUN_KERNEL:
-		DBGC ( image, "COM32 %p: exited to run kernel %p\n",
-		       image, comboot_replacement_image );
-		image->replacement = comboot_replacement_image;
-		comboot_replacement_image = NULL;
+		assert ( image->replacement );
+		DBGC ( image, "COM32 %p: exited to run kernel %s\n",
+		       image, image->replacement->name );
 		break;
 
 	case COMBOOT_EXIT_COMMAND:

src/arch/i386/image/comboot.c

@@ -188,10 +188,9 @@ static int comboot_exec_loop ( struct image *image ) {
 		break;
 
 	case COMBOOT_EXIT_RUN_KERNEL:
-		DBGC ( image, "COMBOOT %p: exited to run kernel %p\n",
-		       image, comboot_replacement_image );
-		image->replacement = comboot_replacement_image;
-		comboot_replacement_image = NULL;
+		assert ( image->replacement );
+		DBGC ( image, "COMBOOT %p: exited to run kernel %s\n",
+		       image, image->replacement->name );
 		break;
 
 	case COMBOOT_EXIT_COMMAND:

src/arch/i386/include/comboot.h

@@ -161,9 +161,6 @@ extern int comboot_resolv ( const char *name, struct in_addr *address );
 /* setjmp/longjmp context buffer used to return after loading an image */
 extern rmjmp_buf comboot_return;
 
-/* Replacement image when exiting with COMBOOT_EXIT_RUN_KERNEL */
-extern struct image *comboot_replacement_image;
-
 extern void *com32_external_esp;
 
 #define COMBOOT_EXIT 1

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

@@ -81,9 +81,6 @@ extern void int22_wrapper ( void );
 /* setjmp/longjmp context buffer used to return after loading an image */
 rmjmp_buf comboot_return;
 
-/* Replacement image when exiting with COMBOOT_EXIT_RUN_KERNEL */
-struct image *comboot_replacement_image;
-
 /* Mode flags set by INT 22h AX=0017h */
 static uint16_t comboot_graphics_mode = 0;
 
@@ -169,8 +166,6 @@ void comboot_force_text_mode ( void ) {
  * Fetch kernel and optional initrd
  */
 static int comboot_fetch_kernel ( char *kernel_file, char *cmdline ) {
-	struct image *kernel = NULL;
-	struct image *initrd = NULL;
 	char *initrd_file;
 	int rc;
 
@@ -188,18 +183,12 @@ static int comboot_fetch_kernel ( char *kernel_file, char *cmdline ) {
 
 		DBG ( "COMBOOT: fetching initrd '%s'\n", initrd_file );
 
-		/* Allocate and fetch initrd */
-		initrd = alloc_image();
-		if ( ! initrd ) {
-			DBG ( "COMBOOT: could not allocate initrd\n" );
-			rc = -ENOMEM;
-			goto out;
-		}
-		if ( ( rc = imgfetch ( initrd, initrd_file,
-				       register_image ) ) != 0 ) {
+		/* Fetch initrd */
+		if ( ( rc = imgdownload_string ( initrd_file, NULL, NULL,
+						 register_and_put_image ))!=0){
 			DBG ( "COMBOOT: could not fetch initrd: %s\n",
 			      strerror ( rc ) );
-			goto out;
+			return rc;
 		}
 
 		/* Restore space after initrd name, if applicable */
@@ -210,36 +199,14 @@ static int comboot_fetch_kernel ( char *kernel_file, char *cmdline ) {
 	DBG ( "COMBOOT: fetching kernel '%s'\n", kernel_file );
 
 	/* Allocate and fetch kernel */
-	kernel = alloc_image();
-	if ( ! kernel ) {
-		DBG ( "COMBOOT: could not allocate kernel\n" );
-		rc = -ENOMEM;
-		goto out;
-	}
-	if ( ( rc = imgfetch ( kernel, kernel_file,
-			       register_and_select_image ) ) != 0 ) {
+	if ( ( rc = imgdownload_string ( kernel_file, NULL, cmdline,
+					 register_and_replace_image ) ) != 0 ) {
 		DBG ( "COMBOOT: could not fetch kernel: %s\n",
 		      strerror ( rc ) );
-		goto out;
-	}
-	if ( ( rc = image_set_cmdline ( kernel, cmdline ) ) != 0 ) {
-		DBG ( "COMBOOT: could not set kernel command line: %s\n",
-		      strerror ( rc ) );
-		goto out;
+		return rc;
 	}
 
-	/* Store kernel as replacement image */
-	assert ( comboot_replacement_image == NULL );
-	comboot_replacement_image = image_get ( kernel );
-
- out:
-	/* Drop image references unconditionally; either we want to
-	 * discard them, or they have been registered and we should
-	 * drop out local reference.
-	 */
-	image_put ( kernel );
-	image_put ( initrd );
-	return rc;
+	return 0;
 }
 
 

src/core/image.c

@@ -130,6 +130,7 @@ int register_image ( struct image *image ) {
 
 	/* Add to image list */
 	image_get ( image );
+	image->flags |= IMAGE_REGISTERED;
 	list_add_tail ( &image->list, &images );
 	DBGC ( image, "IMAGE %s at [%lx,%lx) registered\n",
 	       image->name, user_to_phys ( image->data, 0 ),
@@ -144,8 +145,10 @@ int register_image ( struct image *image ) {
  * @v image		Executable image
  */
 void unregister_image ( struct image *image ) {
+
 	DBGC ( image, "IMAGE %s unregistered\n", image->name );
 	list_del ( &image->list );
+	image->flags &= ~IMAGE_REGISTERED;
 	image_put ( image );
 }
 
@@ -201,6 +204,10 @@ int image_probe ( struct image *image ) {
  *
  * @v image		Executable image
  * @ret rc		Return status code
+ *
+ * The image must already be registered.  Note that executing an image
+ * may cause it to unregister itself.  The caller must therefore
+ * assume that the image pointer becomes invalid.
  */
 int image_exec ( struct image *image ) {
 	struct image *saved_current_image;
@@ -208,6 +215,9 @@ int image_exec ( struct image *image ) {
 	struct uri *old_cwuri;
 	int rc;
 
+	/* Sanity check */
+	assert ( image->flags & IMAGE_REGISTERED );
+
 	/* Check that this image can be executed */
 	if ( ( rc = image_probe ( image ) ) != 0 )
 		return rc;
@@ -233,9 +243,13 @@ int image_exec ( struct image *image ) {
 	}
 
 	/* Pick up replacement image before we drop the original
-	 * image's temporary reference.
+	 * image's temporary reference.  The replacement image must
+	 * already be registered, so we don't need to hold a temporary
+	 * reference (which would complicate the tail-recursion).
 	 */
 	replacement = image->replacement;
+	if ( replacement )
+		assert ( replacement->flags & IMAGE_REGISTERED );
 
 	/* Drop temporary reference to the original image */
 	image_put ( image );
@@ -258,6 +272,41 @@ int image_exec ( struct image *image ) {
 	return rc;
 }
 
+/**
+ * Set replacement image
+ *
+ * @v replacement	Replacement image
+ * @ret rc		Return status code
+ *
+ * The replacement image must already be registered, and must remain
+ * registered until the currently-executing image returns.
+ */
+int image_replace ( struct image *replacement ) {
+	struct image *image = current_image;
+	int rc;
+
+	/* Sanity check */
+	assert ( replacement->flags & IMAGE_REGISTERED );
+
+	/* Fail unless there is a currently-executing image */
+	if ( ! image ) {
+		rc = -ENOTTY;
+		DBGC ( replacement, "IMAGE %s cannot replace non-existent "
+		       "image: %s\n", replacement->name, strerror ( rc ) );
+		return rc;
+	}
+
+	/* Clear any existing replacement */
+	image_put ( image->replacement );
+
+	/* Set replacement */
+	image->replacement = image_get ( replacement );
+	DBGC ( image, "IMAGE %s will replace self with IMAGE %s\n",
+	       image->name, replacement->name );
+
+	return 0;
+}
+
 /**
  * Select image for execution
  *

src/hci/commands/image_cmd.c

@@ -35,30 +35,6 @@ FILE_LICENCE ( GPL2_OR_LATER );
  *
  */
 
-/**
- * Fill in image command line
- *
- * @v image		Image
- * @v args		Argument list
- * @ret rc		Return status code
- */
-static int imgfill_cmdline ( struct image *image, char **args ) {
-	char *cmdline = NULL;
-	int rc;
-
-	/* Construct command line (if arguments are present) */
-	if ( *args ) {
-		cmdline = concat_args ( args );
-		if ( ! cmdline )
-			return -ENOMEM;
-	}
-
-	/* Apply command line */
-	rc = image_set_cmdline ( image, cmdline );
-	free ( cmdline );
-	return rc;
-}
-
 /** "imgfetch" options */
 struct imgfetch_options {
 	/** Image name */
@@ -82,51 +58,54 @@ static struct command_descriptor imgfetch_cmd =
  * @v argc		Argument count
  * @v argv		Argument list
  * @v cmd		Command descriptor
+ * @v action_name	Action name (for error messages)
  * @v action		Action to take upon a successful download
  * @ret rc		Return status code
  */
 static int imgfetch_core_exec ( int argc, char **argv,
-				struct command_descriptor *cmd,
+				const char *action_name,
 				int ( * action ) ( struct image *image ) ) {
 	struct imgfetch_options opts;
-	struct image *image;
 	char *uri_string;
+	char *cmdline = NULL;
 	int rc;
 
 	/* Parse options */
-	if ( ( rc = parse_options ( argc, argv, cmd, &opts ) ) != 0 )
-		return rc;
+	if ( ( rc = parse_options ( argc, argv, &imgfetch_cmd, &opts ) ) != 0 )
+		goto err_parse_options;
 
 	/* Parse URI string */
 	uri_string = argv[optind];
 	if ( ! opts.name )
 		opts.name = basename ( uri_string );
 
-	/* Allocate image */
-	image = alloc_image();
-	if ( ! image ) {
-		printf ( "%s\n", strerror ( -ENOMEM ) );
-		return -ENOMEM;
+	/* Parse command line */
+	if ( argv[ optind + 1 ] != NULL ) {
+		cmdline = concat_args ( &argv[ optind + 1 ] );
+		if ( ! cmdline ) {
+			rc = -ENOMEM;
+			goto err_cmdline;
+		}
 	}
 
-	/* Fill in image name */
-	if ( ( rc = image_set_name ( image, opts.name ) ) != 0 )
-		return rc;
-
-	/* Fill in command line */
-	if ( ( rc = imgfill_cmdline ( image, &argv[ optind + 1 ] ) ) != 0 )
-		return rc;
-
 	/* Fetch the image */
-	if ( ( rc = imgfetch ( image, uri_string, action ) ) != 0 ) {
-		printf ( "Could not fetch %s: %s\n",
-			 uri_string, strerror ( rc ) );
-		image_put ( image );
-		return rc;
+	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;
 	}
 
-	image_put ( image );
+	/* Free command line */
+	free ( cmdline );
+
 	return 0;
+
+ err_imgdownload:
+	free ( cmdline );
+ err_cmdline:
+ err_parse_options:
+	return rc;
 }
 
 /**
@@ -138,8 +117,8 @@ static int imgfetch_core_exec ( int argc, char **argv,
  */
 static int imgfetch_exec ( int argc, char **argv ) {
 
-	return imgfetch_core_exec ( argc, argv, &imgfetch_cmd,
-				    register_image );
+	return imgfetch_core_exec ( argc, argv, "fetch",
+				    register_and_put_image );
 }
 
 /**
@@ -151,7 +130,7 @@ static int imgfetch_exec ( int argc, char **argv ) {
  */
 static int kernel_exec ( int argc, char **argv ) {
 
-	return imgfetch_core_exec ( argc, argv, &imgfetch_cmd,
+	return imgfetch_core_exec ( argc, argv, "select",
 				    register_and_select_image );
 }
 
@@ -164,7 +143,7 @@ static int kernel_exec ( int argc, char **argv ) {
  */
 static int chain_exec ( int argc, char **argv) {
 
-	return imgfetch_core_exec ( argc, argv, &imgfetch_cmd,
+	return imgfetch_core_exec ( argc, argv, "boot",
 				    register_and_boot_image );
 }
 
@@ -230,21 +209,41 @@ static struct command_descriptor imgargs_cmd =
 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 )
-		return rc;
+		goto err_parse_options;
 
 	/* Parse image name */
 	if ( ( rc = parse_image ( argv[optind], &image ) ) != 0 )
-		return rc;
+		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;
+		}
+	}
 
-	/* Fill in command line */
-	if ( ( rc = imgfill_cmdline ( image, &argv[ optind + 1 ] ) ) != 0 )
-		return rc;
+	/* 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;
 }
 
 /** "imgexec" options */

src/include/ipxe/image.h

@@ -58,8 +58,11 @@ struct image {
 	struct image *replacement;
 };
 
+/** Image is registered */
+#define IMAGE_REGISTERED 0x00001
+
 /** Image is selected for execution */
-#define IMAGE_SELECTED 0x0001
+#define IMAGE_SELECTED 0x0002
 
 /** An executable image type */
 struct image_type {
@@ -142,6 +145,7 @@ extern void unregister_image ( struct image *image );
 struct image * find_image ( const char *name );
 extern int image_probe ( struct image *image );
 extern int image_exec ( struct image *image );
+extern int image_replace ( struct image *replacement );
 extern int image_select ( struct image *image );
 extern struct image * image_find_selected ( void );
 

src/include/usr/imgmgmt.h

@@ -11,12 +11,16 @@ FILE_LICENCE ( GPL2_OR_LATER );
 
 #include <ipxe/image.h>
 
+extern int register_and_put_image ( struct image *image );
+extern int register_and_probe_image ( struct image *image );
 extern int register_and_select_image ( struct image *image );
 extern int register_and_boot_image ( struct image *image );
-extern int imgdownload ( struct image *image, struct uri *uri,
+extern int register_and_replace_image ( struct image *image );
+extern int imgdownload ( struct uri *uri, const char *name, const char *cmdline,
 			 int ( * action ) ( struct image *image ) );
-extern int imgfetch ( struct image *image, const char *uri_string,
-		      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 void imgstat ( struct image *image );
 extern void imgfree ( struct image *image );
 

src/usr/autoboot.c

@@ -122,18 +122,9 @@ struct setting skip_san_boot_setting __setting = {
  * @ret rc		Return status code
  */
 int uriboot ( struct uri *filename, struct uri *root_path ) {
-	struct image *image;
 	int drive;
 	int rc;
 
-	/* Allocate image */
-	image = alloc_image();
-	if ( ! image ) {
-		printf ( "Could not allocate image\n" );
-		rc = -ENOMEM;
-		goto err_alloc_image;
-	}
-
 	/* Treat empty URIs as absent */
 	if ( filename && ( ! uri_has_path ( filename ) ) )
 		filename = NULL;
@@ -183,7 +174,7 @@ int uriboot ( struct uri *filename, struct uri *root_path ) {
 
 	/* Attempt filename boot if applicable */
 	if ( filename ) {
-		if ( ( rc = imgdownload ( image, filename,
+		if ( ( rc = imgdownload ( filename, NULL, NULL,
 					  register_and_boot_image ) ) != 0 ) {
 			printf ( "\nCould not chain image: %s\n",
 				 strerror ( rc ) );
@@ -229,8 +220,6 @@ int uriboot ( struct uri *filename, struct uri *root_path ) {
 	}
  err_san_hook:
  err_no_boot:
-	image_put ( image );
- err_alloc_image:
 	return rc;
 }
 

src/usr/imgmgmt.c

@@ -36,20 +36,55 @@ FILE_LICENCE ( GPL2_OR_LATER );
  */
 
 /**
- * Register and select an image
+ * Register an image and leave it registered
  *
  * @v image		Executable image
  * @ret rc		Return status code
+ *
+ * This function assumes an ownership of the passed image.
  */
-int register_and_select_image ( struct image *image ) {
+int register_and_put_image ( struct image *image ) {
+	int rc;
+
+	rc = register_image ( image );
+	image_put ( image );
+	return rc;
+}
+
+/**
+ * Register and probe an image
+ *
+ * @v image		Executable image
+ * @ret rc		Return status code
+ *
+ * This function assumes an ownership of the passed image.
+ */
+int register_and_probe_image ( struct image *image ) {
 	int rc;
 
-	if ( ( rc = register_image ( image ) ) != 0 )
+	if ( ( rc = register_and_put_image ( image ) ) != 0 )
 		return rc;
 
 	if ( ( rc = image_probe ( image ) ) != 0 )
 		return rc;
 
+	return 0;
+}
+
+/**
+ * Register and select an image
+ *
+ * @v image		Executable image
+ * @ret rc		Return status code
+ *
+ * This function assumes an ownership of the passed image.
+ */
+int register_and_select_image ( struct image *image ) {
+	int rc;
+
+	if ( ( rc = register_and_probe_image ( image ) ) != 0 )
+		return rc;
+
 	if ( ( rc = image_select ( image ) ) != 0 )
 		return rc;
 
@@ -61,6 +96,8 @@ int register_and_select_image ( struct image *image ) {
  *
  * @v image		Image
  * @ret rc		Return status code
+ *
+ * This function assumes an ownership of the passed image.
  */
 int register_and_boot_image ( struct image *image ) {
 	int rc;
@@ -75,23 +112,56 @@ int register_and_boot_image ( struct image *image ) {
 }
 
 /**
- * Download an image
+ * Register and replace image
  *
  * @v image		Image
+ * @ret rc		Return status code
+ *
+ * This function assumes an ownership of the passed image.
+ */
+int register_and_replace_image ( struct image *image ) {
+	int rc;
+
+	if ( ( rc = register_and_probe_image ( image ) ) != 0 )
+		return rc;
+
+	if ( ( rc = image_replace ( image ) ) != 0 )
+		return rc;
+
+	return 0;
+}
+
+/**
+ * Download an 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
  * @ret rc		Return status code
  */
-int imgdownload ( struct image *image, struct uri *uri,
+int imgdownload ( struct uri *uri, const char *name, const char *cmdline,
 		  int ( * action ) ( struct image *image ) ) {
+	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 )
+		return -ENOMEM;
+
+	/* Set image 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 )
@@ -102,14 +172,20 @@ int imgdownload ( struct image *image, struct uri *uri,
 
 	/* Create downloader */
 	if ( ( rc = create_downloader ( &monojob, image, LOCATION_URI,
-					uri ) ) != 0 )
+					uri ) ) != 0 ) {
+		image_put ( image );
 		return rc;
+	}
 
 	/* Wait for download to complete */
-	if ( ( rc = monojob_wait ( uri_string_redacted ) ) != 0 )
+	if ( ( rc = monojob_wait ( uri_string_redacted ) ) != 0 ) {
+		image_put ( image );
 		return rc;
+	}
 
-	/* Act upon downloaded image */
+	/* Act upon downloaded image.  This action assumes our
+	 * ownership of the image.
+	 */
 	if ( ( rc = action ( image ) ) != 0 )
 		return rc;
 
@@ -117,22 +193,24 @@ int imgdownload ( struct image *image, struct uri *uri,
 }
 
 /**
- * Fetch an image
+ * Download an image
  *
- * @v image		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
  * @ret rc		Return status code
  */
-int imgfetch ( struct image *image, const char *uri_string,
-	       int ( * action ) ( struct image *image ) ) {
+int imgdownload_string ( const char *uri_string, const char *name,
+			 const char *cmdline,
+			 int ( * action ) ( struct image *image ) ) {
 	struct uri *uri;
 	int rc;
 
 	if ( ! ( uri = parse_uri ( uri_string ) ) )
 		return -ENOMEM;
 
-	rc = imgdownload ( image, uri, action );
+	rc = imgdownload ( uri, name, cmdline, action );
 
 	uri_put ( uri );
 	return rc;