Skip to contentSkip to conversation

block: Purge qerror_report()

PATCH Superseded Markus Armbruster 3 participants 47 messages May 13, 2014 – May 16, 2014 Block layer

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

cover block: Purge qerror_report()

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(-)
Signature · 1 line
1.8.1.4
#1 lore raw

PATCH 1/18 blockdev: Don't use qerror_report_err() in drive_init()

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.

---
 blockdev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
blockdev.c +2 -2
index 1a12e24..24b1c3b 100644
@@ -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;
Signature · 1 line
1.8.1.4
Reviewed-by on v1 1/18
#2 lore raw

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

Quoted 4 lines · Markus Armbruster

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

Quoted 8 lines

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.

---
 blockdev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
#3 lore raw

PATCH 2/18 blockdev: Don't use qerror_report() in do_drive_del()

In reply to 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.

---
 blockdev.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
blockdev.c +1 -2
index 24b1c3b..7478d4d 100644
@@ -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)) {
Signature · 1 line
1.8.1.4
Reviewed-by on v1 2/18
#4 lore raw

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

Quoted 11 lines · 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.

---
 blockdev.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
#5 lore raw

PATCH 3/18 qemu-nbd: Don't use qerror_report()

In reply to 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().

---
 qemu-nbd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
qemu-nbd.c +3 -3
index eed79fa..621e654 100644
@@ -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;
Signature · 1 line
1.8.1.4
Reviewed-by on v1 3/18
#6 lore raw

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

Quoted 8 lines · 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().

---
 qemu-nbd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
#7 lore raw

PATCH 4/18 block/rbd: Propagate errors to open and create methods

In reply to Markus Armbruster

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

---
 block/rbd.c | 66 +++++++++++++++++++++++++++++++------------------------------
 1 file changed, 34 insertions(+), 32 deletions(-)
block/rbd.c +34 -32
index dbc79f4..aa7a864 100644
@@ -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;
     }
 
Signature · 1 line
1.8.1.4
#8 lore raw

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

Quoted 18 lines · Markus Armbruster

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

---
 block/rbd.c | 66 +++++++++++++++++++++++++++++++------------------------------
 1 file changed, 34 insertions(+), 32 deletions(-)
block/rbd.c +1 -0
@@ -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;

block/rbd.c static int qemu_rbd_create(const char *filename, QEMUOptionParameter *options, · line 310

Quoted 6 lines
block/rbd.c +1 -1
@@ -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?

Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
#9 lore raw

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

Quoted 19 lines · Eric Blake
Quoted 18 lines · Markus Armbruster

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

---
 block/rbd.c | 66 +++++++++++++++++++++++++++++++------------------------------
 1 file changed, 34 insertions(+), 32 deletions(-)
block/rbd.c +1 -0
@@ -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,
Quoted 8 lines

block/rbd.c static int qemu_rbd_create(const char *filename, QEMUOptionParameter *options, · line 310

Quoted 6 lines
block/rbd.c +1 -1
     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!

#10 lore raw

PATCH 5/18 block/ssh: Drop superfluous libssh2_session_last_errno() calls

In reply to Markus Armbruster

libssh2_session_last_error() already returns the error code.

Cc "Richard W.M. Jones" <[email protected]>
---
 block/ssh.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
block/ssh.c +4 -5
index aa63c9d..e38d232 100644
@@ -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);
 
Signature · 1 line
1.8.1.4
#11 lore raw

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

Quoted 26 lines · Markus Armbruster

libssh2_session_last_error() already returns the error code.

Cc "Richard W.M. Jones" <[email protected]>
---
 block/ssh.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
block/ssh.c +2 -3
index aa63c9d..e38d232 100644
@@ -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);
     }
 

block/ssh.c sftp_error_report(BDRVSSHState *s, const char *fs, ...) · line 151

Quoted 10 lines
block/ssh.c +2 -2
@@ -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.

Signature · 5 lines
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
#12 lore raw

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

Quoted 43 lines · Richard W.M. Jones
Quoted 26 lines · Markus Armbruster

libssh2_session_last_error() already returns the error code.

Cc "Richard W.M. Jones" <[email protected]>
---
 block/ssh.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
block/ssh.c +2 -3
index aa63c9d..e38d232 100644
@@ -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);
     }
 

block/ssh.c sftp_error_report(BDRVSSHState *s, const char *fs, ...) · line 151

Quoted 10 lines
block/ssh.c +2 -2
@@ -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.

Quoted 1 line

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.

#13 lore raw

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

Quoted 45 lines · Markus Armbruster
Quoted 43 lines · Richard W.M. Jones
Quoted 26 lines · Markus Armbruster

libssh2_session_last_error() already returns the error code.

Cc "Richard W.M. Jones" <[email protected]>
---
 block/ssh.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
block/ssh.c +2 -3
index aa63c9d..e38d232 100644
@@ -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);
     }
 

block/ssh.c sftp_error_report(BDRVSSHState *s, const char *fs, ...) · line 151

Quoted 10 lines
block/ssh.c +2 -2
@@ -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.

Quoted 4 lines
Quoted 1 line

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.

Signature · 4 lines
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
#14 lore raw

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

Quoted 51 lines · Richard W.M. Jones
Quoted 45 lines · Markus Armbruster
Quoted 43 lines · Richard W.M. Jones
Quoted 26 lines · Markus Armbruster

libssh2_session_last_error() already returns the error code.

Cc "Richard W.M. Jones" <[email protected]>
---
 block/ssh.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
block/ssh.c +2 -3
index aa63c9d..e38d232 100644
@@ -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);
     }
 

block/ssh.c sftp_error_report(BDRVSSHState *s, const char *fs, ...) · line 151

Quoted 10 lines
block/ssh.c +2 -2
@@ -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.

Quoted 4 lines
Quoted 1 line

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.

Quoted 6 lines

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 :)

Reviewed-by on v1 5/18
#15 lore raw

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

In reply to Markus Armbruster
Quoted 4 lines · Markus Armbruster

libssh2_session_last_error() already returns the error code.

Cc "Richard W.M. Jones" <[email protected]>
Quoted 34 lines
 block/ssh.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
block/ssh.c +4 -5
index aa63c9d..e38d232 100644
@@ -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);
 
Signature · 1 line
1.8.1.4
Signature · 4 lines
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
#16 lore raw

PATCH 6/18 block/ssh: Propagate errors through check_host_key()

In reply to Markus Armbruster
Cc "Richard W.M. Jones" <[email protected]>
---
 block/ssh.c | 68 ++++++++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 49 insertions(+), 19 deletions(-)
block/ssh.c +49 -19
index e38d232..1df1946 100644
@@ -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;
     }
 
Signature · 1 line
1.8.1.4
Reviewed-by on v1 6/18
#17 lore raw

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

Quoted 2 lines · Markus Armbruster
Cc "Richard W.M. Jones" <[email protected]>
Quoted 170 lines
 block/ssh.c | 68 ++++++++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 49 insertions(+), 19 deletions(-)
block/ssh.c +49 -19
index e38d232..1df1946 100644
@@ -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;
     }
 
Signature · 1 line
1.8.1.4
Signature · 5 lines
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/
#18 lore raw

PATCH 7/18 block/ssh: Propagate errors through authenticate()

In reply to Markus Armbruster
Cc "Richard W.M. Jones" <[email protected]>
---
 block/ssh.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)
block/ssh.c +14 -9
index 1df1946..18186ba 100644
@@ -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;
     }
 
Signature · 1 line
1.8.1.4
Reviewed-by on v1 7/18
#19 lore raw

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

Quoted 2 lines · Markus Armbruster
Cc "Richard W.M. Jones" <[email protected]>
Quoted 83 lines
 block/ssh.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)
block/ssh.c +14 -9
index 1df1946..18186ba 100644
@@ -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;
     }
 
Signature · 1 line
1.8.1.4
Signature · 5 lines
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/
#20 lore raw

PATCH 8/18 block/ssh: Propagate errors through connect_to_ssh()

In reply to Markus Armbruster
Cc "Richard W.M. Jones" <[email protected]>
---
 block/ssh.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)
block/ssh.c +17 -17
index 18186ba..26078c4 100644
@@ -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;
     }
Signature · 1 line
1.8.1.4
Reviewed-by on v1 8/18
#21 lore raw

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

Quoted 2 lines · Markus Armbruster
Cc "Richard W.M. Jones" <[email protected]>
Quoted 120 lines
 block/ssh.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)
block/ssh.c +17 -17
index 18186ba..26078c4 100644
@@ -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;
     }
Signature · 1 line
1.8.1.4
Signature · 5 lines
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
#22 lore raw

PATCH 9/18 block/ssh: Propagate errors to open and create methods

In reply to Markus Armbruster

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

Cc "Richard W.M. Jones" <[email protected]>
---
 block/ssh.c | 47 ++++++++++++++++++++++-------------------------
 1 file changed, 22 insertions(+), 25 deletions(-)
block/ssh.c +22 -25
index 26078c4..b212971 100644
@@ -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;
         }
Signature · 1 line
1.8.1.4
#23 lore raw

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

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

Rich.

Signature · 5 lines
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
Reviewed-by on v1 9/18
#24 lore raw

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

In reply to Markus Armbruster
Quoted 4 lines · Markus Armbruster

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

Cc "Richard W.M. Jones" <[email protected]>
Quoted 133 lines
 block/ssh.c | 47 ++++++++++++++++++++++-------------------------
 1 file changed, 22 insertions(+), 25 deletions(-)
block/ssh.c +22 -25
index 26078c4..b212971 100644
@@ -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;
         }
Signature · 1 line
1.8.1.4
Signature · 5 lines
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/
#25 lore raw

PATCH 10/18 block/vvfat: Propagate errors through enable_write_target()

In reply to Markus Armbruster

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

---
 block/vvfat.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)
block/vvfat.c +6 -10
index c3af7ff..3eccd68 100644
@@ -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;
     }
 
Signature · 1 line
1.8.1.4
Reviewed-by on v1 10/18
#26 lore raw

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

Quoted 12 lines · Markus Armbruster

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

---
 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,

block/vvfat.c static int enable_write_target(BDRVVVFATState *s) · line 2937

Quoted 2 lines
block/vvfat.c +1 -1
-            &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.

Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
#27 lore raw

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

Quoted 10 lines · Eric Blake
Quoted 8 lines · Markus Armbruster

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

---
 block/vvfat.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)
Quoted 8 lines

block/vvfat.c static int enable_write_target(BDRVVVFATState *s) · line 2937

Quoted 5 lines
     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,
block/vvfat.c +1 -1
-            &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.

Quoted 1 line

Thanks!

#28 lore raw

PATCH 11/18 block/vvfat: Propagate errors through init_directories()

In reply to Markus Armbruster

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

---
 block/vvfat.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)
block/vvfat.c +9 -7
index 3eccd68..c6a1211 100644
@@ -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;
     }
Signature · 1 line
1.8.1.4
Reviewed-by on v1 11/18
#29 lore raw

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

Quoted 9 lines · Markus Armbruster

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

---
 block/vvfat.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

block/vvfat.c static int init_directories(BDRVVVFATState* s, · line 897

Quoted 7 lines
block/vvfat.c +2 -2
@@ -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).

Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
#30 lore raw

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

Quoted 9 lines · Eric Blake
Quoted 8 lines · Markus Armbruster

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

---
 block/vvfat.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)
Quoted 13 lines

block/vvfat.c static int init_directories(BDRVVVFATState* s, · line 897

Quoted 8 lines
block/vvfat.c +2 -2
@@ -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 ;)

Quoted 1 line

Thanks!

#31 lore raw

PATCH 12/18 block/sheepdog: Propagate errors through connect_to_sdog()

In reply to Markus Armbruster
---
 block/sheepdog.c | 77 ++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 55 insertions(+), 22 deletions(-)
block/sheepdog.c +55 -22
index 2c3fb01..ff0aa89 100644
@@ -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;
     }
 
Signature · 1 line
1.8.1.4
Reviewed-by on v1 12/18
#32 lore raw

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

Quoted 6 lines · Markus Armbruster
---
 block/sheepdog.c | 77 ++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 55 insertions(+), 22 deletions(-)
Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
#33 lore raw

PATCH 13/18 block/sheepdog: Propagate errors through get_sheep_fd()

In reply to Markus Armbruster
---
 block/sheepdog.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)
block/sheepdog.c +10 -8
index ff0aa89..b932d68 100644
@@ -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;
     }
Signature · 1 line
1.8.1.4
Reviewed-by on v1 13/18
#34 lore raw

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

Quoted 6 lines · Markus Armbruster
---
 block/sheepdog.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)
Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
#35 lore raw

PATCH 14/18 block/sheepdog: Propagate errors through sd_prealloc()

In reply to Markus Armbruster
---
 block/sheepdog.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)
block/sheepdog.c +12 -9
index b932d68..a8385bb 100644
@@ -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;
Signature · 1 line
1.8.1.4
#36 lore raw

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

Quoted 11 lines · Markus Armbruster
---
 block/sheepdog.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)
block/sheepdog.c +0 -1
@@ -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;

block/sheepdog.c static int sd_prealloc(const char *filename) · line 1576

Quoted 10 lines
block/sheepdog.c +4 -1
+            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?

Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
#37 lore raw

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

Quoted 18 lines · Eric Blake
Quoted 17 lines · Markus Armbruster
---
 block/sheepdog.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)
block/sheepdog.c +2 -2
@@ -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;
Quoted 8 lines

block/sheepdog.c static int sd_prealloc(const char *filename) · line 1576

Quoted 4 lines
block/sheepdog.c +2 -0
         }
     }
+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!

#38 lore raw

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

Quoted 29 lines · Markus Armbruster
Quoted 18 lines · Eric Blake
Quoted 17 lines · Markus Armbruster
---
 block/sheepdog.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)
block/sheepdog.c +2 -2
@@ -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;
Quoted 8 lines

block/sheepdog.c static int sd_prealloc(const char *filename) · line 1576

Quoted 4 lines
block/sheepdog.c +2 -0
         }
     }
+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?

Quoted 1 line

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.

#39 lore raw

PATCH 15/18 block/sheepdog: Propagate errors through do_sd_create()

In reply to Markus Armbruster
---
 block/sheepdog.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)
block/sheepdog.c +21 -14
index a8385bb..d20a94c 100644
@@ -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;
Signature · 1 line
1.8.1.4
Reviewed-by on v1 15/18
#40 lore raw

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

Quoted 6 lines · Markus Armbruster
---
 block/sheepdog.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)
Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
#41 lore raw

PATCH 16/18 block/sheepdog: Propagate errors through find_vdi_name()

In reply to Markus Armbruster
---
 block/sheepdog.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)
block/sheepdog.c +11 -9
index d20a94c..2cf192d 100644
@@ -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;
     }
 
Signature · 1 line
1.8.1.4
Reviewed-by on v1 16/18
#42 lore raw

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

Quoted 6 lines · Markus Armbruster
---
 block/sheepdog.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)
Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
#43 lore raw

PATCH 17/18 block/sheepdog: Propagate errors to open and create methods

In reply to Markus Armbruster

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

---
 block/sheepdog.c | 30 +++++++-----------------------
 1 file changed, 7 insertions(+), 23 deletions(-)
block/sheepdog.c +7 -23
index 2cf192d..cd5981e 100644
@@ -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);
Signature · 1 line
1.8.1.4
Reviewed-by on v1 17/18
#44 lore raw

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

Quoted 8 lines · Markus Armbruster

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

---
 block/sheepdog.c | 30 +++++++-----------------------
 1 file changed, 7 insertions(+), 23 deletions(-)
Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
#45 lore raw

PATCH 18/18 block/sheepdog: Don't use qerror_report()

In reply to 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().

---
 block/sheepdog.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)
block/sheepdog.c +13 -13
index cd5981e..d3386b6 100644
@@ -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;
     }
Signature · 1 line
1.8.1.4
Reviewed-by on v1 18/18
#46 lore raw

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

Quoted 10 lines · 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().

---
 block/sheepdog.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)
Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org