From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:53167) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TO9Fu-0001Ma-UE for qemu-devel@nongnu.org; Tue, 16 Oct 2012 11:40:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TO9Fq-00041m-Jn for qemu-devel@nongnu.org; Tue, 16 Oct 2012 11:40:30 -0400 Received: from mx1.redhat.com ([209.132.183.28]:29401) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TO9Fq-00041c-95 for qemu-devel@nongnu.org; Tue, 16 Oct 2012 11:40:26 -0400 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q9GFeK2P009576 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 16 Oct 2012 11:40:25 -0400 Message-ID: <507D7FDF.2000709@redhat.com> Date: Tue, 16 Oct 2012 11:40:15 -0400 From: Jeff Cody MIME-Version: 1.0 References: <507D7945.3060003@redhat.com> In-Reply-To: <507D7945.3060003@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 1/4] block: make bdrv_find_backing_image compare canonical filenames Reply-To: jcody@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: kwolf@redhat.com, qemu-devel@nongnu.org On 10/16/2012 11:12 AM, Eric Blake wrote: > On 10/16/2012 08:44 AM, Jeff Cody wrote: >> Currently, bdrv_find_backing_image compares bs->backing_file with >> what is passed in as a backing_file name. Mismatches may occur, >> however, when bs->backing_file and backing_file are both not > > Reads better as s/both not/not both/. > >> absolute or relative. >> >> Use path_combine() to make sure any relative backing filenames are >> relative to the current image filename being searched, and then use >> realpath() to make all comparisons based on absolute filenames. >> >> This also changes bdrv_find_backing_image to no longer be recursive, >> but iterative. >> >> Signed-off-by: Jeff Cody >> --- >> block.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------ >> 1 file changed, 58 insertions(+), 6 deletions(-) > >> - if (!bs->drv) { >> + char *filename_full = NULL; >> + char *backing_file_full = NULL; >> + char *filename_tmp = NULL; >> + int is_protocol = 0; > > Any reason you didn't use bool here? > path_has_protocol() returns an int, so I preferred to just match that. >> + BlockDriverState *curr_bs = NULL; >> + BlockDriverState *retval = NULL; >> + >> + if (!bs || !bs->drv || !backing_file) { >> return NULL; >> } >> >> - if (bs->backing_hd) { >> - if (strcmp(bs->backing_file, backing_file) == 0) { >> - return bs->backing_hd; >> + filename_full = g_malloc(sizeof(char) * PATH_MAX); > > sizeof(char) is guaranteed to be 1; this can be simplified to > g_malloc(PATH_MAX). > >> + backing_file_full = g_malloc(sizeof(char) * PATH_MAX); >> + filename_tmp = g_malloc(sizeof(char) * PATH_MAX); >> + if (!filename_full || !backing_file_full || !filename_tmp) { >> + goto error; >> + } > > Dead 'if', since g_malloc() is guaranteed to succeed. > I'll go ahead and simplify both of the above with the next spin. >> + >> + is_protocol = path_has_protocol(backing_file); >> + >> + for (curr_bs = bs; curr_bs->backing_hd; curr_bs = curr_bs->backing_hd) { >> + >> + /* If either of the filename paths is actually a protocol, then >> + * compare unmodified paths; otherwise make paths relative */ >> + if (is_protocol || path_has_protocol(curr_bs->backing_file)) { >> + if (strcmp(backing_file, curr_bs->backing_file) == 0) { > > I guess we are guaranteed that if is_protocol and path_has_protocol() > give different answers, then the strcmp() will fail? > Yes, since is_protocol is determined via path_has_protocol() - if they are different, then by definition strcmp() should fail. Jeff