[Qemu-devel] [PATCH 00/18] [PATCH 00/18] block: Purge qerror_report()

[Qemu-devel] [PATCH 00/18] [PATCH 00/18] block: Purge qerror_report()

[Markus Armbruster]

qerror_report() is a transitional interface to help with converting
existing HMP commands to QMP.  It should not be used elsewhere.  This
series purges it from the block subsystem.

It is based on Fam's "[PATCH v19 00/16] Drop in_use from
BlockDriverState and enable point-in-time snapshot exporting over
NBD".

Markus Armbruster (18):
  blockdev: Don't use qerror_report_err() in drive_init()
  blockdev: Don't use qerror_report() in do_drive_del()
  qemu-nbd: Don't use qerror_report()
  block/rbd: Propagate errors to open and create methods
  block/ssh: Drop superfluous libssh2_session_last_errno() calls
  block/ssh: Propagate errors through check_host_key()
  block/ssh: Propagate errors through authenticate()
  block/ssh: Propagate errors through connect_to_ssh()
  block/ssh: Propagate errors to open and create methods
  block/vvfat: Propagate errors through enable_write_target()
  block/vvfat: Propagate errors through init_directories()
  block/sheepdog: Propagate errors through connect_to_sdog()
  block/sheepdog: Propagate errors through get_sheep_fd()
  block/sheepdog: Propagate errors through sd_prealloc()
  block/sheepdog: Propagate errors through do_sd_create()
  block/sheepdog: Propagate errors through find_vdi_name()
  block/sheepdog: Propagate errors to open and create methods
  block/sheepdog: Don't use qerror_report()

 block/rbd.c      |  66 ++++++++++++------------
 block/sheepdog.c | 135 ++++++++++++++++++++++++++++++-------------------
 block/ssh.c      | 151 +++++++++++++++++++++++++++++++++----------------------
 block/vvfat.c    |  32 ++++++------
 blockdev.c       |   7 ++-
 qemu-nbd.c       |   6 +--
 6 files changed, 229 insertions(+), 168 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH 01/18] blockdev: Don't use qerror_report_err() in drive_init()

[Markus Armbruster]

qerror_report_err() is a transitional interface to help with
converting existing HMP commands to QMP.  It should not be used
elsewhere.

drive_init() is not meant to be used by QMP commands.  It uses both
qerror_report_err() and error_report().  Convert the former to the
latter.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 blockdev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 1a12e24..24b1c3b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -691,7 +691,7 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
                                    &error_abort);
     qemu_opts_absorb_qdict(legacy_opts, bs_opts, &local_err);
     if (local_err) {
-        qerror_report_err(local_err);
+        error_report("%s", error_get_pretty(local_err));
         error_free(local_err);
         goto fail;
     }
@@ -903,7 +903,7 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
     dinfo = blockdev_init(filename, bs_opts, &local_err);
     if (dinfo == NULL) {
         if (local_err) {
-            qerror_report_err(local_err);
+            error_report("%s", error_get_pretty(local_err));
             error_free(local_err);
         }
         goto fail;
-- 
1.8.1.4

[Qemu-devel] [PATCH 02/18] blockdev: Don't use qerror_report() in do_drive_del()

[Markus Armbruster]

qerror_report() is a transitional interface to help with converting
existing HMP commands to QMP.  It should not be used elsewhere.

do_drive_del() is an HMP command that won't be converted to QMP (we'll
create a new QMP command instead).  It uses both qerror_report() and
error_report().  Convert the former to the latter.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 blockdev.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 24b1c3b..7478d4d 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -34,7 +34,6 @@
 #include "hw/block/block.h"
 #include "block/blockjob.h"
 #include "monitor/monitor.h"
-#include "qapi/qmp/qerror.h"
 #include "qemu/option.h"
 #include "qemu/config-file.h"
 #include "qapi/qmp/types.h"
