Re: [PATCH for-5.2 01/10] test: new qTest case to test the vhost-user-blk-server

[Coiby Xu]

On Mon, Dec 07, 2020 at 11:28:38AM +0000, Stefan Hajnoczi wrote:
> On Wed, Nov 25, 2020 at 04:28:20PM +0800, Coiby Xu wrote:
> > On Wed, Nov 25, 2020 at 04:20:56PM +0800, Coiby Xu wrote:
> > > On Wed, Nov 11, 2020 at 12:43:22PM +0000, Stefan Hajnoczi wrote:
> > > > +static void quit_storage_daemon(void *qmp_test_state)
> > > > +{
> > > > +    const char quit_str[] = "{ 'execute': 'quit' }";
> > > > +
> > > > +    /* Before quiting storate-daemon, quit qemu to avoid dubious messages 
> > */
> > > > +    qobject_unref(qtest_qmp(global_qtest, quit_str));
> > > > +
> > > > +    /*
> > > > +     * Give storage-daemon enough time to wake up&terminate
> > > > +     * vu_client_trip coroutine so the Coroutine object could
> > > > +     * be cleaned up. Otherwise LeakSanitizer would complain
> > > > +     * about memory leaks.
> > > > +     */
> > > > +    g_usleep(1000);
> > >
> > > Your "[PATCH for-5.2 07/10] vhost-user-blk-test: fix races by using fd 
> > passing"
> > > prompts to me think if there is a race condition under which 1000 ms
> >                                                               ^^^^^^^
> > Sorry, I meant 1000 μs.
>
> In the next revision vhost-user-blk-test sends a SIGTERM signal to
> qemu-storage-daemon and then calls waitpid(2). This way there is a clean
> shutdown without a sleep.

Thank you for the explaining! Do you think checking if
qemu-storage-daemon has exited by detecting if qemu-storage-daemon's
QMP monitor socket has been closed is simpler solution than
waitpid(2) in v2?

> Regarding the LeakSanitizer issue you saw, are you still able to
> reproduce it with commit f10802d2c9fd8bfd92c70f465da1a5992445157f
> ("qemu-storage-daemon: add missing cleanup calls") applied? Maybe
> qemu-storage-daemon is still missing some cleanup code (e.g. to stop
> exports before terminating).

It seems commit f10802d2c9fd8bfd92c70f465da1a5992445157f
("qemu-storage-daemon: add missing cleanup calls") is not related to
the LeakSanitizer issue.

When applying v9 (no "g_usleep(1000)") on
commit eea8f5df4ecc607d64f091b8d916fcc11a697541
("Merge remote-tracking branch 'remotes/awilliam/tags/vfio-update-20200611.0' into 
staging")
which doesn't contain commit f10802d2c9fd8bfd92c70f465da1a5992445157f,
there is no LeakSanitizer issue.

However, if applying v9 on
commit d0ed6a69d399ae193959225cdeaa9382746c91cc ("Update version for v5.1.0 
release")
which contains commit f10802d2c9fd8bfd92c70f465da1a5992445157f, there is
the LeakSanitizer issue.

And if applying this patch set on
commit 23895cbd82be95428e90168b12e925d0d3ca2f06
("Merge remote-tracking branch 'remotes/awilliam/tags/vfio-update-20201123.0' into 
staging"),
the LeakSanitizer issue couldn't be reproduced even without "g_usleep(1000)".

> Stefan



--
Best regards,
Coiby