| Message ID | 1370377419-31788-1-git-send-email-alevy@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 4 June 2013 21:23, Alon Levy <alevy@redhat.com> wrote: > > +int qemu_pipe_non_block(int pipefd[2]) > +{ > + int ret; > + > + ret = qemu_pipe(pipefd); > + 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; > + } qemu_set_nonblock(card->pipe[0]); qemu_set_nonblock(card->pipe[1]); > + if (fcntl(card->pipe[0], F_SETOWN, getpid()) == -1) { > + return -errno; > + } You should either just trust that the fcntl() succeeds (as we do in qemu_set_block() and friends), or you need to close the pipe fds on failure here. > +} You've forgotten to return anything at the end of the function. (surprised the compiler didn't pick that up, maybe it's one of the warnings that needs optimimisation turned on). thanks -- PMM
On 06/04/2013 02:23 PM, Alon Levy wrote: > Used by the followin patch. s/followin/following/ > > +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? > + 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. > + } > + 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? 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
> 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 > >
05.06.2013 00:23, Alon Levy wrote: > Used by the followin patch. > > +int qemu_pipe_non_block(int pipefd[2]); A nitpick. I'd name it qemu_pipe_nonblock(), like O_NONBLOCK is named, but that may be just me. Thanks, /mjt
05.06.2013 00:23, Alon Levy wrote: [PATCH 1/5] oslib-posix: add qemu_pipe_non_block [PATCH 2/5] use qemu_pipe_non_block [PATCH 3/5] libcacard/vscclient: fix leakage of socket on error paths [PATCH 4/5] libcacard/vreader.c: fix possible NULL dereference [PATCH 5/5] libcacard/vscclient.c: fix use of uninitialized variable So, what happened with this series? From the above 5 patches, only 3/5 (leakage of socket on error paths) is ready to be applied. Should I apply it now, or wait for the respin of whole series? (Or both?) Thanks, /mjt
> 05.06.2013 00:23, Alon Levy wrote: > > [PATCH 1/5] oslib-posix: add qemu_pipe_non_block > [PATCH 2/5] use qemu_pipe_non_block > [PATCH 3/5] libcacard/vscclient: fix leakage of socket on error paths > [PATCH 4/5] libcacard/vreader.c: fix possible NULL dereference > [PATCH 5/5] libcacard/vscclient.c: fix use of uninitialized variable > > So, what happened with this series? I still plan to do it, but didn't get to it yet. > > From the above 5 patches, only 3/5 (leakage of socket on error paths) > is ready to be applied. Should I apply it now, or wait for the > respin of whole series? (Or both?) I'll be happy if you do. (Both don't actually make sense :) > > Thanks, > > /mjt > >
diff --git a/include/qemu-common.h b/include/qemu-common.h index cb82ef3..c24d75c 100644 --- a/include/qemu-common.h +++ b/include/qemu-common.h @@ -232,6 +232,7 @@ ssize_t qemu_recv_full(int fd, void *buf, size_t count, int flags) #ifndef _WIN32 int qemu_pipe(int pipefd[2]); +int qemu_pipe_non_block(int pipefd[2]); #endif #ifdef _WIN32 diff --git a/util/oslib-posix.c b/util/oslib-posix.c index 3dc8b1b..bc2ce2e 100644 --- a/util/oslib-posix.c +++ b/util/oslib-posix.c @@ -188,6 +188,25 @@ int qemu_pipe(int pipefd[2]) return ret; } +int qemu_pipe_non_block(int pipefd[2]) +{ + int ret; + + ret = qemu_pipe(pipefd); + 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; + } + if (fcntl(card->pipe[0], F_SETOWN, getpid()) == -1) { + return -errno; + } +} + int qemu_utimens(const char *path, const struct timespec *times) { struct timeval tv[2], tv_now;
Used by the followin patch. Signed-off-by: Alon Levy <alevy@redhat.com> --- include/qemu-common.h | 1 + util/oslib-posix.c | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+)