[Qemu-devel] [PULL v1 0/5] I/O channels fixes

[Qemu-devel] [PULL v1 0/5] I/O channels fixes

[Daniel P. Berrange]

The following changes since commit 3db34bf64ab4f8797565dd8750003156c32b301d:

  Merge remote-tracking branch 'remotes/afaerber/tags/qom-devices-for-peter' into staging (2016-01-18 17:40:50 +0000)

are available in the git repository at:

  git://github.com/berrange/qemu tags/pull-io-next-2016-01-20-1

for you to fetch changes up to ccf1e2dcd6091eea1fc2341c63201aa1a6094978:

  io: use memset instead of { 0 } for initializing array (2016-01-20 11:31:01 +0000)

----------------------------------------------------------------
I/O channels fixes 2016/01/20 v1

----------------------------------------------------------------
Daniel P. Berrange (5):
      io: fix sign of errno value passed to error report
      io: increment counter when killing off subcommand
      io: some fixes to handling of /dev/null when running commands
      io: fix description of @errp parameter initialization
      io: use memset instead of { 0 } for initializing array

 include/io/channel-command.h |  4 ++--
 include/io/channel-socket.h  | 14 +++++++-------
 include/io/channel-tls.h     |  4 ++--
 include/io/channel.h         | 20 ++++++++++----------
 io/channel-command.c         | 23 +++++++++++++++++------
 io/channel-socket.c          |  8 ++++++--
 6 files changed, 44 insertions(+), 29 deletions(-)
-- 
2.5.0

[Qemu-devel] [PULL v1 1/5] io: fix sign of errno value passed to error report

[Daniel P. Berrange]