@@ -1772,7 +1771,7 @@ int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
 
     bs = bdrv_find(id);
     if (!bs) {
-        qerror_report(QERR_DEVICE_NOT_FOUND, id);
+        error_report("Device '%s' not found", id);
         return -1;
     }
     if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, &local_err)) {
-- 
1.8.1.4

[Qemu-devel] [PATCH 03/18] qemu-nbd: Don't use qerror_report()

[Markus Armbruster]

qerror_report() is a transitional interface to help with converting
existing HMP commands to QMP.  It should not be used elsewhere.
Replace by error_report().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qemu-nbd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index eed79fa..621e654 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -223,7 +223,7 @@ static int tcp_socket_incoming(const char *address, uint16_t port)
     int fd = inet_listen(address_and_port, NULL, 0, SOCK_STREAM, 0, &local_err);
 
     if (local_err != NULL) {
-        qerror_report_err(local_err);
+        error_report("%s", error_get_pretty(local_err));
         error_free(local_err);
     }
     return fd;
@@ -235,7 +235,7 @@ static int unix_socket_incoming(const char *path)
     int fd = unix_listen(path, NULL, 0, &local_err);
 
     if (local_err != NULL) {
-        qerror_report_err(local_err);
+        error_report("%s", error_get_pretty(local_err));
         error_free(local_err);
     }
     return fd;
@@ -247,7 +247,7 @@ static int unix_socket_outgoing(const char *path)
     int fd = unix_connect(path, &local_err);
 
     if (local_err != NULL) {
-        qerror_report_err(local_err);
+        error_report("%s", error_get_pretty(local_err));
         error_free(local_err);
     }
     return fd;
-- 
1.8.1.4

[Qemu-devel] [PATCH 04/18] block/rbd: Propagate errors to open and create methods

[Markus Armbruster]

Completes the conversion to Error started in commit 015a103^..d5124c0.

Cc: Josh Durgin <josh.durgin@inktank.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/rbd.c | 66 +++++++++++++++++++++++++++++++------------------------------
 1 file changed, 34 insertions(+), 32 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index dbc79f4..aa7a864 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -105,7 +105,7 @@ typedef struct BDRVRBDState {
 static int qemu_rbd_next_tok(char *dst, int dst_len,
                              char *src, char delim,
                              const char *name,
-                             char **p)
+                             char **p, Error **errp)
 {
     int l;
     char *end;
@@ -128,10 +128,10 @@ static int qemu_rbd_next_tok(char *dst, int dst_len,
     }
     l = strlen(src);
     if (l >= dst_len) {
-        error_report("%s too long", name);
+        error_setg(errp, "%s too long", name);
         return -EINVAL;
     } else if (l == 0) {
-        error_report("%s too short", name);
+        error_setg(errp, "%s too short", name);
         return -EINVAL;
     }
 
@@ -157,13 +157,15 @@ static int qemu_rbd_parsename(const char *filename,
                               char *pool, int pool_len,
                               char *snap, int snap_len,
                               char *name, int name_len,
-                              char *conf, int conf_len)
+                              char *conf, int conf_len,
+                              Error **errp)
 {
     const char *start;
     char *p, *buf;
     int ret;
 
     if (!strstart(filename, "rbd:", &start)) {
+        error_setg(errp, "File name must start with 'rbd:'");
         return -EINVAL;
     }
 
@@ -172,7 +174,7 @@ static int qemu_rbd_parsename(const char *filename,
     *snap = '\0';
     *conf = '\0';
 
-    ret = qemu_rbd_next_tok(pool, pool_len, p, '/', "pool name", &p);
+    ret = qemu_rbd_next_tok(pool, pool_len, p, '/', "pool name", &p, errp);
     if (ret < 0 || !p) {
         ret = -EINVAL;
         goto done;
@@ -180,21 +182,23 @@ static int qemu_rbd_parsename(const char *filename,
     qemu_rbd_unescape(pool);
 
     if (strchr(p, '@')) {
-        ret = qemu_rbd_next_tok(name, name_len, p, '@', "object name", &p);
+        ret = qemu_rbd_next_tok(name, name_len, p, '@', "object name", &p,
+                                errp);
         if (ret < 0) {
             goto done;
         }
-        ret = qemu_rbd_next_tok(snap, snap_len, p, ':', "snap name", &p);
+        ret = qemu_rbd_next_tok(snap, snap_len, p, ':', "snap name", &p, errp);
         qemu_rbd_unescape(snap);
     } else {
-        ret = qemu_rbd_next_tok(name, name_len, p, ':', "object name", &p);
+        ret = qemu_rbd_next_tok(name, name_len, p, ':', "object name", &p,
+                                errp);
     }
     qemu_rbd_unescape(name);
     if (ret < 0 || !p) {
         goto done;
     }
 
-    ret = qemu_rbd_next_tok(conf, conf_len, p, '\0', "configuration", &p);
+    ret = qemu_rbd_next_tok(conf, conf_len, p, '\0', "configuration", &p, errp);
 
 done:
     g_free(buf);
@@ -229,7 +233,7 @@ static char *qemu_rbd_parse_clientname(const char *conf, char *clientname)
     return NULL;
 }
 
-static int qemu_rbd_set_conf(rados_t cluster, const char *conf)
+static int qemu_rbd_set_conf(rados_t cluster, const char *conf, Error **errp)
 {
     char *p, *buf;
     char name[RBD_MAX_CONF_NAME_SIZE];
@@ -241,20 +245,20 @@ static int qemu_rbd_set_conf(rados_t cluster, const char *conf)
 
     while (p) {
         ret = qemu_rbd_next_tok(name, sizeof(name), p,
-                                '=', "conf option name", &p);
+                                '=', "conf option name", &p, errp);
         if (ret < 0) {
             break;
         }
         qemu_rbd_unescape(name);
 
         if (!p) {
-            error_report("conf option %s has no value", name);
+            error_setg(errp, "conf option %s has no value", name);
             ret = -EINVAL;
             break;
         }
 
         ret = qemu_rbd_next_tok(value, sizeof(value), p,
-                                ':', "conf option value", &p);
+                                ':', "conf option value", &p, errp);
         if (ret < 0) {
             break;
         }
@@ -263,7 +267,7 @@ static int qemu_rbd_set_conf(rados_t cluster, const char *conf)
         if (strcmp(name, "conf") == 0) {
             ret = rados_conf_read_file(cluster, value);
             if (ret < 0) {
-                error_report("error reading conf file %s", value);
+                error_setg(errp, "error reading conf file %s", value);
                 break;
             }
         } else if (strcmp(name, "id") == 0) {
@@ -271,7 +275,7 @@ static int qemu_rbd_set_conf(rados_t cluster, const char *conf)
         } else {
             ret = rados_conf_set(cluster, name, value);
             if (ret < 0) {
-                error_report("invalid conf option %s", name);
+                error_setg(errp, "invalid conf option %s", name);
                 ret = -EINVAL;
                 break;
             }
@@ -285,6 +289,7 @@ static int qemu_rbd_set_conf(rados_t cluster, const char *conf)
 static int qemu_rbd_create(const char *filename, QEMUOptionParameter *options,
                            Error **errp)
 {
+    Error *local_err = NULL;
     int64_t bytes = 0;
     int64_t objsize;
     int obj_order = 0;
@@ -301,7 +306,7 @@ static int qemu_rbd_create(const char *filename, QEMUOptionParameter *options,
     if (qemu_rbd_parsename(filename, pool, sizeof(pool),
                            snap_buf, sizeof(snap_buf),
                            name, sizeof(name),
-                           conf, sizeof(conf)) < 0) {
+                           conf, sizeof(conf), &local_err) < 0) {
         return -EINVAL;
     }
 
@@ -313,11 +318,11 @@ static int qemu_rbd_create(const char *filename, QEMUOptionParameter *options,
             if (options->value.n) {
                 objsize = options->value.n;
                 if ((objsize - 1) & objsize) {    /* not a power of 2? */
-                    error_report("obj size needs to be power of 2");
+                    error_setg(errp, "obj size needs to be power of 2");
                     return -EINVAL;
                 }
                 if (objsize < 4096) {
-                    error_report("obj size too small");
+                    error_setg(errp, "obj size too small");
                     return -EINVAL;
                 }
                 obj_order = ffs(objsize) - 1;
@@ -328,7 +333,7 @@ static int qemu_rbd_create(const char *filename, QEMUOptionParameter *options,
 
     clientname = qemu_rbd_parse_clientname(conf, clientname_buf);
     if (rados_create(&cluster, clientname) < 0) {
-        error_report("error initializing");
+        error_setg(errp, "error initializing");
         return -EIO;
     }
 
@@ -338,20 +343,19 @@ static int qemu_rbd_create(const char *filename, QEMUOptionParameter *options,
     }
 
     if (conf[0] != '\0' &&
-        qemu_rbd_set_conf(cluster, conf) < 0) {
-        error_report("error setting config options");
+        qemu_rbd_set_conf(cluster, conf, &local_err) < 0) {
         rados_shutdown(cluster);
         return -EIO;
     }
 
     if (rados_connect(cluster) < 0) {
-        error_report("error connecting");
+        error_setg(errp, "error connecting");
         rados_shutdown(cluster);
         return -EIO;
     }
 
     if (rados_ioctx_create(cluster, pool, &io_ctx) < 0) {
-        error_report("error opening pool %s", pool);
+        error_setg(errp, "error opening pool %s", pool);
         rados_shutdown(cluster);
         return -EIO;
     }
@@ -441,8 +445,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
     opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
     qemu_opts_absorb_qdict(opts, options, &local_err);
     if (local_err) {
-        qerror_report_err(local_err);
-        error_free(local_err);
+        error_propagate(errp, local_err);
         qemu_opts_del(opts);
         return -EINVAL;
     }
@@ -452,7 +455,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
     if (qemu_rbd_parsename(filename, pool, sizeof(pool),
                            snap_buf, sizeof(snap_buf),
                            s->name, sizeof(s->name),
-                           conf, sizeof(conf)) < 0) {
+                           conf, sizeof(conf), &local_err) < 0) {
         r = -EINVAL;
         goto failed_opts;
     }
@@ -460,7 +463,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
     clientname = qemu_rbd_parse_clientname(conf, clientname_buf);
     r = rados_create(&s->cluster, clientname);
     if (r < 0) {
-        error_report("error initializing");
+        error_setg(&local_err, "error initializing");
         goto failed_opts;
     }
 
@@ -488,28 +491,27 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     if (conf[0] != '\0') {
-        r = qemu_rbd_set_conf(s->cluster, conf);
+        r = qemu_rbd_set_conf(s->cluster, conf, &local_err);
         if (r < 0) {
-            error_report("error setting config options");
             goto failed_shutdown;
         }
     }
 
     r = rados_connect(s->cluster);
     if (r < 0) {
-        error_report("error connecting");
+        error_setg(&local_err, "error connecting");
         goto failed_shutdown;
     }
 
     r = rados_ioctx_create(s->cluster, pool, &s->io_ctx);
     if (r < 0) {
-        error_report("error opening pool %s", pool);
+        error_setg(&local_err, "error opening pool %s", pool);
         goto failed_shutdown;
     }
 
     r = rbd_open(s->io_ctx, s->name, &s->image, s->snap);
     if (r < 0) {
-        error_report("error reading header from %s", s->name);
+        error_setg(&local_err, "error reading header from %s", s->name);
         goto failed_open;
     }
 
-- 
1.8.1.4

[Qemu-devel] [PATCH 05/18] block/ssh: Drop superfluous libssh2_session_last_errno() calls

[Markus Armbruster]

libssh2_session_last_error() already returns the error code.

Cc: "Richard W.M. Jones" <rjones@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/ssh.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/block/ssh.c b/block/ssh.c
index aa63c9d..e38d232 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -121,10 +121,9 @@ session_error_report(BDRVSSHState *s, const char *fs, ...)
         char *ssh_err;
         int ssh_err_code;
 
-        libssh2_session_last_error((s)->session, &ssh_err, NULL, 0);
         /* This is not an errno.  See <libssh2.h>. */
-        ssh_err_code = libssh2_session_last_errno((s)->session);
-
+        ssh_err_code = libssh2_session_last_error(s->session,
+                                                  &ssh_err, NULL, 0);
         error_printf(": %s (libssh2 error code: %d)", ssh_err, ssh_err_code);
     }
 
@@ -145,9 +144,9 @@ sftp_error_report(BDRVSSHState *s, const char *fs, ...)
         int ssh_err_code;
         unsigned long sftp_err_code;
 
-        libssh2_session_last_error((s)->session, &ssh_err, NULL, 0);
         /* This is not an errno.  See <libssh2.h>. */
-        ssh_err_code = libssh2_session_last_errno((s)->session);
+        ssh_err_code = libssh2_session_last_error(s->session,
+                                                  &ssh_err, NULL, 0);
         /* See <libssh2_sftp.h>. */
         sftp_err_code = libssh2_sftp_last_error((s)->sftp);
 
-- 
1.8.1.4

[Qemu-devel] [PATCH 06/18] block/ssh: Propagate errors through check_host_key()

[Markus Armbruster]

Cc: "Richard W.M. Jones" <rjones@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/ssh.c | 68 ++++++++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 49 insertions(+), 19 deletions(-)

diff --git a/block/ssh.c b/block/ssh.c
index e38d232..1df1946 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -106,6 +106,31 @@ static void ssh_state_free(BDRVSSHState *s)
     }
 }
 
+static void GCC_FMT_ATTR(3, 4)
+session_error_setg(Error **errp, BDRVSSHState *s, const char *fs, ...)
+{
+    va_list args;
+    char *msg;
+
+    va_start(args, fs);
+    msg = g_strdup_vprintf(fs, args);
+    va_end(args);
+
+    if (s->session) {
+        char *ssh_err;
+        int ssh_err_code;
+
+        /* This is not an errno.  See <libssh2.h>. */
+        ssh_err_code = libssh2_session_last_error(s->session,
+                                                  &ssh_err, NULL, 0);
+        error_setg(errp, "%s: %s (libssh2 error code: %d)",
+                   msg, ssh_err, ssh_err_code);
+    } else {
+        error_setg(errp, "%s", msg);
+    }
+    g_free(msg);
+}
+
 /* Wrappers around error_report which make sure to dump as much
  * information from libssh2 as possible.
  */
@@ -242,7 +267,7 @@ static void ssh_parse_filename(const char *filename, QDict *options,
 }
 
 static int check_host_key_knownhosts(BDRVSSHState *s,
-                                     const char *host, int port)
+                                     const char *host, int port, Error **errp)
 {
     const char *home;
     char *knh_file = NULL;
@@ -256,14 +281,15 @@ static int check_host_key_knownhosts(BDRVSSHState *s,
     hostkey = libssh2_session_hostkey(s->session, &len, &type);
     if (!hostkey) {
         ret = -EINVAL;
-        session_error_report(s, "failed to read remote host key");
+        session_error_setg(errp, s, "failed to read remote host key");
         goto out;
     }
 
     knh = libssh2_knownhost_init(s->session);
     if (!knh) {
         ret = -EINVAL;
-        session_error_report(s, "failed to initialize known hosts support");
+        session_error_setg(errp, s,
+                           "failed to initialize known hosts support");
         goto out;
     }
 
@@ -288,21 +314,23 @@ static int check_host_key_knownhosts(BDRVSSHState *s,
         break;
     case LIBSSH2_KNOWNHOST_CHECK_MISMATCH:
         ret = -EINVAL;
-        session_error_report(s, "host key does not match the one in known_hosts (found key %s)",
-                             found->key);
+        session_error_setg(errp, s,
+                      "host key does not match the one in known_hosts"
+                      " (found key %s)", found->key);
         goto out;
     case LIBSSH2_KNOWNHOST_CHECK_NOTFOUND:
         ret = -EINVAL;
-        session_error_report(s, "no host key was found in known_hosts");
+        session_error_setg(errp, s, "no host key was found in known_hosts");
         goto out;
     case LIBSSH2_KNOWNHOST_CHECK_FAILURE:
         ret = -EINVAL;
-        session_error_report(s, "failure matching the host key with known_hosts");
+        session_error_setg(errp, s,
+                      "failure matching the host key with known_hosts");
         goto out;
     default:
         ret = -EINVAL;
-        session_error_report(s, "unknown error matching the host key with known_hosts (%d)",
-                             r);
+        session_error_setg(errp, s, "unknown error matching the host key"
+                      " with known_hosts (%d)", r);
         goto out;
     }
 
@@ -357,20 +385,20 @@ static int compare_fingerprint(const unsigned char *fingerprint, size_t len,
 
 static int
 check_host_key_hash(BDRVSSHState *s, const char *hash,
-                    int hash_type, size_t fingerprint_len)
+                    int hash_type, size_t fingerprint_len, Error **errp)
 {
     const char *fingerprint;
 
     fingerprint = libssh2_hostkey_hash(s->session, hash_type);
     if (!fingerprint) {
-        session_error_report(s, "failed to read remote host key");
+        session_error_setg(errp, s, "failed to read remote host key");
         return -EINVAL;
     }
 
     if(compare_fingerprint((unsigned char *) fingerprint, fingerprint_len,
                            hash) != 0) {
-        error_report("remote host key does not match host_key_check '%s'",
-                     hash);
+        error_setg(errp, "remote host key does not match host_key_check '%s'",
+                   hash);
         return -EPERM;
     }
 
@@ -378,7 +406,7 @@ check_host_key_hash(BDRVSSHState *s, const char *hash,
 }
 
 static int check_host_key(BDRVSSHState *s, const char *host, int port,
-                          const char *host_key_check)
+                          const char *host_key_check, Error **errp)
 {
     /* host_key_check=no */
     if (strcmp(host_key_check, "no") == 0) {
@@ -388,21 +416,21 @@ static int check_host_key(BDRVSSHState *s, const char *host, int port,
     /* host_key_check=md5:xx:yy:zz:... */
     if (strncmp(host_key_check, "md5:", 4) == 0) {
         return check_host_key_hash(s, &host_key_check[4],
-                                   LIBSSH2_HOSTKEY_HASH_MD5, 16);
+                                   LIBSSH2_HOSTKEY_HASH_MD5, 16, errp);
     }
 
     /* host_key_check=sha1:xx:yy:zz:... */
     if (strncmp(host_key_check, "sha1:", 5) == 0) {
         return check_host_key_hash(s, &host_key_check[5],
-                                   LIBSSH2_HOSTKEY_HASH_SHA1, 20);
+                                   LIBSSH2_HOSTKEY_HASH_SHA1, 20, errp);
     }
 
     /* host_key_check=yes */
     if (strcmp(host_key_check, "yes") == 0) {
-        return check_host_key_knownhosts(s, host, port);
+        return check_host_key_knownhosts(s, host, port, errp);
     }
 
-    error_report("unknown host_key_check setting (%s)", host_key_check);
+    error_setg(errp, "unknown host_key_check setting (%s)", host_key_check);
     return -EINVAL;
 }
 
@@ -541,8 +569,10 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
     }
 
     /* Check the remote host's key against known_hosts. */
-    ret = check_host_key(s, host, port, host_key_check);
+    ret = check_host_key(s, host, port, host_key_check, &err);
     if (ret < 0) {
+        qerror_report_err(err);
+        error_free(err);
         goto err;
     }
 
-- 
1.8.1.4

[Qemu-devel] [PATCH 07/18] block/ssh: Propagate errors through authenticate()

[Markus Armbruster]

Cc: "Richard W.M. Jones" <rjones@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/ssh.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/block/ssh.c b/block/ssh.c
index 1df1946..18186ba 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -434,7 +434,7 @@ static int check_host_key(BDRVSSHState *s, const char *host, int port,
     return -EINVAL;
 }
 
-static int authenticate(BDRVSSHState *s, const char *user)
+static int authenticate(BDRVSSHState *s, const char *user, Error **errp)
 {
     int r, ret;
     const char *userauthlist;
@@ -445,7 +445,8 @@ static int authenticate(BDRVSSHState *s, const char *user)
     userauthlist = libssh2_userauth_list(s->session, user, strlen(user));
     if (strstr(userauthlist, "publickey") == NULL) {
         ret = -EPERM;
-        error_report("remote server does not support \"publickey\" authentication");
+        error_setg(errp,
+                "remote server does not support \"publickey\" authentication");
         goto out;
     }
 
@@ -453,17 +454,18 @@ static int authenticate(BDRVSSHState *s, const char *user)
     agent = libssh2_agent_init(s->session);
     if (!agent) {
         ret = -EINVAL;
-        session_error_report(s, "failed to initialize ssh-agent support");
+        session_error_setg(errp, s, "failed to initialize ssh-agent support");
         goto out;
     }
     if (libssh2_agent_connect(agent)) {
         ret = -ECONNREFUSED;
-        session_error_report(s, "failed to connect to ssh-agent");
+        session_error_setg(errp, s, "failed to connect to ssh-agent");
         goto out;
     }
     if (libssh2_agent_list_identities(agent)) {
         ret = -EINVAL;
-        session_error_report(s, "failed requesting identities from ssh-agent");
+        session_error_setg(errp, s,
+                           "failed requesting identities from ssh-agent");
         goto out;
     }
 
@@ -474,7 +476,8 @@ static int authenticate(BDRVSSHState *s, const char *user)
         }
         if (r < 0) {
             ret = -EINVAL;
-            session_error_report(s, "failed to obtain identity from ssh-agent");
+            session_error_setg(errp, s,
+                               "failed to obtain identity from ssh-agent");
             goto out;
         }
         r = libssh2_agent_userauth(agent, user, identity);
@@ -488,8 +491,8 @@ static int authenticate(BDRVSSHState *s, const char *user)
     }
 
     ret = -EPERM;
-    error_report("failed to authenticate using publickey authentication "
-                 "and the identities held by your ssh-agent");
+    error_setg(errp, "failed to authenticate using publickey authentication "
+               "and the identities held by your ssh-agent");
 
  out:
     if (agent != NULL) {
@@ -577,8 +580,10 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
     }
 
     /* Authenticate. */
-    ret = authenticate(s, user);
+    ret = authenticate(s, user, &err);
     if (ret < 0) {
+        qerror_report_err(err);
+        error_free(err);
         goto err;
     }
 
-- 
1.8.1.4

[Qemu-devel] [PATCH 08/18] block/ssh: Propagate errors through connect_to_ssh()

[Markus Armbruster]

Cc: "Richard W.M. Jones" <rjones@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/ssh.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/block/ssh.c b/block/ssh.c
index 18186ba..26078c4 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -506,10 +506,9 @@ static int authenticate(BDRVSSHState *s, const char *user, Error **errp)
 }
 
 static int connect_to_ssh(BDRVSSHState *s, QDict *options,
-                          int ssh_flags, int creat_mode)
+                          int ssh_flags, int creat_mode, Error **errp)
 {
     int r, ret;
-    Error *err = NULL;
     const char *host, *user, *path, *host_key_check;
     int port;
 
@@ -528,6 +527,7 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
     } else {
         user = g_get_user_name();
         if (!user) {
+            error_setg_errno(errp, errno, "Can't get user name");
             ret = -errno;
             goto err;
         }
@@ -544,11 +544,9 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
     s->hostport = g_strdup_printf("%s:%d", host, port);
 
     /* Open the socket and connect. */
-    s->sock = inet_connect(s->hostport, &err);
-    if (err != NULL) {
+    s->sock = inet_connect(s->hostport, errp);
+    if (s->sock < 0) {
         ret = -errno;
-        qerror_report_err(err);
-        error_free(err);
         goto err;
     }
 
@@ -556,7 +554,7 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
     s->session = libssh2_session_init();
     if (!s->session) {
         ret = -EINVAL;
-        session_error_report(s, "failed to initialize libssh2 session");
+        session_error_setg(errp, s, "failed to initialize libssh2 session");
         goto err;
     }
 
@@ -567,30 +565,26 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
     r = libssh2_session_handshake(s->session, s->sock);
     if (r != 0) {
         ret = -EINVAL;
-        session_error_report(s, "failed to establish SSH session");
+        session_error_setg(errp, s, "failed to establish SSH session");
         goto err;
     }
 
     /* Check the remote host's key against known_hosts. */
-    ret = check_host_key(s, host, port, host_key_check, &err);
+    ret = check_host_key(s, host, port, host_key_check, errp);
     if (ret < 0) {
-        qerror_report_err(err);
-        error_free(err);
         goto err;
     }
 
     /* Authenticate. */
-    ret = authenticate(s, user, &err);
+    ret = authenticate(s, user, errp);
     if (ret < 0) {
-        qerror_report_err(err);
-        error_free(err);
         goto err;
     }
 
     /* Start SFTP. */
     s->sftp = libssh2_sftp_init(s->session);
     if (!s->sftp) {
-        session_error_report(s, "failed to initialize sftp handle");
+        session_error_setg(errp, s, "failed to initialize sftp handle");
         ret = -EINVAL;
         goto err;
     }
@@ -645,6 +639,7 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
 static int ssh_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags,
                          Error **errp)
 {
+    Error *local_err = NULL;
     BDRVSSHState *s = bs->opaque;
     int ret;
     int ssh_flags;
@@ -657,8 +652,10 @@ static int ssh_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags,
     }
 
     /* Start up SSH. */
-    ret = connect_to_ssh(s, options, ssh_flags, 0);
+    ret = connect_to_ssh(s, options, ssh_flags, 0, &local_err);
     if (ret < 0) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         goto err;
     }
 
@@ -718,8 +715,11 @@ static int ssh_create(const char *filename, QEMUOptionParameter *options,
 
     r = connect_to_ssh(&s, uri_options,
                        LIBSSH2_FXF_READ|LIBSSH2_FXF_WRITE|
-                       LIBSSH2_FXF_CREAT|LIBSSH2_FXF_TRUNC, 0644);
+                       LIBSSH2_FXF_CREAT|LIBSSH2_FXF_TRUNC,
+                       0644, &local_err);
     if (r < 0) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         ret = r;
         goto out;
     }
-- 
1.8.1.4

[Qemu-devel] [PATCH 09/18] block/ssh: Propagate errors to open and create methods

[Markus Armbruster]

Completes the conversion to Error started in commit 015a103^..d5124c0.

Cc: "Richard W.M. Jones" <rjones@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/ssh.c | 47 ++++++++++++++++++++++-------------------------
 1 file changed, 22 insertions(+), 25 deletions(-)

diff --git a/block/ssh.c b/block/ssh.c
index 26078c4..b212971 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -131,29 +131,34 @@ session_error_setg(Error **errp, BDRVSSHState *s, const char *fs, ...)
     g_free(msg);
 }
 
