[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 13/18] vnc: add error propagation to vnc_display
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH 13/18] vnc: add error propagation to vnc_display_open |
Date: |
Fri, 05 Oct 2012 08:28:27 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120911 Thunderbird/15.0.1 |
Il 04/10/2012 22:29, Luiz Capitulino ha scritto:
> On Wed, 3 Oct 2012 16:37:00 +0200
> Paolo Bonzini <address@hidden> wrote:
>
>> Before:
>>
>> $ qemu-system-x86_64 -vnc foo.bar:12345
>> getaddrinfo(foo.bar,18245): Name or service not known
>> Failed to start VNC server on `foo.bar:12345'
>>
>> $ qemu-system-x86_64 -vnc localhost:12345,reverse=on
>> inet_connect_opts:
>> connect(ipv4,yakj.usersys.redhat.com,127.0.0.1,12345): Connection refused
>> Failed to start VNC server on `localhost:12345,reverse=on'
>>
>> After:
>>
>> $ x86_64-softmmu/qemu-system-x86_64 -vnc foo.bar:12345
>> qemu-system-x86_64: address resolution failed for foo.bar:18245: Name or
>> service not known
>> Failed to start VNC server on `foo.bar:12345'
>>
>> $ x86_64-softmmu/qemu-system-x86_64 -vnc localhost:12345,reverse=on
>> qemu-system-x86_64: Failed to connect to socket: Connection refused
>> Failed to start VNC server on `localhost:12345,reverse=on'
>>
>> Signed-off-by: Paolo Bonzini <address@hidden>
>> ---
>> console.h | 2 +-
>> qmp.c | 6 ++----
>> ui/vnc.c | 67
>> +++++++++++++++++++++++++++++----------------------------------
>> vl.c | 5 ++++-
>> 4 file modificati, 38 inserzioni(+), 42 rimozioni(-)
>>
>> diff --git a/console.h b/console.h
>> index f990684..fcf2fc5 100644
>> --- a/console.h
>> +++ b/console.h
>> @@ -378,7 +378,7 @@ void cocoa_display_init(DisplayState *ds, int
>> full_screen);
>> /* vnc.c */
>> void vnc_display_init(DisplayState *ds);
>> void vnc_display_close(DisplayState *ds);
>> -int vnc_display_open(DisplayState *ds, const char *display);
>> +int vnc_display_open(DisplayState *ds, const char *display, Error **errp);
>> void vnc_display_add_client(DisplayState *ds, int csock, int skipauth);
>> int vnc_display_disable_login(DisplayState *ds);
>> char *vnc_display_local_addr(DisplayState *ds);
>> diff --git a/qmp.c b/qmp.c
>> index 36c54c5..31bc3bf 100644
>> --- a/qmp.c
>> +++ b/qmp.c
>> @@ -349,11 +349,9 @@ void qmp_change_vnc_password(const char *password,
>> Error **errp)
>> }
>> }
>>
>> -static void qmp_change_vnc_listen(const char *target, Error **err)
>> +static void qmp_change_vnc_listen(const char *target, Error **errp)
>> {
>> - if (vnc_display_open(NULL, target) < 0) {
>> - error_set(err, QERR_VNC_SERVER_FAILED, target);
>> - }
>> + vnc_display_open(NULL, target, errp);
>> }
>>
>> static void qmp_change_vnc(const char *target, bool has_arg, const char
>> *arg,
>> diff --git a/ui/vnc.c b/ui/vnc.c
>> index 235596e..0d92670 100644
>> --- a/ui/vnc.c
>> +++ b/ui/vnc.c
>> @@ -2844,7 +2844,7 @@ char *vnc_display_local_addr(DisplayState *ds)
>> return vnc_socket_local_addr("%s:%s", vs->lsock);
>> }
>>
>> -int vnc_display_open(DisplayState *ds, const char *display)
>> +int vnc_display_open(DisplayState *ds, const char *display, Error **errp)
>
> Can't return void?
Ok.
>> {
>> VncDisplay *vs = ds ? (VncDisplay *)ds->opaque : vnc_display;
>> const char *options;
>> @@ -2863,13 +2863,12 @@ int vnc_display_open(DisplayState *ds, const char
>> *display)
>> int lock_key_sync = 1;
>>
>> if (!vnc_display)
>> - return -1;
>> + goto fail;
>> vnc_display_close(ds);
>> if (strcmp(display, "none") == 0)
>> return 0;
>>
>> - if (!(vs->display = strdup(display)))
>> - return -1;
>> + vs->display = g_strdup(display);
>> vs->share_policy = VNC_SHARE_POLICY_ALLOW_EXCLUSIVE;
>>
>> options = display;
>> @@ -2877,13 +2876,11 @@ int vnc_display_open(DisplayState *ds, const char
>> *display)
>> options++;
>> if (strncmp(options, "password", 8) == 0) {
>> if (fips_get_state()) {
>> - fprintf(stderr,
>> - "VNC password auth disabled due to FIPS mode, "
>> - "consider using the VeNCrypt or SASL authentication
>> "
>> - "methods as an alternative\n");
>> - g_free(vs->display);
>> - vs->display = NULL;
>> - return -1;
>> + error_setg(errp,
>> + "VNC password auth disabled due to FIPS mode, "
>> + "consider using the VeNCrypt or SASL
>> authentication "
>> + "methods as an alternative\n");
>> + goto fail;
>> }
>> password = 1; /* Require password auth */
>> } else if (strncmp(options, "reverse", 7) == 0) {
>> @@ -2913,18 +2910,14 @@ int vnc_display_open(DisplayState *ds, const char
>> *display)
>>
>> VNC_DEBUG("Trying certificate path '%s'\n", path);
>> if (vnc_tls_set_x509_creds_dir(vs, path) < 0) {
>> - fprintf(stderr, "Failed to find x509 certificates/keys
>> in %s\n", path);
>> + error_setg(errp, "Failed to find x509 certificates/keys
>> in %s\n", path);
>> g_free(path);
>> - g_free(vs->display);
>> - vs->display = NULL;
>> - return -1;
>> + goto fail;
>> }
>> g_free(path);
>> } else {
>> - fprintf(stderr, "No certificate path provided\n");
>> - g_free(vs->display);
>> - vs->display = NULL;
>> - return -1;
>> + error_setg(errp, "No certificate path provided\n");
>> + goto fail;
>> }
>> #endif
>> #if defined(CONFIG_VNC_TLS) || defined(CONFIG_VNC_SASL)
>> @@ -2943,10 +2936,8 @@ int vnc_display_open(DisplayState *ds, const char
>> *display)
>> } else if (strncmp(options+6, "force-shared", 12) == 0) {
>> vs->share_policy = VNC_SHARE_POLICY_FORCE_SHARED;
>> } else {
>> - fprintf(stderr, "unknown vnc share= option\n");
>> - g_free(vs->display);
>> - vs->display = NULL;
>> - return -1;
>> + error_setg(errp, "unknown vnc share= option\n");
>> + goto fail;
>> }
>> }
>> }
>> @@ -3047,11 +3038,9 @@ int vnc_display_open(DisplayState *ds, const char
>> *display)
>>
>> #ifdef CONFIG_VNC_SASL
>> if ((saslErr = sasl_server_init(NULL, "qemu")) != SASL_OK) {
>> - fprintf(stderr, "Failed to initialize SASL auth %s",
>> - sasl_errstring(saslErr, NULL, NULL));
>> - g_free(vs->display);
>> - vs->display = NULL;
>> - return -1;
>> + error_setg(errp, "Failed to initialize SASL auth %s",
>> + sasl_errstring(saslErr, NULL, NULL));
>> + goto fail;
>> }
>> #endif
>> vs->lock_key_sync = lock_key_sync;
>> @@ -3059,13 +3048,11 @@ int vnc_display_open(DisplayState *ds, const char
>> *display)
>> if (reverse) {
>> /* connect to viewer */
>> if (strncmp(display, "unix:", 5) == 0)
>> - vs->lsock = unix_connect(display+5, NULL);
>> + vs->lsock = unix_connect(display+5, errp);
>> else
>> - vs->lsock = inet_connect(display, NULL);
>> + vs->lsock = inet_connect(display, errp);
>> if (-1 == vs->lsock) {
>> - g_free(vs->display);
>> - vs->display = NULL;
>> - return -1;
>> + goto fail;
>> } else {
>> int csock = vs->lsock;
>> vs->lsock = -1;
>> @@ -3079,20 +3066,28 @@ int vnc_display_open(DisplayState *ds, const char
>> *display)
>> dpy = g_malloc(256);
>> if (strncmp(display, "unix:", 5) == 0) {
>> pstrcpy(dpy, 256, "unix:");
>> - vs->lsock = unix_listen(display+5, dpy+5, 256-5, NULL);
>> + vs->lsock = unix_listen(display+5, dpy+5, 256-5, errp);
>> } else {
>> vs->lsock = inet_listen(display, dpy, 256,
>> - SOCK_STREAM, 5900, NULL);
>> + SOCK_STREAM, 5900, errp);
>> }
>> if (-1 == vs->lsock) {
>> g_free(dpy);
>> - return -1;
>> + goto fail;
>> } else {
>> g_free(vs->display);
>> vs->display = dpy;
>> }
>> }
>> return qemu_set_fd_handler2(vs->lsock, NULL, vnc_listen_read, NULL, vs);
>> +
>> +fail:
>> + if (!error_is_set(errp)) {
>> + error_set(errp, QERR_VNC_SERVER_FAILED, display);
>
> Please, use error_setg() instead. QERR_ macros are deprecated.
I'm not introducing a new one. One thing at a time, please.
>> + }
>> + g_free(vs->display);
>> + vs->display = NULL;
>> + return -1;
>> }
>>
>> void vnc_display_add_client(DisplayState *ds, int csock, int skipauth)
>> diff --git a/vl.c b/vl.c
>> index 53917c9..45a5ba5 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -3692,8 +3692,11 @@ int main(int argc, char **argv, char **envp)
>> #ifdef CONFIG_VNC
>> /* init remote displays */
>> if (vnc_display) {
>> + Error *local_err = NULL;
>> vnc_display_init(ds);
>> - if (vnc_display_open(ds, vnc_display) < 0) {
>> + if (vnc_display_open(ds, vnc_display, &local_err) < 0) {
>> + qerror_report_err(local_err);
>> + error_free(local_err);
>> fprintf(stderr, "Failed to start VNC server on `%s'\n",
>> vnc_display);
>> exit(1);
>
> Why do you need to call qerror_report_err()? I'd just do:
>
> fprintf(stderr, "Failed to start VNC server on display '%s': %s\n",
> vnc_display, error_get_pretty(local_err));
>
>
Ok.
[Qemu-devel] [PATCH 14/18] qemu-sockets: include strerror or gai_strerror output in error messages, Paolo Bonzini, 2012/10/03
[Qemu-devel] [PATCH 18/18] qemu-sockets: add error propagation to Unix socket functions, Paolo Bonzini, 2012/10/03