Re: [Qemu-devel] [PATCH RFC v5 2/7] ui/vnc.c: polish vnc_init_func

[Markus Armbruster <armbru@redhat.com>]

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.