When reporting the number of FDs has been exceeded, pass
EINVAL to error_setg_errno, rather than -EINVAL.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 io/channel-socket.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/io/channel-socket.c b/io/channel-socket.c
index 10a5b31..eaa411f 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -502,7 +502,7 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc,
 
     if (nfds) {
         if (nfds > SOCKET_MAX_FDS) {
-            error_setg_errno(errp, -EINVAL,
+            error_setg_errno(errp, EINVAL,
                              "Only %d FDs can be sent, got %zu",
                              SOCKET_MAX_FDS, nfds);
             return -1;
-- 
2.5.0

[Qemu-devel] [PULL v1 2/5] io: increment counter when killing off subcommand

[Daniel P. Berrange]

When killing the subcommand, it is intended to first send
SIGTERM, then SIGKILL and only report an error if it still
doesn't die after SIGKILL. The 'step' counter was not
being incremented though, so the code never got past the
SIGTERM stage.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 io/channel-command.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/io/channel-command.c b/io/channel-command.c
index 598fdab..a220fe8 100644
--- a/io/channel-command.c
+++ b/io/channel-command.c
@@ -179,6 +179,7 @@ static int qio_channel_command_abort(QIOChannelCommand *ioc,
                        (unsigned long long)ioc->pid);
             return -1;
         }
+        step++;
         usleep(10 * 1000);
         goto rewait;
     }
-- 
2.5.0

[Qemu-devel] [PULL v1 3/5] io: some fixes to handling of /dev/null when running commands

[Daniel P. Berrange]

The /dev/null file handle was leaked in a couple of places.
There is also the possibility that both readfd and writefd
point to the same /dev/null file handle, so care must be
taken not to close the same file handle twice.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 io/channel-command.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/io/channel-command.c b/io/channel-command.c
index a220fe8..a9c67aa 100644
--- a/io/channel-command.c
+++ b/io/channel-command.c
@@ -66,7 +66,7 @@ qio_channel_command_new_spawn(const char *const argv[],
 
     if (stdinnull || stdoutnull) {
         devnull = open("/dev/null", O_RDWR);
-        if (!devnull) {
+        if (devnull < 0) {
             error_setg_errno(errp, errno,
                              "Unable to open /dev/null");
             goto error;
@@ -98,6 +98,9 @@ qio_channel_command_new_spawn(const char *const argv[],
             close(stdoutfd[0]);
             close(stdoutfd[1]);
         }
+        if (devnull != -1) {
+            close(devnull);
+        }
 
         execv(argv[0], (char * const *)argv);
         _exit(1);
@@ -117,6 +120,9 @@ qio_channel_command_new_spawn(const char *const argv[],
     return ioc;
 
  error:
+    if (devnull != -1) {
+        close(devnull);
+    }
     if (stdinfd[0] != -1) {
         close(stdinfd[0]);
     }
@@ -202,12 +208,12 @@ static void qio_channel_command_finalize(Object *obj)
     QIOChannelCommand *ioc = QIO_CHANNEL_COMMAND(obj);
     if (ioc->readfd != -1) {
         close(ioc->readfd);
-        ioc->readfd = -1;
     }
-    if (ioc->writefd != -1) {
+    if (ioc->writefd != -1 &&
+        ioc->writefd != ioc->readfd) {
         close(ioc->writefd);
-        ioc->writefd = -1;
     }
+    ioc->writefd = ioc->readfd = -1;
     if (ioc->pid > 0) {
 #ifndef WIN32
         qio_channel_command_abort(ioc, NULL);
@@ -299,12 +305,16 @@ static int qio_channel_command_close(QIOChannel *ioc,
     /* We close FDs before killing, because that
      * gives a better chance of clean shutdown
      */
-    if (close(cioc->writefd) < 0) {
+    if (cioc->readfd != -1 &&
+        close(cioc->readfd) < 0) {
         rv = -1;
     }
-    if (close(cioc->readfd) < 0) {
+    if (cioc->writefd != -1 &&
+        cioc->writefd != cioc->readfd &&
+        close(cioc->writefd) < 0) {
         rv = -1;
     }
+    cioc->writefd = cioc->readfd = -1;
 #ifndef WIN32
     if (qio_channel_command_abort(cioc, errp) < 0) {
         return -1;
-- 
2.5.0

[Qemu-devel] [PULL v1 4/5] io: fix description of @errp parameter initialization

[Daniel P. Berrange]

The "Error **errp" parameters must be NULL initialized
not uninitialized.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 include/io/channel-command.h |  4 ++--
 include/io/channel-socket.h  | 14 +++++++-------
 include/io/channel-tls.h     |  4 ++--
 include/io/channel.h         | 20 ++++++++++----------
 4 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/include/io/channel-command.h b/include/io/channel-command.h
index bd3c599..cfc177e 100644
--- a/include/io/channel-command.h
+++ b/include/io/channel-command.h
@@ -51,7 +51,7 @@ struct QIOChannelCommand {
  * @writefd: the FD connected to the command's stdin
  * @readfd: the FD connected to the command's stdout
  * @pid: the PID of the running child command
- * @errp: pointer to an uninitialized error object
+ * @errp: pointer to a NULL-initialized error object
  *
  * Create a channel for performing I/O with the
  * previously spawned command identified by @pid.
@@ -75,7 +75,7 @@ qio_channel_command_new_pid(int writefd,
  * qio_channel_command_new_spawn:
  * @argv: the NULL terminated list of command arguments
  * @flags: the I/O mode, one of O_RDONLY, O_WRONLY, O_RDWR
- * @errp: pointer to an uninitialized error object
+ * @errp: pointer to a NULL-initialized error object
  *
  * Create a channel for performing I/O with the
  * command to be spawned with arguments @argv.
diff --git a/include/io/channel-socket.h b/include/io/channel-socket.h
index 0719757..810537e 100644
--- a/include/io/channel-socket.h
+++ b/include/io/channel-socket.h
@@ -67,7 +67,7 @@ qio_channel_socket_new(void);
 /**
  * qio_channel_socket_new_fd:
  * @fd: the socket file descriptor
- * @errp: pointer to an uninitialized error object
+ * @errp: pointer to a NULL-initialized error object
  *
  * Create a channel for performing I/O on the socket
  * connection represented by the file descriptor @fd.
@@ -83,7 +83,7 @@ qio_channel_socket_new_fd(int fd,
  * qio_channel_socket_connect_sync:
  * @ioc: the socket channel object
  * @addr: the address to connect to
- * @errp: pointer to an uninitialized error object
+ * @errp: pointer to a NULL-initialized error object
  *
  * Attempt to connect to the address @addr. This method
  * will run in the foreground so the caller will not regain
@@ -118,7 +118,7 @@ void qio_channel_socket_connect_async(QIOChannelSocket *ioc,
  * qio_channel_socket_listen_sync:
  * @ioc: the socket channel object
  * @addr: the address to listen to
- * @errp: pointer to an uninitialized error object
+ * @errp: pointer to a NULL-initialized error object
  *
  * Attempt to listen to the address @addr. This method
  * will run in the foreground so the caller will not regain
@@ -154,7 +154,7 @@ void qio_channel_socket_listen_async(QIOChannelSocket *ioc,
  * @ioc: the socket channel object
  * @localAddr: the address to local bind address
  * @remoteAddr: the address to remote peer address
- * @errp: pointer to an uninitialized error object
+ * @errp: pointer to a NULL-initialized error object
  *
  * Attempt to initialize a datagram socket bound to
  * @localAddr and communicating with peer @remoteAddr.
@@ -193,7 +193,7 @@ void qio_channel_socket_dgram_async(QIOChannelSocket *ioc,
 /**
  * qio_channel_socket_get_local_address:
  * @ioc: the socket channel object
- * @errp: pointer to an uninitialized error object
+ * @errp: pointer to a NULL-initialized error object
  *
  * Get the string representation of the local socket
  * address. A pointer to the allocated address information
@@ -210,7 +210,7 @@ qio_channel_socket_get_local_address(QIOChannelSocket *ioc,
 /**
  * qio_channel_socket_get_remote_address:
  * @ioc: the socket channel object
- * @errp: pointer to an uninitialized error object
+ * @errp: pointer to a NULL-initialized error object
  *
  * Get the string representation of the local socket
  * address. A pointer to the allocated address information
@@ -228,7 +228,7 @@ qio_channel_socket_get_remote_address(QIOChannelSocket *ioc,
 /**
  * qio_channel_socket_accept:
  * @ioc: the socket channel object
- * @errp: pointer to an uninitialized error object
+ * @errp: pointer to a NULL-initialized error object
  *
  * If the socket represents a server, then this accepts
  * a new client connection. The returned channel will
diff --git a/include/io/channel-tls.h b/include/io/channel-tls.h
index 0298b17..322eccb 100644
--- a/include/io/channel-tls.h
+++ b/include/io/channel-tls.h
@@ -55,7 +55,7 @@ struct QIOChannelTLS {
  * @master: the underlying channel object
  * @creds: the credentials to use for TLS handshake
  * @aclname: the access control list for validating clients
- * @errp: pointer to an uninitialized error object
+ * @errp: pointer to a NULL-initialized error object
  *
  * Create a new TLS channel that runs the server side of
  * a TLS session. The TLS session handshake will use the
@@ -85,7 +85,7 @@ qio_channel_tls_new_server(QIOChannel *master,
  * @master: the underlying channel object
  * @creds: the credentials to use for TLS handshake
  * @hostname: the user specified server hostname
- * @errp: pointer to an uninitialized error object
+ * @errp: pointer to a NULL-initialized error object
  *
  * Create a new TLS channel that runs the client side of
  * a TLS session. The TLS session handshake will use the
diff --git a/include/io/channel.h b/include/io/channel.h
index 4ecec98..3e17fe7 100644
--- a/include/io/channel.h
+++ b/include/io/channel.h
@@ -152,7 +152,7 @@ bool qio_channel_has_feature(QIOChannel *ioc,
  * @niov: the length of the @iov array
  * @fds: pointer to an array that will received file handles
  * @nfds: pointer filled with number of elements in @fds on return
- * @errp: pointer to an uninitialized error object
+ * @errp: pointer to a NULL-initialized error object
  *
  * Read data from the IO channel, storing it in the
  * memory regions referenced by @iov. Each element
@@ -198,7 +198,7 @@ ssize_t qio_channel_readv_full(QIOChannel *ioc,
  * @niov: the length of the @iov array
  * @fds: an array of file handles to send
  * @nfds: number of file handles in @fds
- * @errp: pointer to an uninitialized error object
+ * @errp: pointer to a NULL-initialized error object
  *
  * Write data to the IO channel, reading it from the
  * memory regions referenced by @iov. Each element
@@ -237,7 +237,7 @@ ssize_t qio_channel_writev_full(QIOChannel *ioc,
  * @ioc: the channel object
  * @iov: the array of memory regions to read data into
  * @niov: the length of the @iov array
- * @errp: pointer to an uninitialized error object
+ * @errp: pointer to a NULL-initialized error object
  *
  * Behaves as qio_channel_readv_full() but does not support
  * receiving of file handles.
@@ -252,7 +252,7 @@ ssize_t qio_channel_readv(QIOChannel *ioc,
  * @ioc: the channel object
  * @iov: the array of memory regions to write data from
  * @niov: the length of the @iov array
- * @errp: pointer to an uninitialized error object
+ * @errp: pointer to a NULL-initialized error object
  *
  * Behaves as qio_channel_writev_full() but does not support
  * sending of file handles.
@@ -267,7 +267,7 @@ ssize_t qio_channel_writev(QIOChannel *ioc,
  * @ioc: the channel object
  * @buf: the memory region to read data into
  * @buflen: the length of @buf
- * @errp: pointer to an uninitialized error object
+ * @errp: pointer to a NULL-initialized error object
  *
  * Behaves as qio_channel_readv_full() but does not support
  * receiving of file handles, and only supports reading into
@@ -283,7 +283,7 @@ ssize_t qio_channel_read(QIOChannel *ioc,
  * @ioc: the channel object
  * @buf: the memory regions to send data from
  * @buflen: the length of @buf
- * @errp: pointer to an uninitialized error object
+ * @errp: pointer to a NULL-initialized error object
  *
  * Behaves as qio_channel_writev_full() but does not support
  * sending of file handles, and only supports writing from a
@@ -298,7 +298,7 @@ ssize_t qio_channel_write(QIOChannel *ioc,
  * qio_channel_set_blocking:
  * @ioc: the channel object
  * @enabled: the blocking flag state
- * @errp: pointer to an uninitialized error object
+ * @errp: pointer to a NULL-initialized error object
  *
  * If @enabled is true, then the channel is put into
  * blocking mode, otherwise it will be non-blocking.
@@ -314,7 +314,7 @@ int qio_channel_set_blocking(QIOChannel *ioc,
 /**
  * qio_channel_close:
  * @ioc: the channel object
- * @errp: pointer to an uninitialized error object
+ * @errp: pointer to a NULL-initialized error object
  *
  * Close the channel, flushing any pending I/O
  *
@@ -327,7 +327,7 @@ int qio_channel_close(QIOChannel *ioc,
  * qio_channel_shutdown:
  * @ioc: the channel object
  * @how: the direction to shutdown
- * @errp: pointer to an uninitialized error object
+ * @errp: pointer to a NULL-initialized error object
  *
  * Shutdowns transmission and/or receiving of data
  * without closing the underlying transport.
@@ -403,7 +403,7 @@ void qio_channel_set_cork(QIOChannel *ioc,
  * @ioc: the channel object
  * @offset: the position to seek to, relative to @whence
  * @whence: one of the (POSIX) SEEK_* constants listed below
- * @errp: pointer to an uninitialized error object
+ * @errp: pointer to a NULL-initialized error object
  *
  * Moves the current I/O position within the channel
  * @ioc, to be @offset. The value of @offset is
-- 
2.5.0

[Qemu-devel] [PULL v1 5/5] io: use memset instead of { 0 } for initializing array

[Daniel P. Berrange]

Some versions of GCC on OS-X complain about CMSG_SPACE
not being constant size, which prevents use of { 0 }

io/channel-socket.c: In function 'qio_channel_socket_writev':
io/channel-socket.c:497:18: error: variable-sized object may not be initialized
     char control[CMSG_SPACE(sizeof(int) * SOCKET_MAX_FDS)] = { 0 };

The compiler is at fault here, but it is nicer to avoid
tickling this compiler bug by using memset instead.

Reviewed-By: John Arbuckle <programmingkidx@gmail.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 io/channel-socket.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/io/channel-socket.c b/io/channel-socket.c
index eaa411f..bc117b1 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -449,6 +449,8 @@ static ssize_t qio_channel_socket_readv(QIOChannel *ioc,
     char control[CMSG_SPACE(sizeof(int) * SOCKET_MAX_FDS)];
     int sflags = 0;
 
+    memset(control, 0, CMSG_SPACE(sizeof(int) * SOCKET_MAX_FDS));
+
 #ifdef MSG_CMSG_CLOEXEC
     sflags |= MSG_CMSG_CLOEXEC;
 #endif
@@ -493,10 +495,12 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc,
     QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(ioc);
     ssize_t ret;
     struct msghdr msg = { NULL, };
-    char control[CMSG_SPACE(sizeof(int) * SOCKET_MAX_FDS)] = { 0 };
+    char control[CMSG_SPACE(sizeof(int) * SOCKET_MAX_FDS)];
     size_t fdsize = sizeof(int) * nfds;
     struct cmsghdr *cmsg;
 
+    memset(control, 0, CMSG_SPACE(sizeof(int) * SOCKET_MAX_FDS));
+
     msg.msg_iov = (struct iovec *)iov;
     msg.msg_iovlen = niov;
 
-- 
2.5.0

Re: [Qemu-devel] [PULL v1 0/5] I/O channels fixes

[Peter Maydell]

On 20 January 2016 at 12:01, Daniel P. Berrange <berrange@redhat.com> wrote:
> The following changes since commit 3db34bf64ab4f8797565dd8750003156c32b301d:
>
>   Merge remote-tracking branch 'remotes/afaerber/tags/qom-devices-for-peter' into staging (2016-01-18 17:40:50 +0000)
>
> are available in the git repository at:
>
>   git://github.com/berrange/qemu tags/pull-io-next-2016-01-20-1
>
> for you to fetch changes up to ccf1e2dcd6091eea1fc2341c63201aa1a6094978:
>
>   io: use memset instead of { 0 } for initializing array (2016-01-20 11:31:01 +0000)
>
> ----------------------------------------------------------------
> I/O channels fixes 2016/01/20 v1
>
> ----------------------------------------------------------------
> Daniel P. Berrange (5):
>       io: fix sign of errno value passed to error report
>       io: increment counter when killing off subcommand
>       io: some fixes to handling of /dev/null when running commands
>       io: fix description of @errp parameter initialization
>       io: use memset instead of { 0 } for initializing array

Applied, thanks.

-- PMM