| Message ID | f1c0516355e0a60439760f2f39588e31092aa0a5.1350398238.git.jcody@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <jcody@redhat.com> > --- > 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? > + 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. > + > + 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?
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 <jcody@redhat.com> >> --- >> 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
diff --git a/block.c b/block.c index e95f613..165905a 100644 --- a/block.c +++ b/block.c @@ -3123,22 +3123,74 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs, return -ENOTSUP; } +/* backing_file can either be relative, or absolute, or a protocol. If it is + * relative, it must be relative to the chain. So, passing in bs->filename + * from a BDS as backing_file should not be done, as that may be relative to + * the CWD rather than the chain. */ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs, const char *backing_file) { - if (!bs->drv) { + char *filename_full = NULL; + char *backing_file_full = NULL; + char *filename_tmp = NULL; + int is_protocol = 0; + 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); + 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; + } + + 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) { + retval = curr_bs->backing_hd; + break; + } } else { - return bdrv_find_backing_image(bs->backing_hd, backing_file); + /* If not an absolute filename path, make it relative to the current + * image's filename path */ + path_combine(filename_tmp, PATH_MAX, curr_bs->filename, + backing_file); + + /* We are going to compare absolute pathnames */ + if (!realpath(filename_tmp, filename_full)) { + continue; + } + + /* We need to make sure the backing filename we are comparing against + * is relative to the current image filename (or absolute) */ + path_combine(filename_tmp, PATH_MAX, curr_bs->filename, + curr_bs->backing_file); + + if (!realpath(filename_tmp, backing_file_full)) { + continue; + } + + if (strcmp(backing_file_full, filename_full) == 0) { + retval = curr_bs->backing_hd; + break; + } } } - return NULL; +error: + g_free(filename_full); + g_free(backing_file_full); + g_free(filename_tmp); + return retval; } int bdrv_get_backing_file_depth(BlockDriverState *bs)
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 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 <jcody@redhat.com> --- block.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 58 insertions(+), 6 deletions(-)