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

[Fei Li <fli@suse.com>]

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