[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH RFC v5 2/7] ui/vnc.c: polish vnc_init_func
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH RFC v5 2/7] ui/vnc.c: polish vnc_init_func |
Date: |
Thu, 11 Oct 2018 15:13:40 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
Fei Li <address@hidden> writes:
> Add a new Error parameter for vnc_display_init() to handle errors
> in its caller: vnc_init_func(), just like vnc_display_open() does.
> And let the call trace propagate the Error.
>
> Besides, make vnc_start_worker_thread() return a bool to indicate
> whether it succeeds instead of returning nothing.
>
> Signed-off-by: Fei Li <address@hidden>
> Reviewed-by: Fam Zheng <address@hidden>
> ---
> include/ui/console.h | 2 +-
> ui/vnc-jobs.c | 9 ++++++---
> ui/vnc-jobs.h | 2 +-
> ui/vnc.c | 12 +++++++++---
> 4 files changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/include/ui/console.h b/include/ui/console.h
> index fb969caf70..c17803c530 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -453,7 +453,7 @@ void qemu_display_early_init(DisplayOptions *opts);
> void qemu_display_init(DisplayState *ds, DisplayOptions *opts);
>
> /* vnc.c */
> -void vnc_display_init(const char *id);
> +void vnc_display_init(const char *id, Error **errp);
> void vnc_display_open(const char *id, Error **errp);
> void vnc_display_add_client(const char *id, int csock, bool skipauth);
> int vnc_display_password(const char *id, const char *password);
> diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
> index 929391f85d..8807d7217c 100644
> --- a/ui/vnc-jobs.c
> +++ b/ui/vnc-jobs.c
> @@ -331,15 +331,18 @@ static bool vnc_worker_thread_running(void)
> return queue; /* Check global queue */
> }
>
> -void vnc_start_worker_thread(void)
> +bool vnc_start_worker_thread(Error **errp)
> {
> VncJobQueue *q;
>
> - if (vnc_worker_thread_running())
> - return ;
> + if (vnc_worker_thread_running()) {
> + goto out;
> + }
>
> q = vnc_queue_init();
> qemu_thread_create(&q->thread, "vnc_worker", vnc_worker_thread, q,
> QEMU_THREAD_DETACHED);
> queue = q; /* Set global queue */
> +out:
> + return true;
> }
This function cannot fail. Why convert it to Error, and complicate its
caller?
> diff --git a/ui/vnc-jobs.h b/ui/vnc-jobs.h
> index 59f66bcc35..14640593db 100644
> --- a/ui/vnc-jobs.h
> +++ b/ui/vnc-jobs.h
> @@ -37,7 +37,7 @@ void vnc_job_push(VncJob *job);
> void vnc_jobs_join(VncState *vs);
>
> void vnc_jobs_consume_buffer(VncState *vs);
> -void vnc_start_worker_thread(void);
> +bool vnc_start_worker_thread(Error **errp);
>
> /* Locks */
> static inline int vnc_trylock_display(VncDisplay *vd)
> diff --git a/ui/vnc.c b/ui/vnc.c
> index cf221c83cc..f3806e76db 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -3205,7 +3205,7 @@ static const DisplayChangeListenerOps dcl_ops = {
> .dpy_cursor_define = vnc_dpy_cursor_define,
> };
>
> -void vnc_display_init(const char *id)
> +void vnc_display_init(const char *id, Error **errp)
> {
> VncDisplay *vd;
>
if (vnc_display_find(id) != NULL) {
return;
}
vd = g_malloc0(sizeof(*vd));
vd->id = strdup(id);
QTAILQ_INSERT_TAIL(&vnc_displays, vd, next);
QTAILQ_INIT(&vd->clients);
vd->expires = TIME_MAX;
if (keyboard_layout) {
trace_vnc_key_map_init(keyboard_layout);
vd->kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout);
} else {
vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us");
}
if (!vd->kbd_layout) {
exit(1);
Uh, init_keyboard_layout() reports errors to stderr, and the exit(1)
here makes them fatal. Okay before this patch, but inappropriate
afterwards. I'm afraid you have to convert init_keyboard_layout() to
Error first.
}
vd->share_policy = VNC_SHARE_POLICY_ALLOW_EXCLUSIVE;
> @@ -3235,7 +3235,9 @@ void vnc_display_init(const char *id)
> vd->connections_limit = 32;
>
> qemu_mutex_init(&vd->mutex);
> - vnc_start_worker_thread();
> + if (!vnc_start_worker_thread(errp)) {
> + return;
> + }
>
> vd->dcl.ops = &dcl_ops;
> register_displaychangelistener(&vd->dcl);
> @@ -4079,7 +4081,11 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error
> **errp)
> char *id = (char *)qemu_opts_id(opts);
>
> assert(id);
> - vnc_display_init(id);
> + vnc_display_init(id, &local_err);
> + if (local_err) {
> + error_reportf_err(local_err, "Failed to init VNC server: ");
> + exit(1);
> + }
> vnc_display_open(id, &local_err);
> if (local_err != NULL) {
> error_reportf_err(local_err, "Failed to start VNC server: ");
Conflicts with my "[PATCH 24/31] vl: Clean up error reporting in
vnc_init_func()". Your patch shows that mine is incomplete.
Without my patch, there's no real reason for yours, however. The two
should be squashed together.
[Qemu-devel] [PATCH RFC v5 2/7] ui/vnc.c: polish vnc_init_func, Fei Li, 2018/10/10
- Re: [Qemu-devel] [PATCH RFC v5 2/7] ui/vnc.c: polish vnc_init_func,
Markus Armbruster <=
[Qemu-devel] [PATCH RFC v5 3/7] qemu_init_vcpu: add a new Error parameter to propagate, Fei Li, 2018/10/10
[Qemu-devel] [PATCH RFC v5 4/7] qemu_thread_join: fix segmentation fault, Fei Li, 2018/10/10