From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1aWdW4-0006oX-1w for mharc-grub-devel@gnu.org; Fri, 19 Feb 2016 00:22:08 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46066) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aWdW1-0006no-RK for grub-devel@gnu.org; Fri, 19 Feb 2016 00:22:06 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aWdVw-0007rC-Pg for grub-devel@gnu.org; Fri, 19 Feb 2016 00:22:05 -0500 Received: from mx2.suse.de ([195.135.220.15]:39586) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aWdVw-0007r3-Fd for grub-devel@gnu.org; Fri, 19 Feb 2016 00:22:00 -0500 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id B93D1AAB4; Fri, 19 Feb 2016 05:21:59 +0000 (UTC) Subject: Re: [PATCH v3 01/10] xen: make xen loader callable multiple times To: Daniel Kiper References: <1455729577-23702-1-git-send-email-jgross@suse.com> <1455729577-23702-2-git-send-email-jgross@suse.com> <20160218101229.GW3482@olila.local.net-space.pl> <56C59DB0.6020709@suse.com> <20160218164933.GG3482@olila.local.net-space.pl> From: Juergen Gross Message-ID: <56C6A677.4020508@suse.com> Date: Fri, 19 Feb 2016 06:21:59 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <20160218164933.GG3482@olila.local.net-space.pl> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x (no timestamps) [generic] X-Received-From: 195.135.220.15 Cc: grub-devel@gnu.org, phcoder@gmail.com, mchang@suse.com, xen-devel@lists.xen.org X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: The development of GNU GRUB List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 19 Feb 2016 05:22:07 -0000 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 >>>> --- >>>> 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