From 3f9286b7214fbc7135d4fc223f07b0b30b91e2f1 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 19 May 2014 18:57:37 +0200 Subject: [PATCH 1/5] qemu-socket: Clean up inet_connect_opts() 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 Reviewed-by: Paolo Bonzini Signed-off-by: Gerd Hoffmann --- 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 8818d7c0de..627e60931a 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; From 5f758366c0710d23e43f4d0f83816b98616a13d0 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 19 May 2014 18:57:34 +0200 Subject: [PATCH 2/5] char: Use return values instead of error_is_set(errp) 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 Signed-off-by: Gerd Hoffmann --- qemu-char.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/qemu-char.c b/qemu-char.c index 54ed244542..3eaefc9244 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); From 0aff637e920c0cdb74338c6f57005f2d7cab63d6 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 19 May 2014 18:57:35 +0200 Subject: [PATCH 3/5] char: Clean up fragile use of error_is_set() 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 Signed-off-by: Gerd Hoffmann --- qemu-char.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/qemu-char.c b/qemu-char.c index 3eaefc9244..5a7975f393 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; } } From 3894c78764e1e6d8ef82c7c097749d10cf8c3981 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 19 May 2014 18:57:36 +0200 Subject: [PATCH 4/5] char: Explain qmp_chardev_add()'s unusual error handling 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 Signed-off-by: Gerd Hoffmann --- qemu-char.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/qemu-char.c b/qemu-char.c index 5a7975f393..17b476edf0 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) { From d2e064a73ee7e5af244ff7b6406ac2344bbaa231 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 19 May 2014 18:57:38 +0200 Subject: [PATCH 5/5] error: error_is_set() is finally unused; remove Signed-off-by: Markus Armbruster Signed-off-by: Gerd Hoffmann --- 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 79958011db..d712089f1a 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 66245ccd1f..2ace0d8dd9 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;