[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 01/10] xen: make xen loader callable multiple times
From: |
Daniel Kiper |
Subject: |
Re: [PATCH v3 01/10] xen: make xen loader callable multiple times |
Date: |
Thu, 18 Feb 2016 11:12:29 +0100 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Wed, Feb 17, 2016 at 06:19:28PM +0100, Juergen Gross wrote:
> The loader for xen paravirtualized environment isn't callable multiple
> times as it won't free any memory in case of failure.
>
> Call grub_relocator_unload() as other modules do it before allocating
Do you mean grub_xen_reset?
> a new relocator or when unloading the module.
>
> Signed-off-by: Juergen Gross <address@hidden>
> ---
> grub-core/loader/i386/xen.c | 28 +++++++++++++++++++---------
> grub-core/loader/i386/xen_fileXX.c | 17 +++++++++++------
> 2 files changed, 30 insertions(+), 15 deletions(-)
>
> diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c
> index c4d9689..ff7c553 100644
> --- a/grub-core/loader/i386/xen.c
> +++ b/grub-core/loader/i386/xen.c
> @@ -316,11 +316,23 @@ grub_xen_boot (void)
> xen_inf.virt_base);
> }
>
> +static void
> +grub_xen_reset (void)
> +{
> + grub_memset (&next_start, 0, sizeof (next_start));
> + xen_module_info_page = NULL;
> + n_modules = 0;
> +
> + grub_relocator_unload (relocator);
> + relocator = NULL;
> + loaded = 0;
> +}
> +
> static grub_err_t
> grub_xen_unload (void)
> {
> + grub_xen_reset ();
> grub_dl_unref (my_mod);
> - loaded = 0;
> return GRUB_ERR_NONE;
> }
>
> @@ -403,10 +415,7 @@ grub_cmd_xen (grub_command_t cmd __attribute__
> ((unused)),
>
> grub_loader_unset ();
>
> - grub_memset (&next_start, 0, sizeof (next_start));
> -
> - xen_module_info_page = NULL;
> - n_modules = 0;
> + grub_xen_reset ();
>
> grub_create_loader_cmdline (argc - 1, argv + 1,
> (char *) next_start.cmd_line,
> @@ -503,16 +512,17 @@ grub_cmd_xen (grub_command_t cmd __attribute__
> ((unused)),
> goto fail;
>
> fail:
> + err = grub_errno;
I do not think this is needed.
> if (elf)
> grub_elf_close (elf);
> else if (file)
> grub_file_close (file);
>
> - if (grub_errno != GRUB_ERR_NONE)
> - loaded = 0;
> + if (err != GRUB_ERR_NONE)
> + grub_xen_reset ();
>
> - return grub_errno;
> + return err;
> }
>
> static grub_err_t
> @@ -552,7 +562,7 @@ grub_cmd_initrd (grub_command_t cmd __attribute__
> ((unused)),
> {
> err = grub_relocator_alloc_chunk_addr (relocator, &ch, max_addr, size);
> if (err)
> - return err;
> + goto fail;
It looks that this change should not be part of this patch.
> if (grub_initrd_load (&initrd_ctx, argv,
> get_virtual_current_address (ch)))
> diff --git a/grub-core/loader/i386/xen_fileXX.c
> b/grub-core/loader/i386/xen_fileXX.c
> index 1ba5649..5475819 100644
> --- a/grub-core/loader/i386/xen_fileXX.c
> +++ b/grub-core/loader/i386/xen_fileXX.c
> @@ -35,7 +35,8 @@ parse_xen_guest (grub_elf_t elf, struct grub_xen_file_info
> *xi,
> if (grub_file_read (elf->file, buf, sz) != (grub_ssize_t) sz)
> {
> if (grub_errno)
> - return grub_errno;
> + goto out;
> + grub_free (buf);
Ditto.
> return grub_error (GRUB_ERR_BAD_OS, N_("premature end of file %s"),
> elf->file->name);
> }
> @@ -123,14 +124,14 @@ parse_xen_guest (grub_elf_t elf, struct
> grub_xen_file_info *xi,
> {
> xi->virt_base = grub_strtoull (ptr + sizeof ("VIRT_BASE=") - 1, &ptr,
> 16);
> if (grub_errno)
> - return grub_errno;
> + goto out;
Ditto.
> continue;
> }
> if (grub_strncmp (ptr, "VIRT_ENTRY=", sizeof ("VIRT_ENTRY=") - 1) == 0)
> {
> xi->entry_point = grub_strtoull (ptr + sizeof ("VIRT_ENTRY=") - 1,
> &ptr, 16);
> if (grub_errno)
> - return grub_errno;
> + goto out;
Ditto.
> continue;
> }
> if (grub_strncmp (ptr, "HYPERCALL_PAGE=", sizeof ("HYPERCALL_PAGE=") -
> 1) == 0)
> @@ -138,7 +139,7 @@ parse_xen_guest (grub_elf_t elf, struct
> grub_xen_file_info *xi,
> xi->hypercall_page = grub_strtoull (ptr + sizeof ("HYPERCALL_PAGE=")
> - 1, &ptr, 16);
> xi->has_hypercall_page = 1;
> if (grub_errno)
> - return grub_errno;
> + goto out;
Ditto.
> continue;
> }
> if (grub_strncmp (ptr, "ELF_PADDR_OFFSET=", sizeof
> ("ELF_PADDR_OFFSET=") - 1) == 0)
> @@ -146,7 +147,7 @@ parse_xen_guest (grub_elf_t elf, struct
> grub_xen_file_info *xi,
> xi->paddr_offset = grub_strtoull (ptr + sizeof ("ELF_PADDR_OFFSET=")
> - 1, &ptr, 16);
> has_paddr = 1;
> if (grub_errno)
> - return grub_errno;
> + goto out;
Ditto.
> continue;
> }
> }
> @@ -154,7 +155,11 @@ parse_xen_guest (grub_elf_t elf, struct
> grub_xen_file_info *xi,
> xi->hypercall_page = (xi->hypercall_page << 12) + xi->virt_base;
> if (!has_paddr)
> xi->paddr_offset = xi->virt_base;
> - return GRUB_ERR_NONE;
> +
> +out:
> + grub_free (buf);
> +
> + return grub_errno;
Make sense but this should be separate patch.
Daniel
- Re: [PATCH v3 03/10] xen: add elfnote.h to avoid using numbers instead of constants, (continued)
[PATCH v3 10/10] xen: add capability to load p2m list outside of kernel mapping, Juergen Gross, 2016/02/17
[PATCH v3 05/10] xen: factor out p2m list allocation into separate function, Juergen Gross, 2016/02/17
[PATCH v3 09/10] xen: modify page table construction, Juergen Gross, 2016/02/17
[PATCH v3 01/10] xen: make xen loader callable multiple times, Juergen Gross, 2016/02/17
- Re: [PATCH v3 01/10] xen: make xen loader callable multiple times,
Daniel Kiper <=
[PATCH v3 08/10] xen: add capability to load initrd outside of initial mapping, Juergen Gross, 2016/02/17
[PATCH v3 04/10] xen: synchronize xen header, Juergen Gross, 2016/02/17