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...@gmail.com> wrote: > > > From: Bin Meng <bin.m...@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.m...@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 :|