[Qemu-devel] [PATCH v2 0/5] char: Purge error_is_set()

[Qemu-devel] [PATCH v2 0/5] char: Purge error_is_set()

[Markus Armbruster]

This is a trivial respin of all error_is_set() patches that haven't
been applied, yet: character devices, qemu-sockets, and error_is_set()
removal.

Markus Armbruster (5):
  char: Use return values instead of error_is_set(errp)
  char: Clean up fragile use of error_is_set()
  char: Explain qmp_chardev_add()'s unusual error handling
  qemu-socket: Clean up inet_connect_opts()
  error: error_is_set() is finally unused; remove

 include/qapi/error.h |  6 ------
 qemu-char.c          | 32 ++++++++++++++++++++------------
 util/error.c         |  5 -----
 util/qemu-sockets.c  | 28 ++++++++++++++++------------
 4 files changed, 36 insertions(+), 35 deletions(-)

-- 
1.9.0

[Qemu-devel] [PATCH v2 1/5] char: Use return values instead of error_is_set(errp)

[Markus Armbruster]

Using error_is_set(errp) to check whether a function call failed is
fragile: it breaks when errp is null.  Check perfectly suitable return
values instead when possible.  As far as I can tell, errp can't be
null there, but this is more robust and more obviously correct

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

diff --git a/qemu-char.c b/qemu-char.c
index 54ed244..3eaefc9 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3251,7 +3251,7 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
             }
         }
         ret = qmp_chardev_add(bid ? bid : id, backend, errp);
-        if (error_is_set(errp)) {
+        if (!ret) {
             goto qapi_out;
         }
 
@@ -3263,7 +3263,7 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
             backend->kind = CHARDEV_BACKEND_KIND_MUX;
             backend->mux->chardev = g_strdup(bid);
             ret = qmp_chardev_add(id, backend, errp);
-            if (error_is_set(errp)) {
+            if (!ret) {
                 chr = qemu_chr_find(bid);
                 qemu_chr_delete(chr);
                 chr = NULL;
@@ -3620,18 +3620,18 @@ static int qmp_chardev_open_file_source(char *src, int flags,
 
 static CharDriverState *qmp_chardev_open_file(ChardevFile *file, Error **errp)
 {
-    int flags, in = -1, out = -1;
+    int flags, in = -1, out;
 
     flags = O_WRONLY | O_TRUNC | O_CREAT | O_BINARY;
     out = qmp_chardev_open_file_source(file->out, flags, errp);
-    if (error_is_set(errp)) {
+    if (out < 0) {
         return NULL;
     }
 
     if (file->has_in) {
         flags = O_RDONLY;
         in = qmp_chardev_open_file_source(file->in, flags, errp);
-        if (error_is_set(errp)) {
+        if (in < 0) {
             qemu_close(out);
             return NULL;
         }
@@ -3647,7 +3647,7 @@ static CharDriverState *qmp_chardev_open_serial(ChardevHostdev *serial,
     int fd;
 
     fd = qmp_chardev_open_file_source(serial->device, O_RDWR, errp);
-    if (error_is_set(errp)) {
+    if (fd < 0) {
         return NULL;
     }
     qemu_set_nonblock(fd);
@@ -3665,7 +3665,7 @@ static CharDriverState *qmp_chardev_open_parallel(ChardevHostdev *parallel,
     int fd;
 
     fd = qmp_chardev_open_file_source(parallel->device, O_RDWR, errp);
-    if (error_is_set(errp)) {
+    if (fd < 0) {
         return NULL;
     }
     return qemu_chr_open_pp_fd(fd);
@@ -3692,7 +3692,7 @@ static CharDriverState *qmp_chardev_open_socket(ChardevSocket *sock,
     } else {
         fd = socket_connect(addr, errp, NULL, NULL);
     }
-    if (error_is_set(errp)) {
+    if (fd < 0) {
         return NULL;
     }
     return qemu_chr_open_socket_fd(fd, do_nodelay, is_listen,
@@ -3705,7 +3705,7 @@ static CharDriverState *qmp_chardev_open_udp(ChardevUdp *udp,
     int fd;
 
     fd = socket_dgram(udp->remote, udp->local, errp);
-    if (error_is_set(errp)) {
+    if (fd < 0) {
         return NULL;
     }
     return qemu_chr_open_udp_fd(fd);
-- 
1.9.0

[Qemu-devel] [PATCH v2 2/5] char: Clean up fragile use of error_is_set()

[Markus Armbruster]

Using error_is_set(ERRP) to find out whether a function failed is
either wrong, fragile, or unnecessarily opaque.  It's wrong when ERRP
may be null, because errors go undetected when it is.  It's fragile
when proving ERRP non-null involves a non-local argument.  Else, it's
unnecessarily opaque (see commit 84d18f0).

The error_is_set(errp) in qemu_chr_new_from_opts() is merely fragile,
because the callers never pass a null errp argument.

Make the code more robust and more obviously correct: receive the
error in a local variable, then propagate it through the parameter.

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

diff --git a/qemu-char.c b/qemu-char.c
index 3eaefc9..5a7975f 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3204,6 +3204,7 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
                                     void (*init)(struct CharDriverState *s),
                                     Error **errp)
 {
+    Error *local_err = NULL;
     CharDriver *cd;
     CharDriverState *chr;
     GSList *i;
@@ -3245,8 +3246,9 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
         chr = NULL;
         backend->kind = cd->kind;
         if (cd->parse) {
-            cd->parse(opts, backend, errp);
-            if (error_is_set(errp)) {
+            cd->parse(opts, backend, &local_err);
+            if (local_err) {
+                error_propagate(errp, local_err);
                 goto qapi_out;
             }
         }
-- 
1.9.0

[Qemu-devel] [PATCH v2 3/5] char: Explain qmp_chardev_add()'s unusual error handling

[Markus Armbruster]

Character backend open hasn't been fully converted to the Error API.
Some opens fail without setting an error.  qmp_chardev_add() needs to
detect when that happens, and set a generic error.  Explain that in a
comment, and inline error_is_set() for clarity.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qemu-char.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/qemu-char.c b/qemu-char.c
index 5a7975f..17b476e 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3798,7 +3798,13 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
         break;
     }
 
-    if (chr == NULL && !error_is_set(errp)) {
+    /*
+     * Character backend open hasn't been fully converted to the Error
+     * API.  Some opens fail without setting an error.  Set a generic
+     * error then.
+     * TODO full conversion to Error API
+     */
+    if (chr == NULL && errp && !*errp) {
         error_setg(errp, "Failed to create chardev");
     }
     if (chr) {
-- 
1.9.0

[Qemu-devel] [PATCH v2 4/5] qemu-socket: Clean up inet_connect_opts()

[Markus Armbruster]

Separate the search for a working addrinfo from the code that does
something with it.  Makes for a clearer search loop.

Use a local Error * to simplify resetting the error in the search
loop.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
---
 util/qemu-sockets.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 8818d7c..627e609 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -354,6 +354,7 @@ static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
 int inet_connect_opts(QemuOpts *opts, Error **errp,
                       NonBlockingConnectHandler *callback, void *opaque)
 {
+    Error *local_err = NULL;
     struct addrinfo *res, *e;
     int sock = -1;
     bool in_progress;
@@ -372,24 +373,27 @@ int inet_connect_opts(QemuOpts *opts, Error **errp,
     }
 
     for (e = res; e != NULL; e = e->ai_next) {
-        if (error_is_set(errp)) {
-            error_free(*errp);
-            *errp = NULL;
-        }
+        error_free(local_err);
+        local_err = NULL;
         if (connect_state != NULL) {
             connect_state->current_addr = e;
         }
-        sock = inet_connect_addr(e, &in_progress, connect_state, errp);
-        if (in_progress) {
-            return sock;
-        } else if (sock >= 0) {
-            /* non blocking socket immediate success, call callback */
-            if (callback != NULL) {
-                callback(sock, opaque);
-            }
+        sock = inet_connect_addr(e, &in_progress, connect_state, &local_err);
+        if (sock >= 0) {
             break;
         }
     }
+
+    if (sock < 0) {
+        error_propagate(errp, local_err);
+    } else if (in_progress) {
+        /* wait_for_connect() will do the rest */
+        return sock;
+    } else {
+        if (callback) {
+            callback(sock, opaque);
+        }
+    }
     g_free(connect_state);
     freeaddrinfo(res);
     return sock;
-- 
1.9.0

[Qemu-devel] [PATCH v2 5/5] error: error_is_set() is finally unused; remove

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qapi/error.h | 6 ------
 util/error.c         | 5 -----
 2 files changed, 11 deletions(-)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index 7995801..d712089 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -67,12 +67,6 @@ void error_set_win32(Error **errp, int win32_err, ErrorClass err_class,
  */
 void error_setg_file_open(Error **errp, int os_errno, const char *filename);
 
-/**
- * Returns true if an indirect pointer to an error is pointing to a valid
- * error object.
- */
-bool error_is_set(Error **errp);
-
 /*
  * Get the error class of an error object.
  */
diff --git a/util/error.c b/util/error.c
index 66245cc..2ace0d8 100644
--- a/util/error.c
+++ b/util/error.c
@@ -142,11 +142,6 @@ Error *error_copy(const Error *err)
     return err_new;
 }
 
-bool error_is_set(Error **errp)
-{
-    return (errp && *errp);
-}
-
 ErrorClass error_get_class(const Error *err)
 {
     return err->err_class;
-- 
1.9.0

Re: [Qemu-devel] [PATCH v2 1/5] char: Use return values instead of error_is_set(errp)

[Eric Blake]

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

On 05/19/2014 10:57 AM, Markus Armbruster wrote:
> Using error_is_set(errp) to check whether a function call failed is
> fragile: it breaks when errp is null.  Check perfectly suitable return
> values instead when possible.  As far as I can tell, errp can't be
> null there, but this is more robust and more obviously correct
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qemu-char.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)

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

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


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

Re: [Qemu-devel] [PATCH v2 2/5] char: Clean up fragile use of error_is_set()

[Eric Blake]

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

On 05/19/2014 10:57 AM, Markus Armbruster wrote:
> Using error_is_set(ERRP) to find out whether a function failed is
> either wrong, fragile, or unnecessarily opaque.  It's wrong when ERRP
> may be null, because errors go undetected when it is.  It's fragile
> when proving ERRP non-null involves a non-local argument.  Else, it's
> unnecessarily opaque (see commit 84d18f0).
> 
> The error_is_set(errp) in qemu_chr_new_from_opts() is merely fragile,
> because the callers never pass a null errp argument.
> 
> Make the code more robust and more obviously correct: receive the
> error in a local variable, then propagate it through the parameter.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qemu-char.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

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

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


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

Re: [Qemu-devel] [PATCH v2 3/5] char: Explain qmp_chardev_add()'s unusual error handling

[Eric Blake]

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

On 05/19/2014 10:57 AM, Markus Armbruster wrote:
> Character backend open hasn't been fully converted to the Error API.
> Some opens fail without setting an error.  qmp_chardev_add() needs to
> detect when that happens, and set a generic error.  Explain that in a
> comment, and inline error_is_set() for clarity.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qemu-char.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

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

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


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

Re: [Qemu-devel] [PATCH v2 5/5] error: error_is_set() is finally unused; remove

[Eric Blake]

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

On 05/19/2014 10:57 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/qapi/error.h | 6 ------
>  util/error.c         | 5 -----
>  2 files changed, 11 deletions(-)

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

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


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