[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-trivial] [Qemu-devel] [PATCH 1/5] oslib-posix: add qemu_pipe_n
From: |
Alon Levy |
Subject: |
Re: [Qemu-trivial] [Qemu-devel] [PATCH 1/5] oslib-posix: add qemu_pipe_non_block |
Date: |
Tue, 4 Jun 2013 17:41:41 -0400 (EDT) |
> On 06/04/2013 02:23 PM, Alon Levy wrote:
> > Used by the followin patch.
>
> s/followin/following/
Thanks.
>
> >
> > +int qemu_pipe_non_block(int pipefd[2])
> > +{
> > + int ret;
> > +
> > + ret = qemu_pipe(pipefd);
>
> qemu_pipe() already uses pipe2() when available; it seems like it would
> be nicer to use pipe2's O_NONBLOCK option directly in one syscall (where
> supported) instead of having to make additional syscalls after the fact.
> Would it just be smarter to change the signature of qemu_pipe() to add
> a bool block parameter, and then change the 5 existing callers to pass
> false with your later patch in the series passing true, and do it
> without creating a new wrapper?
Answered below.
>
> > + if (ret) {
> > + return ret;
> > + }
> > + if (fcntl(card->pipe[0], F_SETFL, O_NONBLOCK) == -1) {
> > + return -errno;
> > + }
> > + if (fcntl(card->pipe[1], F_SETFL, O_NONBLOCK) == -1) {
> > + return -errno;
>
> Leaks fds. If you're going to report error, then you must close the fds
> already created.
As Peter pointed out, I should not go here, so I'll drop these checks, instead
doing naked fcntl calls, so no fd leak possible (no returns).
>
> > + }
> > + if (fcntl(card->pipe[0], F_SETOWN, getpid()) == -1) {
> > + return -errno;
>
> Same comment about fd leaks.
>
> This part seems like a useful change, IF you plan on using SIGIO and
> SIGURG signals; and it is something which pipe2() cannot optimize, so I
> can see why you are adding a new function instead of changing
> qemu_pipe() and adjust all its callers to pass an additional parameter.
> But are you really planning on using SIGIO/SIGURG?
I don't plan on using those signals, so I'll add a parameter instead.
>
> Furthermore, this is undefined behavior. According to POSIX, use of
> F_SETOWN is only portable on sockets, not pipes. It may work on Linux,
> but you'll need to be aware of what it does on other platforms.
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/fcntl.html
> --
> Eric Blake eblake redhat com +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
>
- Re: [Qemu-trivial] [Qemu-devel] [PATCH 2/5] use qemu_pipe_non_block, (continued)
- [Qemu-trivial] [PATCH 3/5] libcacard/vscclient: fix leakage of socket on error paths, Alon Levy, 2013/06/04
- [Qemu-trivial] [PATCH 4/5] libcacard/vreader.c: fix possible NULL dereference, Alon Levy, 2013/06/04
- [Qemu-trivial] [PATCH 5/5] libcacard/vscclient.c: fix use of uninitialized variable, Alon Levy, 2013/06/04
- Re: [Qemu-trivial] [Qemu-devel] [PATCH 1/5] oslib-posix: add qemu_pipe_non_block, Peter Maydell, 2013/06/04
- Re: [Qemu-trivial] [Qemu-devel] [PATCH 1/5] oslib-posix: add qemu_pipe_non_block, Eric Blake, 2013/06/04
- Re: [Qemu-trivial] [Qemu-devel] [PATCH 1/5] oslib-posix: add qemu_pipe_non_block,
Alon Levy <=
- Re: [Qemu-trivial] [PATCH 1/5] oslib-posix: add qemu_pipe_non_block, Michael Tokarev, 2013/06/05
- Re: [Qemu-trivial] [PATCH 1/5] oslib-posix: add qemu_pipe_non_block, Michael Tokarev, 2013/06/12