[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 03/51] block: Unify the get_tmp_filename() implementation
From: |
Daniel P . Berrangé |
Subject: |
Re: [PATCH 03/51] block: Unify the get_tmp_filename() implementation |
Date: |
Wed, 31 Aug 2022 14:19:45 +0100 |
User-agent: |
Mutt/2.2.6 (2022-06-05) |
On Wed, Aug 31, 2022 at 04:54:41PM +0400, Marc-André Lureau wrote:
> Hi Bin
>
> On Wed, Aug 24, 2022 at 1:42 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> > From: Bin Meng <bin.meng@windriver.com>
> >
> > At present get_tmp_filename() has platform specific implementations
> > to get the directory to use for temporary files. Switch over to use
> > g_get_tmp_dir() which works on all supported platforms.
> >
> >
> It "works" quite differently though. Is this patch really necessary here?
>
> If yes, please explain why.
>
> If not, I suggest you drop optional / rfc / "nice to have" patches from the
> series. It will help to get it merged faster.
>
> thanks
>
>
>
> > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > ---
> >
> > block.c | 16 ++--------------
> > 1 file changed, 2 insertions(+), 14 deletions(-)
> >
> > diff --git a/block.c b/block.c
> > index bc85f46eed..d06df47f72 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -864,21 +864,10 @@ int bdrv_probe_geometry(BlockDriverState *bs,
> > HDGeometry *geo)
> > */
> > int get_tmp_filename(char *filename, int size)
> > {
> > -#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
> > int fd;
> > const char *tmpdir;
> > - tmpdir = getenv("TMPDIR");
> > - if (!tmpdir) {
> > - tmpdir = "/var/tmp";
> > - }
> > + tmpdir = g_get_tmp_dir();
> > +
> > if (snprintf(filename, size, "%s/vl.XXXXXX", tmpdir) >= size) {
> > return -EOVERFLOW;
> > }
I know this is pre-existing, but this use of snprintf is really
undesirable and should be culled while we're touching this code.
There are only two callers of get_tmp_filename and they're
inconsistent too
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);
either the comment is wrong and adding "+1" to PATH_MAX is not
required, or the second caller is wrong on Windows. This may even
be totally irrelevant with the switch to g_get_tmp_dir. Whatever
the answer is, at least 1 of the callers needs updating.
It would be way better if the method signature was
char *get_tmp_filename(void);
and we uses g_strdup_printf() instead of snprintf so the corret
size is allocated right away, removing the question about whether
we need PATH_MAX or PATH_MAX + 1 entirely.
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 :|
- [PATCH 00/51] tests/qtest: Enable running qtest on Windows, Bin Meng, 2022/08/24
- [PATCH 01/51] tests/qtest: Use g_setenv(), Bin Meng, 2022/08/24
- [PATCH 02/51] tests/qtest: Use g_mkdtemp(), Bin Meng, 2022/08/24
- [PATCH 03/51] block: Unify the get_tmp_filename() implementation, Bin Meng, 2022/08/24
- [PATCH 04/51] semihosting/arm-compat-semi: Avoid using hardcoded /tmp, Bin Meng, 2022/08/24
- [PATCH 05/51] tcg: Avoid using hardcoded /tmp, Bin Meng, 2022/08/24
- [PATCH 06/51] util/qemu-sockets: Use g_get_tmp_dir() to get the directory for temporary files, Bin Meng, 2022/08/24
- [PATCH 09/51] fsdev/virtfs-proxy-helper: Use g_mkdir_with_parents(), Bin Meng, 2022/08/24