Patchwork [1/5] oslib-posix: add qemu_pipe_non_block

login
register
mail settings
Submitter Alon Levy
Date June 4, 2013, 8:23 p.m.
Message ID <1370377419-31788-1-git-send-email-alevy@redhat.com>
Download mbox | patch
Permalink /patch/248852/
State New
Headers show

Comments

Alon Levy - June 4, 2013, 8:23 p.m.
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(+)
Peter Maydell - June 4, 2013, 8:48 p.m.
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
Eric Blake - June 4, 2013, 9:11 p.m.
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
Alon Levy - June 4, 2013, 9:41 p.m.
> 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
> 
>
Michael Tokarev - June 5, 2013, 7:43 p.m.
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
Michael Tokarev - June 12, 2013, 9:38 a.m.
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
Alon Levy - June 12, 2013, 11:21 a.m.
> 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
> 
>

Patch

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;