-/* Wrappers around error_report which make sure to dump as much
- * information from libssh2 as possible.
- */
-static void GCC_FMT_ATTR(2, 3)
-session_error_report(BDRVSSHState *s, const char *fs, ...)
+static void GCC_FMT_ATTR(3, 4)
+sftp_error_setg(Error **errp, BDRVSSHState *s, const char *fs, ...)
 {
     va_list args;
+    char *msg;
 
     va_start(args, fs);
-    error_vprintf(fs, args);
+    msg = g_strdup_vprintf(fs, args);
+    va_end(args);
 
-    if ((s)->session) {
+    if (s->sftp) {
         char *ssh_err;
         int ssh_err_code;
+        unsigned long sftp_err_code;
 
         /* This is not an errno.  See <libssh2.h>. */
         ssh_err_code = libssh2_session_last_error(s->session,
                                                   &ssh_err, NULL, 0);
-        error_printf(": %s (libssh2 error code: %d)", ssh_err, ssh_err_code);
-    }
+        /* See <libssh2_sftp.h>. */
+        sftp_err_code = libssh2_sftp_last_error((s)->sftp);
 
-    va_end(args);
-    error_printf("\n");
+        error_setg(errp,
+                   "%s: %s (libssh2 error code: %d, sftp error code: %lu)",
+                   msg, ssh_err, ssh_err_code, sftp_err_code);
+    } else {
+        error_setg(errp, "%s", msg);
+    }
+    g_free(msg);
 }
 
 static void GCC_FMT_ATTR(2, 3)
@@ -594,14 +599,14 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
             path, ssh_flags, creat_mode);
     s->sftp_handle = libssh2_sftp_open(s->sftp, path, ssh_flags, creat_mode);
     if (!s->sftp_handle) {
-        session_error_report(s, "failed to open remote file '%s'", path);
+        session_error_setg(errp, s, "failed to open remote file '%s'", path);
         ret = -EINVAL;
         goto err;
     }
 
     r = libssh2_sftp_fstat(s->sftp_handle, &s->attrs);
     if (r < 0) {
-        sftp_error_report(s, "failed to read file attributes");
+        sftp_error_setg(errp, s, "failed to read file attributes");
         return -EINVAL;
     }
 
@@ -639,7 +644,6 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
 static int ssh_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags,
                          Error **errp)
 {
-    Error *local_err = NULL;
     BDRVSSHState *s = bs->opaque;
     int ret;
     int ssh_flags;
@@ -652,10 +656,8 @@ static int ssh_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags,
     }
 
     /* Start up SSH. */
-    ret = connect_to_ssh(s, options, ssh_flags, 0, &local_err);
+    ret = connect_to_ssh(s, options, ssh_flags, 0, errp);
     if (ret < 0) {
-        qerror_report_err(local_err);
-        error_free(local_err);
         goto err;
     }
 
@@ -686,7 +688,6 @@ static int ssh_create(const char *filename, QEMUOptionParameter *options,
                       Error **errp)
 {
     int r, ret;
-    Error *local_err = NULL;
     int64_t total_size = 0;
     QDict *uri_options = NULL;
     BDRVSSHState s;
@@ -705,10 +706,8 @@ static int ssh_create(const char *filename, QEMUOptionParameter *options,
     DPRINTF("total_size=%" PRIi64, total_size);
 
     uri_options = qdict_new();
-    r = parse_uri(filename, uri_options, &local_err);
+    r = parse_uri(filename, uri_options, errp);
     if (r < 0) {
-        qerror_report_err(local_err);
-        error_free(local_err);
         ret = r;
         goto out;
     }
@@ -716,10 +715,8 @@ static int ssh_create(const char *filename, QEMUOptionParameter *options,
     r = connect_to_ssh(&s, uri_options,
                        LIBSSH2_FXF_READ|LIBSSH2_FXF_WRITE|
                        LIBSSH2_FXF_CREAT|LIBSSH2_FXF_TRUNC,
-                       0644, &local_err);
+                       0644, errp);
     if (r < 0) {
-        qerror_report_err(local_err);
-        error_free(local_err);
         ret = r;
         goto out;
     }
@@ -728,7 +725,7 @@ static int ssh_create(const char *filename, QEMUOptionParameter *options,
         libssh2_sftp_seek64(s.sftp_handle, total_size-1);
         r2 = libssh2_sftp_write(s.sftp_handle, c, 1);
         if (r2 < 0) {
-            sftp_error_report(&s, "truncate failed");
+            sftp_error_setg(errp, &s, "truncate failed");
             ret = -EINVAL;
             goto out;
         }
-- 
1.8.1.4

[Qemu-devel] [PATCH 10/18] block/vvfat: Propagate errors through enable_write_target()

[Markus Armbruster]

Continues the conversion of the open method to Error started in commit
015a103.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/vvfat.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index c3af7ff..3eccd68 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -979,7 +979,7 @@ static int init_directories(BDRVVVFATState* s,
 static BDRVVVFATState *vvv = NULL;
 #endif
 
-static int enable_write_target(BDRVVVFATState *s);
+static int enable_write_target(BDRVVVFATState *s, Error **errp);
 static int is_consistent(BDRVVVFATState *s);
 
 static void vvfat_rebind(BlockDriverState *bs)
@@ -1160,7 +1160,7 @@ DLOG(if (stderr == NULL) {
     s->sector_count = cyls * heads * secs - (s->first_sectors_number - 1);
 
     if (qemu_opt_get_bool(opts, "rw", false)) {
-        ret = enable_write_target(s);
+        ret = enable_write_target(s, errp);
         if (ret < 0) {
             goto fail;
         }
@@ -2904,11 +2904,10 @@ static BlockDriver vvfat_write_target = {
     .bdrv_close         = write_target_close,
 };
 
-static int enable_write_target(BDRVVVFATState *s)
+static int enable_write_target(BDRVVVFATState *s, Error **errp)
 {
     BlockDriver *bdrv_qcow;
     QEMUOptionParameter *options;
-    Error *local_err = NULL;
     int ret;
     int size = sector2cluster(s, s->sector_count);
     s->used_clusters = calloc(size, 1);
@@ -2918,6 +2917,7 @@ static int enable_write_target(BDRVVVFATState *s)
     s->qcow_filename = g_malloc(1024);
     ret = get_tmp_filename(s->qcow_filename, 1024);
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "can't create temporary file");
         goto err;
     }
 
@@ -2926,20 +2926,16 @@ static int enable_write_target(BDRVVVFATState *s)
     set_option_parameter_int(options, BLOCK_OPT_SIZE, s->sector_count * 512);
     set_option_parameter(options, BLOCK_OPT_BACKING_FILE, "fat:");
 
-    ret = bdrv_create(bdrv_qcow, s->qcow_filename, options, &local_err);
+    ret = bdrv_create(bdrv_qcow, s->qcow_filename, options, errp);
     if (ret < 0) {
-        qerror_report_err(local_err);
-        error_free(local_err);
         goto err;
     }
 
     s->qcow = NULL;
     ret = bdrv_open(&s->qcow, s->qcow_filename, NULL, NULL,
             BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, bdrv_qcow,
-            &local_err);
+            errp);
     if (ret < 0) {
-        qerror_report_err(local_err);
-        error_free(local_err);
         goto err;
     }
 
-- 
1.8.1.4

[Qemu-devel] [PATCH 11/18] block/vvfat: Propagate errors through init_directories()

[Markus Armbruster]

Completes the conversion of the open method to Error started in commit
015a103.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/vvfat.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 3eccd68..c6a1211 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -831,7 +831,8 @@ static inline off_t cluster2sector(BDRVVVFATState* s, uint32_t cluster_num)
 }
 
 static int init_directories(BDRVVVFATState* s,
-                            const char *dirname, int heads, int secs)
+                            const char *dirname, int heads, int secs,
+                            Error **errp)
 {
     bootsector_t* bootsector;
     mapping_t* mapping;
@@ -892,8 +893,8 @@ static int init_directories(BDRVVVFATState* s,
         if (mapping->mode & MODE_DIRECTORY) {
 	    mapping->begin = cluster;
 	    if(read_directory(s, i)) {
-		fprintf(stderr, "Could not read directory %s\n",
-			mapping->path);
+                error_setg(errp, "Could not read directory %s",
+                           mapping->path);
 		return -1;
 	    }
 	    mapping = array_get(&(s->mapping), i);
@@ -919,9 +920,10 @@ static int init_directories(BDRVVVFATState* s,
 	cluster = mapping->end;
 
 	if(cluster > s->cluster_count) {
-	    fprintf(stderr,"Directory does not fit in FAT%d (capacity %.2f MB)\n",
-		    s->fat_type, s->sector_count / 2000.0);
-	    return -EINVAL;
+            error_setg(errp,
+                       "Directory does not fit in FAT%d (capacity %.2f MB)",
+                       s->fat_type, s->sector_count / 2000.0);
+            return -1;
 	}
 
 	/* fix fat for entry */
@@ -1169,7 +1171,7 @@ DLOG(if (stderr == NULL) {
 
     bs->total_sectors = cyls * heads * secs;
 
-    if (init_directories(s, dirname, heads, secs)) {
+    if (init_directories(s, dirname, heads, secs, errp)) {
         ret = -EIO;
         goto fail;
     }
-- 
1.8.1.4

[Qemu-devel] [PATCH 12/18] block/sheepdog: Propagate errors through connect_to_sdog()

[Markus Armbruster]

Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/sheepdog.c | 77 ++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 55 insertions(+), 22 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 2c3fb01..ff0aa89 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -526,17 +526,16 @@ static SheepdogAIOCB *sd_aio_setup(BlockDriverState *bs, QEMUIOVector *qiov,
     return acb;
 }
 
-static int connect_to_sdog(BDRVSheepdogState *s)
+static int connect_to_sdog(BDRVSheepdogState *s, Error **errp)
 {
     int fd;
-    Error *err = NULL;
 
     if (s->is_unix) {
-        fd = unix_connect(s->host_spec, &err);
+        fd = unix_connect(s->host_spec, errp);
     } else {
-        fd = inet_connect(s->host_spec, &err);
+        fd = inet_connect(s->host_spec, errp);
 
-        if (err == NULL) {
+        if (fd >= 0) {
             int ret = socket_set_nodelay(fd);
             if (ret < 0) {
                 error_report("%s", strerror(errno));
@@ -544,10 +543,7 @@ static int connect_to_sdog(BDRVSheepdogState *s)
         }
     }
 
-    if (err != NULL) {
-        qerror_report_err(err);
-        error_free(err);
-    } else {
+    if (fd >= 0) {
         qemu_set_nonblock(fd);
     }
 
@@ -916,10 +912,13 @@ static void co_write_request(void *opaque)
  */
 static int get_sheep_fd(BDRVSheepdogState *s)
 {
+    Error *local_err = NULL;
     int fd;
 
-    fd = connect_to_sdog(s);
+    fd = connect_to_sdog(s, &local_err);
     if (fd < 0) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         return fd;
     }
 
@@ -1063,14 +1062,17 @@ static int find_vdi_name(BDRVSheepdogState *s, const char *filename,
                          uint32_t snapid, const char *tag, uint32_t *vid,
                          bool lock)
 {
+    Error *local_err = NULL;
     int ret, fd;
     SheepdogVdiReq hdr;
     SheepdogVdiRsp *rsp = (SheepdogVdiRsp *)&hdr;
     unsigned int wlen, rlen = 0;
     char buf[SD_MAX_VDI_LEN + SD_MAX_VDI_TAG_LEN];
 
-    fd = connect_to_sdog(s);
+    fd = connect_to_sdog(s, &local_err);
     if (fd < 0) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         return fd;
     }
 
@@ -1263,12 +1265,15 @@ static int write_object(int fd, char *buf, uint64_t oid, uint8_t copies,
 /* update inode with the latest state */
 static int reload_inode(BDRVSheepdogState *s, uint32_t snapid, const char *tag)
 {
+    Error *local_err = NULL;
     SheepdogInode *inode;
     int ret = 0, fd;
     uint32_t vid = 0;
 
-    fd = connect_to_sdog(s);
+    fd = connect_to_sdog(s, &local_err);
     if (fd < 0) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         return -EIO;
     }
 
@@ -1436,8 +1441,10 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,
         s->is_snapshot = true;
     }
 
-    fd = connect_to_sdog(s);
+    fd = connect_to_sdog(s, &local_err);
     if (fd < 0) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         ret = fd;
         goto out;
     }
@@ -1474,14 +1481,17 @@ out:
 
 static int do_sd_create(BDRVSheepdogState *s, uint32_t *vdi_id, int snapshot)
 {
+    Error *local_err = NULL;
     SheepdogVdiReq hdr;
     SheepdogVdiRsp *rsp = (SheepdogVdiRsp *)&hdr;
     int fd, ret;
     unsigned int wlen, rlen = 0;
     char buf[SD_MAX_VDI_LEN];
 
-    fd = connect_to_sdog(s);
+    fd = connect_to_sdog(s, &local_err);
     if (fd < 0) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         return fd;
     }
 
@@ -1730,6 +1740,7 @@ out:
 
 static void sd_close(BlockDriverState *bs)
 {
+    Error *local_err = NULL;
     BDRVSheepdogState *s = bs->opaque;
     SheepdogVdiReq hdr;
     SheepdogVdiRsp *rsp = (SheepdogVdiRsp *)&hdr;
@@ -1738,8 +1749,10 @@ static void sd_close(BlockDriverState *bs)
 
     DPRINTF("%s\n", s->name);
 
-    fd = connect_to_sdog(s);
+    fd = connect_to_sdog(s, &local_err);
     if (fd < 0) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         return;
     }
 
@@ -1774,6 +1787,7 @@ static int64_t sd_getlength(BlockDriverState *bs)
 
 static int sd_truncate(BlockDriverState *bs, int64_t offset)
 {
+    Error *local_err = NULL;
     BDRVSheepdogState *s = bs->opaque;
     int ret, fd;
     unsigned int datalen;
@@ -1786,8 +1800,10 @@ static int sd_truncate(BlockDriverState *bs, int64_t offset)
         return -EINVAL;
     }
 
-    fd = connect_to_sdog(s);
+    fd = connect_to_sdog(s, &local_err);
     if (fd < 0) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         return fd;
     }
 
@@ -1846,6 +1862,7 @@ static void coroutine_fn sd_write_done(SheepdogAIOCB *acb)
 /* Delete current working VDI on the snapshot chain */
 static bool sd_delete(BDRVSheepdogState *s)
 {
+    Error *local_err = NULL;
     unsigned int wlen = SD_MAX_VDI_LEN, rlen = 0;
     SheepdogVdiReq hdr = {
         .opcode = SD_OP_DEL_VDI,
@@ -1856,8 +1873,10 @@ static bool sd_delete(BDRVSheepdogState *s)
     SheepdogVdiRsp *rsp = (SheepdogVdiRsp *)&hdr;
     int fd, ret;
 
-    fd = connect_to_sdog(s);
+    fd = connect_to_sdog(s, &local_err);
     if (fd < 0) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         return false;
     }
 
@@ -1885,6 +1904,7 @@ static bool sd_delete(BDRVSheepdogState *s)
  */
 static int sd_create_branch(BDRVSheepdogState *s)
 {
+    Error *local_err = NULL;
     int ret, fd;
     uint32_t vid;
     char *buf;
@@ -1907,8 +1927,10 @@ static int sd_create_branch(BDRVSheepdogState *s)
 
     DPRINTF("%" PRIx32 " is created.\n", vid);
 
-    fd = connect_to_sdog(s);
+    fd = connect_to_sdog(s, &local_err);
     if (fd < 0) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         ret = fd;
         goto out;
     }
@@ -2122,6 +2144,7 @@ static int coroutine_fn sd_co_flush_to_disk(BlockDriverState *bs)
 
 static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
 {
+    Error *local_err = NULL;
     BDRVSheepdogState *s = bs->opaque;
     int ret, fd;
     uint32_t new_vid;
@@ -2151,8 +2174,10 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
     datalen = SD_INODE_SIZE - sizeof(s->inode.data_vdi_id);
 
     /* refresh inode. */
-    fd = connect_to_sdog(s);
+    fd = connect_to_sdog(s, &local_err);
     if (fd < 0) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         ret = fd;
         goto cleanup;
     }
@@ -2249,6 +2274,7 @@ static int sd_snapshot_delete(BlockDriverState *bs,
 
 static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
 {
+    Error *local_err = NULL;
     BDRVSheepdogState *s = bs->opaque;
     SheepdogReq req;
     int fd, nr = 1024, ret, max = BITS_TO_LONGS(SD_NR_VDIS) * sizeof(long);
@@ -2263,8 +2289,10 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
 
     vdi_inuse = g_malloc(max);
 
-    fd = connect_to_sdog(s);
+    fd = connect_to_sdog(s, &local_err);
     if (fd < 0) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         ret = fd;
         goto out;
     }
@@ -2290,8 +2318,10 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
     hval = fnv_64a_buf(s->name, strlen(s->name), FNV1A_64_INIT);
     start_nr = hval & (SD_NR_VDIS - 1);
 
-    fd = connect_to_sdog(s);
+    fd = connect_to_sdog(s, &local_err);
     if (fd < 0) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         ret = fd;
         goto out;
     }
@@ -2341,6 +2371,7 @@ out:
 static int do_load_save_vmstate(BDRVSheepdogState *s, uint8_t *data,
                                 int64_t pos, int size, int load)
 {
+    Error *local_err = NULL;
     bool create;
     int fd, ret = 0, remaining = size;
     unsigned int data_len;
@@ -2349,8 +2380,10 @@ static int do_load_save_vmstate(BDRVSheepdogState *s, uint8_t *data,
     uint32_t vdi_index;
     uint32_t vdi_id = load ? s->inode.parent_vdi_id : s->inode.vdi_id;
 
-    fd = connect_to_sdog(s);
+    fd = connect_to_sdog(s, &local_err);
     if (fd < 0) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         return fd;
     }
 
-- 
1.8.1.4

[Qemu-devel] [PATCH 13/18] block/sheepdog: Propagate errors through get_sheep_fd()

[Markus Armbruster]

Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/sheepdog.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index ff0aa89..b932d68 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -668,7 +668,7 @@ static void coroutine_fn add_aio_request(BDRVSheepdogState *s, AIOReq *aio_req,
                            enum AIOCBState aiocb_type);
 static void coroutine_fn resend_aioreq(BDRVSheepdogState *s, AIOReq *aio_req);
 static int reload_inode(BDRVSheepdogState *s, uint32_t snapid, const char *tag);
-static int get_sheep_fd(BDRVSheepdogState *s);
+static int get_sheep_fd(BDRVSheepdogState *s, Error **errp);
 static void co_write_request(void *opaque);
 
 static AIOReq *find_pending_req(BDRVSheepdogState *s, uint64_t oid)
@@ -705,6 +705,7 @@ static void coroutine_fn send_pending_req(BDRVSheepdogState *s, uint64_t oid)
 
 static coroutine_fn void reconnect_to_sdog(void *opaque)
 {
+    Error *local_err = NULL;
     BDRVSheepdogState *s = opaque;
     AIOReq *aio_req, *next;
 
@@ -719,9 +720,11 @@ static coroutine_fn void reconnect_to_sdog(void *opaque)
 
     /* Try to reconnect the sheepdog server every one second. */
     while (s->fd < 0) {
-        s->fd = get_sheep_fd(s);
+        s->fd = get_sheep_fd(s, &local_err);
         if (s->fd < 0) {
             DPRINTF("Wait for connection to be established\n");
+            qerror_report_err(local_err);
+            error_free(local_err);
             co_aio_sleep_ns(bdrv_get_aio_context(s->bs), QEMU_CLOCK_REALTIME,
                             1000000000ULL);
         }
@@ -910,15 +913,12 @@ static void co_write_request(void *opaque)
  * We cannot use this descriptor for other operations because
  * the block driver may be on waiting response from the server.
  */
-static int get_sheep_fd(BDRVSheepdogState *s)
+static int get_sheep_fd(BDRVSheepdogState *s, Error **errp)
 {
-    Error *local_err = NULL;
     int fd;
 
-    fd = connect_to_sdog(s, &local_err);
+    fd = connect_to_sdog(s, errp);
     if (fd < 0) {
-        qerror_report_err(local_err);
-        error_free(local_err);
         return fd;
     }
 
@@ -1415,8 +1415,10 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,
     if (ret < 0) {
         goto out;
     }
-    s->fd = get_sheep_fd(s);
+    s->fd = get_sheep_fd(s, &local_err);
     if (s->fd < 0) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         ret = s->fd;
         goto out;
     }
-- 
1.8.1.4

[Qemu-devel] [PATCH 14/18] block/sheepdog: Propagate errors through sd_prealloc()

[Markus Armbruster]

Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/sheepdog.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index b932d68..a8385bb 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1537,27 +1537,24 @@ static int do_sd_create(BDRVSheepdogState *s, uint32_t *vdi_id, int snapshot)
     return 0;
 }
 
-static int sd_prealloc(const char *filename)
+static int sd_prealloc(const char *filename, Error **errp)
 {
     BlockDriverState *bs = NULL;
     uint32_t idx, max_idx;
     int64_t vdi_size;
     void *buf = g_malloc0(SD_DATA_OBJ_SIZE);
-    Error *local_err = NULL;
     int ret;
 
     ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
-                    NULL, &local_err);
+                    NULL, errp);
     if (ret < 0) {
-        qerror_report_err(local_err);
-        error_free(local_err);
         goto out;
     }
 
     vdi_size = bdrv_getlength(bs);
     if (vdi_size < 0) {
         ret = vdi_size;
-        goto out;
+        goto out_setg;
     }
     max_idx = DIV_ROUND_UP(vdi_size, SD_DATA_OBJ_SIZE);
 
@@ -1568,13 +1565,15 @@ static int sd_prealloc(const char *filename)
          */
         ret = bdrv_pread(bs, idx * SD_DATA_OBJ_SIZE, buf, SD_DATA_OBJ_SIZE);
         if (ret < 0) {
-            goto out;
+            goto out_setg;
         }
         ret = bdrv_pwrite(bs, idx * SD_DATA_OBJ_SIZE, buf, SD_DATA_OBJ_SIZE);
         if (ret < 0) {
-            goto out;
+            goto out_setg;
         }
     }
+out_setg:
+    error_setg_errno(errp, -ret, "Can't pre-allocate");
 out:
     if (bs) {
         bdrv_unref(bs);
@@ -1734,7 +1733,11 @@ static int sd_create(const char *filename, QEMUOptionParameter *options,
         goto out;
     }
 
-    ret = sd_prealloc(filename);
+    ret = sd_prealloc(filename, &local_err);
+    if (ret < 0) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+    }
 out:
     g_free(s);
     return ret;
-- 
1.8.1.4

[Qemu-devel] [PATCH 15/18] block/sheepdog: Propagate errors through do_sd_create()

[Markus Armbruster]

Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/sheepdog.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index a8385bb..d20a94c 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1481,19 +1481,17 @@ out:
     return ret;
 }
 
-static int do_sd_create(BDRVSheepdogState *s, uint32_t *vdi_id, int snapshot)
+static int do_sd_create(BDRVSheepdogState *s, uint32_t *vdi_id, int snapshot,
+                        Error **errp)
 {
-    Error *local_err = NULL;
     SheepdogVdiReq hdr;
     SheepdogVdiRsp *rsp = (SheepdogVdiRsp *)&hdr;
     int fd, ret;
     unsigned int wlen, rlen = 0;
     char buf[SD_MAX_VDI_LEN];
 
-    fd = connect_to_sdog(s, &local_err);
+    fd = connect_to_sdog(s, errp);
     if (fd < 0) {
-        qerror_report_err(local_err);
-        error_free(local_err);
         return fd;
     }
 
@@ -1522,11 +1520,12 @@ static int do_sd_create(BDRVSheepdogState *s, uint32_t *vdi_id, int snapshot)
     closesocket(fd);
 
     if (ret) {
+        error_setg_errno(errp, -ret, "create failed");
         return ret;
     }
 
     if (rsp->result != SD_RES_SUCCESS) {
-        error_report("%s, %s", sd_strerror(rsp->result), s->inode.name);
+        error_setg(errp, "%s, %s", sd_strerror(rsp->result), s->inode.name);
         return -EIO;
     }
 
@@ -1728,15 +1727,19 @@ static int sd_create(const char *filename, QEMUOptionParameter *options,
         bdrv_unref(bs);
     }
 
-    ret = do_sd_create(s, &vid, 0);
-    if (!prealloc || ret) {
+    ret = do_sd_create(s, &vid, 0, &local_err);
+    if (ret) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         goto out;
     }
 
-    ret = sd_prealloc(filename, &local_err);
-    if (ret < 0) {
-        qerror_report_err(local_err);
-        error_free(local_err);
+    if (prealloc) {
+        ret = sd_prealloc(filename, &local_err);
+        if (ret < 0) {
+            qerror_report_err(local_err);
+            error_free(local_err);
+        }
     }
 out:
     g_free(s);
@@ -1925,8 +1928,10 @@ static int sd_create_branch(BDRVSheepdogState *s)
      * false bail out.
      */
     deleted = sd_delete(s);
-    ret = do_sd_create(s, &vid, !deleted);
+    ret = do_sd_create(s, &vid, !deleted, &local_err);
     if (ret) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         goto out;
     }
 
@@ -2194,8 +2199,10 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
         goto cleanup;
     }
 
-    ret = do_sd_create(s, &new_vid, 1);
+    ret = do_sd_create(s, &new_vid, 1, &local_err);
     if (ret < 0) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         error_report("failed to create inode for snapshot. %s",
                      strerror(errno));
         goto cleanup;
-- 
1.8.1.4

[Qemu-devel] [PATCH 16/18] block/sheepdog: Propagate errors through find_vdi_name()

[Markus Armbruster]

Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/sheepdog.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index d20a94c..2cf192d 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1060,19 +1060,16 @@ static int parse_vdiname(BDRVSheepdogState *s, const char *filename,
 
 static int find_vdi_name(BDRVSheepdogState *s, const char *filename,
                          uint32_t snapid, const char *tag, uint32_t *vid,
-                         bool lock)
+                         bool lock, Error **errp)
 {
-    Error *local_err = NULL;
     int ret, fd;
     SheepdogVdiReq hdr;
     SheepdogVdiRsp *rsp = (SheepdogVdiRsp *)&hdr;
     unsigned int wlen, rlen = 0;
     char buf[SD_MAX_VDI_LEN + SD_MAX_VDI_TAG_LEN];
 
-    fd = connect_to_sdog(s, &local_err);
+    fd = connect_to_sdog(s, errp);
     if (fd < 0) {
-        qerror_report_err(local_err);
-        error_free(local_err);
         return fd;
     }
 
@@ -1097,12 +1094,13 @@ static int find_vdi_name(BDRVSheepdogState *s, const char *filename,
 
     ret = do_req(fd, (SheepdogReq *)&hdr, buf, &wlen, &rlen);
     if (ret) {
+        error_setg_errno(errp, -ret, "cannot get vdi info");
         goto out;
     }
 
     if (rsp->result != SD_RES_SUCCESS) {
-        error_report("cannot get vdi info, %s, %s %" PRIu32 " %s",
-                     sd_strerror(rsp->result), filename, snapid, tag);
+        error_setg(errp, "cannot get vdi info, %s, %s %" PRIu32 " %s",
+                   sd_strerror(rsp->result), filename, snapid, tag);
         if (rsp->result == SD_RES_NO_VDI) {
             ret = -ENOENT;
         } else {
@@ -1279,8 +1277,10 @@ static int reload_inode(BDRVSheepdogState *s, uint32_t snapid, const char *tag)
 
     inode = g_malloc(sizeof(s->inode));
 
-    ret = find_vdi_name(s, s->name, snapid, tag, &vid, false);
+    ret = find_vdi_name(s, s->name, snapid, tag, &vid, false, &local_err);
     if (ret) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         goto out;
     }
 
@@ -1423,8 +1423,10 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,
         goto out;
     }
 
-    ret = find_vdi_name(s, vdi, snapid, tag, &vid, true);
+    ret = find_vdi_name(s, vdi, snapid, tag, &vid, true, &local_err);
     if (ret) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         goto out;
     }
 
-- 
1.8.1.4

[Qemu-devel] [PATCH 17/18] block/sheepdog: Propagate errors to open and create methods

[Markus Armbruster]

Completes the conversion to Error started in commit 015a103^..d5124c0.

Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/sheepdog.c | 30 +++++++-----------------------
 1 file changed, 7 insertions(+), 23 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 2cf192d..cd5981e 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1391,8 +1391,7 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,
     opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
     qemu_opts_absorb_qdict(opts, options, &local_err);
     if (local_err) {
-        qerror_report_err(local_err);
-        error_free(local_err);
+        error_propagate(errp, local_err);
         ret = -EINVAL;
         goto out;
     }
@@ -1415,18 +1414,14 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,
     if (ret < 0) {
         goto out;
     }
-    s->fd = get_sheep_fd(s, &local_err);
+    s->fd = get_sheep_fd(s, errp);
     if (s->fd < 0) {
-        qerror_report_err(local_err);
-        error_free(local_err);
         ret = s->fd;
         goto out;
     }
 
-    ret = find_vdi_name(s, vdi, snapid, tag, &vid, true, &local_err);
+    ret = find_vdi_name(s, vdi, snapid, tag, &vid, true, errp);
     if (ret) {
-        qerror_report_err(local_err);
-        error_free(local_err);
         goto out;
     }
 
@@ -1445,10 +1440,8 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,
         s->is_snapshot = true;
     }
 
-    fd = connect_to_sdog(s, &local_err);
+    fd = connect_to_sdog(s, errp);
     if (fd < 0) {
-        qerror_report_err(local_err);
-        error_free(local_err);
         ret = fd;
         goto out;
     }
@@ -1648,7 +1641,6 @@ static int sd_create(const char *filename, QEMUOptionParameter *options,
     char tag[SD_MAX_VDI_TAG_LEN];
     uint32_t snapid;
     bool prealloc = false;
-    Error *local_err = NULL;
 
     s = g_malloc0(sizeof(BDRVSheepdogState));
 
@@ -1710,10 +1702,8 @@ static int sd_create(const char *filename, QEMUOptionParameter *options,
 
         bs = NULL;
         ret = bdrv_open(&bs, backing_file, NULL, NULL, BDRV_O_PROTOCOL, NULL,
-                        &local_err);
+                        errp);
         if (ret < 0) {
-            qerror_report_err(local_err);
-            error_free(local_err);
             goto out;
         }
 
@@ -1729,19 +1719,13 @@ static int sd_create(const char *filename, QEMUOptionParameter *options,
         bdrv_unref(bs);
     }
 
-    ret = do_sd_create(s, &vid, 0, &local_err);
+    ret = do_sd_create(s, &vid, 0, errp);
     if (ret) {
-        qerror_report_err(local_err);
-        error_free(local_err);
         goto out;
     }
 
     if (prealloc) {
-        ret = sd_prealloc(filename, &local_err);
-        if (ret < 0) {
-            qerror_report_err(local_err);
-            error_free(local_err);
-        }
+        ret = sd_prealloc(filename, errp);
     }
 out:
     g_free(s);
-- 
1.8.1.4

[Qemu-devel] [PATCH 18/18] block/sheepdog: Don't use qerror_report()

[Markus Armbruster]

qerror_report() is a transitional interface to help with converting
existing HMP commands to QMP.  It should not be used elsewhere.
Replace by error_report().

Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/sheepdog.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index cd5981e..d3386b6 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -723,7 +723,7 @@ static coroutine_fn void reconnect_to_sdog(void *opaque)
         s->fd = get_sheep_fd(s, &local_err);
         if (s->fd < 0) {
             DPRINTF("Wait for connection to be established\n");
-            qerror_report_err(local_err);
+            error_report("%s", error_get_pretty(local_err));
             error_free(local_err);
             co_aio_sleep_ns(bdrv_get_aio_context(s->bs), QEMU_CLOCK_REALTIME,
                             1000000000ULL);
@@ -1270,7 +1270,7 @@ static int reload_inode(BDRVSheepdogState *s, uint32_t snapid, const char *tag)
 
     fd = connect_to_sdog(s, &local_err);
     if (fd < 0) {
-        qerror_report_err(local_err);
+        error_report("%s", error_get_pretty(local_err));;
         error_free(local_err);
         return -EIO;
     }
@@ -1279,7 +1279,7 @@ static int reload_inode(BDRVSheepdogState *s, uint32_t snapid, const char *tag)
 
     ret = find_vdi_name(s, s->name, snapid, tag, &vid, false, &local_err);
     if (ret) {
-        qerror_report_err(local_err);
+        error_report("%s", error_get_pretty(local_err));;
         error_free(local_err);
         goto out;
     }
@@ -1745,7 +1745,7 @@ static void sd_close(BlockDriverState *bs)
 
     fd = connect_to_sdog(s, &local_err);
     if (fd < 0) {
-        qerror_report_err(local_err);
+        error_report("%s", error_get_pretty(local_err));;
         error_free(local_err);
         return;
     }
@@ -1796,7 +1796,7 @@ static int sd_truncate(BlockDriverState *bs, int64_t offset)
 
     fd = connect_to_sdog(s, &local_err);
     if (fd < 0) {
-        qerror_report_err(local_err);
+        error_report("%s", error_get_pretty(local_err));;
         error_free(local_err);
         return fd;
     }
@@ -1869,7 +1869,7 @@ static bool sd_delete(BDRVSheepdogState *s)
 
     fd = connect_to_sdog(s, &local_err);
     if (fd < 0) {
-        qerror_report_err(local_err);
+        error_report("%s", error_get_pretty(local_err));;
         error_free(local_err);
         return false;
     }
@@ -1916,7 +1916,7 @@ static int sd_create_branch(BDRVSheepdogState *s)
     deleted = sd_delete(s);
     ret = do_sd_create(s, &vid, !deleted, &local_err);
     if (ret) {
-        qerror_report_err(local_err);
+        error_report("%s", error_get_pretty(local_err));;
         error_free(local_err);
         goto out;
     }
@@ -1925,7 +1925,7 @@ static int sd_create_branch(BDRVSheepdogState *s)
 
     fd = connect_to_sdog(s, &local_err);
     if (fd < 0) {
-        qerror_report_err(local_err);
+        error_report("%s", error_get_pretty(local_err));;
         error_free(local_err);
         ret = fd;
         goto out;
@@ -2172,7 +2172,7 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
     /* refresh inode. */
     fd = connect_to_sdog(s, &local_err);
     if (fd < 0) {
-        qerror_report_err(local_err);
+        error_report("%s", error_get_pretty(local_err));;
         error_free(local_err);
         ret = fd;
         goto cleanup;
@@ -2187,7 +2187,7 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
 
     ret = do_sd_create(s, &new_vid, 1, &local_err);
     if (ret < 0) {
-        qerror_report_err(local_err);
+        error_report("%s", error_get_pretty(local_err));;
         error_free(local_err);
         error_report("failed to create inode for snapshot. %s",
                      strerror(errno));
@@ -2289,7 +2289,7 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
 
     fd = connect_to_sdog(s, &local_err);
     if (fd < 0) {
-        qerror_report_err(local_err);
+        error_report("%s", error_get_pretty(local_err));;
         error_free(local_err);
         ret = fd;
         goto out;
@@ -2318,7 +2318,7 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
 
     fd = connect_to_sdog(s, &local_err);
     if (fd < 0) {
-        qerror_report_err(local_err);
+        error_report("%s", error_get_pretty(local_err));;
         error_free(local_err);
         ret = fd;
         goto out;
@@ -2380,7 +2380,7 @@ static int do_load_save_vmstate(BDRVSheepdogState *s, uint8_t *data,
 
     fd = connect_to_sdog(s, &local_err);
     if (fd < 0) {
-        qerror_report_err(local_err);
+        error_report("%s", error_get_pretty(local_err));;
         error_free(local_err);
         return fd;
     }
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 01/18] blockdev: Don't use qerror_report_err() in drive_init()

[Eric Blake]

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

On 05/13/2014 10:02 AM, Markus Armbruster wrote:
> qerror_report_err() is a transitional interface to help with
> converting existing HMP commands to QMP.  It should not be used
> elsewhere.
> 
> drive_init() is not meant to be used by QMP commands.  It uses both
> qerror_report_err() and error_report().  Convert the former to the
> latter.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  blockdev.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH 02/18] blockdev: Don't use qerror_report() in do_drive_del()

[Eric Blake]

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

On 05/13/2014 10:02 AM, Markus Armbruster wrote:
> qerror_report() is a transitional interface to help with converting
> existing HMP commands to QMP.  It should not be used elsewhere.
> 
> do_drive_del() is an HMP command that won't be converted to QMP (we'll
> create a new QMP command instead).  It uses both qerror_report() and
> error_report().  Convert the former to the latter.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  blockdev.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH 03/18] qemu-nbd: Don't use qerror_report()

[Eric Blake]

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

On 05/13/2014 10:02 AM, Markus Armbruster wrote:
> qerror_report() is a transitional interface to help with converting
> existing HMP commands to QMP.  It should not be used elsewhere.
> Replace by error_report().
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qemu-nbd.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH 04/18] block/rbd: Propagate errors to open and create methods

[Eric Blake]

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

On 05/13/2014 10:02 AM, Markus Armbruster wrote:
> Completes the conversion to Error started in commit 015a103^..d5124c0.
> 
> Cc: Josh Durgin <josh.durgin@inktank.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/rbd.c | 66 +++++++++++++++++++++++++++++++------------------------------
>  1 file changed, 34 insertions(+), 32 deletions(-)
> 

> @@ -285,6 +289,7 @@ static int qemu_rbd_set_conf(rados_t cluster, const char *conf)
>  static int qemu_rbd_create(const char *filename, QEMUOptionParameter *options,
>                             Error **errp)
>  {
> +    Error *local_err = NULL;
>      int64_t bytes = 0;
>      int64_t objsize;
>      int obj_order = 0;
> @@ -301,7 +306,7 @@ static int qemu_rbd_create(const char *filename, QEMUOptionParameter *options,
>      if (qemu_rbd_parsename(filename, pool, sizeof(pool),
>                             snap_buf, sizeof(snap_buf),
>                             name, sizeof(name),
> -                           conf, sizeof(conf)) < 0) {
> +                           conf, sizeof(conf), &local_err) < 0) {
>          return -EINVAL;

Doesn't this leak local_err?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH 04/18] block/rbd: Propagate errors to open and create methods

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 05/13/2014 10:02 AM, Markus Armbruster wrote:
>> Completes the conversion to Error started in commit 015a103^..d5124c0.
>> 
>> Cc: Josh Durgin <josh.durgin@inktank.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block/rbd.c | 66 +++++++++++++++++++++++++++++++------------------------------
>>  1 file changed, 34 insertions(+), 32 deletions(-)
>> 
>
>> @@ -285,6 +289,7 @@ static int qemu_rbd_set_conf(rados_t cluster, const char *conf)
>>  static int qemu_rbd_create(const char *filename, QEMUOptionParameter *options,
>>                             Error **errp)
>>  {
>> +    Error *local_err = NULL;
>>      int64_t bytes = 0;
>>      int64_t objsize;
>>      int obj_order = 0;
>> @@ -301,7 +306,7 @@ static int qemu_rbd_create(const char *filename, QEMUOptionParameter *options,
>>      if (qemu_rbd_parsename(filename, pool, sizeof(pool),
>>                             snap_buf, sizeof(snap_buf),
>>                             name, sizeof(name),
>> -                           conf, sizeof(conf)) < 0) {
>> +                           conf, sizeof(conf), &local_err) < 0) {
>>          return -EINVAL;
>
> Doesn't this leak local_err?

error_propagate() missing, will fix, thanks!

Re: [Qemu-devel] [PATCH 05/18] block/ssh: Drop superfluous libssh2_session_last_errno() calls

[Richard W.M. Jones]

On Tue, May 13, 2014 at 06:02:39PM +0200, Markus Armbruster wrote:
> libssh2_session_last_error() already returns the error code.
> 
> Cc: "Richard W.M. Jones" <rjones@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/ssh.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/block/ssh.c b/block/ssh.c
> index aa63c9d..e38d232 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c
> @@ -121,10 +121,9 @@ session_error_report(BDRVSSHState *s, const char *fs, ...)
>          char *ssh_err;
>          int ssh_err_code;
>  
> -        libssh2_session_last_error((s)->session, &ssh_err, NULL, 0);
>          /* This is not an errno.  See <libssh2.h>. */
> -        ssh_err_code = libssh2_session_last_errno((s)->session);
> -
> +        ssh_err_code = libssh2_session_last_error(s->session,
> +                                                  &ssh_err, NULL, 0);
>          error_printf(": %s (libssh2 error code: %d)", ssh_err, ssh_err_code);
>      }
>  
> @@ -145,9 +144,9 @@ sftp_error_report(BDRVSSHState *s, const char *fs, ...)
>          int ssh_err_code;
>          unsigned long sftp_err_code;
>  
> -        libssh2_session_last_error((s)->session, &ssh_err, NULL, 0);
>          /* This is not an errno.  See <libssh2.h>. */
> -        ssh_err_code = libssh2_session_last_errno((s)->session);
> +        ssh_err_code = libssh2_session_last_error(s->session,
> +                                                  &ssh_err, NULL, 0);
>          /* See <libssh2_sftp.h>. */
>          sftp_err_code = libssh2_sftp_last_error((s)->sftp);

Yes, I'm not quite sure what was happening here.  I checked the source
of libssh2 and as you say, libssh2_session_last_error returns the
error code, so there is no need to call libssh2_session_last_errno as
well.

Therefore, ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

Re: [Qemu-devel] [PATCH 09/18] block/ssh: Propagate errors to open and create methods

[Richard W.M. Jones]

The last four patches are just a refactoring to use qemu error
handling, therefore ACK to all four of them.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

Re: [Qemu-devel] [PATCH 05/18] block/ssh: Drop superfluous libssh2_session_last_errno() calls

[Markus Armbruster]

"Richard W.M. Jones" <rjones@redhat.com> writes:

> On Tue, May 13, 2014 at 06:02:39PM +0200, Markus Armbruster wrote:
>> libssh2_session_last_error() already returns the error code.
>> 
>> Cc: "Richard W.M. Jones" <rjones@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block/ssh.c | 9 ++++-----
>>  1 file changed, 4 insertions(+), 5 deletions(-)
>> 
>> diff --git a/block/ssh.c b/block/ssh.c
>> index aa63c9d..e38d232 100644
>> --- a/block/ssh.c
>> +++ b/block/ssh.c
>> @@ -121,10 +121,9 @@ session_error_report(BDRVSSHState *s, const char *fs, ...)
>>          char *ssh_err;
>>          int ssh_err_code;
>>  
>> -        libssh2_session_last_error((s)->session, &ssh_err, NULL, 0);
>>          /* This is not an errno.  See <libssh2.h>. */
>> -        ssh_err_code = libssh2_session_last_errno((s)->session);
>> -
>> +        ssh_err_code = libssh2_session_last_error(s->session,
>> +                                                  &ssh_err, NULL, 0);
>>          error_printf(": %s (libssh2 error code: %d)", ssh_err, ssh_err_code);
>>      }
>>  
>> @@ -145,9 +144,9 @@ sftp_error_report(BDRVSSHState *s, const char *fs, ...)
>>          int ssh_err_code;
>>          unsigned long sftp_err_code;
>>  
>> -        libssh2_session_last_error((s)->session, &ssh_err, NULL, 0);
>>          /* This is not an errno.  See <libssh2.h>. */
>> -        ssh_err_code = libssh2_session_last_errno((s)->session);
>> +        ssh_err_code = libssh2_session_last_error(s->session,
>> +                                                  &ssh_err, NULL, 0);
>>          /* See <libssh2_sftp.h>. */
>>          sftp_err_code = libssh2_sftp_last_error((s)->sftp);
>
> Yes, I'm not quite sure what was happening here.  I checked the source
> of libssh2 and as you say, libssh2_session_last_error returns the
> error code, so there is no need to call libssh2_session_last_errno as
> well.
>
> Therefore, ACK.

Did you review the patch?  If yes, I'd like to convert your ACK to a
formal Reviewed-by.  Same for PATCH 06-09.

Re: [Qemu-devel] [PATCH 05/18] block/ssh: Drop superfluous libssh2_session_last_errno() calls

[Richard W.M. Jones]

On Wed, May 14, 2014 at 01:06:21PM +0200, Markus Armbruster wrote:
> "Richard W.M. Jones" <rjones@redhat.com> writes:
> 
> > On Tue, May 13, 2014 at 06:02:39PM +0200, Markus Armbruster wrote:
> >> libssh2_session_last_error() already returns the error code.
> >> 
> >> Cc: "Richard W.M. Jones" <rjones@redhat.com>
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  block/ssh.c | 9 ++++-----
> >>  1 file changed, 4 insertions(+), 5 deletions(-)
> >> 
> >> diff --git a/block/ssh.c b/block/ssh.c
> >> index aa63c9d..e38d232 100644
> >> --- a/block/ssh.c
> >> +++ b/block/ssh.c
> >> @@ -121,10 +121,9 @@ session_error_report(BDRVSSHState *s, const char *fs, ...)
> >>          char *ssh_err;
> >>          int ssh_err_code;
> >>  
> >> -        libssh2_session_last_error((s)->session, &ssh_err, NULL, 0);
> >>          /* This is not an errno.  See <libssh2.h>. */
> >> -        ssh_err_code = libssh2_session_last_errno((s)->session);
> >> -
> >> +        ssh_err_code = libssh2_session_last_error(s->session,
> >> +                                                  &ssh_err, NULL, 0);
> >>          error_printf(": %s (libssh2 error code: %d)", ssh_err, ssh_err_code);
> >>      }
> >>  
> >> @@ -145,9 +144,9 @@ sftp_error_report(BDRVSSHState *s, const char *fs, ...)
> >>          int ssh_err_code;
> >>          unsigned long sftp_err_code;
> >>  
> >> -        libssh2_session_last_error((s)->session, &ssh_err, NULL, 0);
> >>          /* This is not an errno.  See <libssh2.h>. */
> >> -        ssh_err_code = libssh2_session_last_errno((s)->session);
> >> +        ssh_err_code = libssh2_session_last_error(s->session,
> >> +                                                  &ssh_err, NULL, 0);
> >>          /* See <libssh2_sftp.h>. */
> >>          sftp_err_code = libssh2_sftp_last_error((s)->sftp);
> >
> > Yes, I'm not quite sure what was happening here.  I checked the source
> > of libssh2 and as you say, libssh2_session_last_error returns the
> > error code, so there is no need to call libssh2_session_last_errno as
> > well.
> >
> > Therefore, ACK.
> 
> Did you review the patch?  If yes, I'd like to convert your ACK to a
> formal Reviewed-by.  Same for PATCH 06-09.

Yes, I applied all 5 of them to qemu.git, compiled it and also checked
over the patches.

So ACK to all of them.

Do I need to do something else?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

Re: [Qemu-devel] [PATCH 05/18] block/ssh: Drop superfluous libssh2_session_last_errno() calls

[Markus Armbruster]

"Richard W.M. Jones" <rjones@redhat.com> writes:

> On Wed, May 14, 2014 at 01:06:21PM +0200, Markus Armbruster wrote:
>> "Richard W.M. Jones" <rjones@redhat.com> writes:
>> 
>> > On Tue, May 13, 2014 at 06:02:39PM +0200, Markus Armbruster wrote:
>> >> libssh2_session_last_error() already returns the error code.
>> >> 
>> >> Cc: "Richard W.M. Jones" <rjones@redhat.com>
>> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> >> ---
>> >>  block/ssh.c | 9 ++++-----
>> >>  1 file changed, 4 insertions(+), 5 deletions(-)
>> >> 
>> >> diff --git a/block/ssh.c b/block/ssh.c
>> >> index aa63c9d..e38d232 100644
>> >> --- a/block/ssh.c
>> >> +++ b/block/ssh.c
>> >> @@ -121,10 +121,9 @@ session_error_report(BDRVSSHState *s, const char *fs, ...)
>> >>          char *ssh_err;
>> >>          int ssh_err_code;
>> >>  
>> >> -        libssh2_session_last_error((s)->session, &ssh_err, NULL, 0);
>> >>          /* This is not an errno.  See <libssh2.h>. */
>> >> -        ssh_err_code = libssh2_session_last_errno((s)->session);
>> >> -
>> >> +        ssh_err_code = libssh2_session_last_error(s->session,
>> >> +                                                  &ssh_err, NULL, 0);
>> >>          error_printf(": %s (libssh2 error code: %d)", ssh_err, ssh_err_code);
>> >>      }
>> >>  
>> >> @@ -145,9 +144,9 @@ sftp_error_report(BDRVSSHState *s, const char *fs, ...)
>> >>          int ssh_err_code;
>> >>          unsigned long sftp_err_code;
>> >>  
>> >> -        libssh2_session_last_error((s)->session, &ssh_err, NULL, 0);
>> >>          /* This is not an errno.  See <libssh2.h>. */
>> >> -        ssh_err_code = libssh2_session_last_errno((s)->session);
>> >> +        ssh_err_code = libssh2_session_last_error(s->session,
>> >> +                                                  &ssh_err, NULL, 0);
>> >>          /* See <libssh2_sftp.h>. */
>> >>          sftp_err_code = libssh2_sftp_last_error((s)->sftp);
>> >
>> > Yes, I'm not quite sure what was happening here.  I checked the source
>> > of libssh2 and as you say, libssh2_session_last_error returns the
>> > error code, so there is no need to call libssh2_session_last_errno as
>> > well.
>> >
>> > Therefore, ACK.
>> 
>> Did you review the patch?  If yes, I'd like to convert your ACK to a
>> formal Reviewed-by.  Same for PATCH 06-09.
>
> Yes, I applied all 5 of them to qemu.git, compiled it and also checked
> over the patches.
>
> So ACK to all of them.
>
> Do I need to do something else?

We're pretty formal about commit tags on this list.

We interpret "Acked-by" conservatively.  Something like "this patch
touches stuff I'm responsible for, I'm aware of the patch, and I don't
object to it".

To say "I reviewed this patch, and I think it's ready for commit", we
use "Reviewed-by".

"ACK" is neither, so I asked for clarification :)

Re: [Qemu-devel] [PATCH 05/18] block/ssh: Drop superfluous libssh2_session_last_errno() calls

[Richard W.M. Jones]

On Tue, May 13, 2014 at 06:02:39PM +0200, Markus Armbruster wrote:
> libssh2_session_last_error() already returns the error code.
> 
> Cc: "Richard W.M. Jones" <rjones@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>

>  block/ssh.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/block/ssh.c b/block/ssh.c
> index aa63c9d..e38d232 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c
> @@ -121,10 +121,9 @@ session_error_report(BDRVSSHState *s, const char *fs, ...)
>          char *ssh_err;
>          int ssh_err_code;
>  
> -        libssh2_session_last_error((s)->session, &ssh_err, NULL, 0);
>          /* This is not an errno.  See <libssh2.h>. */
> -        ssh_err_code = libssh2_session_last_errno((s)->session);
> -
> +        ssh_err_code = libssh2_session_last_error(s->session,
> +                                                  &ssh_err, NULL, 0);
>          error_printf(": %s (libssh2 error code: %d)", ssh_err, ssh_err_code);
>      }
>  
> @@ -145,9 +144,9 @@ sftp_error_report(BDRVSSHState *s, const char *fs, ...)
>          int ssh_err_code;
>          unsigned long sftp_err_code;
>  
> -        libssh2_session_last_error((s)->session, &ssh_err, NULL, 0);
>          /* This is not an errno.  See <libssh2.h>. */
> -        ssh_err_code = libssh2_session_last_errno((s)->session);
> +        ssh_err_code = libssh2_session_last_error(s->session,
> +                                                  &ssh_err, NULL, 0);
>          /* See <libssh2_sftp.h>. */
>          sftp_err_code = libssh2_sftp_last_error((s)->sftp);
>  
> -- 
> 1.8.1.4

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Re: [Qemu-devel] [PATCH 07/18] block/ssh: Propagate errors through authenticate()

[Richard W.M. Jones]

On Tue, May 13, 2014 at 06:02:41PM +0200, Markus Armbruster wrote:
> Cc: "Richard W.M. Jones" <rjones@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>

>  block/ssh.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/block/ssh.c b/block/ssh.c
> index 1df1946..18186ba 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c
> @@ -434,7 +434,7 @@ static int check_host_key(BDRVSSHState *s, const char *host, int port,
>      return -EINVAL;
>  }
>  
> -static int authenticate(BDRVSSHState *s, const char *user)
> +static int authenticate(BDRVSSHState *s, const char *user, Error **errp)
>  {
>      int r, ret;
>      const char *userauthlist;
> @@ -445,7 +445,8 @@ static int authenticate(BDRVSSHState *s, const char *user)
>      userauthlist = libssh2_userauth_list(s->session, user, strlen(user));
>      if (strstr(userauthlist, "publickey") == NULL) {
>          ret = -EPERM;
> -        error_report("remote server does not support \"publickey\" authentication");
> +        error_setg(errp,
> +                "remote server does not support \"publickey\" authentication");
>          goto out;
>      }
>  
> @@ -453,17 +454,18 @@ static int authenticate(BDRVSSHState *s, const char *user)
>      agent = libssh2_agent_init(s->session);
>      if (!agent) {
>          ret = -EINVAL;
> -        session_error_report(s, "failed to initialize ssh-agent support");
> +        session_error_setg(errp, s, "failed to initialize ssh-agent support");
>          goto out;
>      }
>      if (libssh2_agent_connect(agent)) {
>          ret = -ECONNREFUSED;
> -        session_error_report(s, "failed to connect to ssh-agent");
> +        session_error_setg(errp, s, "failed to connect to ssh-agent");
>          goto out;
>      }
>      if (libssh2_agent_list_identities(agent)) {
>          ret = -EINVAL;
> -        session_error_report(s, "failed requesting identities from ssh-agent");
> +        session_error_setg(errp, s,
> +                           "failed requesting identities from ssh-agent");
>          goto out;
>      }
>  
> @@ -474,7 +476,8 @@ static int authenticate(BDRVSSHState *s, const char *user)
>          }
>          if (r < 0) {
>              ret = -EINVAL;
> -            session_error_report(s, "failed to obtain identity from ssh-agent");
> +            session_error_setg(errp, s,
> +                               "failed to obtain identity from ssh-agent");
>              goto out;
>          }
>          r = libssh2_agent_userauth(agent, user, identity);
> @@ -488,8 +491,8 @@ static int authenticate(BDRVSSHState *s, const char *user)
>      }
>  
>      ret = -EPERM;
> -    error_report("failed to authenticate using publickey authentication "
> -                 "and the identities held by your ssh-agent");
> +    error_setg(errp, "failed to authenticate using publickey authentication "
> +               "and the identities held by your ssh-agent");
>  
>   out:
>      if (agent != NULL) {
> @@ -577,8 +580,10 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
>      }
>  
>      /* Authenticate. */
> -    ret = authenticate(s, user);
> +    ret = authenticate(s, user, &err);
>      if (ret < 0) {
> +        qerror_report_err(err);
> +        error_free(err);
>          goto err;
>      }
>  
> -- 
> 1.8.1.4

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

Re: [Qemu-devel] [PATCH 06/18] block/ssh: Propagate errors through check_host_key()

[Richard W.M. Jones]

On Tue, May 13, 2014 at 06:02:40PM +0200, Markus Armbruster wrote:
> Cc: "Richard W.M. Jones" <rjones@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>

>  block/ssh.c | 68 ++++++++++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 49 insertions(+), 19 deletions(-)
> 
> diff --git a/block/ssh.c b/block/ssh.c
> index e38d232..1df1946 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c
> @@ -106,6 +106,31 @@ static void ssh_state_free(BDRVSSHState *s)
>      }
>  }
>  
> +static void GCC_FMT_ATTR(3, 4)
> +session_error_setg(Error **errp, BDRVSSHState *s, const char *fs, ...)
> +{
> +    va_list args;
> +    char *msg;
> +
> +    va_start(args, fs);
> +    msg = g_strdup_vprintf(fs, args);
> +    va_end(args);
> +
> +    if (s->session) {
> +        char *ssh_err;
> +        int ssh_err_code;
> +
> +        /* This is not an errno.  See <libssh2.h>. */
> +        ssh_err_code = libssh2_session_last_error(s->session,
> +                                                  &ssh_err, NULL, 0);
> +        error_setg(errp, "%s: %s (libssh2 error code: %d)",
> +                   msg, ssh_err, ssh_err_code);
> +    } else {
> +        error_setg(errp, "%s", msg);
> +    }
> +    g_free(msg);
> +}
> +
>  /* Wrappers around error_report which make sure to dump as much
>   * information from libssh2 as possible.
>   */
> @@ -242,7 +267,7 @@ static void ssh_parse_filename(const char *filename, QDict *options,
>  }
>  
>  static int check_host_key_knownhosts(BDRVSSHState *s,
> -                                     const char *host, int port)
> +                                     const char *host, int port, Error **errp)
>  {
>      const char *home;
>      char *knh_file = NULL;
> @@ -256,14 +281,15 @@ static int check_host_key_knownhosts(BDRVSSHState *s,
>      hostkey = libssh2_session_hostkey(s->session, &len, &type);
>      if (!hostkey) {
>          ret = -EINVAL;
> -        session_error_report(s, "failed to read remote host key");
> +        session_error_setg(errp, s, "failed to read remote host key");
>          goto out;
>      }
>  
>      knh = libssh2_knownhost_init(s->session);
>      if (!knh) {
>          ret = -EINVAL;
> -        session_error_report(s, "failed to initialize known hosts support");
> +        session_error_setg(errp, s,
> +                           "failed to initialize known hosts support");
>          goto out;
>      }
>  
> @@ -288,21 +314,23 @@ static int check_host_key_knownhosts(BDRVSSHState *s,
>          break;
>      case LIBSSH2_KNOWNHOST_CHECK_MISMATCH:
>          ret = -EINVAL;
> -        session_error_report(s, "host key does not match the one in known_hosts (found key %s)",
> -                             found->key);
> +        session_error_setg(errp, s,
> +                      "host key does not match the one in known_hosts"
> +                      " (found key %s)", found->key);
>          goto out;
>      case LIBSSH2_KNOWNHOST_CHECK_NOTFOUND:
>          ret = -EINVAL;
> -        session_error_report(s, "no host key was found in known_hosts");
> +        session_error_setg(errp, s, "no host key was found in known_hosts");
>          goto out;
>      case LIBSSH2_KNOWNHOST_CHECK_FAILURE:
>          ret = -EINVAL;
> -        session_error_report(s, "failure matching the host key with known_hosts");
> +        session_error_setg(errp, s,
> +                      "failure matching the host key with known_hosts");
>          goto out;
>      default:
>          ret = -EINVAL;
> -        session_error_report(s, "unknown error matching the host key with known_hosts (%d)",
> -                             r);
> +        session_error_setg(errp, s, "unknown error matching the host key"
> +                      " with known_hosts (%d)", r);
>          goto out;
>      }
>  
> @@ -357,20 +385,20 @@ static int compare_fingerprint(const unsigned char *fingerprint, size_t len,
>  
>  static int
>  check_host_key_hash(BDRVSSHState *s, const char *hash,
> -                    int hash_type, size_t fingerprint_len)
> +                    int hash_type, size_t fingerprint_len, Error **errp)
>  {
>      const char *fingerprint;
>  
>      fingerprint = libssh2_hostkey_hash(s->session, hash_type);
>      if (!fingerprint) {
> -        session_error_report(s, "failed to read remote host key");
> +        session_error_setg(errp, s, "failed to read remote host key");
>          return -EINVAL;
>      }
>  
>      if(compare_fingerprint((unsigned char *) fingerprint, fingerprint_len,
>                             hash) != 0) {
> -        error_report("remote host key does not match host_key_check '%s'",
> -                     hash);
> +        error_setg(errp, "remote host key does not match host_key_check '%s'",
> +                   hash);
>          return -EPERM;
>      }
>  
> @@ -378,7 +406,7 @@ check_host_key_hash(BDRVSSHState *s, const char *hash,
>  }
>  
>  static int check_host_key(BDRVSSHState *s, const char *host, int port,
> -                          const char *host_key_check)
> +                          const char *host_key_check, Error **errp)
>  {
>      /* host_key_check=no */
>      if (strcmp(host_key_check, "no") == 0) {
> @@ -388,21 +416,21 @@ static int check_host_key(BDRVSSHState *s, const char *host, int port,
>      /* host_key_check=md5:xx:yy:zz:... */
>      if (strncmp(host_key_check, "md5:", 4) == 0) {
>          return check_host_key_hash(s, &host_key_check[4],
> -                                   LIBSSH2_HOSTKEY_HASH_MD5, 16);
> +                                   LIBSSH2_HOSTKEY_HASH_MD5, 16, errp);
>      }
>  
>      /* host_key_check=sha1:xx:yy:zz:... */
>      if (strncmp(host_key_check, "sha1:", 5) == 0) {
>          return check_host_key_hash(s, &host_key_check[5],
> -                                   LIBSSH2_HOSTKEY_HASH_SHA1, 20);
> +                                   LIBSSH2_HOSTKEY_HASH_SHA1, 20, errp);
>      }
>  
>      /* host_key_check=yes */
>      if (strcmp(host_key_check, "yes") == 0) {
> -        return check_host_key_knownhosts(s, host, port);
> +        return check_host_key_knownhosts(s, host, port, errp);
>      }
>  
> -    error_report("unknown host_key_check setting (%s)", host_key_check);
> +    error_setg(errp, "unknown host_key_check setting (%s)", host_key_check);
>      return -EINVAL;
>  }
>  
> @@ -541,8 +569,10 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
>      }
>  
>      /* Check the remote host's key against known_hosts. */
> -    ret = check_host_key(s, host, port, host_key_check);
> +    ret = check_host_key(s, host, port, host_key_check, &err);
>      if (ret < 0) {
> +        qerror_report_err(err);
> +        error_free(err);
>          goto err;
>      }
>  
> -- 
> 1.8.1.4

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

Re: [Qemu-devel] [PATCH 08/18] block/ssh: Propagate errors through connect_to_ssh()

[Richard W.M. Jones]

On Tue, May 13, 2014 at 06:02:42PM +0200, Markus Armbruster wrote:
> Cc: "Richard W.M. Jones" <rjones@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>

>  block/ssh.c | 34 +++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/block/ssh.c b/block/ssh.c
> index 18186ba..26078c4 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c
> @@ -506,10 +506,9 @@ static int authenticate(BDRVSSHState *s, const char *user, Error **errp)
>  }
>  
>  static int connect_to_ssh(BDRVSSHState *s, QDict *options,
> -                          int ssh_flags, int creat_mode)
> +                          int ssh_flags, int creat_mode, Error **errp)
>  {
>      int r, ret;
> -    Error *err = NULL;
>      const char *host, *user, *path, *host_key_check;
>      int port;
>  
> @@ -528,6 +527,7 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
>      } else {
>          user = g_get_user_name();
>          if (!user) {
> +            error_setg_errno(errp, errno, "Can't get user name");
>              ret = -errno;
>              goto err;
>          }
> @@ -544,11 +544,9 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
>      s->hostport = g_strdup_printf("%s:%d", host, port);
>  
>      /* Open the socket and connect. */
> -    s->sock = inet_connect(s->hostport, &err);
> -    if (err != NULL) {
> +    s->sock = inet_connect(s->hostport, errp);
> +    if (s->sock < 0) {
>          ret = -errno;
> -        qerror_report_err(err);
> -        error_free(err);
>          goto err;
>      }
>  
> @@ -556,7 +554,7 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
>      s->session = libssh2_session_init();
>      if (!s->session) {
>          ret = -EINVAL;
> -        session_error_report(s, "failed to initialize libssh2 session");
> +        session_error_setg(errp, s, "failed to initialize libssh2 session");
>          goto err;
>      }
>  
> @@ -567,30 +565,26 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
>      r = libssh2_session_handshake(s->session, s->sock);
>      if (r != 0) {
>          ret = -EINVAL;
> -        session_error_report(s, "failed to establish SSH session");
> +        session_error_setg(errp, s, "failed to establish SSH session");
>          goto err;
>      }
>  
>      /* Check the remote host's key against known_hosts. */
> -    ret = check_host_key(s, host, port, host_key_check, &err);
> +    ret = check_host_key(s, host, port, host_key_check, errp);
>      if (ret < 0) {
> -        qerror_report_err(err);
> -        error_free(err);
>          goto err;
>      }
>  
>      /* Authenticate. */
> -    ret = authenticate(s, user, &err);
> +    ret = authenticate(s, user, errp);
>      if (ret < 0) {
> -        qerror_report_err(err);
> -        error_free(err);
>          goto err;
>      }
>  
>      /* Start SFTP. */
>      s->sftp = libssh2_sftp_init(s->session);
>      if (!s->sftp) {
> -        session_error_report(s, "failed to initialize sftp handle");
> +        session_error_setg(errp, s, "failed to initialize sftp handle");
>          ret = -EINVAL;
>          goto err;
>      }
> @@ -645,6 +639,7 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
>  static int ssh_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags,
>                           Error **errp)
>  {
> +    Error *local_err = NULL;
>      BDRVSSHState *s = bs->opaque;
>      int ret;
>      int ssh_flags;
> @@ -657,8 +652,10 @@ static int ssh_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags,
>      }
>  
>      /* Start up SSH. */
> -    ret = connect_to_ssh(s, options, ssh_flags, 0);
> +    ret = connect_to_ssh(s, options, ssh_flags, 0, &local_err);
>      if (ret < 0) {
> +        qerror_report_err(local_err);
> +        error_free(local_err);
>          goto err;
>      }
>  
> @@ -718,8 +715,11 @@ static int ssh_create(const char *filename, QEMUOptionParameter *options,
>  
>      r = connect_to_ssh(&s, uri_options,
>                         LIBSSH2_FXF_READ|LIBSSH2_FXF_WRITE|
> -                       LIBSSH2_FXF_CREAT|LIBSSH2_FXF_TRUNC, 0644);
> +                       LIBSSH2_FXF_CREAT|LIBSSH2_FXF_TRUNC,
> +                       0644, &local_err);
>      if (r < 0) {
> +        qerror_report_err(local_err);
> +        error_free(local_err);
>          ret = r;
>          goto out;
>      }
> -- 
> 1.8.1.4

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

Re: [Qemu-devel] [PATCH 09/18] block/ssh: Propagate errors to open and create methods

[Richard W.M. Jones]

On Tue, May 13, 2014 at 06:02:43PM +0200, Markus Armbruster wrote:
> Completes the conversion to Error started in commit 015a103^..d5124c0.
> 
> Cc: "Richard W.M. Jones" <rjones@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>

>  block/ssh.c | 47 ++++++++++++++++++++++-------------------------
>  1 file changed, 22 insertions(+), 25 deletions(-)
> 
> diff --git a/block/ssh.c b/block/ssh.c
> index 26078c4..b212971 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c
> @@ -131,29 +131,34 @@ session_error_setg(Error **errp, BDRVSSHState *s, const char *fs, ...)
>      g_free(msg);
>  }
>  
> -/* Wrappers around error_report which make sure to dump as much
> - * information from libssh2 as possible.
> - */
> -static void GCC_FMT_ATTR(2, 3)
> -session_error_report(BDRVSSHState *s, const char *fs, ...)
> +static void GCC_FMT_ATTR(3, 4)
> +sftp_error_setg(Error **errp, BDRVSSHState *s, const char *fs, ...)
>  {
>      va_list args;
> +    char *msg;
>  
>      va_start(args, fs);
> -    error_vprintf(fs, args);
> +    msg = g_strdup_vprintf(fs, args);
> +    va_end(args);
>  
> -    if ((s)->session) {
> +    if (s->sftp) {
>          char *ssh_err;
>          int ssh_err_code;
> +        unsigned long sftp_err_code;
>  
>          /* This is not an errno.  See <libssh2.h>. */
>          ssh_err_code = libssh2_session_last_error(s->session,
>                                                    &ssh_err, NULL, 0);
> -        error_printf(": %s (libssh2 error code: %d)", ssh_err, ssh_err_code);
> -    }
> +        /* See <libssh2_sftp.h>. */
> +        sftp_err_code = libssh2_sftp_last_error((s)->sftp);
>  
> -    va_end(args);
> -    error_printf("\n");
> +        error_setg(errp,
> +                   "%s: %s (libssh2 error code: %d, sftp error code: %lu)",
> +                   msg, ssh_err, ssh_err_code, sftp_err_code);
> +    } else {
> +        error_setg(errp, "%s", msg);
> +    }
> +    g_free(msg);
>  }
>  
>  static void GCC_FMT_ATTR(2, 3)
> @@ -594,14 +599,14 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
>              path, ssh_flags, creat_mode);
>      s->sftp_handle = libssh2_sftp_open(s->sftp, path, ssh_flags, creat_mode);
>      if (!s->sftp_handle) {
> -        session_error_report(s, "failed to open remote file '%s'", path);
> +        session_error_setg(errp, s, "failed to open remote file '%s'", path);
>          ret = -EINVAL;
>          goto err;
>      }
>  
>      r = libssh2_sftp_fstat(s->sftp_handle, &s->attrs);
>      if (r < 0) {
> -        sftp_error_report(s, "failed to read file attributes");
> +        sftp_error_setg(errp, s, "failed to read file attributes");
>          return -EINVAL;
>      }
>  
> @@ -639,7 +644,6 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
>  static int ssh_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags,
>                           Error **errp)
>  {
> -    Error *local_err = NULL;
>      BDRVSSHState *s = bs->opaque;
>      int ret;
>      int ssh_flags;
> @@ -652,10 +656,8 @@ static int ssh_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags,
>      }
>  
>      /* Start up SSH. */
> -    ret = connect_to_ssh(s, options, ssh_flags, 0, &local_err);
> +    ret = connect_to_ssh(s, options, ssh_flags, 0, errp);
>      if (ret < 0) {
> -        qerror_report_err(local_err);
> -        error_free(local_err);
>          goto err;
>      }
>  
> @@ -686,7 +688,6 @@ static int ssh_create(const char *filename, QEMUOptionParameter *options,
>                        Error **errp)
>  {
>      int r, ret;
> -    Error *local_err = NULL;
>      int64_t total_size = 0;
>      QDict *uri_options = NULL;
>      BDRVSSHState s;
> @@ -705,10 +706,8 @@ static int ssh_create(const char *filename, QEMUOptionParameter *options,
>      DPRINTF("total_size=%" PRIi64, total_size);
>  
>      uri_options = qdict_new();
> -    r = parse_uri(filename, uri_options, &local_err);
> +    r = parse_uri(filename, uri_options, errp);
>      if (r < 0) {
> -        qerror_report_err(local_err);
> -        error_free(local_err);
>          ret = r;
>          goto out;
>      }
> @@ -716,10 +715,8 @@ static int ssh_create(const char *filename, QEMUOptionParameter *options,
>      r = connect_to_ssh(&s, uri_options,
>                         LIBSSH2_FXF_READ|LIBSSH2_FXF_WRITE|
>                         LIBSSH2_FXF_CREAT|LIBSSH2_FXF_TRUNC,
> -                       0644, &local_err);
> +                       0644, errp);
>      if (r < 0) {
> -        qerror_report_err(local_err);
> -        error_free(local_err);
>          ret = r;
>          goto out;
>      }
> @@ -728,7 +725,7 @@ static int ssh_create(const char *filename, QEMUOptionParameter *options,
>          libssh2_sftp_seek64(s.sftp_handle, total_size-1);
>          r2 = libssh2_sftp_write(s.sftp_handle, c, 1);
>          if (r2 < 0) {
> -            sftp_error_report(&s, "truncate failed");
> +            sftp_error_setg(errp, &s, "truncate failed");
>              ret = -EINVAL;
>              goto out;
>          }
> -- 
> 1.8.1.4

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

Re: [Qemu-devel] [PATCH 10/18] block/vvfat: Propagate errors through enable_write_target()

[Eric Blake]

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

On 05/13/2014 10:02 AM, Markus Armbruster wrote:
> Continues the conversion of the open method to Error started in commit
> 015a103.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/vvfat.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 

>      s->qcow = NULL;
>      ret = bdrv_open(&s->qcow, s->qcow_filename, NULL, NULL,
>              BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, bdrv_qcow,
> -            &local_err);
> +            errp);

Pre-existing; indentation looks odd here if you want to touch it up at
the same time.  But that's not a show-stopper.

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH 10/18] block/vvfat: Propagate errors through enable_write_target()

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 05/13/2014 10:02 AM, Markus Armbruster wrote:
>> Continues the conversion of the open method to Error started in commit
>> 015a103.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block/vvfat.c | 16 ++++++----------
>>  1 file changed, 6 insertions(+), 10 deletions(-)
>> 
>
>>      s->qcow = NULL;
>>      ret = bdrv_open(&s->qcow, s->qcow_filename, NULL, NULL,
>>              BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, bdrv_qcow,
>> -            &local_err);
>> +            errp);
>
> Pre-existing; indentation looks odd here if you want to touch it up at
> the same time.  But that's not a show-stopper.

Can take care of it when I respin to correct the leak in 04/18.

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

Thanks!

Re: [Qemu-devel] [PATCH 11/18] block/vvfat: Propagate errors through init_directories()

[Eric Blake]

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

On 05/13/2014 10:02 AM, Markus Armbruster wrote:
> Completes the conversion of the open method to Error started in commit
> 015a103.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/vvfat.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> @@ -892,8 +893,8 @@ static int init_directories(BDRVVVFATState* s,
>          if (mapping->mode & MODE_DIRECTORY) {
>  	    mapping->begin = cluster;
>  	    if(read_directory(s, i)) {
> -		fprintf(stderr, "Could not read directory %s\n",
> -			mapping->path);
> +                error_setg(errp, "Could not read directory %s",
> +                           mapping->path);

I see you fixed some TABs in the process; is it worth widening the fix
to the rest of the 'if' statement?  I don't care either way, as long as
checkpatch.pl didn't call you out (the new code is correct, even though
the existing code was not).

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH 12/18] block/sheepdog: Propagate errors through connect_to_sdog()

[Eric Blake]

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

On 05/13/2014 10:02 AM, Markus Armbruster wrote:
> Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/sheepdog.c | 77 ++++++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 55 insertions(+), 22 deletions(-)
> 

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


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH 11/18] block/vvfat: Propagate errors through init_directories()

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 05/13/2014 10:02 AM, Markus Armbruster wrote:
>> Completes the conversion of the open method to Error started in commit
>> 015a103.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block/vvfat.c | 16 +++++++++-------
>>  1 file changed, 9 insertions(+), 7 deletions(-)
>> 
>> @@ -892,8 +893,8 @@ static int init_directories(BDRVVVFATState* s,
>>          if (mapping->mode & MODE_DIRECTORY) {
>>  	    mapping->begin = cluster;
>>  	    if(read_directory(s, i)) {
>> -		fprintf(stderr, "Could not read directory %s\n",
>> -			mapping->path);
>> +                error_setg(errp, "Could not read directory %s",
>> +                           mapping->path);
>
> I see you fixed some TABs in the process; is it worth widening the fix
> to the rest of the 'if' statement?  I don't care either way, as long as
> checkpatch.pl didn't call you out (the new code is correct, even though
> the existing code was not).

checkpatch is happy.

The fewer lines in vvfat.c git blames on me, the happier I am ;)

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

Thanks!

Re: [Qemu-devel] [PATCH 13/18] block/sheepdog: Propagate errors through get_sheep_fd()

[Eric Blake]

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

On 05/13/2014 10:02 AM, Markus Armbruster wrote:
> Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/sheepdog.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH 14/18] block/sheepdog: Propagate errors through sd_prealloc()

[Eric Blake]

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

On 05/13/2014 10:02 AM, Markus Armbruster wrote:
> Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/sheepdog.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> @@ -1568,13 +1565,15 @@ static int sd_prealloc(const char *filename)
>           */
>          ret = bdrv_pread(bs, idx * SD_DATA_OBJ_SIZE, buf, SD_DATA_OBJ_SIZE);
>          if (ret < 0) {
> -            goto out;
> +            goto out_setg;
>          }
>          ret = bdrv_pwrite(bs, idx * SD_DATA_OBJ_SIZE, buf, SD_DATA_OBJ_SIZE);
>          if (ret < 0) {
> -            goto out;
> +            goto out_setg;
>          }
>      }
> +out_setg:
> +    error_setg_errno(errp, -ret, "Can't pre-allocate");

This unconditionally sets errp even when the for loop completes
normally.  Are you sure you want this control flow?  Maybe you are
missing a 'goto out' prior to the 'out_setg' label?


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH 15/18] block/sheepdog: Propagate errors through do_sd_create()

[Eric Blake]

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

On 05/13/2014 10:02 AM, Markus Armbruster wrote:
> Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/sheepdog.c | 35 +++++++++++++++++++++--------------
>  1 file changed, 21 insertions(+), 14 deletions(-)
> 

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH 16/18] block/sheepdog: Propagate errors through find_vdi_name()

[Eric Blake]

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

On 05/13/2014 10:02 AM, Markus Armbruster wrote:
> Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/sheepdog.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH 17/18] block/sheepdog: Propagate errors to open and create methods

[Eric Blake]

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

On 05/13/2014 10:02 AM, Markus Armbruster wrote:
> Completes the conversion to Error started in commit 015a103^..d5124c0.
> 
> Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/sheepdog.c | 30 +++++++-----------------------
>  1 file changed, 7 insertions(+), 23 deletions(-)
> 

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH 18/18] block/sheepdog: Don't use qerror_report()

[Eric Blake]

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

On 05/13/2014 10:02 AM, Markus Armbruster wrote:
> qerror_report() is a transitional interface to help with converting
> existing HMP commands to QMP.  It should not be used elsewhere.
> Replace by error_report().
> 
> Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/sheepdog.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH 14/18] block/sheepdog: Propagate errors through sd_prealloc()

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 05/13/2014 10:02 AM, Markus Armbruster wrote:
>> Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block/sheepdog.c | 21 ++++++++++++---------
>>  1 file changed, 12 insertions(+), 9 deletions(-)
>> 
>> @@ -1568,13 +1565,15 @@ static int sd_prealloc(const char *filename)
>>           */
>>          ret = bdrv_pread(bs, idx * SD_DATA_OBJ_SIZE, buf, SD_DATA_OBJ_SIZE);
>>          if (ret < 0) {
>> -            goto out;
>> +            goto out_setg;
>>          }
>>          ret = bdrv_pwrite(bs, idx * SD_DATA_OBJ_SIZE, buf, SD_DATA_OBJ_SIZE);
>>          if (ret < 0) {
>> -            goto out;
>> +            goto out_setg;
>>          }
>>      }
>> +out_setg:
>> +    error_setg_errno(errp, -ret, "Can't pre-allocate");
>
> This unconditionally sets errp even when the for loop completes
> normally.  Are you sure you want this control flow?  Maybe you are
> missing a 'goto out' prior to the 'out_setg' label?

