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

[Juergen Gross]

On 18/02/16 17:58, Daniel Kiper wrote:
> 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 <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.
>>
>> grub_elf_close() and others might clobber grub_errno.
> 
> OK, so, please say it in comment. It is not obvious.

Okay.

>>>>    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.

Okay.


Juergen