On 10/17/2016 01:08 PM, Frediano Ziglio wrote:
Also replaced "continue" in while block with an empty
block (added curly braces).
I think you split this in 7/8.
Right, I'll remove it.
Found by coverity.
Signed-off-by: Uri Lublin <u...@redhat.com>
---
server/red-record-qxl.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/server/red-record-qxl.c b/server/red-record-qxl.c
index adc487b..21fc35f 100644
--- a/server/red-record-qxl.c
+++ b/server/red-record-qxl.c
@@ -845,13 +845,18 @@ void red_record_qxl_command(RedRecord *record,
RedMemSlotInfo *slots,
static void child_output_setup(gpointer user_data)
{
int fd = GPOINTER_TO_INT(user_data);
+ int r;
while (dup2(fd, STDOUT_FILENO) < 0 && errno == EINTR) {
}
close(fd);
// make sure file is not closed calling exec()
- fcntl(STDOUT_FILENO, F_SETFD, 0);
+ while ((r = fcntl(STDOUT_FILENO, F_SETFD, 0)) < 0 && errno == EINTR) {
+ }
+ if (r < 0) {
+ spice_error("fcntl F_SETFD failed (%d)\n", errno);
+ }
}
RedRecord *red_record_new(const char *filename)
I tried to understand better this.
First: fcntl(F_SETFD) cannot return EINTR so there's no reason to check
(unless you check for kernel bugs).
This would change the code to
if (fcntl(STDOUT_FILENO, F_SETFD, 0) < 0) {
spice_error("fcntl F_SETFD failed (%d)\n", errno);
}
Note however that probably this won't do what you are expecting.
This will put the message in the log and then spice-server will continue
and write the recording into a closed pipe so all fprintf internally will
call write which will return EPIPE.
Looking at the called function the file descriptor is the file descriptor
of "f" which is not created with O_CLOEXEC flag so the easier way to remove
this warning is just remove the fcntl line (which is doing nothing).
I think you are right, but also because of the dup2 that is a few lines
above the fcntl.
About O_CLOEXEC would be worth perhaps setting this flag after setting
up file/pipe (before the call to fwrite in red_record_new), something
like
if (fcntl(fileno(f), F_SETFD, O_CLOEXEC) < 0) {
spice_error("fcntl F_SETFD failed (%d)\n", errno);
}
it's a bit racy but not racy would require using G_SPAWN_CLOEXEC_PIPES
in g_spawn_async_with_pipes which is supported since GLib 2.40 or manually
build the pipe with pipe2 and O_CLOEXEC and passing it to
g_spawn_async_with_pipes
(and opening the record file with "w+e" instead of just "w+").
But I don't think that risking to leak this file descriptor is a big deal...
right again.
I also noticed that if there are 2 (or more) QXL
devices, the same file is used for all, which is problematic
Thanks,
Uri.
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
