Re: [Spice-devel] [spice-server PATCH 8/8] red-record-qxl: child_output_setup: check fcntl return value

[Frediano Ziglio]

> 
> 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 
> >> ---
> >>  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.
> 
> 

Well, this is a known issue actually :(
Basically we don't support recording for multiple cards.
I proposed some patches to start addressing this issue recording
everything into a single file.

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-server PATCH 8/8] red-record-qxl: child_output_setup: check fcntl return value

[Uri Lublin]

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 
---
 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

Re: [Spice-devel] [spice-server PATCH 8/8] red-record-qxl: child_output_setup: check fcntl return value

[Frediano Ziglio]

> 
> Also replaced "continue" in while block with an empty
> block (added curly braces).
> 

I think you split this in 7/8.

> Found by coverity.
> 
> Signed-off-by: Uri Lublin 
> ---
>  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).

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...

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [spice-server PATCH 8/8] red-record-qxl: child_output_setup: check fcntl return value

[Uri Lublin]

Also replaced "continue" in while block with an empty
block (added curly braces).

Found by coverity.

Signed-off-by: Uri Lublin 
---
 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)
-- 
2.7.4

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel