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

From: Juergen Gross <jgross@suse.com>
To: Daniel Kiper <daniel.kiper@oracle.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 11:32:16 +0100	[thread overview]
Message-ID: <56C59DB0.6020709@suse.com> (raw)
In-Reply-To: <20160218101229.GW3482@olila.local.net-space.pl>

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.

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

Juergen

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