On 2017-10-30 15:47, Alberto Garcia wrote:
> On Fri 29 Sep 2017 06:53:31 PM CEST, Max Reitz wrote:
>> @@ -4096,22 +4086,31 @@ BlockDriverState
>> *bdrv_find_backing_image(BlockDriverState *bs,
>> } else {
>> /* If not an absolute filename path, make it relative to the
>> current
>> * image's filename path */
>> - path_combine_deprecated(filename_tmp, PATH_MAX,
>> curr_bs->filename,
>> - backing_file);
>> + filename_tmp = bdrv_make_absolute_filename(curr_bs,
>> backing_file,
>> + NULL);
>> + if (!filename_tmp) {
>> + continue;
>> + }
>>
>> /* We are going to compare absolute pathnames */
>> if (!realpath(filename_tmp, filename_full)) {
>> + g_free(filename_tmp);
>> continue;
>> }
>> + g_free(filename_tmp);
>
> This feels a bit too verbose, doesn't it? (especially because you're
> doing the same thing twice, see below). It could be made a bit shorter,
> something like:
>
> bool have_filename = filename_tmp && realpath(filename_tmp,
> filename_full);
> g_free(filename_tmp);
> if (!have_filename) {
> continue;
> }
Well, but then again
if (filename_tmp && realpath(filename_tmp, filename_full)) {
g_free(filename_tmp);
continue;
}
g_free(filename_tmp);
has the same length. :-)
(Actually, overall it'd be one line shorter because I'd have to use an
own line for the "bool have_filename" declaration (to put it at the
start of the block).)
So I'll do that, yes.
Thanks for reviewing (and this suggestion)!
Max
>
>> - path_combine_deprecated(filename_tmp, PATH_MAX,
>> curr_bs->filename,
>> - curr_bs->backing_file);
>> + filename_tmp = bdrv_get_full_backing_filename(curr_bs, NULL);
>> + if (!filename_tmp) {
>> + continue;
>> + }
>>
>> if (!realpath(filename_tmp, backing_file_full)) {
>> + g_free(filename_tmp);
>> continue;
>> }
>> + g_free(filename_tmp);
>
> Berto
>
signature.asc
Description: OpenPGP digital signature
