* [RFC PATCH] linux-user/syscall: Use g_file_open_tmp() @ 2020-02-27 10:06 Philippe Mathieu-Daudé 2020-02-27 10:25 ` Marc-André Lureau 2020-02-27 10:31 ` Daniel P. Berrangé 0 siblings, 2 replies; 4+ messages in thread From: Philippe Mathieu-Daudé @ 2020-02-27 10:06 UTC (permalink / raw) To: qemu-devel Cc: Marc-André Lureau, Riku Voipio, Daniel P . Berrange, Laurent Vivier, Philippe Mathieu-Daudé Use GLib g_file_open_tmp() instead of getenv + snprintf + mkstemp. Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- RFC because I'm not sure g_autoptr(GError) works this way. linux-user/syscall.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/linux-user/syscall.c b/linux-user/syscall.c index 8d27d10807..0e44969e16 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -7282,17 +7282,14 @@ static int do_openat(void *cpu_env, int dirfd, const char *pathname, int flags, } if (fake_open->filename) { - const char *tmpdir; - char filename[PATH_MAX]; + g_autoptr(GError) gerr = NULL; + g_autofree gchar *filename = NULL; int fd, r; /* create temporary file to map stat to */ - tmpdir = getenv("TMPDIR"); - if (!tmpdir) - tmpdir = "/tmp"; - snprintf(filename, sizeof(filename), "%s/qemu-open.XXXXXX", tmpdir); - fd = mkstemp(filename); + fd = g_file_open_tmp("qemu-open.XXXXXX", &filename, &gerr); if (fd < 0) { + fprintf(stderr, "Error opening %s: %s\n", filename, gerr->message); return fd; } unlink(filename); -- 2.21.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [RFC PATCH] linux-user/syscall: Use g_file_open_tmp() 2020-02-27 10:06 [RFC PATCH] linux-user/syscall: Use g_file_open_tmp() Philippe Mathieu-Daudé @ 2020-02-27 10:25 ` Marc-André Lureau 2020-02-27 10:31 ` Daniel P. Berrangé 1 sibling, 0 replies; 4+ messages in thread From: Marc-André Lureau @ 2020-02-27 10:25 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Riku Voipio, Daniel P . Berrange, qemu-devel, Laurent Vivier Hi On Thu, Feb 27, 2020 at 11:06 AM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > Use GLib g_file_open_tmp() instead of getenv + snprintf + mkstemp. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > RFC because I'm not sure g_autoptr(GError) works this way. G_DEFINE_AUTOPTR_CLEANUP_FUNC(GError, g_error_free) it will call g_error_free() at the end of the scope if the variable is != NULL. > > linux-user/syscall.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index 8d27d10807..0e44969e16 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -7282,17 +7282,14 @@ static int do_openat(void *cpu_env, int dirfd, const char *pathname, int flags, > } > > if (fake_open->filename) { > - const char *tmpdir; > - char filename[PATH_MAX]; > + g_autoptr(GError) gerr = NULL; > + g_autofree gchar *filename = NULL; > int fd, r; > > /* create temporary file to map stat to */ > - tmpdir = getenv("TMPDIR"); > - if (!tmpdir) > - tmpdir = "/tmp"; > - snprintf(filename, sizeof(filename), "%s/qemu-open.XXXXXX", tmpdir); > - fd = mkstemp(filename); > + fd = g_file_open_tmp("qemu-open.XXXXXX", &filename, &gerr); > if (fd < 0) { > + fprintf(stderr, "Error opening %s: %s\n", filename, gerr->message); I am not sure if it's a good idea to printf() here, this may be confused with the program output being run. However, having a good errno value is probably necessary. And here, glib doesn't guarantee that for this function, since it relies on GFileError. So you would need something like g_file_error_to_errno() which doesn't exist... > return fd; > } > unlink(filename); > -- > 2.21.1 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC PATCH] linux-user/syscall: Use g_file_open_tmp() 2020-02-27 10:06 [RFC PATCH] linux-user/syscall: Use g_file_open_tmp() Philippe Mathieu-Daudé 2020-02-27 10:25 ` Marc-André Lureau @ 2020-02-27 10:31 ` Daniel P. Berrangé 2020-02-27 10:56 ` Ján Tomko 1 sibling, 1 reply; 4+ messages in thread From: Daniel P. Berrangé @ 2020-02-27 10:31 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Marc-André Lureau, Riku Voipio, qemu-devel, Laurent Vivier On Thu, Feb 27, 2020 at 11:06:21AM +0100, Philippe Mathieu-Daudé wrote: > Use GLib g_file_open_tmp() instead of getenv + snprintf + mkstemp. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > RFC because I'm not sure g_autoptr(GError) works this way. It does work. Any struct that's defined in GLib has support for g_autoptr(). If you aren't suyre though, just check for a G_DEFINE_AUTOPTR_CLEANUP_FUNC() macro usage that refers to the struct in question $ grep -r 'G_DEFINE_AUTOPTR_CLEANUP_FUNC(GError' /usr/include/glib-2.0 /usr/include/glib-2.0/glib/glib-autocleanups.h:G_DEFINE_AUTOPTR_CLEANUP_FUNC(GError, g_error_free) > linux-user/syscall.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index 8d27d10807..0e44969e16 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -7282,17 +7282,14 @@ static int do_openat(void *cpu_env, int dirfd, const char *pathname, int flags, > } > > if (fake_open->filename) { > - const char *tmpdir; > - char filename[PATH_MAX]; > + g_autoptr(GError) gerr = NULL; > + g_autofree gchar *filename = NULL; > int fd, r; > > /* create temporary file to map stat to */ > - tmpdir = getenv("TMPDIR"); > - if (!tmpdir) > - tmpdir = "/tmp"; > - snprintf(filename, sizeof(filename), "%s/qemu-open.XXXXXX", tmpdir); > - fd = mkstemp(filename); > + fd = g_file_open_tmp("qemu-open.XXXXXX", &filename, &gerr); g_file_open_tmp, calls g_get_tmp_name, which calls g_get_tmp_dir, which defaults to $TMPDIR, falling back to /tmp. So we're using the same dir as before. > if (fd < 0) { > + fprintf(stderr, "Error opening %s: %s\n", filename, gerr->message); This is wrong - the returned "filename" is only valid when g_file_open_tmp succeeds. So the use of "filename" here is likely a NULL. Given that the only place you use "filename" is in the error path, and that's not valid, we can simply eliminate it entirely, and pass NULL into g_file_open_tmp > return fd; > } > unlink(filename); 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 :| ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC PATCH] linux-user/syscall: Use g_file_open_tmp() 2020-02-27 10:31 ` Daniel P. Berrangé @ 2020-02-27 10:56 ` Ján Tomko 0 siblings, 0 replies; 4+ messages in thread From: Ján Tomko @ 2020-02-27 10:56 UTC (permalink / raw) To: Daniel P. Berrangé Cc: Marc-André Lureau, Riku Voipio, Philippe Mathieu-Daudé, qemu-devel, Laurent Vivier [-- Attachment #1: Type: text/plain, Size: 2789 bytes --] On a Thursday in 2020, Daniel P. Berrangé wrote: >On Thu, Feb 27, 2020 at 11:06:21AM +0100, Philippe Mathieu-Daudé wrote: >> Use GLib g_file_open_tmp() instead of getenv + snprintf + mkstemp. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> RFC because I'm not sure g_autoptr(GError) works this way. > >It does work. Any struct that's defined in GLib has support for >g_autoptr(). If you aren't suyre though, just check for a >G_DEFINE_AUTOPTR_CLEANUP_FUNC() macro usage that refers to the >struct in question > >$ grep -r 'G_DEFINE_AUTOPTR_CLEANUP_FUNC(GError' /usr/include/glib-2.0 >/usr/include/glib-2.0/glib/glib-autocleanups.h:G_DEFINE_AUTOPTR_CLEANUP_FUNC(GError, g_error_free) > >> linux-user/syscall.c | 11 ++++------- >> 1 file changed, 4 insertions(+), 7 deletions(-) >> >> diff --git a/linux-user/syscall.c b/linux-user/syscall.c >> index 8d27d10807..0e44969e16 100644 >> --- a/linux-user/syscall.c >> +++ b/linux-user/syscall.c >> @@ -7282,17 +7282,14 @@ static int do_openat(void *cpu_env, int dirfd, const char *pathname, int flags, >> } >> >> if (fake_open->filename) { >> - const char *tmpdir; >> - char filename[PATH_MAX]; >> + g_autoptr(GError) gerr = NULL; >> + g_autofree gchar *filename = NULL; >> int fd, r; >> >> /* create temporary file to map stat to */ >> - tmpdir = getenv("TMPDIR"); >> - if (!tmpdir) >> - tmpdir = "/tmp"; >> - snprintf(filename, sizeof(filename), "%s/qemu-open.XXXXXX", tmpdir); >> - fd = mkstemp(filename); >> + fd = g_file_open_tmp("qemu-open.XXXXXX", &filename, &gerr); > >g_file_open_tmp, calls g_get_tmp_name, which calls >g_get_tmp_dir, which defaults to $TMPDIR, falling back >to /tmp. So we're using the same dir as before. > >> if (fd < 0) { >> + fprintf(stderr, "Error opening %s: %s\n", filename, gerr->message); > >This is wrong - the returned "filename" is only valid when >g_file_open_tmp succeeds. So the use of "filename" here >is likely a NULL. Given that the only place you use "filename" >is in the error path, and that's not valid, we can simply >eliminate it entirely, and pass NULL into g_file_open_tmp > 'filename' is used right below to unlink it. But it can be dropped from the error message here since it's included in the error reported by GLib. Jano >> return fd; >> } >> unlink(filename); > >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 :| > > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-02-27 10:57 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-02-27 10:06 [RFC PATCH] linux-user/syscall: Use g_file_open_tmp() Philippe Mathieu-Daudé 2020-02-27 10:25 ` Marc-André Lureau 2020-02-27 10:31 ` Daniel P. Berrangé 2020-02-27 10:56 ` Ján Tomko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).