| Message ID | 20220924163200.1910807-1-bmeng.cn@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] block: Refactor get_tmp_filename() | expand |
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
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); void bdrv_parse_filename_strip_prefix(const char *filename, const char *prefix, QDict *options); diff --git a/block.c b/block.c index bc85f46eed..55ad257241 100644 --- a/block.c +++ b/block.c @@ -861,37 +861,24 @@ int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry *geo) /* * Create a uniquely-named empty temporary file. * Return 0 upon success, otherwise a negative errno value. + * The caller function is responsible for freeing *filename. */ -int get_tmp_filename(char *filename, int size) +int get_tmp_filename(char **filename) { -#ifdef _WIN32 - char temp_dir[MAX_PATH]; - /* GetTempFileName requires that its output buffer (4th param) - have length MAX_PATH or greater. */ - assert(size >= MAX_PATH); - return (GetTempPath(MAX_PATH, temp_dir) - && GetTempFileName(temp_dir, "qem", 0, filename) - ? 0 : -GetLastError()); -#else + g_autofree char *name = NULL; int fd; - const char *tmpdir; - tmpdir = getenv("TMPDIR"); - if (!tmpdir) { - tmpdir = "/var/tmp"; - } - if (snprintf(filename, size, "%s/vl.XXXXXX", tmpdir) >= size) { - return -EOVERFLOW; - } - fd = mkstemp(filename); + + fd = g_file_open_tmp("vl.XXXXXX", &name, NULL); if (fd < 0) { - return -errno; + return -ENOENT; } if (close(fd) != 0) { - unlink(filename); + unlink(name); return -errno; } + + *filename = g_steal_pointer(&name); return 0; -#endif } /* @@ -3717,8 +3704,7 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs, QDict *snapshot_options, Error **errp) { - /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */ - char *tmp_filename = g_malloc0(PATH_MAX + 1); + char *tmp_filename = NULL; int64_t total_size; QemuOpts *opts = NULL; BlockDriverState *bs_snapshot = NULL; @@ -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"); 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;