| Message ID | 20161130011851.24696-8-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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);
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(-)