[Qemu-devel] [PATCH 00/14] Trivial cleanups around error reporting

[Qemu-devel] [PATCH 00/14] Trivial cleanups around error reporting

[Fam Zheng]

These are patches to:

1) reorder the function parameters so that Error **errp comes
last.

Error pointer in the middle of a function parameter list is very uncommon, and
does caused mistakes, thus is not a good style. Change to the usual way.

2) apply the error_propagate_null.cocci semantics patch again.

Fam Zheng (14):
  socket: Make errp the last parameter of socket_connect
  socket: Make errp the last parameter of inet_connect_saddr
  socket: Make errp the last parameter of unix_connect_saddr
  socket: Make errp the last parameter of vsock_connect_saddr
  block: Make errp the last parameter of bdrv_img_create
  crypto: Make errp the last parameter of functions
  mirror: Make errp the last parameter of mirror_start_job
  block: Make errp the last parameter of commit_active_start
  nfs: Make errp the last parameter of nfs_client_open
  fdc: Make errp the last parameter of fdctrl_connect_drives
  scsi: Make errp the last parameter of virtio_scsi_common_realize
  migration: Make errp the last parameter of local functions
  qga: Make errp the last parameter of qga_vss_fsfreeze
  error: Apply error_propagate_null.cocci again

 block.c                         |  4 ++--
 block/crypto.c                  | 12 ++++++------
 block/mirror.c                  | 17 +++++++++--------
 block/nfs.c                     |  6 +++---
 block/replication.c             |  2 +-
 block/sheepdog.c                |  2 +-
 block/ssh.c                     |  2 +-
 blockdev.c                      | 12 ++++++------
 crypto/block-luks.c             | 21 +++++++++------------
 hw/block/fdc.c                  |  6 +++---
 hw/i386/pc.c                    |  4 +---
 hw/s390x/virtio-ccw.c           |  4 +---
 hw/scsi/vhost-scsi.c            |  6 ++++--
 hw/scsi/virtio-scsi.c           | 11 +++++++----
 hw/usb/bus.c                    |  4 +---
 include/block/block.h           |  2 +-
 include/block/block_int.h       |  6 +++---
 include/crypto/block.h          | 12 ++++++------
 include/hw/virtio/virtio-scsi.h |  8 +++++---
 include/qemu/sockets.h          |  9 +++++----
 io/channel-socket.c             |  2 +-
 migration/rdma.c                | 12 ++++++------
 net/socket.c                    |  2 +-
 qemu-img.c                      |  4 ++--
 qga/commands-win32.c            |  4 ++--
 qga/vss-win32.c                 |  2 +-
 qga/vss-win32.h                 |  2 +-
 tests/test-crypto-block.c       | 12 ++++++------
 tests/test-replication.c        |  8 ++++----
 util/qemu-sockets.c             | 38 +++++++++++++++++++++-----------------
 30 files changed, 120 insertions(+), 116 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH 01/14] socket: Make errp the last parameter of socket_connect

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/sheepdog.c       | 2 +-
 include/qemu/sockets.h | 4 ++--
 io/channel-socket.c    | 2 +-
 net/socket.c           | 2 +-
 util/qemu-sockets.c    | 4 ++--
 5 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index fb9203e..b2a5998 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -595,7 +595,7 @@ static int connect_to_sdog(BDRVSheepdogState *s, Error **errp)
 {
     int fd;
 
-    fd = socket_connect(s->addr, errp, NULL, NULL);
+    fd = socket_connect(s->addr, NULL, NULL, errp);
 
     if (s->addr->type == SOCKET_ADDRESS_KIND_INET && fd >= 0) {
         int ret = socket_set_nodelay(fd);
diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index 7842f6d..567eef1 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -45,8 +45,8 @@ int unix_listen(const char *path, char *ostr, int olen, Error **errp);
 int unix_connect(const char *path, Error **errp);
 
 SocketAddress *socket_parse(const char *str, Error **errp);
-int socket_connect(SocketAddress *addr, Error **errp,
-                   NonBlockingConnectHandler *callback, void *opaque);
+int socket_connect(SocketAddress *addr, NonBlockingConnectHandler *callback,
+                   void *opaque, Error **errp);
 int socket_listen(SocketAddress *addr, Error **errp);
 void socket_listen_cleanup(int fd, Error **errp);
 int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp);
diff --git a/io/channel-socket.c b/io/channel-socket.c
index 64b36f5..53386b7 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -140,7 +140,7 @@ int qio_channel_socket_connect_sync(QIOChannelSocket *ioc,
     int fd;
 
     trace_qio_channel_socket_connect_sync(ioc, addr);
-    fd = socket_connect(addr, errp, NULL, NULL);
+    fd = socket_connect(addr, NULL, NULL, errp);
     if (fd < 0) {
         trace_qio_channel_socket_connect_fail(ioc);
         return -1;
diff --git a/net/socket.c b/net/socket.c
index fe3547b..b8c931e 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -578,7 +578,7 @@ static int net_socket_connect_init(NetClientState *peer,
         goto err;
     }
 
-    fd = socket_connect(c->saddr, &local_error, net_socket_connected, c);
+    fd = socket_connect(c->saddr, net_socket_connected, c, &local_error);
     if (fd < 0) {
         goto err;
     }
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 21442c3..f79d334 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -1074,8 +1074,8 @@ fail:
     return NULL;
 }
 
-int socket_connect(SocketAddress *addr, Error **errp,
-                   NonBlockingConnectHandler *callback, void *opaque)
+int socket_connect(SocketAddress *addr, NonBlockingConnectHandler *callback,
+                   void *opaque, Error **errp)
 {
     int fd;
 
-- 
2.9.3

[Qemu-devel] [PATCH 02/14] socket: Make errp the last parameter of inet_connect_saddr

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/ssh.c            | 2 +-
 include/qemu/sockets.h | 5 +++--
 util/qemu-sockets.c    | 9 +++++----
 3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/block/ssh.c b/block/ssh.c
index 471ba8a..df09f6c 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -681,7 +681,7 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
     }
 
     /* Open the socket and connect. */
-    s->sock = inet_connect_saddr(s->inet, errp, NULL, NULL);
+    s->sock = inet_connect_saddr(s->inet, NULL, NULL, errp);
     if (s->sock < 0) {
         ret = -EIO;
         goto err;
diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index 567eef1..af28532 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -36,8 +36,9 @@ int inet_ai_family_from_address(InetSocketAddress *addr,
                                 Error **errp);
 InetSocketAddress *inet_parse(const char *str, Error **errp);
 int inet_connect(const char *str, Error **errp);
-int inet_connect_saddr(InetSocketAddress *saddr, Error **errp,
-                       NonBlockingConnectHandler *callback, void *opaque);
+int inet_connect_saddr(InetSocketAddress *saddr,
+                       NonBlockingConnectHandler *callback, void *opaque,
+                       Error **errp);
 
 NetworkAddressFamily inet_netfamily(int family);
 
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index f79d334..8e11349 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -427,8 +427,9 @@ static struct addrinfo *inet_parse_connect_saddr(InetSocketAddress *saddr,
  * function succeeds, callback will be called when the connection
  * completes, with the file descriptor on success, or -1 on error.
  */
-int inet_connect_saddr(InetSocketAddress *saddr, Error **errp,
-                       NonBlockingConnectHandler *callback, void *opaque)
+int inet_connect_saddr(InetSocketAddress *saddr,
+                       NonBlockingConnectHandler *callback, void *opaque,
+                       Error **errp)
 {
     Error *local_err = NULL;
     struct addrinfo *res, *e;
@@ -659,7 +660,7 @@ int inet_connect(const char *str, Error **errp)
 
     addr = inet_parse(str, errp);
     if (addr != NULL) {
-        sock = inet_connect_saddr(addr, errp, NULL, NULL);
+        sock = inet_connect_saddr(addr, NULL, NULL, errp);
         qapi_free_InetSocketAddress(addr);
     }
     return sock;
@@ -1081,7 +1082,7 @@ int socket_connect(SocketAddress *addr, NonBlockingConnectHandler *callback,
 
     switch (addr->type) {
     case SOCKET_ADDRESS_KIND_INET:
-        fd = inet_connect_saddr(addr->u.inet.data, errp, callback, opaque);
+        fd = inet_connect_saddr(addr->u.inet.data, callback, opaque, errp);
         break;
 
     case SOCKET_ADDRESS_KIND_UNIX:
-- 
2.9.3

[Qemu-devel] [PATCH 03/14] socket: Make errp the last parameter of unix_connect_saddr

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 util/qemu-sockets.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 8e11349..0fe5f13 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -911,8 +911,9 @@ err:
     return -1;
 }
 
-static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp,
-                              NonBlockingConnectHandler *callback, void *opaque)
+static int unix_connect_saddr(UnixSocketAddress *saddr,
+                              NonBlockingConnectHandler *callback, void *opaque,
+                              Error **errp)
 {
     struct sockaddr_un un;
     ConnectState *connect_state = NULL;
@@ -979,8 +980,9 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
     return -1;
 }
 
-static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp,
-                              NonBlockingConnectHandler *callback, void *opaque)
+static int unix_connect_saddr(UnixSocketAddress *saddr,
+                              NonBlockingConnectHandler *callback, void *opaque,
+                              Error **errp)
 {
     error_setg(errp, "unix sockets are not available on windows");
     errno = ENOTSUP;
@@ -1026,7 +1028,7 @@ int unix_connect(const char *path, Error **errp)
 
     saddr = g_new0(UnixSocketAddress, 1);
     saddr->path = g_strdup(path);
-    sock = unix_connect_saddr(saddr, errp, NULL, NULL);
+    sock = unix_connect_saddr(saddr, NULL, NULL, errp);
     qapi_free_UnixSocketAddress(saddr);
     return sock;
 }
@@ -1086,7 +1088,7 @@ int socket_connect(SocketAddress *addr, NonBlockingConnectHandler *callback,
         break;
 
     case SOCKET_ADDRESS_KIND_UNIX:
-        fd = unix_connect_saddr(addr->u.q_unix.data, errp, callback, opaque);
+        fd = unix_connect_saddr(addr->u.q_unix.data, callback, opaque, errp);
         break;
 
     case SOCKET_ADDRESS_KIND_FD:
-- 
2.9.3

[Qemu-devel] [PATCH 04/14] socket: Make errp the last parameter of vsock_connect_saddr

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 util/qemu-sockets.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 0fe5f13..8188d9a 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -728,9 +728,10 @@ static int vsock_connect_addr(const struct sockaddr_vm *svm, bool *in_progress,
     return sock;
 }
 
-static int vsock_connect_saddr(VsockSocketAddress *vaddr, Error **errp,
+static int vsock_connect_saddr(VsockSocketAddress *vaddr,
                                NonBlockingConnectHandler *callback,
-                               void *opaque)
+                               void *opaque,
+                               Error **errp)
 {
     struct sockaddr_vm svm;
     int sock = -1;
@@ -819,9 +820,9 @@ static void vsock_unsupported(Error **errp)
     error_setg(errp, "socket family AF_VSOCK unsupported");
 }
 
-static int vsock_connect_saddr(VsockSocketAddress *vaddr, Error **errp,
+static int vsock_connect_saddr(VsockSocketAddress *vaddr,
                                NonBlockingConnectHandler *callback,
-                               void *opaque)
+                               void *opaque, Error **errp)
 {
     vsock_unsupported(errp);
     return -1;
@@ -1100,7 +1101,7 @@ int socket_connect(SocketAddress *addr, NonBlockingConnectHandler *callback,
         break;
 
     case SOCKET_ADDRESS_KIND_VSOCK:
-        fd = vsock_connect_saddr(addr->u.vsock.data, errp, callback, opaque);
+        fd = vsock_connect_saddr(addr->u.vsock.data, callback, opaque, errp);
         break;
 
     default:
-- 
2.9.3

[Qemu-devel] [PATCH 05/14] block: Make errp the last parameter of bdrv_img_create

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c                  |  4 ++--
 blockdev.c               | 10 +++++-----
 include/block/block.h    |  2 +-
 qemu-img.c               |  2 +-
 tests/test-replication.c |  8 ++++----
 5 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/block.c b/block.c
index 1fbbb8d..7eda9a4 100644
--- a/block.c
+++ b/block.c
@@ -4161,8 +4161,8 @@ bool bdrv_op_blocker_is_empty(BlockDriverState *bs)
 
 void bdrv_img_create(const char *filename, const char *fmt,
                      const char *base_filename, const char *base_fmt,
-                     char *options, uint64_t img_size, int flags,
-                     Error **errp, bool quiet)
+                     char *options, uint64_t img_size, int flags, bool quiet,
+                     Error **errp)
 {
     QemuOptsList *create_opts = NULL;
     QemuOpts *opts = NULL;
diff --git a/blockdev.c b/blockdev.c
index 4927914..94726ed 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1728,7 +1728,7 @@ static void external_snapshot_prepare(BlkActionState *common,
             bdrv_img_create(new_image_file, format,
                             state->old_bs->filename,
                             state->old_bs->drv->format_name,
-                            NULL, size, flags, &local_err, false);
+                            NULL, size, flags, false, &local_err);
             if (local_err) {
                 error_propagate(errp, local_err);
                 return;
@@ -3237,10 +3237,10 @@ static BlockJob *do_drive_backup(DriveBackup *backup, BlockJobTxn *txn,
         if (source) {
             bdrv_img_create(backup->target, backup->format, source->filename,
                             source->drv->format_name, NULL,
-                            size, flags, &local_err, false);
+                            size, flags, false, &local_err);
         } else {
             bdrv_img_create(backup->target, backup->format, NULL, NULL, NULL,
-                            size, flags, &local_err, false);
+                            size, flags, false, &local_err);
         }
     }
 
@@ -3531,7 +3531,7 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
         /* create new image w/o backing file */
         assert(format);
         bdrv_img_create(arg->target, format,
-                        NULL, NULL, NULL, size, flags, &local_err, false);
+                        NULL, NULL, NULL, size, flags, false, &local_err);
     } else {
         switch (arg->mode) {
         case NEW_IMAGE_MODE_EXISTING:
@@ -3541,7 +3541,7 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
             bdrv_img_create(arg->target, format,
                             source->filename,
                             source->drv->format_name,
-                            NULL, size, flags, &local_err, false);
+                            NULL, size, flags, false, &local_err);
             break;
         default:
             abort();
diff --git a/include/block/block.h b/include/block/block.h
index 5ddc0cf..466de49 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -509,7 +509,7 @@ int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf,
 void bdrv_img_create(const char *filename, const char *fmt,
                      const char *base_filename, const char *base_fmt,
                      char *options, uint64_t img_size, int flags,
-                     Error **errp, bool quiet);
+                     bool quiet, Error **errp);
 
 /* Returns the alignment in bytes that is required so that no bounce buffer
  * is required throughout the stack */
diff --git a/qemu-img.c b/qemu-img.c
index b220cf7..2c09053 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -516,7 +516,7 @@ static int img_create(int argc, char **argv)
     }
 
     bdrv_img_create(filename, fmt, base_filename, base_fmt,
-                    options, img_size, 0, &local_err, quiet);
+                    options, img_size, 0, quiet, &local_err);
     if (local_err) {
         error_reportf_err(local_err, "%s: ", filename);
         goto fail;
diff --git a/tests/test-replication.c b/tests/test-replication.c
index fac2da3..3016c6f 100644
--- a/tests/test-replication.c
+++ b/tests/test-replication.c
@@ -144,18 +144,18 @@ static void prepare_imgs(void)
 
     /* Primary */
     bdrv_img_create(p_local_disk, "qcow2", NULL, NULL, NULL, IMG_SIZE,
-                    BDRV_O_RDWR, &local_err, true);
+                    BDRV_O_RDWR, true, &local_err);
     g_assert(!local_err);
 
     /* Secondary */
     bdrv_img_create(s_local_disk, "qcow2", NULL, NULL, NULL, IMG_SIZE,
-                    BDRV_O_RDWR, &local_err, true);
+                    BDRV_O_RDWR, true, &local_err);
     g_assert(!local_err);
     bdrv_img_create(s_active_disk, "qcow2", NULL, NULL, NULL, IMG_SIZE,
-                    BDRV_O_RDWR, &local_err, true);
+                    BDRV_O_RDWR, true, &local_err);
     g_assert(!local_err);
     bdrv_img_create(s_hidden_disk, "qcow2", NULL, NULL, NULL, IMG_SIZE,
-                    BDRV_O_RDWR, &local_err, true);
+                    BDRV_O_RDWR, true, &local_err);
     g_assert(!local_err);
 }
 
-- 
2.9.3

[Qemu-devel] [PATCH 06/14] crypto: Make errp the last parameter of functions

[Fam Zheng]

Move opaque to 2nd instead of the 2nd to last, so that compilers help
check with the convertion.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/crypto.c            | 12 ++++++------
 crypto/block-luks.c       | 21 +++++++++------------
 include/crypto/block.h    | 12 ++++++------
 tests/test-crypto-block.c | 12 ++++++------
 4 files changed, 27 insertions(+), 30 deletions(-)

diff --git a/block/crypto.c b/block/crypto.c
index 4a20388..34549b2 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -56,11 +56,11 @@ static int block_crypto_probe_generic(QCryptoBlockFormat format,
 
 
 static ssize_t block_crypto_read_func(QCryptoBlock *block,
+                                      void *opaque,
                                       size_t offset,
                                       uint8_t *buf,
                                       size_t buflen,
-                                      Error **errp,
-                                      void *opaque)
+                                      Error **errp)
 {
     BlockDriverState *bs = opaque;
     ssize_t ret;
@@ -83,11 +83,11 @@ struct BlockCryptoCreateData {
 
 
 static ssize_t block_crypto_write_func(QCryptoBlock *block,
+                                       void *opaque,
                                        size_t offset,
                                        const uint8_t *buf,
                                        size_t buflen,
-                                       Error **errp,
-                                       void *opaque)
+                                       Error **errp)
 {
     struct BlockCryptoCreateData *data = opaque;
     ssize_t ret;
@@ -102,9 +102,9 @@ static ssize_t block_crypto_write_func(QCryptoBlock *block,
 
 
 static ssize_t block_crypto_init_func(QCryptoBlock *block,
+                                      void *opaque,
                                       size_t headerlen,
-                                      Error **errp,
-                                      void *opaque)
+                                      Error **errp)
 {
     struct BlockCryptoCreateData *data = opaque;
     int ret;
diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index 4530f82..d5a31bb 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -473,10 +473,10 @@ qcrypto_block_luks_load_key(QCryptoBlock *block,
      * then encrypted.
      */
     rv = readfunc(block,
+                  opaque,
                   slot->key_offset * QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
                   splitkey, splitkeylen,
-                  errp,
-                  opaque);
+                  errp);
     if (rv < 0) {
         goto cleanup;
     }
@@ -676,11 +676,10 @@ qcrypto_block_luks_open(QCryptoBlock *block,
 
     /* Read the entire LUKS header, minus the key material from
      * the underlying device */
-    rv = readfunc(block, 0,
+    rv = readfunc(block, opaque, 0,
                   (uint8_t *)&luks->header,
                   sizeof(luks->header),
-                  errp,
-                  opaque);
+                  errp);
     if (rv < 0) {
         ret = rv;
         goto fail;
@@ -1246,7 +1245,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,
         QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;
 
     /* Reserve header space to match payload offset */
-    initfunc(block, block->payload_offset, &local_err, opaque);
+    initfunc(block, opaque, block->payload_offset, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         goto error;
@@ -1268,11 +1267,10 @@ qcrypto_block_luks_create(QCryptoBlock *block,
 
 
     /* Write out the partition header and key slot headers */
-    writefunc(block, 0,
+    writefunc(block, opaque, 0,
               (const uint8_t *)&luks->header,
               sizeof(luks->header),
-              &local_err,
-              opaque);
+              &local_err);
 
     /* Delay checking local_err until we've byte-swapped */
 
@@ -1297,12 +1295,11 @@ qcrypto_block_luks_create(QCryptoBlock *block,
 
     /* Write out the master key material, starting at the
      * sector immediately following the partition header. */
-    if (writefunc(block,
+    if (writefunc(block, opaque,
                   luks->header.key_slots[0].key_offset *
                   QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
                   splitkey, splitkeylen,
-                  errp,
-                  opaque) != splitkeylen) {
+                  errp) != splitkeylen) {
         goto error;
     }
 
diff --git a/include/crypto/block.h b/include/crypto/block.h
index b6971de..4a053a3 100644
--- a/include/crypto/block.h
+++ b/include/crypto/block.h
@@ -30,23 +30,23 @@ typedef struct QCryptoBlock QCryptoBlock;
  * and QCryptoBlockOpenOptions in qapi/crypto.json */
 
 typedef ssize_t (*QCryptoBlockReadFunc)(QCryptoBlock *block,
+                                        void *opaque,
                                         size_t offset,
                                         uint8_t *buf,
                                         size_t buflen,
-                                        Error **errp,
-                                        void *opaque);
+                                        Error **errp);
 
 typedef ssize_t (*QCryptoBlockInitFunc)(QCryptoBlock *block,
+                                        void *opaque,
                                         size_t headerlen,
-                                        Error **errp,
-                                        void *opaque);
+                                        Error **errp);
 
 typedef ssize_t (*QCryptoBlockWriteFunc)(QCryptoBlock *block,
+                                         void *opaque,
                                          size_t offset,
                                          const uint8_t *buf,
                                          size_t buflen,
-                                         Error **errp,
-                                         void *opaque);
+                                         Error **errp);
 
 /**
  * qcrypto_block_has_format:
diff --git a/tests/test-crypto-block.c b/tests/test-crypto-block.c
index 1957a86..85e6603 100644
--- a/tests/test-crypto-block.c
+++ b/tests/test-crypto-block.c
@@ -187,11 +187,11 @@ static struct QCryptoBlockTestData {
 
 
 static ssize_t test_block_read_func(QCryptoBlock *block,
+                                    void *opaque,
                                     size_t offset,
                                     uint8_t *buf,
                                     size_t buflen,
-                                    Error **errp,
-                                    void *opaque)
+                                    Error **errp)
 {
     Buffer *header = opaque;
 
@@ -204,9 +204,9 @@ static ssize_t test_block_read_func(QCryptoBlock *block,
 
 
 static ssize_t test_block_init_func(QCryptoBlock *block,
+                                    void *opaque,
                                     size_t headerlen,
-                                    Error **errp,
-                                    void *opaque)
+                                    Error **errp)
 {
     Buffer *header = opaque;
 
@@ -219,11 +219,11 @@ static ssize_t test_block_init_func(QCryptoBlock *block,
 
 
 static ssize_t test_block_write_func(QCryptoBlock *block,
+                                     void *opaque,
                                      size_t offset,
                                      const uint8_t *buf,
                                      size_t buflen,
-                                     Error **errp,
-                                     void *opaque)
+                                     Error **errp)
 {
     Buffer *header = opaque;
 
-- 
2.9.3

[Qemu-devel] [PATCH 07/14] mirror: Make errp the last parameter of mirror_start_job

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/mirror.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 164438f..0c1a56c 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1112,10 +1112,11 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
                              BlockdevOnError on_target_error,
                              bool unmap,
                              BlockCompletionFunc *cb,
-                             void *opaque, Error **errp,
+                             void *opaque,
                              const BlockJobDriver *driver,
                              bool is_none_mode, BlockDriverState *base,
-                             bool auto_complete, const char *filter_node_name)
+                             bool auto_complete, const char *filter_node_name,
+                             Error **errp)
 {
     MirrorBlockJob *s;
     BlockDriverState *mirror_top_bs;
@@ -1280,9 +1281,9 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
     base = mode == MIRROR_SYNC_MODE_TOP ? backing_bs(bs) : NULL;
     mirror_start_job(job_id, bs, BLOCK_JOB_DEFAULT, target, replaces,
                      speed, granularity, buf_size, backing_mode,
-                     on_source_error, on_target_error, unmap, NULL, NULL, errp,
+                     on_source_error, on_target_error, unmap, NULL, NULL,
                      &mirror_job_driver, is_none_mode, base, false,
-                     filter_node_name);
+                     filter_node_name, errp);
 }
 
 void commit_active_start(const char *job_id, BlockDriverState *bs,
@@ -1303,9 +1304,9 @@ void commit_active_start(const char *job_id, BlockDriverState *bs,
 
     mirror_start_job(job_id, bs, creation_flags, base, NULL, speed, 0, 0,
                      MIRROR_LEAVE_BACKING_CHAIN,
-                     on_error, on_error, true, cb, opaque, &local_err,
+                     on_error, on_error, true, cb, opaque,
                      &commit_active_job_driver, false, base, auto_complete,
-                     filter_node_name);
+                     filter_node_name, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         goto error_restore_flags;
-- 
2.9.3

[Qemu-devel] [PATCH 08/14] block: Make errp the last parameter of commit_active_start

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/mirror.c            | 4 ++--
 block/replication.c       | 2 +-
 blockdev.c                | 2 +-
 include/block/block_int.h | 6 +++---
 qemu-img.c                | 2 +-
 5 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 0c1a56c..9f5eb69 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1290,8 +1290,8 @@ void commit_active_start(const char *job_id, BlockDriverState *bs,
                          BlockDriverState *base, int creation_flags,
                          int64_t speed, BlockdevOnError on_error,
                          const char *filter_node_name,
-                         BlockCompletionFunc *cb, void *opaque, Error **errp,
-                         bool auto_complete)
+                         BlockCompletionFunc *cb, void *opaque,
+                         bool auto_complete, Error **errp)
 {
     int orig_base_flags;
     Error *local_err = NULL;
diff --git a/block/replication.c b/block/replication.c
index bf3c395..d300c15 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -656,7 +656,7 @@ static void replication_stop(ReplicationState *rs, bool failover, Error **errp)
         s->replication_state = BLOCK_REPLICATION_FAILOVER;
         commit_active_start(NULL, s->active_disk->bs, s->secondary_disk->bs,
                             BLOCK_JOB_INTERNAL, 0, BLOCKDEV_ON_ERROR_REPORT,
-                            NULL, replication_done, bs, errp, true);
+                            NULL, replication_done, bs, true, errp);
         break;
     default:
         aio_context_release(aio_context);
diff --git a/blockdev.c b/blockdev.c
index 94726ed..6428206 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3142,7 +3142,7 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
         }
         commit_active_start(has_job_id ? job_id : NULL, bs, base_bs,
                             BLOCK_JOB_DEFAULT, speed, on_error,
-                            filter_node_name, NULL, NULL, &local_err, false);
+                            filter_node_name, NULL, NULL, false, &local_err);
     } else {
         BlockDriverState *overlay_bs = bdrv_find_overlay(bs, top_bs);
         if (bdrv_op_is_blocked(overlay_bs, BLOCK_OP_TYPE_COMMIT_TARGET, errp)) {
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 59400bd..4f8cd29 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -805,16 +805,16 @@ void commit_start(const char *job_id, BlockDriverState *bs,
  * a node name should be autogenerated.
  * @cb: Completion function for the job.
  * @opaque: Opaque pointer value passed to @cb.
- * @errp: Error object.
  * @auto_complete: Auto complete the job.
+ * @errp: Error object.
  *
  */
 void commit_active_start(const char *job_id, BlockDriverState *bs,
                          BlockDriverState *base, int creation_flags,
                          int64_t speed, BlockdevOnError on_error,
                          const char *filter_node_name,
-                         BlockCompletionFunc *cb, void *opaque, Error **errp,
-                         bool auto_complete);
+                         BlockCompletionFunc *cb, void *opaque,
+                         bool auto_complete, Error **errp);
 /*
  * mirror_start:
  * @job_id: The id of the newly-created job, or %NULL to use the
diff --git a/qemu-img.c b/qemu-img.c
index 2c09053..bbe1574 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -984,7 +984,7 @@ static int img_commit(int argc, char **argv)
     aio_context_acquire(aio_context);
     commit_active_start("commit", bs, base_bs, BLOCK_JOB_DEFAULT, 0,
                         BLOCKDEV_ON_ERROR_REPORT, NULL, common_block_job_cb,
-                        &cbi, &local_err, false);
+                        &cbi, false, &local_err);
     aio_context_release(aio_context);
     if (local_err) {
         goto done;
-- 
2.9.3

[Qemu-devel] [PATCH 09/14] nfs: Make errp the last parameter of nfs_client_open

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/nfs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/nfs.c b/block/nfs.c
index 0816678..6541dec 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -497,7 +497,7 @@ out:
 
 
 static int64_t nfs_client_open(NFSClient *client, QDict *options,
-                               int flags, Error **errp, int open_flags)
+                               int flags, int open_flags, Error **errp)
 {
     int ret = -EINVAL;
     QemuOpts *opts = NULL;
@@ -663,7 +663,7 @@ static int nfs_file_open(BlockDriverState *bs, QDict *options, int flags,
 
     ret = nfs_client_open(client, options,
                           (flags & BDRV_O_RDWR) ? O_RDWR : O_RDONLY,
-                          errp, bs->open_flags);
+                          bs->open_flags, errp);
     if (ret < 0) {
         return ret;
     }
@@ -705,7 +705,7 @@ static int nfs_file_create(const char *url, QemuOpts *opts, Error **errp)
         goto out;
     }
 
-    ret = nfs_client_open(client, options, O_CREAT, errp, 0);
+    ret = nfs_client_open(client, options, O_CREAT, 0, errp);
     if (ret < 0) {
         goto out;
     }
-- 
2.9.3

[Qemu-devel] [PATCH 10/14] fdc: Make errp the last parameter of fdctrl_connect_drives

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/block/fdc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index a328693..2e629b3 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2521,8 +2521,8 @@ static void fdctrl_result_timer(void *opaque)
 }
 
 /* Init functions */
-static void fdctrl_connect_drives(FDCtrl *fdctrl, Error **errp,
-                                  DeviceState *fdc_dev)
+static void fdctrl_connect_drives(FDCtrl *fdctrl, DeviceState *fdc_dev,
+                                  Error **errp)
 {
     unsigned int i;
     FDrive *drive;
@@ -2675,7 +2675,7 @@ static void fdctrl_realize_common(DeviceState *dev, FDCtrl *fdctrl,
     }
 
     floppy_bus_create(fdctrl, &fdctrl->bus, dev);
-    fdctrl_connect_drives(fdctrl, errp, dev);
+    fdctrl_connect_drives(fdctrl, dev, errp);
 }
 
 static const MemoryRegionPortio fdc_portio_list[] = {
-- 
2.9.3

[Qemu-devel] [PATCH 11/14] scsi: Make errp the last parameter of virtio_scsi_common_realize

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/scsi/vhost-scsi.c            |  6 ++++--
 hw/scsi/virtio-scsi.c           | 11 +++++++----
 include/hw/virtio/virtio-scsi.h |  8 +++++---
 3 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index c491ece..f53bc17 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -233,9 +233,11 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
         }
     }
 
-    virtio_scsi_common_realize(dev, &err, vhost_dummy_handle_output,
+    virtio_scsi_common_realize(dev,
                                vhost_dummy_handle_output,
-                               vhost_dummy_handle_output);
+                               vhost_dummy_handle_output,
+                               vhost_dummy_handle_output,
+                               &err);
     if (err != NULL) {
         error_propagate(errp, err);
         goto close_fd;
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index bd62d08..46a3e3f 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -841,10 +841,11 @@ static struct SCSIBusInfo virtio_scsi_scsi_info = {
     .load_request = virtio_scsi_load_request,
 };
 
-void virtio_scsi_common_realize(DeviceState *dev, Error **errp,
+void virtio_scsi_common_realize(DeviceState *dev,
                                 VirtIOHandleOutput ctrl,
                                 VirtIOHandleOutput evt,
-                                VirtIOHandleOutput cmd)
+                                VirtIOHandleOutput cmd,
+                                Error **errp)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(dev);
@@ -878,9 +879,11 @@ static void virtio_scsi_device_realize(DeviceState *dev, Error **errp)
     VirtIOSCSI *s = VIRTIO_SCSI(dev);
     Error *err = NULL;
 
-    virtio_scsi_common_realize(dev, &err, virtio_scsi_handle_ctrl,
+    virtio_scsi_common_realize(dev,
+                               virtio_scsi_handle_ctrl,
                                virtio_scsi_handle_event,
-                               virtio_scsi_handle_cmd);
+                               virtio_scsi_handle_cmd,
+                               &err);
     if (err != NULL) {
         error_propagate(errp, err);
         return;
diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index 8ae0aca..8c8453c 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -135,9 +135,11 @@ static inline void virtio_scsi_release(VirtIOSCSI *s)
     }
 }
 
-void virtio_scsi_common_realize(DeviceState *dev, Error **errp,
-                                VirtIOHandleOutput ctrl, VirtIOHandleOutput evt,
-                                VirtIOHandleOutput cmd);
+void virtio_scsi_common_realize(DeviceState *dev,
+                                VirtIOHandleOutput ctrl,
+                                VirtIOHandleOutput evt,
+                                VirtIOHandleOutput cmd,
+                                Error **errp);
 
 void virtio_scsi_common_unrealize(DeviceState *dev, Error **errp);
 bool virtio_scsi_handle_event_vq(VirtIOSCSI *s, VirtQueue *vq);
-- 
2.9.3

[Qemu-devel] [PATCH 12/14] migration: Make errp the last parameter of local functions

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 migration/rdma.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/migration/rdma.c b/migration/rdma.c
index 674ccab..fe0a4b5 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -809,7 +809,7 @@ static void qemu_rdma_dump_gid(const char *who, struct rdma_cm_id *id)
  *
  * Patches are being reviewed on linux-rdma.
  */
-static int qemu_rdma_broken_ipv6_kernel(Error **errp, struct ibv_context *verbs)
+static int qemu_rdma_broken_ipv6_kernel(struct ibv_context *verbs, Error **errp)
 {
     struct ibv_port_attr port_attr;
 
@@ -950,7 +950,7 @@ static int qemu_rdma_resolve_host(RDMAContext *rdma, Error **errp)
                 RDMA_RESOLVE_TIMEOUT_MS);
         if (!ret) {
             if (e->ai_family == AF_INET6) {
-                ret = qemu_rdma_broken_ipv6_kernel(errp, rdma->cm_id->verbs);
+                ret = qemu_rdma_broken_ipv6_kernel(rdma->cm_id->verbs, errp);
                 if (ret) {
                     continue;
                 }
@@ -2277,7 +2277,7 @@ static void qemu_rdma_cleanup(RDMAContext *rdma)
 }
 
 
-static int qemu_rdma_source_init(RDMAContext *rdma, Error **errp, bool pin_all)
+static int qemu_rdma_source_init(RDMAContext *rdma, bool pin_all, Error **errp)
 {
     int ret, idx;
     Error *local_err = NULL, **temp = &local_err;
@@ -2469,7 +2469,7 @@ static int qemu_rdma_dest_init(RDMAContext *rdma, Error **errp)
             continue;
         }
         if (e->ai_family == AF_INET6) {
-            ret = qemu_rdma_broken_ipv6_kernel(errp, listen_id->verbs);
+            ret = qemu_rdma_broken_ipv6_kernel(listen_id->verbs, errp);
             if (ret) {
                 continue;
             }
@@ -3676,8 +3676,8 @@ void rdma_start_outgoing_migration(void *opaque,
         goto err;
     }
 
-    ret = qemu_rdma_source_init(rdma, errp,
-        s->enabled_capabilities[MIGRATION_CAPABILITY_RDMA_PIN_ALL]);
+    ret = qemu_rdma_source_init(rdma,
+        s->enabled_capabilities[MIGRATION_CAPABILITY_RDMA_PIN_ALL], errp);
 
     if (ret) {
         goto err;
-- 
2.9.3

[Qemu-devel] [PATCH 13/14] qga: Make errp the last parameter of qga_vss_fsfreeze

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 qga/commands-win32.c | 4 ++--
 qga/vss-win32.c      | 2 +-
 qga/vss-win32.h      | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 19d72b2..04026ee 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -768,7 +768,7 @@ int64_t qmp_guest_fsfreeze_freeze(Error **errp)
     /* cannot risk guest agent blocking itself on a write in this state */
     ga_set_frozen(ga_state);
 
-    qga_vss_fsfreeze(&i, &local_err, true);
+    qga_vss_fsfreeze(&i, true, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         goto error;
@@ -807,7 +807,7 @@ int64_t qmp_guest_fsfreeze_thaw(Error **errp)
         return 0;
     }
 
-    qga_vss_fsfreeze(&i, errp, false);
+    qga_vss_fsfreeze(&i, false, errp);
 
     ga_unset_frozen(ga_state);
     return i;
diff --git a/qga/vss-win32.c b/qga/vss-win32.c
index 9a0e463..a80933c 100644
--- a/qga/vss-win32.c
+++ b/qga/vss-win32.c
@@ -145,7 +145,7 @@ void ga_uninstall_vss_provider(void)
 }
 
 /* Call VSS requester and freeze/thaw filesystems and applications */
-void qga_vss_fsfreeze(int *nr_volume, Error **errp, bool freeze)
+void qga_vss_fsfreeze(int *nr_volume, bool freeze, Error **errp)
 {
     const char *func_name = freeze ? "requester_freeze" : "requester_thaw";
     QGAVSSRequesterFunc func;
diff --git a/qga/vss-win32.h b/qga/vss-win32.h
index 4d1d150..51d303a 100644
--- a/qga/vss-win32.h
+++ b/qga/vss-win32.h
@@ -21,6 +21,6 @@ bool vss_initialized(void);
 int ga_install_vss_provider(void);
 void ga_uninstall_vss_provider(void);
 
-void qga_vss_fsfreeze(int *nr_volume, Error **errp, bool freeze);
+void qga_vss_fsfreeze(int *nr_volume, bool freeze, Error **errp);
 
 #endif
-- 
2.9.3

[Qemu-devel] [PATCH 14/14] error: Apply error_propagate_null.cocci again

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/i386/pc.c          | 4 +---
 hw/s390x/virtio-ccw.c | 4 +---
 hw/usb/bus.c          | 4 +---
 3 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index d24388e..f3b372a 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1104,9 +1104,7 @@ static void pc_new_cpu(const char *typename, int64_t apic_id, Error **errp)
     object_property_set_bool(cpu, true, "realized", &local_err);
 
     object_unref(cpu);
-    if (local_err) {
-        error_propagate(errp, local_err);
-    }
+    error_propagate(errp, local_err);
 }
 
 void pc_hot_add_cpu(const int64_t id, Error **errp)
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 00b3bde..ba4979c 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1757,9 +1757,7 @@ static void vhost_vsock_ccw_realize(VirtioCcwDevice *ccw_dev, Error **errp)
 
     qdev_set_parent_bus(vdev, BUS(&ccw_dev->bus));
     object_property_set_bool(OBJECT(vdev), true, "realized", &err);
-    if (err) {
-        error_propagate(errp, err);
-    }
+    error_propagate(errp, err);
 }
 
 static void vhost_vsock_ccw_class_init(ObjectClass *klass, void *data)
diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index 24f1608..5939b27 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -762,9 +762,7 @@ static void usb_set_attached(Object *obj, bool value, Error **errp)
 
     if (value) {
         usb_device_attach(dev, &err);
-        if (err) {
-            error_propagate(errp, err);
-        }
+        error_propagate(errp, err);
     } else {
         usb_device_detach(dev);
     }
-- 
2.9.3

Re: [Qemu-devel] [PATCH 06/14] crypto: Make errp the last parameter of functions

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 763 bytes --]

On 04/21/2017 07:27 AM, Fam Zheng wrote:
> Move opaque to 2nd instead of the 2nd to last, so that compilers help
> check with the convertion.

s/convertion/conversion/

> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/crypto.c            | 12 ++++++------
>  crypto/block-luks.c       | 21 +++++++++------------
>  include/crypto/block.h    | 12 ++++++------
>  tests/test-crypto-block.c | 12 ++++++------
>  4 files changed, 27 insertions(+), 30 deletions(-)
> 

crypto/block-qcow.c also refers to the changed the types, but does not
care about its parameters, so it looks like you got everything.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 00/14] Trivial cleanups around error reporting

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1501 bytes --]

On 04/21/2017 07:26 AM, Fam Zheng wrote:
> These are patches to:
> 
> 1) reorder the function parameters so that Error **errp comes
> last.
> 
> Error pointer in the middle of a function parameter list is very uncommon, and
> does caused mistakes, thus is not a good style. Change to the usual way.
> 
> 2) apply the error_propagate_null.cocci semantics patch again.
> 
> Fam Zheng (14):
>   socket: Make errp the last parameter of socket_connect
>   socket: Make errp the last parameter of inet_connect_saddr
>   socket: Make errp the last parameter of unix_connect_saddr
>   socket: Make errp the last parameter of vsock_connect_saddr
>   block: Make errp the last parameter of bdrv_img_create
>   crypto: Make errp the last parameter of functions
>   mirror: Make errp the last parameter of mirror_start_job
>   block: Make errp the last parameter of commit_active_start
>   nfs: Make errp the last parameter of nfs_client_open
>   fdc: Make errp the last parameter of fdctrl_connect_drives
>   scsi: Make errp the last parameter of virtio_scsi_common_realize
>   migration: Make errp the last parameter of local functions
>   qga: Make errp the last parameter of qga_vss_fsfreeze
>   error: Apply error_propagate_null.cocci again

Series:
Reviewed-by: Eric Blake <eblake@redhat.com>

but you have a typo in the commit message of 6/14

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 00/14] Trivial cleanups around error reporting

[Markus Armbruster]

Fam Zheng <famz@redhat.com> writes:

> These are patches to:
>
> 1) reorder the function parameters so that Error **errp comes
> last.
>
> Error pointer in the middle of a function parameter list is very uncommon, and
> does caused mistakes, thus is not a good style. Change to the usual way.
>
> 2) apply the error_propagate_null.cocci semantics patch again.

Thanks for taking on this grunt work.  Can take it through my tree.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH 00/14] Trivial cleanups around error reporting

[Markus Armbruster]

Fam Zheng <famz@redhat.com> writes:

> These are patches to:
>
> 1) reorder the function parameters so that Error **errp comes
> last.
>
> Error pointer in the middle of a function parameter list is very uncommon, and
> does caused mistakes, thus is not a good style. Change to the usual way.
>
> 2) apply the error_propagate_null.cocci semantics patch again.

Applied with the commit message typo found by Eric corrected.  Thanks!

Re: [Qemu-devel] [PATCH 06/14] crypto: Make errp the last parameter of functions

[Daniel P. Berrange]

On Fri, Apr 21, 2017 at 08:27:02PM +0800, Fam Zheng wrote:
> Move opaque to 2nd instead of the 2nd to last, so that compilers help
> check with the convertion.

Moving 'opaque' like this should not be done.

If you want the compiler to check the fixes, it should be done in
just two stages. First move errp & move opaque to start, compile
it & verify. Then put opaque back to where it was, and compile
again. The resulting commit thus only has the errp move, not the
unrelated & uneccessary opaque move.

> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/crypto.c            | 12 ++++++------
>  crypto/block-luks.c       | 21 +++++++++------------
>  include/crypto/block.h    | 12 ++++++------
>  tests/test-crypto-block.c | 12 ++++++------
>  4 files changed, 27 insertions(+), 30 deletions(-)
> 
> diff --git a/block/crypto.c b/block/crypto.c
> index 4a20388..34549b2 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -56,11 +56,11 @@ static int block_crypto_probe_generic(QCryptoBlockFormat format,
>  
>  
>  static ssize_t block_crypto_read_func(QCryptoBlock *block,
> +                                      void *opaque,
>                                        size_t offset,
>                                        uint8_t *buf,
>                                        size_t buflen,
> -                                      Error **errp,
> -                                      void *opaque)
> +                                      Error **errp)
>  {
>      BlockDriverState *bs = opaque;
>      ssize_t ret;
> @@ -83,11 +83,11 @@ struct BlockCryptoCreateData {
>  
>  
>  static ssize_t block_crypto_write_func(QCryptoBlock *block,
> +                                       void *opaque,
>                                         size_t offset,
>                                         const uint8_t *buf,
>                                         size_t buflen,
> -                                       Error **errp,
> -                                       void *opaque)
> +                                       Error **errp)
>  {
>      struct BlockCryptoCreateData *data = opaque;
>      ssize_t ret;
> @@ -102,9 +102,9 @@ static ssize_t block_crypto_write_func(QCryptoBlock *block,
>  
>  
>  static ssize_t block_crypto_init_func(QCryptoBlock *block,
> +                                      void *opaque,
>                                        size_t headerlen,
> -                                      Error **errp,
> -                                      void *opaque)
> +                                      Error **errp)
>  {
>      struct BlockCryptoCreateData *data = opaque;
>      int ret;
> diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> index 4530f82..d5a31bb 100644
> --- a/crypto/block-luks.c
> +++ b/crypto/block-luks.c
> @@ -473,10 +473,10 @@ qcrypto_block_luks_load_key(QCryptoBlock *block,
>       * then encrypted.
>       */
>      rv = readfunc(block,
> +                  opaque,
>                    slot->key_offset * QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
>                    splitkey, splitkeylen,
> -                  errp,
> -                  opaque);
> +                  errp);
>      if (rv < 0) {
>          goto cleanup;
>      }
> @@ -676,11 +676,10 @@ qcrypto_block_luks_open(QCryptoBlock *block,
>  
>      /* Read the entire LUKS header, minus the key material from
>       * the underlying device */
> -    rv = readfunc(block, 0,
> +    rv = readfunc(block, opaque, 0,
>                    (uint8_t *)&luks->header,
>                    sizeof(luks->header),
> -                  errp,
> -                  opaque);
> +                  errp);
>      if (rv < 0) {
>          ret = rv;
>          goto fail;
> @@ -1246,7 +1245,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,
>          QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;
>  
>      /* Reserve header space to match payload offset */
> -    initfunc(block, block->payload_offset, &local_err, opaque);
> +    initfunc(block, opaque, block->payload_offset, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          goto error;
> @@ -1268,11 +1267,10 @@ qcrypto_block_luks_create(QCryptoBlock *block,
>  
>  
>      /* Write out the partition header and key slot headers */
> -    writefunc(block, 0,
> +    writefunc(block, opaque, 0,
>                (const uint8_t *)&luks->header,
>                sizeof(luks->header),
> -              &local_err,
> -              opaque);
> +              &local_err);
>  
>      /* Delay checking local_err until we've byte-swapped */
>  
> @@ -1297,12 +1295,11 @@ qcrypto_block_luks_create(QCryptoBlock *block,
>  
>      /* Write out the master key material, starting at the
>       * sector immediately following the partition header. */
> -    if (writefunc(block,
> +    if (writefunc(block, opaque,
>                    luks->header.key_slots[0].key_offset *
>                    QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
>                    splitkey, splitkeylen,
> -                  errp,
> -                  opaque) != splitkeylen) {
> +                  errp) != splitkeylen) {
>          goto error;
>      }
>  
> diff --git a/include/crypto/block.h b/include/crypto/block.h
> index b6971de..4a053a3 100644
> --- a/include/crypto/block.h
> +++ b/include/crypto/block.h
> @@ -30,23 +30,23 @@ typedef struct QCryptoBlock QCryptoBlock;
>   * and QCryptoBlockOpenOptions in qapi/crypto.json */
>  
>  typedef ssize_t (*QCryptoBlockReadFunc)(QCryptoBlock *block,
> +                                        void *opaque,
>                                          size_t offset,
>                                          uint8_t *buf,
>                                          size_t buflen,
> -                                        Error **errp,
> -                                        void *opaque);
> +                                        Error **errp);
>  
>  typedef ssize_t (*QCryptoBlockInitFunc)(QCryptoBlock *block,
> +                                        void *opaque,
>                                          size_t headerlen,
> -                                        Error **errp,
> -                                        void *opaque);
> +                                        Error **errp);
>  
>  typedef ssize_t (*QCryptoBlockWriteFunc)(QCryptoBlock *block,
> +                                         void *opaque,
>                                           size_t offset,
>                                           const uint8_t *buf,
>                                           size_t buflen,
> -                                         Error **errp,
> -                                         void *opaque);
> +                                         Error **errp);
>  
>  /**
>   * qcrypto_block_has_format:
> diff --git a/tests/test-crypto-block.c b/tests/test-crypto-block.c
> index 1957a86..85e6603 100644
> --- a/tests/test-crypto-block.c
> +++ b/tests/test-crypto-block.c
> @@ -187,11 +187,11 @@ static struct QCryptoBlockTestData {
>  
>  
>  static ssize_t test_block_read_func(QCryptoBlock *block,
> +                                    void *opaque,
>                                      size_t offset,
>                                      uint8_t *buf,
>                                      size_t buflen,
> -                                    Error **errp,
> -                                    void *opaque)
> +                                    Error **errp)
>  {
>      Buffer *header = opaque;
>  
> @@ -204,9 +204,9 @@ static ssize_t test_block_read_func(QCryptoBlock *block,
>  
>  
>  static ssize_t test_block_init_func(QCryptoBlock *block,
> +                                    void *opaque,
>                                      size_t headerlen,
> -                                    Error **errp,
> -                                    void *opaque)
> +                                    Error **errp)
>  {
>      Buffer *header = opaque;
>  
> @@ -219,11 +219,11 @@ static ssize_t test_block_init_func(QCryptoBlock *block,
>  
>  
>  static ssize_t test_block_write_func(QCryptoBlock *block,
> +                                     void *opaque,
>                                       size_t offset,
>                                       const uint8_t *buf,
>                                       size_t buflen,
> -                                     Error **errp,
> -                                     void *opaque)
> +                                     Error **errp)
>  {
>      Buffer *header = opaque;
>  
> -- 
> 2.9.3
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 06/14] crypto: Make errp the last parameter of functions

[Fam Zheng]

On Mon, 04/24 09:28, Daniel P. Berrange wrote:
> On Fri, Apr 21, 2017 at 08:27:02PM +0800, Fam Zheng wrote:
> > Move opaque to 2nd instead of the 2nd to last, so that compilers help
> > check with the convertion.
> 
> Moving 'opaque' like this should not be done.
> 
> If you want the compiler to check the fixes, it should be done in
> just two stages. First move errp & move opaque to start, compile
> it & verify. Then put opaque back to where it was, and compile
> again. The resulting commit thus only has the errp move, not the
> unrelated & uneccessary opaque move.

The idea is to let everyone's compiler verifies this patch, and also to avoid
possible bugs introduced in backporting/rebasing/merging - for example if a
patch in another tree addes one more implementation that uses the old order, we
can notice.

If you don't like this, we can be careful and don't move opaque; or after a
short while, move opaque back in a separate commit (since Markus already sent a
pull request).

Fam

Re: [Qemu-devel] [PATCH 06/14] crypto: Make errp the last parameter of functions

[Markus Armbruster]

Fam Zheng <famz@redhat.com> writes:

> On Mon, 04/24 09:28, Daniel P. Berrange wrote:
>> On Fri, Apr 21, 2017 at 08:27:02PM +0800, Fam Zheng wrote:
>> > Move opaque to 2nd instead of the 2nd to last, so that compilers help
>> > check with the convertion.
>> 
>> Moving 'opaque' like this should not be done.
>> 
>> If you want the compiler to check the fixes, it should be done in
>> just two stages. First move errp & move opaque to start, compile
>> it & verify. Then put opaque back to where it was, and compile
>> again. The resulting commit thus only has the errp move, not the
>> unrelated & uneccessary opaque move.
>
> The idea is to let everyone's compiler verifies this patch, and also to avoid
> possible bugs introduced in backporting/rebasing/merging - for example if a
> patch in another tree addes one more implementation that uses the old order, we
> can notice.
>
> If you don't like this, we can be careful and don't move opaque; or after a
> short while, move opaque back in a separate commit (since Markus already sent a
> pull request).

I'm prepared to NAK my pull request if we think we need more time to
discuss the patches.

Re: [Qemu-devel] [PATCH 06/14] crypto: Make errp the last parameter of functions

[Daniel P. Berrange]

On Mon, Apr 24, 2017 at 11:24:31AM +0200, Markus Armbruster wrote:
> Fam Zheng <famz@redhat.com> writes:
> 
> > On Mon, 04/24 09:28, Daniel P. Berrange wrote:
> >> On Fri, Apr 21, 2017 at 08:27:02PM +0800, Fam Zheng wrote:
> >> > Move opaque to 2nd instead of the 2nd to last, so that compilers help
> >> > check with the convertion.
> >> 
> >> Moving 'opaque' like this should not be done.
> >> 
> >> If you want the compiler to check the fixes, it should be done in
> >> just two stages. First move errp & move opaque to start, compile
> >> it & verify. Then put opaque back to where it was, and compile
> >> again. The resulting commit thus only has the errp move, not the
> >> unrelated & uneccessary opaque move.
> >
> > The idea is to let everyone's compiler verifies this patch, and also to avoid
> > possible bugs introduced in backporting/rebasing/merging - for example if a
> > patch in another tree addes one more implementation that uses the old order, we
> > can notice.

That is true in general, but in reality this particular function is not
widely used and so chance of there being another out of tree impl is
essentially zero.

> >
> > If you don't like this, we can be careful and don't move opaque; or after a
> > short while, move opaque back in a separate commit (since Markus already sent a
> > pull request).
> 
> I'm prepared to NAK my pull request if we think we need more time to
> discuss the patches.

It isn't a big deal either way. I can just send a follow up patch if needed

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|