On Sun, Sep 25, 2022 at 12:32:00AM +0800, Bin Meng wrote:
> From: Bin Meng <bin.m...@windriver.com>
>
> At present there are two callers of get_tmp_filename() and they are
> inconsistent.
>
> One does:
>
> /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
> char *tmp_filename = g_malloc0(PATH_MAX + 1);
> ...
> ret = get_tmp_filename(tmp_filename, PATH_MAX + 1);
>
> while the other does:
>
> s->qcow_filename = g_malloc(PATH_MAX);
> ret = get_tmp_filename(s->qcow_filename, PATH_MAX);
>
> As we can see different 'size' arguments are passed. There are also
> platform specific implementations inside the function, and this use
> of snprintf is really undesirable.
>
> Refactor this routine by changing its signature to:
>
> int get_tmp_filename(char **filename)
>
> and use g_file_open_tmp() for a consistent implementation.
>
> Signed-off-by: Bin Meng <bin.m...@windriver.com>
> ---
>
> Changes in v3:
> - Do not use errno directly, instead still let get_tmp_filename() return
> a negative number to indicate error
>
> Changes in v2:
> - Use g_autofree and g_steal_pointer
>
> include/block/block_int-common.h | 2 +-
> block.c | 36 ++++++++++----------------------
> block/vvfat.c | 3 +--
> 3 files changed, 13 insertions(+), 28 deletions(-)
>
> diff --git a/include/block/block_int-common.h
> b/include/block/block_int-common.h
> index 8947abab76..eb30193198 100644
> --- a/include/block/block_int-common.h
> +++ b/include/block/block_int-common.h
> @@ -1230,7 +1230,7 @@ static inline BlockDriverState *child_bs(BdrvChild
> *child)
> }
>
> int bdrv_check_request(int64_t offset, int64_t bytes, Error **errp);
> -int get_tmp_filename(char *filename, int size);
> +int get_tmp_filename(char **filename);
Change it to:
char *get_tmp_filename(Error **errp);
> @@ -3737,7 +3723,7 @@ static BlockDriverState
> *bdrv_append_temp_snapshot(BlockDriverState *bs,
> }
>
> /* Create the temporary image */
> - ret = get_tmp_filename(tmp_filename, PATH_MAX + 1);
> + ret = get_tmp_filename(&tmp_filename);
> if (ret < 0) {
> error_setg_errno(errp, -ret, "Could not get temporary filename");
And pass 'errp' straight into get_tmp_filename, thus avoid the
different error message text here vs below
eg
tmp_filename = get_tmp_filename(errp);
if (!tmp_filename) {
goto out;
}
> goto out;
> diff --git a/block/vvfat.c b/block/vvfat.c
> index d6dd919683..43f42fd1ea 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -3146,8 +3146,7 @@ static int enable_write_target(BlockDriverState *bs,
> Error **errp)
>
> array_init(&(s->commits), sizeof(commit_t));
>
> - s->qcow_filename = g_malloc(PATH_MAX);
> - ret = get_tmp_filename(s->qcow_filename, PATH_MAX);
> + ret = get_tmp_filename(&s->qcow_filename);
> if (ret < 0) {
> error_setg_errno(errp, -ret, "can't create temporary file");
> goto err;
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
