>
> 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@xxxxxxxxxx>
> >> ---
> >> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel