Re: [PATCH 3/8] KVM: selftests: Factor out guest mode code

[Andrew Jones <drjones@redhat.com>]

On Tue, Nov 10, 2020 at 01:52:56PM -0800, Ben Gardon wrote:
> On Tue, Nov 10, 2020 at 12:48 PM Andrew Jones <drjones@redhat.com> wrote:
> >
> > demand_paging_test, dirty_log_test, and dirty_log_perf_test have
> > redundant guest mode code. Factor it out.
> >
> > Also, while adding a new include, remove the ones we don't need.
> >
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  tools/testing/selftests/kvm/Makefile          |   2 +-
> >  .../selftests/kvm/demand_paging_test.c        | 107 ++++-----------
> >  .../selftests/kvm/dirty_log_perf_test.c       | 119 +++++------------
> >  tools/testing/selftests/kvm/dirty_log_test.c  | 125 ++++++------------
> >  .../selftests/kvm/include/guest_modes.h       |  21 +++
> >  tools/testing/selftests/kvm/lib/guest_modes.c |  70 ++++++++++
> >  6 files changed, 188 insertions(+), 256 deletions(-)
> >  create mode 100644 tools/testing/selftests/kvm/include/guest_modes.h
> >  create mode 100644 tools/testing/selftests/kvm/lib/guest_modes.c
> >
> > diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> > index 3d14ef77755e..ca6b64d9ab64 100644
> > --- a/tools/testing/selftests/kvm/Makefile
> > +++ b/tools/testing/selftests/kvm/Makefile
> > @@ -33,7 +33,7 @@ ifeq ($(ARCH),s390)
> >         UNAME_M := s390x
> >  endif
> >
> > -LIBKVM = lib/assert.c lib/elf.c lib/io.c lib/kvm_util.c lib/sparsebit.c lib/test_util.c
> > +LIBKVM = lib/assert.c lib/elf.c lib/io.c lib/kvm_util.c lib/sparsebit.c lib/test_util.c lib/guest_modes.c
> >  LIBKVM_x86_64 = lib/x86_64/processor.c lib/x86_64/vmx.c lib/x86_64/svm.c lib/x86_64/ucall.c lib/x86_64/handlers.S
> >  LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c
> >  LIBKVM_s390x = lib/s390x/processor.c lib/s390x/ucall.c
> > diff --git a/tools/testing/selftests/kvm/demand_paging_test.c b/tools/testing/selftests/kvm/demand_paging_test.c
> > index 3d96a7bfaff3..946161a9ce2d 100644
> > --- a/tools/testing/selftests/kvm/demand_paging_test.c
> > +++ b/tools/testing/selftests/kvm/demand_paging_test.c
> > @@ -7,23 +7,20 @@
> >   * Copyright (C) 2019, Google, Inc.
> >   */
> >
> > -#define _GNU_SOURCE /* for program_invocation_name */
> > +#define _GNU_SOURCE /* for program_invocation_name and pipe2 */
> 
> What is the purpose of pipe2 in this patch / why add it to this
> comment but not the comments in the other files modified here?

Only this file uses pipe2. If we do a later cleanup removing the
program_invocation_name usage from this file, then I want to point out
that we need to keep _GNU_SOURCE defined for pipe2. Actually, the only
reason we still need program_invocation_name at this point in the
series is because program_invocation_name is used in perf_test_util.h
in a function we include in this file. I should have removed
program_invocation_name from the comment with the "KVM: selftests: Use
vm_create_with_vcpus in create_vm" patch, but I forgot.

Thanks,
drew