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

[Fei Li]

On 10/11/2018 09:13 PM, Markus Armbruster wrote:
> 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?
[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