| Message ID | 20180316142223.28745-1-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | tests: fix tpm-crb tpm-tis tests race | expand |
On Fri, Mar 16, 2018 at 03:22:23PM +0100, Marc-André Lureau wrote: > No need to close the TPM data socket on the emulator end, qemu will > close it after a SHUTDOWN. This avoids a race between close() and > read() in the TPM data thread. > > Reported-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > tests/tpm-emu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tests/tpm-emu.c b/tests/tpm-emu.c > index 4dada76834..8c2bd53cad 100644 > --- a/tests/tpm-emu.c > +++ b/tests/tpm-emu.c > @@ -125,7 +125,7 @@ void *tpm_emu_ctrl_thread(void *data) > case CMD_SHUTDOWN: { > ptm_res res = 0; > qio_channel_write(ioc, (char *)&res, sizeof(res), &error_abort); > - qio_channel_close(s->tpm_ioc, &error_abort); > + /* the tpm data thread is expected to finish now */ > g_thread_join(s->emu_tpm_thread); Won't this leave an orphaed FD open in the test suite ? Is it perhaps sufficient to just swap the order of the g_thread_join and qio_channel_close calls, so that we join the thread before we close the channel that the thread is using ? Regards, Daniel
Hi On Fri, Mar 16, 2018 at 3:25 PM, Daniel P. Berrangé <berrange@redhat.com> wrote: > On Fri, Mar 16, 2018 at 03:22:23PM +0100, Marc-André Lureau wrote: >> No need to close the TPM data socket on the emulator end, qemu will >> close it after a SHUTDOWN. This avoids a race between close() and >> read() in the TPM data thread. >> >> Reported-by: Peter Maydell <peter.maydell@linaro.org> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >> --- >> tests/tpm-emu.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/tests/tpm-emu.c b/tests/tpm-emu.c >> index 4dada76834..8c2bd53cad 100644 >> --- a/tests/tpm-emu.c >> +++ b/tests/tpm-emu.c >> @@ -125,7 +125,7 @@ void *tpm_emu_ctrl_thread(void *data) >> case CMD_SHUTDOWN: { >> ptm_res res = 0; >> qio_channel_write(ioc, (char *)&res, sizeof(res), &error_abort); >> - qio_channel_close(s->tpm_ioc, &error_abort); >> + /* the tpm data thread is expected to finish now */ >> g_thread_join(s->emu_tpm_thread); > > Won't this leave an orphaed FD open in the test suite ? Is it perhaps > sufficient to just swap the order of the g_thread_join and qio_channel_close > calls, so that we join the thread before we close the channel that the > thread is using ? Isn't the socket close on the last unref? (at end of tpm_emu_tpm_thread()) > > Regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| >
On Fri, Mar 16, 2018 at 03:29:04PM +0100, Marc-André Lureau wrote: > Hi > > On Fri, Mar 16, 2018 at 3:25 PM, Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Fri, Mar 16, 2018 at 03:22:23PM +0100, Marc-André Lureau wrote: > >> No need to close the TPM data socket on the emulator end, qemu will > >> close it after a SHUTDOWN. This avoids a race between close() and > >> read() in the TPM data thread. > >> > >> Reported-by: Peter Maydell <peter.maydell@linaro.org> > >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > >> --- > >> tests/tpm-emu.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/tests/tpm-emu.c b/tests/tpm-emu.c > >> index 4dada76834..8c2bd53cad 100644 > >> --- a/tests/tpm-emu.c > >> +++ b/tests/tpm-emu.c > >> @@ -125,7 +125,7 @@ void *tpm_emu_ctrl_thread(void *data) > >> case CMD_SHUTDOWN: { > >> ptm_res res = 0; > >> qio_channel_write(ioc, (char *)&res, sizeof(res), &error_abort); > >> - qio_channel_close(s->tpm_ioc, &error_abort); > >> + /* the tpm data thread is expected to finish now */ > >> g_thread_join(s->emu_tpm_thread); > > > > Won't this leave an orphaed FD open in the test suite ? Is it perhaps > > sufficient to just swap the order of the g_thread_join and qio_channel_close > > calls, so that we join the thread before we close the channel that the > > thread is using ? > > Isn't the socket close on the last unref? (at end of tpm_emu_tpm_thread()) Oh yes, that is true. I should have looked at more than just diff context. In that case Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel
On 03/16/2018 10:30 AM, Daniel P. Berrangé wrote: > On Fri, Mar 16, 2018 at 03:29:04PM +0100, Marc-André Lureau wrote: >> Hi >> >> On Fri, Mar 16, 2018 at 3:25 PM, Daniel P. Berrangé <berrange@redhat.com> wrote: >>> On Fri, Mar 16, 2018 at 03:22:23PM +0100, Marc-André Lureau wrote: >>>> No need to close the TPM data socket on the emulator end, qemu will >>>> close it after a SHUTDOWN. This avoids a race between close() and >>>> read() in the TPM data thread. >>>> >>>> Reported-by: Peter Maydell <peter.maydell@linaro.org> >>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >>>> --- >>>> tests/tpm-emu.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/tests/tpm-emu.c b/tests/tpm-emu.c >>>> index 4dada76834..8c2bd53cad 100644 >>>> --- a/tests/tpm-emu.c >>>> +++ b/tests/tpm-emu.c >>>> @@ -125,7 +125,7 @@ void *tpm_emu_ctrl_thread(void *data) >>>> case CMD_SHUTDOWN: { >>>> ptm_res res = 0; >>>> qio_channel_write(ioc, (char *)&res, sizeof(res), &error_abort); >>>> - qio_channel_close(s->tpm_ioc, &error_abort); >>>> + /* the tpm data thread is expected to finish now */ >>>> g_thread_join(s->emu_tpm_thread); >>> Won't this leave an orphaed FD open in the test suite ? Is it perhaps >>> sufficient to just swap the order of the g_thread_join and qio_channel_close >>> calls, so that we join the thread before we close the channel that the >>> thread is using ? >> Isn't the socket close on the last unref? (at end of tpm_emu_tpm_thread()) > Oh yes, that is true. I should have looked at more than just diff context. > In that case > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Ha. It's already done :-) Reviewed-by: Stefan Berger <stefanb@linux.vnet.ibm.com> > > Regards, > Daniel
Marc-André Lureau <marcandre.lureau@redhat.com> writes: > No need to close the TPM data socket on the emulator end, qemu will > close it after a SHUTDOWN. This avoids a race between close() and > read() in the TPM data thread. > > Reported-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> Tested-by: Alex Bennée <alex.bennee@linaro.org> > --- > tests/tpm-emu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tests/tpm-emu.c b/tests/tpm-emu.c > index 4dada76834..8c2bd53cad 100644 > --- a/tests/tpm-emu.c > +++ b/tests/tpm-emu.c > @@ -125,7 +125,7 @@ void *tpm_emu_ctrl_thread(void *data) > case CMD_SHUTDOWN: { > ptm_res res = 0; > qio_channel_write(ioc, (char *)&res, sizeof(res), &error_abort); > - qio_channel_close(s->tpm_ioc, &error_abort); > + /* the tpm data thread is expected to finish now */ > g_thread_join(s->emu_tpm_thread); > break; > } -- Alex Bennée
diff --git a/tests/tpm-emu.c b/tests/tpm-emu.c index 4dada76834..8c2bd53cad 100644 --- a/tests/tpm-emu.c +++ b/tests/tpm-emu.c @@ -125,7 +125,7 @@ void *tpm_emu_ctrl_thread(void *data) case CMD_SHUTDOWN: { ptm_res res = 0; qio_channel_write(ioc, (char *)&res, sizeof(res), &error_abort); - qio_channel_close(s->tpm_ioc, &error_abort); + /* the tpm data thread is expected to finish now */ g_thread_join(s->emu_tpm_thread); break; }
No need to close the TPM data socket on the emulator end, qemu will close it after a SHUTDOWN. This avoids a race between close() and read() in the TPM data thread. Reported-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- tests/tpm-emu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)