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

From: Daniel Kiper <daniel.kiper@oracle.com>
To: Juergen Gross <jgross@suse.com>
Cc: grub-devel@gnu.org, phcoder@gmail.com, mchang@suse.com,
	xen-devel@lists.xen.org
Subject: Re: [PATCH v3 01/10] xen: make xen loader callable multiple times
Date: Thu, 18 Feb 2016 17:58:09 +0100	[thread overview]
Message-ID: <20160218164933.GG3482@olila.local.net-space.pl> (raw)
In-Reply-To: <56C59DB0.6020709@suse.com>

On Thu, Feb 18, 2016 at 11:32:16AM +0100, Juergen Gross wrote:
> On 18/02/16 11:12, Daniel Kiper wrote:
> > 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?
>
> No. I do want to call grub_relocator_unload() and I'm doing it in
> grub_xen_reset(). Other modules don't call grub_xen_reset(). :-)
>
> >
> >> a new relocator or when unloading the module.
> >>
> >> Signed-off-by: Juergen Gross <jgross@suse.com>
> >> ---
> >>  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.
>
> grub_elf_close() and others might clobber grub_errno.

OK, so, please say it in comment. It is not obvious.

> >>    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.
>
> Why not? It's correcting a memory leak in case of failure. Like the
> other cases below, too. That's the purpose of this patch, after all.

OK but you are referring to grub_relocator_unload() in commit message
and below you are trying to fix different memory leak in the same patch.
So, I think everything below should be separate patch.

Daniel