From 9fa4ac2e9a781861e36e618eb1d461d8dc53a27c Mon Sep 17 00:00:00 2001 From: Michael Brown Date: Wed, 9 Mar 2011 16:55:51 +0000 Subject: [PATCH] [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 --- src/arch/i386/image/com32.c | 7 +- src/arch/i386/image/comboot.c | 7 +- src/arch/i386/include/comboot.h | 3 - .../i386/interface/syslinux/comboot_call.c | 49 ++------ src/core/image.c | 51 +++++++- src/hci/commands/image_cmd.c | 111 +++++++++--------- src/include/ipxe/image.h | 6 +- src/include/usr/imgmgmt.h | 10 +- src/usr/autoboot.c | 13 +- src/usr/imgmgmt.c | 106 ++++++++++++++--- 10 files changed, 224 insertions(+), 139 deletions(-) diff --git a/src/arch/i386/image/com32.c b/src/arch/i386/image/com32.c index 39ff4e66..d6e48ebe 100644 --- a/src/arch/i386/image/com32.c +++ b/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: diff --git a/src/arch/i386/image/comboot.c b/src/arch/i386/image/comboot.c index 26bb1139..0b924cce 100644 --- a/src/arch/i386/image/comboot.c +++ b/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: diff --git a/src/arch/i386/include/comboot.h b/src/arch/i386/include/comboot.h index 39d1d2f1..b3434139 100644 --- a/src/arch/i386/include/comboot.h +++ b/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 diff --git a/src/arch/i386/interface/syslinux/comboot_call.c b/src/arch/i386/interface/syslinux/comboot_call.c index 1dbc830f..221b597d 100644 --- a/src/arch/i386/interface/syslinux/comboot_call.c +++ b/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; } diff --git a/src/core/image.c b/src/core/image.c index d205768a..01091f1b 100644 --- a/src/core/image.c +++ b/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 * diff --git a/src/hci/commands/image_cmd.c b/src/hci/commands/image_cmd.c index 2a90b7cb..c9a188a2 100644 --- a/src/hci/commands/image_cmd.c +++ b/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; - /* Fill in command line */ - if ( ( rc = imgfill_cmdline ( image, &argv[ optind + 1 ] ) ) != 0 ) - return rc; + /* 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; } /** "imgexec" options */ diff --git a/src/include/ipxe/image.h b/src/include/ipxe/image.h index 32844413..dbcd6d64 100644 --- a/src/include/ipxe/image.h +++ b/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 ); diff --git a/src/include/usr/imgmgmt.h b/src/include/usr/imgmgmt.h index 4299937f..1c0a212e 100644 --- a/src/include/usr/imgmgmt.h +++ b/src/include/usr/imgmgmt.h @@ -11,12 +11,16 @@ FILE_LICENCE ( GPL2_OR_LATER ); #include +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 ); diff --git a/src/usr/autoboot.c b/src/usr/autoboot.c index 7b851b3b..f8eb71cd 100644 --- a/src/usr/autoboot.c +++ b/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; } diff --git a/src/usr/imgmgmt.c b/src/usr/imgmgmt.c index f375db86..b1e8cbb1 100644 --- a/src/usr/imgmgmt.c +++ b/src/usr/imgmgmt.c @@ -35,19 +35,54 @@ FILE_LICENCE ( GPL2_OR_LATER ); * */ +/** + * 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_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_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_image ( image ) ) != 0 ) - return rc; - - if ( ( rc = image_probe ( image ) ) != 0 ) + if ( ( rc = register_and_probe_image ( image ) ) != 0 ) return rc; if ( ( rc = image_select ( image ) ) != 0 ) @@ -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;