Re: [PATCH v3] block: Refactor get_tmp_filename()

[Daniel P . Berrangé]

On Sun, Sep 25, 2022 at 12:32:00AM +0800, Bin Meng wrote:
> From: Bin Meng <bin.meng@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.meng@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 :|