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 <fli@suse.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
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;
}
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;
@@ -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: ");
--
2.13.7
Fei Li <fli@suse.com> 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 <fli@suse.com>
> Reviewed-by: Fam Zheng <famz@redhat.com>
> ---
> 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.
On 10/11/2018 09:13 PM, Markus Armbruster wrote:
> Fei Li <fli@suse.com> 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 <fli@suse.com>
>> Reviewed-by: Fam Zheng <famz@redhat.com>
>> ---
>> 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?
[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(&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.
[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(&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.
Ah, I noticed your patch 24/31. OK, let's squash.
Should I write a new patch by
- updating to error_propagate(...) if vnc_display_init() fails
&&
- modifying [Issue2] ?
Or you do this in your original patch?
BTW, for your patch 24/31, the updated passed errp for vnc_init_func is
&error_fatal,
then the system will exit(1) when running error_propagate(...) which calls
error_handle_fatal(...). This means the following two lines will not be
touched.
But if we want the following prepended error message, could we move it
earlier than
the error_propagare? I mean:
if (local_err != NULL) {
- error_reportf_err(local_err, "Failed to start VNC server: ");
- exit(1);
+ error_prepend(&local_err, "Failed to start VNC server: ");
+ error_propagate(errp, local_err);
+ return -1;
}
Have a nice day
Fei
Fei Li <fli@suse.com> writes:
> On 10/11/2018 09:13 PM, Markus Armbruster wrote:
>> Fei Li <fli@suse.com> 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 <fli@suse.com>
>>> Reviewed-by: Fam Zheng <famz@redhat.com>
>>> ---
>>> 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?
> [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(&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.
> [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 &error_fatal instead of NULL
to preserve "report error and exit" semantics.
>>
>> }
>>
>> 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.
> Ah, I noticed your patch 24/31. OK, let's squash.
> Should I write a new patch by
> - updating to error_propagate(...) if vnc_display_init() fails
> &&
> - modifying [Issue2] ?
> Or you do this in your original patch?
If you send a v2 along that lines, I'll probably pick your patch into my
series. Should work even in case your series gets merged first.
> BTW, for your patch 24/31, the updated passed errp for vnc_init_func
> is &error_fatal,
> then the system will exit(1) when running error_propagate(...) which calls
> error_handle_fatal(...). This means the following two lines will not
> be touched.
> But if we want the following prepended error message, could we move it
> earlier than
> the error_propagare? I mean:
>
> if (local_err != NULL) {
> - error_reportf_err(local_err, "Failed to start VNC server: ");
> - exit(1);
> + error_prepend(&local_err, "Failed to start VNC server: ");
> + error_propagate(errp, local_err);
> + return -1;
> }
Both
error_propagate(errp, local_err);
error_prepend(errp, "Failed to start VNC server: ");
and
error_prepend(&local_err, "Failed to start VNC server: ");
error_propagate(errp, local_err);
work. The former is slightly more efficient when errp is null. But
you're right, it fails to include the "Failed to start VNC server: "
prefix with &error_fatal. Thus, the latter is preferrable.
On 10/12/2018 04:18 PM, Markus Armbruster wrote:
> Fei Li <fli@suse.com> writes:
>
>> On 10/11/2018 09:13 PM, Markus Armbruster wrote:
>>> Fei Li <fli@suse.com> 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 <fli@suse.com>
>>>> Reviewed-by: Fam Zheng <famz@redhat.com>
>>>> ---
>>>> 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?
>> [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(&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.
>> [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 &error_fatal instead of NULL
> to preserve "report error and exit" semantics.
Yes, you are right. I should pass &error_fatal and let the following
error_setg() handle this.
@@ -94,7 +94,7 @@ static kbd_layout_t *parse_keyboard_layout(const
name2keysym_t *table,
f = filename ? fopen(filename, "r") : NULL;
g_free(filename);
if (!f) {
- fprintf(stderr, "Could not read keymap file: '%s'\n", language);
+ error_setg(errp, "could not read keymap file: '%s'\n", language);
return NULL;
}
>
>>> }
>>>
>>> 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.
>> Ah, I noticed your patch 24/31. OK, let's squash.
>> Should I write a new patch by
>> - updating to error_propagate(...) if vnc_display_init() fails
>> &&
>> - modifying [Issue2] ?
>> Or you do this in your original patch?
> If you send a v2 along that lines, I'll probably pick your patch into my
> series. Should work even in case your series gets merged first.
Good idea. I will send a separate v2 patch which also include the
above change mentioned in [Issue1].
>
>> BTW, for your patch 24/31, the updated passed errp for vnc_init_func
>> is &error_fatal,
>> then the system will exit(1) when running error_propagate(...) which calls
>> error_handle_fatal(...). This means the following two lines will not
>> be touched.
>> But if we want the following prepended error message, could we move it
>> earlier than
>> the error_propagare? I mean:
>>
>> if (local_err != NULL) {
>> - error_reportf_err(local_err, "Failed to start VNC server: ");
>> - exit(1);
>> + error_prepend(&local_err, "Failed to start VNC server: ");
>> + error_propagate(errp, local_err);
>> + return -1;
>> }
> Both
>
> error_propagate(errp, local_err);
> error_prepend(errp, "Failed to start VNC server: ");
>
> and
>
> error_prepend(&local_err, "Failed to start VNC server: ");
> error_propagate(errp, local_err);
>
> work. The former is slightly more efficient when errp is null. But
> you're right, it fails to include the "Failed to start VNC server: "
> prefix with &error_fatal. Thus, the latter is preferrable.
>
>
Have a nice day, thanks
Fei
On 10/12/2018 06:23 PM, Fei Li wrote:
>
>
> On 10/12/2018 04:18 PM, Markus Armbruster wrote:
>> Fei Li <fli@suse.com> writes:
>>
>>> On 10/11/2018 09:13 PM, Markus Armbruster wrote:
>>>> Fei Li <fli@suse.com> 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 <fli@suse.com>
>>>>> Reviewed-by: Fam Zheng <famz@redhat.com>
>>>>> ---
>>>>> 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?
>>> [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(&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.
>>> [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 &error_fatal instead of NULL
>> to preserve "report error and exit" semantics.
> Yes, you are right. I should pass &error_fatal and let the following
> error_setg() handle this.
>
> @@ -94,7 +94,7 @@ static kbd_layout_t *parse_keyboard_layout(const
> name2keysym_t *table,
> f = filename ? fopen(filename, "r") : NULL;
> g_free(filename);
> if (!f) {
> - fprintf(stderr, "Could not read keymap file: '%s'\n", language);
> + error_setg(errp, "could not read keymap file: '%s'\n",
> language);
> return NULL;
> }
>
>>
>>>> }
>>>>
>>>> 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.
>>> Ah, I noticed your patch 24/31. OK, let's squash.
>>> Should I write a new patch by
>>> - updating to error_propagate(...) if vnc_display_init() fails
>>> &&
>>> - modifying [Issue2] ?
>>> Or you do this in your original patch?
>> If you send a v2 along that lines, I'll probably pick your patch into my
>> series. Should work even in case your series gets merged first.
> Good idea. I will send a separate v2 patch which also include the
> above change mentioned in [Issue1].
After thinking twice, I think move the above change mentioned in [Issue1]
to patch 7/7 is better. Thus my next separate v2 will not include it.
>>
>>> BTW, for your patch 24/31, the updated passed errp for vnc_init_func
>>> is &error_fatal,
>>> then the system will exit(1) when running error_propagate(...) which
>>> calls
>>> error_handle_fatal(...). This means the following two lines will not
>>> be touched.
>>> But if we want the following prepended error message, could we move it
>>> earlier than
>>> the error_propagare? I mean:
>>>
>>> if (local_err != NULL) {
>>> - error_reportf_err(local_err, "Failed to start VNC server: ");
>>> - exit(1);
>>> + error_prepend(&local_err, "Failed to start VNC server: ");
>>> + error_propagate(errp, local_err);
>>> + return -1;
>>> }
>> Both
>>
>> error_propagate(errp, local_err);
>> error_prepend(errp, "Failed to start VNC server: ");
>>
>> and
>>
>> error_prepend(&local_err, "Failed to start VNC server: ");
>> error_propagate(errp, local_err);
>>
>> work. The former is slightly more efficient when errp is null. But
>> you're right, it fails to include the "Failed to start VNC server: "
>> prefix with &error_fatal. Thus, the latter is preferrable.
>>
>>
> Have a nice day, thanks
> Fei
>
>
>
© 2016 - 2025 Red Hat, Inc.