Re: [PATCH 2/2] selftests/ublk: fix garbage output and cleanup on failure

[Ming Lei <ming.lei@redhat.com>]

On Mon, Jan 12, 2026 at 09:36:39AM -0800, Caleb Sander Mateos wrote:
> On Sun, Jan 11, 2026 at 8:12 PM Ming Lei <ming.lei@redhat.com> wrote:
> >
> > Fix several issues in kublk:
> >
> > 1. Initialize _evtfd to -1 in struct dev_ctx to prevent garbage output
> >    in foreground mode. Without this, _evtfd is zero-initialized to 0
> >    (stdin), and when ublk_send_dev_event() is called on failure, it
> >    writes binary data to stdin which appears as garbage on the terminal.
> 
> Nice, I always wondered why that happened!
> 
> >
> > 2. Move fail label in ublk_start_daemon() to ensure pthread_join() is
> >    called before queue deinit on the error path. This ensures proper
> >    thread cleanup when startup fails.
> >
> > 3. Add async parameter to ublk_ctrl_del_dev() and use async deletion
> >    when the daemon fails to start. This prevents potential hangs when
> >    deleting a device that failed during startup.
> >
> > Also fix a debug message format string that was missing __func__ and
> > had wrong escape character.
> 
> These all look good, but maybe they would make sense as separate commits?

Fine, I will split it into two patches:

- one is for fixing start_dev failure handling

- another one is for fixing misc debug message problems

> 
> >
> > Fixes: 6aecda00b7d1 ("selftests: ublk: add kernel selftests for ublk")
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >  tools/testing/selftests/ublk/kublk.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/tools/testing/selftests/ublk/kublk.c b/tools/testing/selftests/ublk/kublk.c
> > index 185ba553686a..0c62a967f2cb 100644
> > --- a/tools/testing/selftests/ublk/kublk.c
> > +++ b/tools/testing/selftests/ublk/kublk.c
> > @@ -153,11 +153,10 @@ static int ublk_ctrl_add_dev(struct ublk_dev *dev)
> >         return __ublk_ctrl_cmd(dev, &data);
> >  }
> >
> > -static int ublk_ctrl_del_dev(struct ublk_dev *dev)
> > +static int ublk_ctrl_del_dev(struct ublk_dev *dev, bool async)
> >  {
> >         struct ublk_ctrl_cmd_data data = {
> > -               .cmd_op = UBLK_U_CMD_DEL_DEV,
> > -               .flags = 0,
> > +               .cmd_op = async ? UBLK_U_CMD_DEL_DEV_ASYNC: UBLK_U_CMD_DEL_DEV,
> >         };
> >
> >         return __ublk_ctrl_cmd(dev, &data);
> > @@ -1063,11 +1062,11 @@ static int ublk_start_daemon(const struct dev_ctx *ctx, struct ublk_dev *dev)
> >         else
> >                 ublk_send_dev_event(ctx, dev, dev->dev_info.dev_id);
> >
> > + fail:
> >         /* wait until we are terminated */
> >         for (i = 0; i < dev->nthreads; i++)
> >                 pthread_join(tinfo[i].thread, &thread_ret);
> 
> Is it valid to call pthread_join() on a zeroed pthread_t value? If
> ublk_queue_init() fails, there is a goto fail before any of the
> tinfo[i].thread have been assigned. And there's no checking of the
> return value from pthread_create(), so if pthread_create() fails,
> tinfo[i].thread may not be assigned either.

Good catch, will add new fail_queue_init label for covering it.
 

Thanks,
Ming