[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 <uril at 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.
> 
> 

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