| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJHvVcizVoAs+-wOXeO7bc=8c2G3oEC4KSVyPm5E9Z6YMCsvaw@mail.gmail.com>
Date: Wed, 19 May 2021 15:14:21 -0700
From: Axel Rasmussen <axelrasmussen@...gle.com>
To: Ben Gardon <bgardon@...gle.com>
Cc: Aaron Lewis <aaronlewis@...gle.com>,
Alexander Graf <graf@...zon.com>,
Andrew Jones <drjones@...hat.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Emanuele Giuseppe Esposito <eesposit@...hat.com>,
Eric Auger <eric.auger@...hat.com>,
Jacob Xu <jacobhxu@...gle.com>,
Makarand Sonare <makarandsonare@...gle.com>,
Oliver Upton <oupton@...gle.com>,
Paolo Bonzini <pbonzini@...hat.com>,
Peter Xu <peterx@...hat.com>, Shuah Khan <shuah@...nel.org>,
Yanan Wang <wangyanan55@...wei.com>, kvm <kvm@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>,
linux-kselftest@...r.kernel.org
Subject: Re: [PATCH v2 02/10] KVM: selftests: simplify setup_demand_paging
error handling
On Wed, May 19, 2021 at 2:45 PM Ben Gardon <bgardon@...gle.com> wrote:
>
> On Wed, May 19, 2021 at 1:03 PM Axel Rasmussen <axelrasmussen@...gle.com> wrote:
> >
> > A small cleanup. Our caller writes:
> >
> > r = setup_demand_paging(...);
> > if (r < 0) exit(-r);
> >
> > Since we're just going to exit anyway, instead of returning an error we
> > can just re-use TEST_ASSERT. This makes the caller simpler, as well as
> > the function itself - no need to write our branches, etc.
> >
> > Signed-off-by: Axel Rasmussen <axelrasmussen@...gle.com>
> > ---
> > .../selftests/kvm/demand_paging_test.c | 51 +++++++------------
> > 1 file changed, 19 insertions(+), 32 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/demand_paging_test.c b/tools/testing/selftests/kvm/demand_paging_test.c
> > index 9398ba6ef023..601a1df24dd2 100644
> > --- a/tools/testing/selftests/kvm/demand_paging_test.c
> > +++ b/tools/testing/selftests/kvm/demand_paging_test.c
> > @@ -9,6 +9,8 @@
> >
> > #define _GNU_SOURCE /* for pipe2 */
> >
> > +#include <inttypes.h>
> > +#include <stdint.h>
>
> Why do the includes need to change in this commit? Is it for the PRIu64 below?
Right, I didn't actually try compiling without these, but inttypes.h
defines PRIu64 and stdint.h defines uint64_t. In general I tend to
prefer including things like this because we're using their
definitions directly, even if we might be picking them up transiently
some other way.
>
> > #include <stdio.h>
> > #include <stdlib.h>
> > #include <time.h>
> > @@ -198,42 +200,32 @@ static void *uffd_handler_thread_fn(void *arg)
> > return NULL;
> > }
> >
> > -static int setup_demand_paging(struct kvm_vm *vm,
> > - pthread_t *uffd_handler_thread, int pipefd,
> > - useconds_t uffd_delay,
> > - struct uffd_handler_args *uffd_args,
> > - void *hva, uint64_t len)
> > +static void setup_demand_paging(struct kvm_vm *vm,
> > + pthread_t *uffd_handler_thread, int pipefd,
> > + useconds_t uffd_delay,
> > + struct uffd_handler_args *uffd_args,
> > + void *hva, uint64_t len)
> > {
> > int uffd;
> > struct uffdio_api uffdio_api;
> > struct uffdio_register uffdio_register;
> >
> > uffd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK);
> > - if (uffd == -1) {
> > - pr_info("uffd creation failed\n");
> > - return -1;
> > - }
> > + TEST_ASSERT(uffd >= 0, "uffd creation failed, errno: %d", errno);
> >
> > uffdio_api.api = UFFD_API;
> > uffdio_api.features = 0;
> > - if (ioctl(uffd, UFFDIO_API, &uffdio_api) == -1) {
> > - pr_info("ioctl uffdio_api failed\n");
> > - return -1;
> > - }
> > + TEST_ASSERT(ioctl(uffd, UFFDIO_API, &uffdio_api) != -1,
> > + "ioctl UFFDIO_API failed: %" PRIu64,
> > + (uint64_t)uffdio_api.api);
> >
> > uffdio_register.range.start = (uint64_t)hva;
> > uffdio_register.range.len = len;
> > uffdio_register.mode = UFFDIO_REGISTER_MODE_MISSING;
> > - if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register) == -1) {
> > - pr_info("ioctl uffdio_register failed\n");
> > - return -1;
> > - }
> > -
> > - if ((uffdio_register.ioctls & UFFD_API_RANGE_IOCTLS) !=
> > - UFFD_API_RANGE_IOCTLS) {
> > - pr_info("unexpected userfaultfd ioctl set\n");
> > - return -1;
> > - }
> > + TEST_ASSERT(ioctl(uffd, UFFDIO_REGISTER, &uffdio_register) != -1,
> > + "ioctl UFFDIO_REGISTER failed");
> > + TEST_ASSERT((uffdio_register.ioctls & UFFD_API_RANGE_IOCTLS) ==
> > + UFFD_API_RANGE_IOCTLS, "unexpected userfaultfd ioctl set");
> >
> > uffd_args->uffd = uffd;
> > uffd_args->pipefd = pipefd;
> > @@ -243,8 +235,6 @@ static int setup_demand_paging(struct kvm_vm *vm,
> >
> > PER_VCPU_DEBUG("Created uffd thread for HVA range [%p, %p)\n",
> > hva, hva + len);
> > -
> > - return 0;
> > }
> >
> > struct test_params {
> > @@ -321,13 +311,10 @@ static void run_test(enum vm_guest_mode mode, void *arg)
> > O_CLOEXEC | O_NONBLOCK);
> > TEST_ASSERT(!r, "Failed to set up pipefd");
> >
> > - r = setup_demand_paging(vm,
> > - &uffd_handler_threads[vcpu_id],
> > - pipefds[vcpu_id * 2],
> > - p->uffd_delay, &uffd_args[vcpu_id],
> > - vcpu_hva, vcpu_mem_size);
> > - if (r < 0)
> > - exit(-r);
> > + setup_demand_paging(vm, &uffd_handler_threads[vcpu_id],
> > + pipefds[vcpu_id * 2], p->uffd_delay,
> > + &uffd_args[vcpu_id], vcpu_hva,
> > + vcpu_mem_size);
> > }
> > }
> >
> > --
> > 2.31.1.751.gd2f1c929bd-goog
> >
Powered by blists - more mailing lists