You're right, will fix, thanks!

Re: [Qemu-devel] [PATCH 14/18] block/sheepdog: Propagate errors through sd_prealloc()

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Eric Blake <eblake@redhat.com> writes:
>
>> On 05/13/2014 10:02 AM, Markus Armbruster wrote:
>>> Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>  block/sheepdog.c | 21 ++++++++++++---------
>>>  1 file changed, 12 insertions(+), 9 deletions(-)
>>> 
>>> @@ -1568,13 +1565,15 @@ static int sd_prealloc(const char *filename)
>>>           */
>>>          ret = bdrv_pread(bs, idx * SD_DATA_OBJ_SIZE, buf, SD_DATA_OBJ_SIZE);
>>>          if (ret < 0) {
>>> -            goto out;
>>> +            goto out_setg;
>>>          }
>>>          ret = bdrv_pwrite(bs, idx * SD_DATA_OBJ_SIZE, buf, SD_DATA_OBJ_SIZE);
>>>          if (ret < 0) {
>>> -            goto out;
>>> +            goto out_setg;
>>>          }
>>>      }
>>> +out_setg:
>>> +    error_setg_errno(errp, -ret, "Can't pre-allocate");
>>
>> This unconditionally sets errp even when the for loop completes
>> normally.  Are you sure you want this control flow?  Maybe you are
>> missing a 'goto out' prior to the 'out_setg' label?
>
> You're right, will fix, thanks!

I went over the whole series again looking for similar mistakes, and
found a few in PATCH 04 and 17.  Respin coming.