qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 1/5] oslib-posix: add qemu_pipe_non_block


From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH 1/5] oslib-posix: add qemu_pipe_non_block
Date: Tue, 04 Jun 2013 15:11:36 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130514 Thunderbird/17.0.6

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
-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]