Re: [Qemu-devel] [PATCH v3 07/25] block: bdrv_get_full_backing_filename's ret. val.

[Max Reitz]

On 15.12.2016 17:39, Alberto Garcia wrote:
> On Wed 30 Nov 2016 02:18:33 AM CET, Max Reitz wrote:
>>  int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
>>                             const char *bdref_key, Error **errp)
>>  {
>> -    char *backing_filename = g_malloc0(PATH_MAX);
>> +    char *backing_filename = NULL;
>>      char *bdref_key_dot;
>>      const char *reference = NULL;
>>      int ret = 0;
>> @@ -1511,7 +1505,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict 
>> *parent_options,
>>  
>>      reference = qdict_get_try_str(parent_options, bdref_key);
>>      if (reference || qdict_haskey(options, "file.filename")) {
>> -        backing_filename[0] = '\0';
>> +        backing_filename = NULL;
> 
> You're making it NULL, but it's NULL already.

Yes, so I hope the compiler can optimize it away. :-)

I initialized it to NULL here to make it more clear that under the given
condition we do not have a backing filename (so that it's clear from a
single look at this if-else block what exactly happens in each case
without having to know the state of variables beforehand).

So it was intentional, but I can either change it so that
backing_filename would no longer be initialized to NULL (meaning the
first "goto free_exit;" would need to be just a "return 0;") or make it
a comment (/* backing_filename is already NULL */). I'm leaning towards
the latter because it would mean less changes to this series, although I
personally think that simply explicitly setting backing_filename to NULL
instead of such a comment is just as good.

So if I have to send a v4 (which I probably will), I'll make it a
comment, but I probably won't send a v4 just for this (I'm sure there
are other good reasons, however :-)).

And thanks for reviewing!

Max

signature.asc
Description: OpenPGP digital signature