On Fri 16 Dec 2016 02:26:29 PM 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).
Ok, I suspected that. Even if we keep it, I think it wouldn't be bad to
leave a comment saying that it's there on purpose and it's not being set
twice accidentally.
Otherwise whoever reads that code in the future might think that it's a
sign of a merge that wasn't carefully reviewed.
But I won't oppose if you prefer to leave it like that.
Reviewed-by: Alberto Garcia <be...@igalia.com>
Berto
