| Message ID | 20220824094029.1634519-4-bmeng.cn@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tests/qtest: Enable running qtest on Windows | expand |
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; > } > @@ -891,7 +880,6 @@ int get_tmp_filename(char *filename, int size) > return -errno; > } > return 0; > -#endif > } > > /* > -- > 2.34.1 > > >
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
Hi Marc-André, On Wed, Aug 31, 2022 at 8:54 PM Marc-André Lureau <marcandre.lureau@gmail.com> 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? Without this patch the qtest cases builds on Windows do not have any problem. So it is optional. I put it in the same series as it has the same context of using hardcoded /tmp directory name. > > 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. I can drop this single patch and send another single patch if this is the desired practice. > > thanks Regards, Bin
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; } @@ -891,7 +880,6 @@ int get_tmp_filename(char *filename, int size) return -errno; } return 0; -#endif } /*