| Message ID | 20200227100621.21844-1-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] linux-user/syscall: Use g_file_open_tmp() | expand |
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 >
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
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 :| > >
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);
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(-)