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(-)
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 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,
--
Peter Xu
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
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
© 2016 - 2026 Red Hat, Inc.