| Message ID | 950007e82e19e75831b29fac07ab990c213d2352.1616368879.git.lukasstraub2@web.de |
|---|---|
| State | New |
| Headers | show |
| Series | yank: Add chardev tests and fixes | expand |
On 22/03/2021 00.31, Lukas Straub wrote: > Use the normal yank code instead of stubs in relevant tests to > increase coverage and to ensure that registering and unregistering > of yank instances and functions is done correctly. > > Signed-off-by: Lukas Straub <lukasstraub2@web.de> > --- > tests/qtest/meson.build | 6 +++--- > tests/unit/meson.build | 4 ++-- > 2 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build > index 66ee9fbf45..40e1f495f7 100644 > --- a/tests/qtest/meson.build > +++ b/tests/qtest/meson.build > @@ -234,9 +234,9 @@ tpmemu_files = ['tpm-emu.c', 'tpm-util.c', 'tpm-tests.c'] > qtests = { > 'bios-tables-test': [io, 'boot-sector.c', 'acpi-utils.c', 'tpm-emu.c'], > 'cdrom-test': files('boot-sector.c'), > - 'dbus-vmstate-test': files('migration-helpers.c') + dbus_vmstate1, > + 'dbus-vmstate-test': ['migration-helpers.c', dbus_vmstate1, '../../monitor/yank.c'], > 'ivshmem-test': [rt, '../../contrib/ivshmem-server/ivshmem-server.c'], > - 'migration-test': files('migration-helpers.c'), > + 'migration-test': ['migration-helpers.c', io, '../../monitor/yank.c'], > 'pxe-test': files('boot-sector.c'), > 'qos-test': [chardev, io, qos_test_ss.apply(config_host, strict: false).sources()], > 'tpm-crb-swtpm-test': [io, tpmemu_files], Is this really necessary for the qtests? I can understand the change for the unit tests, but the qtests are separate programs where I could not imagine that they use the yank functions in any way? Thomas PS: Please add a proper description about the yank feature to either that yank.c file or to include/qemu/yank.h ... I had a hard time to find out what this code is all about until I finally looked up your cover letter of the original series on the mailing list.
On Mon, 22 Mar 2021 06:20:50 +0100 Thomas Huth <thuth@redhat.com> wrote: > On 22/03/2021 00.31, Lukas Straub wrote: > > Use the normal yank code instead of stubs in relevant tests to > > increase coverage and to ensure that registering and unregistering > > of yank instances and functions is done correctly. > > > > Signed-off-by: Lukas Straub <lukasstraub2@web.de> > > --- > > tests/qtest/meson.build | 6 +++--- > > tests/unit/meson.build | 4 ++-- > > 2 files changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build > > index 66ee9fbf45..40e1f495f7 100644 > > --- a/tests/qtest/meson.build > > +++ b/tests/qtest/meson.build > > @@ -234,9 +234,9 @@ tpmemu_files = ['tpm-emu.c', 'tpm-util.c', 'tpm-tests.c'] > > qtests = { > > 'bios-tables-test': [io, 'boot-sector.c', 'acpi-utils.c', 'tpm-emu.c'], > > 'cdrom-test': files('boot-sector.c'), > > - 'dbus-vmstate-test': files('migration-helpers.c') + dbus_vmstate1, > > + 'dbus-vmstate-test': ['migration-helpers.c', dbus_vmstate1, '../../monitor/yank.c'], > > 'ivshmem-test': [rt, '../../contrib/ivshmem-server/ivshmem-server.c'], > > - 'migration-test': files('migration-helpers.c'), > > + 'migration-test': ['migration-helpers.c', io, '../../monitor/yank.c'], > > 'pxe-test': files('boot-sector.c'), > > 'qos-test': [chardev, io, qos_test_ss.apply(config_host, strict: false).sources()], > > 'tpm-crb-swtpm-test': [io, tpmemu_files], > > Is this really necessary for the qtests? I can understand the change for the > unit tests, but the qtests are separate programs where I could not imagine > that they use the yank functions in any way? Yes, it is necessary. While the yank functions are not called in these tests, it still checks that registering and unregistering of yank instances and functions is done correctly. I.e. That no yank functions are registered before the instance, that the yank instance is only unregistered after all functions where unregistered, that the same instance is not registered twice and that the yank instance actually exists before it is unregistered. > Thomas > > > PS: Please add a proper description about the yank feature to either that > yank.c file or to include/qemu/yank.h ... I had a hard time to find out what > this code is all about until I finally looked up your cover letter of the > original series on the mailing list. > Will do. Regards, Lukas Straub --
On 22/03/2021 08.35, Lukas Straub wrote: > On Mon, 22 Mar 2021 06:20:50 +0100 > Thomas Huth <thuth@redhat.com> wrote: > >> On 22/03/2021 00.31, Lukas Straub wrote: >>> Use the normal yank code instead of stubs in relevant tests to >>> increase coverage and to ensure that registering and unregistering >>> of yank instances and functions is done correctly. >>> >>> Signed-off-by: Lukas Straub <lukasstraub2@web.de> >>> --- >>> tests/qtest/meson.build | 6 +++--- >>> tests/unit/meson.build | 4 ++-- >>> 2 files changed, 5 insertions(+), 5 deletions(-) >>> >>> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build >>> index 66ee9fbf45..40e1f495f7 100644 >>> --- a/tests/qtest/meson.build >>> +++ b/tests/qtest/meson.build >>> @@ -234,9 +234,9 @@ tpmemu_files = ['tpm-emu.c', 'tpm-util.c', 'tpm-tests.c'] >>> qtests = { >>> 'bios-tables-test': [io, 'boot-sector.c', 'acpi-utils.c', 'tpm-emu.c'], >>> 'cdrom-test': files('boot-sector.c'), >>> - 'dbus-vmstate-test': files('migration-helpers.c') + dbus_vmstate1, >>> + 'dbus-vmstate-test': ['migration-helpers.c', dbus_vmstate1, '../../monitor/yank.c'], >>> 'ivshmem-test': [rt, '../../contrib/ivshmem-server/ivshmem-server.c'], >>> - 'migration-test': files('migration-helpers.c'), >>> + 'migration-test': ['migration-helpers.c', io, '../../monitor/yank.c'], >>> 'pxe-test': files('boot-sector.c'), >>> 'qos-test': [chardev, io, qos_test_ss.apply(config_host, strict: false).sources()], >>> 'tpm-crb-swtpm-test': [io, tpmemu_files], >> >> Is this really necessary for the qtests? I can understand the change for the >> unit tests, but the qtests are separate programs where I could not imagine >> that they use the yank functions in any way? > > Yes, it is necessary. While the yank functions are not called in these tests, > it still checks that registering and unregistering of yank instances and > functions is done correctly. I.e. That no yank functions are registered before > the instance, that the yank instance is only unregistered after all functions > where unregistered, that the same instance is not registered twice and that > the yank instance actually exists before it is unregistered. Now you even confused me more. Could you elaborate a little bit? If none of the functions are called by the test, which part of yank.c is excercised here at all? Could you give a more detailed example? The only thing I could imagine is yank_init(), but that does not look like something we need to check in a qtest ? Thomas
On Mon, 22 Mar 2021 17:00:23 +0100 Thomas Huth <thuth@redhat.com> wrote: > On 22/03/2021 08.35, Lukas Straub wrote: > > On Mon, 22 Mar 2021 06:20:50 +0100 > > Thomas Huth <thuth@redhat.com> wrote: > > > >> On 22/03/2021 00.31, Lukas Straub wrote: > >>> Use the normal yank code instead of stubs in relevant tests to > >>> increase coverage and to ensure that registering and unregistering > >>> of yank instances and functions is done correctly. > >>> > >>> Signed-off-by: Lukas Straub <lukasstraub2@web.de> > >>> --- > >>> tests/qtest/meson.build | 6 +++--- > >>> tests/unit/meson.build | 4 ++-- > >>> 2 files changed, 5 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build > >>> index 66ee9fbf45..40e1f495f7 100644 > >>> --- a/tests/qtest/meson.build > >>> +++ b/tests/qtest/meson.build > >>> @@ -234,9 +234,9 @@ tpmemu_files = ['tpm-emu.c', 'tpm-util.c', 'tpm-tests.c'] > >>> qtests = { > >>> 'bios-tables-test': [io, 'boot-sector.c', 'acpi-utils.c', 'tpm-emu.c'], > >>> 'cdrom-test': files('boot-sector.c'), > >>> - 'dbus-vmstate-test': files('migration-helpers.c') + dbus_vmstate1, > >>> + 'dbus-vmstate-test': ['migration-helpers.c', dbus_vmstate1, '../../monitor/yank.c'], > >>> 'ivshmem-test': [rt, '../../contrib/ivshmem-server/ivshmem-server.c'], > >>> - 'migration-test': files('migration-helpers.c'), > >>> + 'migration-test': ['migration-helpers.c', io, '../../monitor/yank.c'], > >>> 'pxe-test': files('boot-sector.c'), > >>> 'qos-test': [chardev, io, qos_test_ss.apply(config_host, strict: false).sources()], > >>> 'tpm-crb-swtpm-test': [io, tpmemu_files], > >> > >> Is this really necessary for the qtests? I can understand the change for the > >> unit tests, but the qtests are separate programs where I could not imagine > >> that they use the yank functions in any way? > > > > Yes, it is necessary. While the yank functions are not called in these tests, > > it still checks that registering and unregistering of yank instances and > > functions is done correctly. I.e. That no yank functions are registered before > > the instance, that the yank instance is only unregistered after all functions > > where unregistered, that the same instance is not registered twice and that > > the yank instance actually exists before it is unregistered. > > Now you even confused me more. Could you elaborate a little bit? If none of > the functions are called by the test, which part of yank.c is excercised > here at all? Could you give a more detailed example? The only thing I could > imagine is yank_init(), but that does not look like something we need to > check in a qtest ? Oh, sorry. I meant yank's concept of a yank function here. It works this way: The different subsystems first register a yank instance. So in this case when starting migration in the test, the migration code first registers a yank instance. Then, it registers _yank functions_ with this instance, for for example to shutdown a socket. Now, (in the real-world qemu case) if qemu hangs, the user can use the 'yank' qmp command to call the _yank functions_ to recover. The test doesn't test the 'yank' qmp command, but yank_register_instance, yank_register_function (which are called by the migration code and thus also the migration test) still check if everything is done correctly. Regards, Lukas Straub > Thomas > --
On 22/03/2021 18.48, Lukas Straub wrote: > On Mon, 22 Mar 2021 17:00:23 +0100 > Thomas Huth <thuth@redhat.com> wrote: > >> On 22/03/2021 08.35, Lukas Straub wrote: >>> On Mon, 22 Mar 2021 06:20:50 +0100 >>> Thomas Huth <thuth@redhat.com> wrote: >>> >>>> On 22/03/2021 00.31, Lukas Straub wrote: >>>>> Use the normal yank code instead of stubs in relevant tests to >>>>> increase coverage and to ensure that registering and unregistering >>>>> of yank instances and functions is done correctly. >>>>> >>>>> Signed-off-by: Lukas Straub <lukasstraub2@web.de> >>>>> --- >>>>> tests/qtest/meson.build | 6 +++--- >>>>> tests/unit/meson.build | 4 ++-- >>>>> 2 files changed, 5 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build >>>>> index 66ee9fbf45..40e1f495f7 100644 >>>>> --- a/tests/qtest/meson.build >>>>> +++ b/tests/qtest/meson.build >>>>> @@ -234,9 +234,9 @@ tpmemu_files = ['tpm-emu.c', 'tpm-util.c', 'tpm-tests.c'] >>>>> qtests = { >>>>> 'bios-tables-test': [io, 'boot-sector.c', 'acpi-utils.c', 'tpm-emu.c'], >>>>> 'cdrom-test': files('boot-sector.c'), >>>>> - 'dbus-vmstate-test': files('migration-helpers.c') + dbus_vmstate1, >>>>> + 'dbus-vmstate-test': ['migration-helpers.c', dbus_vmstate1, '../../monitor/yank.c'], >>>>> 'ivshmem-test': [rt, '../../contrib/ivshmem-server/ivshmem-server.c'], >>>>> - 'migration-test': files('migration-helpers.c'), >>>>> + 'migration-test': ['migration-helpers.c', io, '../../monitor/yank.c'], >>>>> 'pxe-test': files('boot-sector.c'), >>>>> 'qos-test': [chardev, io, qos_test_ss.apply(config_host, strict: false).sources()], >>>>> 'tpm-crb-swtpm-test': [io, tpmemu_files], >>>> >>>> Is this really necessary for the qtests? I can understand the change for the >>>> unit tests, but the qtests are separate programs where I could not imagine >>>> that they use the yank functions in any way? >>> >>> Yes, it is necessary. While the yank functions are not called in these tests, >>> it still checks that registering and unregistering of yank instances and >>> functions is done correctly. I.e. That no yank functions are registered before >>> the instance, that the yank instance is only unregistered after all functions >>> where unregistered, that the same instance is not registered twice and that >>> the yank instance actually exists before it is unregistered. >> >> Now you even confused me more. Could you elaborate a little bit? If none of >> the functions are called by the test, which part of yank.c is excercised >> here at all? Could you give a more detailed example? The only thing I could >> imagine is yank_init(), but that does not look like something we need to >> check in a qtest ? > > Oh, sorry. I meant yank's concept of a yank function here. It works this way: > The different subsystems first register a yank instance. So in this case > when starting migration in the test, the migration code first registers a > yank instance. Then, it registers _yank functions_ with this instance, for > for example to shutdown a socket. But these are the qtest, separate stand-alone programs. The migration code of QEMU (i.e. the code in the main "migration" folder) is not linked into these binaries. Doing something like: grep -r yank tests/qtest/migration-test should give you zero results. Thus it IMHO does not make sense to add the yank.c to these tests here. Having said that, it seems like the qos-test is linking against the chardev code and thus might use indirectly the yank code there. So you maybe might want to add it to the qos-test instead? Thomas
On Tue, 23 Mar 2021 05:46:24 +0100 Thomas Huth <thuth@redhat.com> wrote: > On 22/03/2021 18.48, Lukas Straub wrote: > > On Mon, 22 Mar 2021 17:00:23 +0100 > > Thomas Huth <thuth@redhat.com> wrote: > > > >> On 22/03/2021 08.35, Lukas Straub wrote: > >>> On Mon, 22 Mar 2021 06:20:50 +0100 > >>> Thomas Huth <thuth@redhat.com> wrote: > >>> > >>>> On 22/03/2021 00.31, Lukas Straub wrote: > >>>>> Use the normal yank code instead of stubs in relevant tests to > >>>>> increase coverage and to ensure that registering and unregistering > >>>>> of yank instances and functions is done correctly. > >>>>> > >>>>> Signed-off-by: Lukas Straub <lukasstraub2@web.de> > >>>>> --- > >>>>> tests/qtest/meson.build | 6 +++--- > >>>>> tests/unit/meson.build | 4 ++-- > >>>>> 2 files changed, 5 insertions(+), 5 deletions(-) > >>>>> > >>>>> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build > >>>>> index 66ee9fbf45..40e1f495f7 100644 > >>>>> --- a/tests/qtest/meson.build > >>>>> +++ b/tests/qtest/meson.build > >>>>> @@ -234,9 +234,9 @@ tpmemu_files = ['tpm-emu.c', 'tpm-util.c', 'tpm-tests.c'] > >>>>> qtests = { > >>>>> 'bios-tables-test': [io, 'boot-sector.c', 'acpi-utils.c', 'tpm-emu.c'], > >>>>> 'cdrom-test': files('boot-sector.c'), > >>>>> - 'dbus-vmstate-test': files('migration-helpers.c') + dbus_vmstate1, > >>>>> + 'dbus-vmstate-test': ['migration-helpers.c', dbus_vmstate1, '../../monitor/yank.c'], > >>>>> 'ivshmem-test': [rt, '../../contrib/ivshmem-server/ivshmem-server.c'], > >>>>> - 'migration-test': files('migration-helpers.c'), > >>>>> + 'migration-test': ['migration-helpers.c', io, '../../monitor/yank.c'], > >>>>> 'pxe-test': files('boot-sector.c'), > >>>>> 'qos-test': [chardev, io, qos_test_ss.apply(config_host, strict: false).sources()], > >>>>> 'tpm-crb-swtpm-test': [io, tpmemu_files], > >>>> > >>>> Is this really necessary for the qtests? I can understand the change for the > >>>> unit tests, but the qtests are separate programs where I could not imagine > >>>> that they use the yank functions in any way? > >>> > >>> Yes, it is necessary. While the yank functions are not called in these tests, > >>> it still checks that registering and unregistering of yank instances and > >>> functions is done correctly. I.e. That no yank functions are registered before > >>> the instance, that the yank instance is only unregistered after all functions > >>> where unregistered, that the same instance is not registered twice and that > >>> the yank instance actually exists before it is unregistered. > >> > >> Now you even confused me more. Could you elaborate a little bit? If none of > >> the functions are called by the test, which part of yank.c is excercised > >> here at all? Could you give a more detailed example? The only thing I could > >> imagine is yank_init(), but that does not look like something we need to > >> check in a qtest ? > > > > Oh, sorry. I meant yank's concept of a yank function here. It works this way: > > The different subsystems first register a yank instance. So in this case > > when starting migration in the test, the migration code first registers a > > yank instance. Then, it registers _yank functions_ with this instance, for > > for example to shutdown a socket. > > But these are the qtest, separate stand-alone programs. The migration code > of QEMU (i.e. the code in the main "migration" folder) is not linked into > these binaries. Doing something like: > > grep -r yank tests/qtest/migration-test > > should give you zero results. Thus it IMHO does not make sense to add the > yank.c to these tests here. > > Having said that, it seems like the qos-test is linking against the chardev > code and thus might use indirectly the yank code there. So you maybe might > want to add it to the qos-test instead? Ok, now I understand. In that case it doesn't matter if full yank is linked into qtest. Regards, Lukas Straub > Thomas > --
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build index 66ee9fbf45..40e1f495f7 100644 --- a/tests/qtest/meson.build +++ b/tests/qtest/meson.build @@ -234,9 +234,9 @@ tpmemu_files = ['tpm-emu.c', 'tpm-util.c', 'tpm-tests.c'] qtests = { 'bios-tables-test': [io, 'boot-sector.c', 'acpi-utils.c', 'tpm-emu.c'], 'cdrom-test': files('boot-sector.c'), - 'dbus-vmstate-test': files('migration-helpers.c') + dbus_vmstate1, + 'dbus-vmstate-test': ['migration-helpers.c', dbus_vmstate1, '../../monitor/yank.c'], 'ivshmem-test': [rt, '../../contrib/ivshmem-server/ivshmem-server.c'], - 'migration-test': files('migration-helpers.c'), + 'migration-test': ['migration-helpers.c', io, '../../monitor/yank.c'], 'pxe-test': files('boot-sector.c'), 'qos-test': [chardev, io, qos_test_ss.apply(config_host, strict: false).sources()], 'tpm-crb-swtpm-test': [io, tpmemu_files], @@ -266,7 +266,7 @@ foreach dir : target_dirs endif qtest_env.set('G_TEST_DBUS_DAEMON', meson.source_root() / 'tests/dbus-vmstate-daemon.sh') qtest_env.set('QTEST_QEMU_BINARY', './qemu-system-' + target_base) - + foreach test : target_qtests # Executables are shared across targets, declare them only the first time we # encounter them diff --git a/tests/unit/meson.build b/tests/unit/meson.build index 4bfe4627ba..8ccf60af66 100644 --- a/tests/unit/meson.build +++ b/tests/unit/meson.build @@ -123,7 +123,7 @@ if have_system 'test-util-sockets': ['socket-helpers.c'], 'test-base64': [], 'test-bufferiszero': [], - 'test-vmstate': [migration, io] + 'test-vmstate': [migration, io, '../../monitor/yank.c'] } if 'CONFIG_INOTIFY1' in config_host tests += {'test-util-filemonitor': []} @@ -135,7 +135,7 @@ if have_system if 'CONFIG_TSAN' not in config_host if 'CONFIG_POSIX' in config_host tests += { - 'test-char': ['socket-helpers.c', qom, io, chardev] + 'test-char': ['socket-helpers.c', qom, io, chardev, '../../monitor/yank.c'] } endif
Use the normal yank code instead of stubs in relevant tests to increase coverage and to ensure that registering and unregistering of yank instances and functions is done correctly. Signed-off-by: Lukas Straub <lukasstraub2@web.de> --- tests/qtest/meson.build | 6 +++--- tests/unit/meson.build | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) -- 2.30.2