Re: [Qemu-devel] [PATCH RFC v5 2/7] ui/vnc.c: polish vnc_init_func
On 10/12/2018 06:23 PM, Fei Li wrote:
On 10/12/2018 04:18 PM, Markus Armbruster wrote:
Fei Li writes:
On 10/11/2018 09:13 PM, Markus Armbruster wrote:
Fei Li 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
Reviewed-by: Fam Zheng
---
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(>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?
[Issue1]
Actually, this is to pave the way for patch 7/7, in case
qemu_thread_create() fails.
Patch 7/7 is long enough, thus I wrote the patch 1/2/3 separately to
mainly
connects the broken errp for callers who initially have the errp.
[I am back... If the other codes in this patches be squashed, maybe
merge [Issue1]
into patch 7/7 looks more intuitive.]
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(_displays, vd, next);
QTAILQ_INIT(>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.
[Issue2]
Right. But I notice the returned kbd_layout_t *kbd_layout is a static
value and also
will be used by others, how about passing the errp parameter to
init_keyboard_layout()
as follows? And for its other callers, just pass NULL.
@@ -3222,13 +3222,13 @@ void vnc_display_init(const char *id, Error
**errp)
if (keyboard_layout) {
trace_vnc_key_map_init(keyboard_layout);
- vd->kbd_layout = init_keyboard_layout(name2keysym,
keyboard_layout);
+ vd->kbd_layout = init_keyboard_layout(name2keysym,
keyboard_layout, errp);
} else {
- vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us");
+ vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us",
errp);
}
if (!vd->kbd_layout) {
- exit(1);
+ return;
}
Sounds good to me, except you should pass _fatal instead of NULL
to preserve "report error and exit" semantics.
Yes, you are right. I should pass
Re: [Qemu-devel] [PATCH RFC v5 2/7] ui/vnc.c: polish vnc_init_func
On 10/12/2018 04:18 PM, Markus Armbruster wrote:
Fei Li writes:
On 10/11/2018 09:13 PM, Markus Armbruster wrote:
Fei Li 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
Reviewed-by: Fam Zheng
---
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(>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?
[Issue1]
Actually, this is to pave the way for patch 7/7, in case
qemu_thread_create() fails.
Patch 7/7 is long enough, thus I wrote the patch 1/2/3 separately to mainly
connects the broken errp for callers who initially have the errp.
[I am back... If the other codes in this patches be squashed, maybe
merge [Issue1]
into patch 7/7 looks more intuitive.]
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(_displays, vd, next);
QTAILQ_INIT(>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.
[Issue2]
Right. But I notice the returned kbd_layout_t *kbd_layout is a static
value and also
will be used by others, how about passing the errp parameter to
init_keyboard_layout()
as follows? And for its other callers, just pass NULL.
@@ -3222,13 +3222,13 @@ void vnc_display_init(const char *id, Error **errp)
if (keyboard_layout) {
trace_vnc_key_map_init(keyboard_layout);
- vd->kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout);
+ vd->kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout,
errp);
} else {
- vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us");
+ vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us", errp);
}
if (!vd->kbd_layout) {
- exit(1);
+ return;
}
Sounds good to me, except you should pass _fatal instead of NULL
to preserve "report error and exit" semantics.
Yes, you are right. I should pass _fatal and let the following
error_setg() handle this.
Re: [Qemu-devel] [PATCH RFC v5 2/7] ui/vnc.c: polish vnc_init_func
Fei Li writes:
> On 10/11/2018 09:13 PM, Markus Armbruster wrote:
>> Fei Li 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
>>> Reviewed-by: Fam Zheng
>>> ---
>>> 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(>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?
> [Issue1]
> Actually, this is to pave the way for patch 7/7, in case
> qemu_thread_create() fails.
> Patch 7/7 is long enough, thus I wrote the patch 1/2/3 separately to mainly
> connects the broken errp for callers who initially have the errp.
>
> [I am back... If the other codes in this patches be squashed, maybe
> merge [Issue1]
> into patch 7/7 looks more intuitive.]
>>> 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(_displays, vd, next);
>>
>> QTAILQ_INIT(>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.
> [Issue2]
> Right. But I notice the returned kbd_layout_t *kbd_layout is a static
> value and also
> will be used by others, how about passing the errp parameter to
> init_keyboard_layout()
> as follows? And for its other callers, just pass NULL.
>
> @@ -3222,13 +3222,13 @@ void vnc_display_init(const char *id, Error **errp)
>
> if (keyboard_layout) {
> trace_vnc_key_map_init(keyboard_layout);
> - vd->kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout);
> + vd->kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout,
> errp);
> } else {
> - vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us");
> + vd->kbd_layout =
Re: [Qemu-devel] [PATCH RFC v5 2/7] ui/vnc.c: polish vnc_init_func
On 10/11/2018 09:13 PM, Markus Armbruster wrote:
Fei Li 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
Reviewed-by: Fam Zheng
---
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(>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?
[Issue1]
Actually, this is to pave the way for patch 7/7, in case
qemu_thread_create() fails.
Patch 7/7 is long enough, thus I wrote the patch 1/2/3 separately to mainly
connects the broken errp for callers who initially have the errp.
[I am back... If the other codes in this patches be squashed, maybe
merge [Issue1]
into patch 7/7 looks more intuitive.]
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(_displays, vd, next);
QTAILQ_INIT(>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.
[Issue2]
Right. But I notice the returned kbd_layout_t *kbd_layout is a static
value and also
will be used by others, how about passing the errp parameter to
init_keyboard_layout()
as follows? And for its other callers, just pass NULL.
@@ -3222,13 +3222,13 @@ void vnc_display_init(const char *id, Error **errp)
if (keyboard_layout) {
trace_vnc_key_map_init(keyboard_layout);
- vd->kbd_layout = init_keyboard_layout(name2keysym,
keyboard_layout);
+ vd->kbd_layout = init_keyboard_layout(name2keysym,
keyboard_layout, errp);
} else {
- vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us");
+ vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us", errp);
}
if (!vd->kbd_layout) {
- exit(1);
+ return;
}
}
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(>mutex);
-vnc_start_worker_thread();
+if (!vnc_start_worker_thread(errp)) {
+return;
+}
Re: [Qemu-devel] [PATCH RFC v5 2/7] ui/vnc.c: polish vnc_init_func
Fei Li 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
> Reviewed-by: Fam Zheng
> ---
> 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(>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(_displays, vd, next);
QTAILQ_INIT(>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(>mutex);
> -vnc_start_worker_thread();
> +if (!vnc_start_worker_thread(errp)) {
> +return;
> +}
>
> vd->dcl.ops = _ops;
> register_displaychangelistener(>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, _err);
> +if (local_err) {
> +error_reportf_err(local_err, "Failed to init VNC server: ");
> +exit(1);
> +}
> vnc_display_open(id, _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.
