Re: [Xen-devel] [PATCH v3 01/10] xen: make xen loader callable multiple times

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

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 <jgross@xxxxxxxx>
> ---
>  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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel