| Message ID | 20190115145256.9593-7-berrange@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | chardev: refactoring & many bugfixes related tcp_chr_wait_connected | expand |
On Tue, Jan 15, 2019 at 6:53 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > > The 'sioc' variable in qmp_chardev_open_socket was unused since > > commit 3e7d4d20d3a528b1ed10b1dc3d83119bfb0c5f24 > Author: Peter Xu <peterx@redhat.com> > Date: Tue Mar 6 13:33:17 2018 +0800 > > chardev: use chardev's gcontext for async connect > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > chardev/char-socket.c | 22 +++++++--------------- > 1 file changed, 7 insertions(+), 15 deletions(-) > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c > index ba86284ea9..3b6ff6619b 100644 > --- a/chardev/char-socket.c > +++ b/chardev/char-socket.c > @@ -1073,7 +1073,6 @@ static void qmp_chardev_open_socket(Chardev *chr, > bool is_waitconnect = sock->has_wait ? sock->wait : false; > bool is_websock = sock->has_websocket ? sock->websocket : false; > int64_t reconnect = sock->has_reconnect ? sock->reconnect : 0; > - QIOChannelSocket *sioc = NULL; > SocketAddress *addr; > > s->is_listen = is_listen; > @@ -1088,7 +1087,7 @@ static void qmp_chardev_open_socket(Chardev *chr, > if (!creds) { > error_setg(errp, "No TLS credentials with id '%s'", > sock->tls_creds); > - goto error; > + return; > } > s->tls_creds = (QCryptoTLSCreds *) > object_dynamic_cast(creds, > @@ -1096,20 +1095,20 @@ static void qmp_chardev_open_socket(Chardev *chr, > if (!s->tls_creds) { > error_setg(errp, "Object with id '%s' is not TLS credentials", > sock->tls_creds); > - goto error; > + return; > } > object_ref(OBJECT(s->tls_creds)); > if (is_listen) { > if (s->tls_creds->endpoint != QCRYPTO_TLS_CREDS_ENDPOINT_SERVER) { > error_setg(errp, "%s", > "Expected TLS credentials for server endpoint"); > - goto error; > + return; > } > } else { > if (s->tls_creds->endpoint != QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT) { > error_setg(errp, "%s", > "Expected TLS credentials for client endpoint"); > - goto error; > + return; > } > } > } > @@ -1117,7 +1116,7 @@ static void qmp_chardev_open_socket(Chardev *chr, > s->addr = addr = socket_address_flatten(sock->addr); > > if (!qmp_chardev_validate_socket(sock, addr, errp)) { > - goto error; > + return; > } > > qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_RECONNECTABLE); > @@ -1153,7 +1152,7 @@ static void qmp_chardev_open_socket(Chardev *chr, > if (qio_net_listener_open_sync(s->listener, s->addr, errp) < 0) { > object_unref(OBJECT(s->listener)); > s->listener = NULL; > - goto error; > + return; > } > > qapi_free_SocketAddress(s->addr); > @@ -1171,16 +1170,9 @@ static void qmp_chardev_open_socket(Chardev *chr, > chr->gcontext); > } > } else if (qemu_chr_wait_connected(chr, errp) < 0) { > - goto error; > + return; > } > } > - > - return; > - > -error: > - if (sioc) { > - object_unref(OBJECT(sioc)); > - } > } > > static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend, > -- > 2.20.1 >
On 2019-01-15 15:52, Daniel P. Berrangé wrote: > The 'sioc' variable in qmp_chardev_open_socket was unused since > > commit 3e7d4d20d3a528b1ed10b1dc3d83119bfb0c5f24 > Author: Peter Xu <peterx@redhat.com> > Date: Tue Mar 6 13:33:17 2018 +0800 > > chardev: use chardev's gcontext for async connect [...] > -error: > - if (sioc) { > - object_unref(OBJECT(sioc)); > - } So who is doing the object_unref() now in case of errors? That commit did not take care of it ... so it sounds like we could be leaving references behind in case of errors here? Thomas
On Wed, Jan 16, 2019 at 06:24:49AM +0100, Thomas Huth wrote: > On 2019-01-15 15:52, Daniel P. Berrangé wrote: > > The 'sioc' variable in qmp_chardev_open_socket was unused since > > > > commit 3e7d4d20d3a528b1ed10b1dc3d83119bfb0c5f24 > > Author: Peter Xu <peterx@redhat.com> > > Date: Tue Mar 6 13:33:17 2018 +0800 > > > > chardev: use chardev's gcontext for async connect > [...] > > -error: > > - if (sioc) { > > - object_unref(OBJECT(sioc)); > > - } > > So who is doing the object_unref() now in case of errors? That commit > did not take care of it ... so it sounds like we could be leaving > references behind in case of errors here? IMHO it'll be done finally in qemu_chr_socket_connected() no matter whether it's succeeded or not: cleanup: object_unref(OBJECT(sioc)); In other words, I think the old error path is not valid even before commit 3e7d4d20d3 because IIUC when reaching the error label the sioc should never be set (and if it tries to do an object_unref here it would be a real bug). Thanks,
On 2019-01-16 06:47, Peter Xu wrote: > On Wed, Jan 16, 2019 at 06:24:49AM +0100, Thomas Huth wrote: >> On 2019-01-15 15:52, Daniel P. Berrangé wrote: >>> The 'sioc' variable in qmp_chardev_open_socket was unused since >>> >>> commit 3e7d4d20d3a528b1ed10b1dc3d83119bfb0c5f24 >>> Author: Peter Xu <peterx@redhat.com> >>> Date: Tue Mar 6 13:33:17 2018 +0800 >>> >>> chardev: use chardev's gcontext for async connect >> [...] >>> -error: >>> - if (sioc) { >>> - object_unref(OBJECT(sioc)); >>> - } >> >> So who is doing the object_unref() now in case of errors? That commit >> did not take care of it ... so it sounds like we could be leaving >> references behind in case of errors here? > > IMHO it'll be done finally in qemu_chr_socket_connected() no matter > whether it's succeeded or not: > > cleanup: > object_unref(OBJECT(sioc)); > > In other words, I think the old error path is not valid even before > commit 3e7d4d20d3 because IIUC when reaching the error label the sioc > should never be set (and if it tries to do an object_unref here it > would be a real bug). Right, looking at the qmp_chardev_open_socket() function again (before the rework in 3e7d4d20d3a5), I see now that all locations that use "goto error" should have sioc = NULL. So this patch here is fine: Reviewed-by: Thomas Huth <thuth@redhat.com>
On Wed, Jan 16, 2019 at 06:24:49AM +0100, Thomas Huth wrote: > On 2019-01-15 15:52, Daniel P. Berrangé wrote: > > The 'sioc' variable in qmp_chardev_open_socket was unused since > > > > commit 3e7d4d20d3a528b1ed10b1dc3d83119bfb0c5f24 > > Author: Peter Xu <peterx@redhat.com> > > Date: Tue Mar 6 13:33:17 2018 +0800 > > > > chardev: use chardev's gcontext for async connect > [...] > > -error: > > - if (sioc) { > > - object_unref(OBJECT(sioc)); > > - } > > So who is doing the object_unref() now in case of errors? That commit > did not take care of it ... so it sounds like we could be leaving > references behind in case of errors here? As shown in the patch diff chunks, no code is ever setting "sioc" in this method, so it can't ever be non-NULL Regards, Daniel
diff --git a/chardev/char-socket.c b/chardev/char-socket.c index ba86284ea9..3b6ff6619b 100644 --- a/chardev/char-socket.c +++ b/chardev/char-socket.c @@ -1073,7 +1073,6 @@ static void qmp_chardev_open_socket(Chardev *chr, bool is_waitconnect = sock->has_wait ? sock->wait : false; bool is_websock = sock->has_websocket ? sock->websocket : false; int64_t reconnect = sock->has_reconnect ? sock->reconnect : 0; - QIOChannelSocket *sioc = NULL; SocketAddress *addr; s->is_listen = is_listen; @@ -1088,7 +1087,7 @@ static void qmp_chardev_open_socket(Chardev *chr, if (!creds) { error_setg(errp, "No TLS credentials with id '%s'", sock->tls_creds); - goto error; + return; } s->tls_creds = (QCryptoTLSCreds *) object_dynamic_cast(creds, @@ -1096,20 +1095,20 @@ static void qmp_chardev_open_socket(Chardev *chr, if (!s->tls_creds) { error_setg(errp, "Object with id '%s' is not TLS credentials", sock->tls_creds); - goto error; + return; } object_ref(OBJECT(s->tls_creds)); if (is_listen) { if (s->tls_creds->endpoint != QCRYPTO_TLS_CREDS_ENDPOINT_SERVER) { error_setg(errp, "%s", "Expected TLS credentials for server endpoint"); - goto error; + return; } } else { if (s->tls_creds->endpoint != QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT) { error_setg(errp, "%s", "Expected TLS credentials for client endpoint"); - goto error; + return; } } } @@ -1117,7 +1116,7 @@ static void qmp_chardev_open_socket(Chardev *chr, s->addr = addr = socket_address_flatten(sock->addr); if (!qmp_chardev_validate_socket(sock, addr, errp)) { - goto error; + return; } qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_RECONNECTABLE); @@ -1153,7 +1152,7 @@ static void qmp_chardev_open_socket(Chardev *chr, if (qio_net_listener_open_sync(s->listener, s->addr, errp) < 0) { object_unref(OBJECT(s->listener)); s->listener = NULL; - goto error; + return; } qapi_free_SocketAddress(s->addr); @@ -1171,16 +1170,9 @@ static void qmp_chardev_open_socket(Chardev *chr, chr->gcontext); } } else if (qemu_chr_wait_connected(chr, errp) < 0) { - goto error; + return; } } - - return; - -error: - if (sioc) { - object_unref(OBJECT(sioc)); - } } static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
The 'sioc' variable in qmp_chardev_open_socket was unused since commit 3e7d4d20d3a528b1ed10b1dc3d83119bfb0c5f24 Author: Peter Xu <peterx@redhat.com> Date: Tue Mar 6 13:33:17 2018 +0800 chardev: use chardev's gcontext for async connect Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- chardev/char-socket.c | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-)