[v3,07/25] block: bdrv_get_full_backing_filename's ret. val.
diff mbox

Message ID 20161130011851.24696-8-mreitz@redhat.com
State New
Headers show

Commit Message

Max Reitz Nov. 30, 2016, 1:18 a.m. UTC
Make bdrv_get_full_backing_filename() return an allocated string instead
of placing the result in a caller-provided buffer.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c               | 26 +++++++++-----------------
 block/qapi.c          | 12 ++----------
 include/block/block.h |  3 +--
 3 files changed, 12 insertions(+), 29 deletions(-)

Comments

Alberto Garcia Dec. 15, 2016, 4:39 p.m. UTC | #1
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.

Otherwise the patch looks fine.

Berto
Max Reitz Dec. 16, 2016, 1:26 p.m. UTC | #2
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
Alberto Garcia Dec. 16, 2016, 1:51 p.m. UTC | #3
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 <berto@igalia.com>

Berto

Patch
diff mbox

diff --git a/block.c b/block.c
index b02e492..58c7094 100644
--- a/block.c
+++ b/block.c
@@ -220,19 +220,13 @@  char *bdrv_get_full_backing_filename_from_filename(const char *backed,
     }
 }
 
-void bdrv_get_full_backing_filename(BlockDriverState *bs, char *dest, size_t sz,
-                                    Error **errp)
+char *bdrv_get_full_backing_filename(BlockDriverState *bs, Error **errp)
 {
     char *backed = bs->exact_filename[0] ? bs->exact_filename : bs->filename;
-    char *full_name;
 
-    full_name = bdrv_get_full_backing_filename_from_filename(backed,
-                                                             bs->backing_file,
-                                                             errp);
-    if (full_name) {
-        pstrcpy(dest, sz, full_name);
-        g_free(full_name);
-    }
+    return bdrv_get_full_backing_filename_from_filename(backed,
+                                                        bs->backing_file,
+                                                        errp);
 }
 
 void bdrv_register(BlockDriver *bdrv)
@@ -1484,7 +1478,7 @@  out:
 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;
 
         /* FIXME: Should also be set to true if @options contains other runtime
          *        options which control the data that is read from the backing
@@ -1521,8 +1515,7 @@  int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
         QDECREF(options);
         goto free_exit;
     } else {
-        bdrv_get_full_backing_filename(bs, backing_filename, PATH_MAX,
-                                       &local_err);
+        backing_filename = bdrv_get_full_backing_filename(bs, &local_err);
         if (local_err) {
             ret = -EINVAL;
             error_propagate(errp, local_err);
@@ -1542,9 +1535,8 @@  int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
         qdict_put(options, "driver", qstring_from_str(bs->backing_format));
     }
 
-    backing_hd = bdrv_open_inherit(*backing_filename ? backing_filename : NULL,
-                                   reference, options, 0, bs, &child_backing,
-                                   errp);
+    backing_hd = bdrv_open_inherit(backing_filename, reference, options, 0, bs,
+                                   &child_backing, errp);
     if (!backing_hd) {
         bs->open_flags |= BDRV_O_NO_BACKING;
         error_prepend(errp, "Could not open backing file: ");
diff --git a/block/qapi.c b/block/qapi.c
index a62e862..1543dd6 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -265,18 +265,10 @@  void bdrv_query_image_info(BlockDriverState *bs,
 
     backing_filename = bs->backing_file;
     if (backing_filename[0] != '\0') {
-        char *backing_filename2 = g_malloc0(PATH_MAX);
+        char *backing_filename2;
         info->backing_filename = g_strdup(backing_filename);
         info->has_backing_filename = true;
-        bdrv_get_full_backing_filename(bs, backing_filename2, PATH_MAX, &err);
-        if (err) {
-            /* Can't reconstruct the full backing filename, so we must omit
-             * this field and apply a Best Effort to this query. */
-            g_free(backing_filename2);
-            backing_filename2 = NULL;
-            error_free(err);
-            err = NULL;
-        }
+        backing_filename2 = bdrv_get_full_backing_filename(bs, NULL);
 
         /* Always report the full_backing_filename if present, even if it's the
          * same as backing_filename. That they are same is useful info. */
diff --git a/include/block/block.h b/include/block/block.h
index fe16e45..eb41956 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -435,8 +435,7 @@  void bdrv_round_to_clusters(BlockDriverState *bs,
 const char *bdrv_get_encrypted_filename(BlockDriverState *bs);
 void bdrv_get_backing_filename(BlockDriverState *bs,
                                char *filename, int filename_size);
-void bdrv_get_full_backing_filename(BlockDriverState *bs,
-                                    char *dest, size_t sz, Error **errp);
+char *bdrv_get_full_backing_filename(BlockDriverState *bs, Error **errp);
 char *bdrv_get_full_backing_filename_from_filename(const char *backed,
                                                    const char *backing,
                                                    Error **errp);