Re: [PATCH v3 02/22] KVM: selftests: Use EPOLL in userfaultfd_util reader threads and signal errors via TEST_ASSERT

From: Anish Moorthy <amoorthy@google.com>
To: Hoo Robert <robert.hoo.linux@gmail.com>
Cc: pbonzini@redhat.com, maz@kernel.org, oliver.upton@linux.dev,
	seanjc@google.com, jthoughton@google.com, bgardon@google.com,
	dmatlack@google.com, ricarkol@google.com,
	axelrasmussen@google.com, peterx@redhat.com, kvm@vger.kernel.org,
	kvmarm@lists.linux.dev
Subject: Re: [PATCH v3 02/22] KVM: selftests: Use EPOLL in userfaultfd_util reader threads and signal errors via TEST_ASSERT
Date: Wed, 19 Apr 2023 16:26:05 -0700	[thread overview]
Message-ID: <CAF7b7mph_3gtYc=EEJ4fiVLZHRPUpSUY5eYzTsitdXG=pu_1kg@mail.gmail.com> (raw)
In-Reply-To: <a43e5deb-211a-c4c0-6b1d-7715c3665017@gmail.com>

On Wed, Apr 19, 2023 at 6:36 AM Hoo Robert <robert.hoo.linux@gmail.com> wrote:
>
> How about goto
>         ts_diff = timespec_elapsed(start);
> Otherwise last stats won't get chances to be calc'ed.

Good idea, done.

> > +             TEST_ASSERT(r == 1,
> > +                                     "Unexpected number of events (%d) from epoll, errno = %d",
> > +                                     r, errno);
> >
> too much indentation, also seen elsewhere.

Augh, my editor has been set to a tab width of 4 this entire time.
That... explains a lot >:(

> >               }
> >
> > -             if (!(pollfd[0].revents & POLLIN))
> > -                     continue;
> > +             TEST_ASSERT(!(evt.events & (EPOLLERR | EPOLLHUP)),
> > +                                     "Reader thread received EPOLLERR or EPOLLHUP on uffd.");
> >
> >               r = read(uffd, &msg, sizeof(msg));
> >               if (r == -1) {
> > -                     if (errno == EAGAIN)
> > -                             continue;
> > -                     pr_info("Read of uffd got errno %d\n", errno);
> > -                     return NULL;
> > +                     TEST_ASSERT(errno == EAGAIN,
> > +                                             "Error reading from UFFD: errno = %d", errno);
> > +                     continue;
> >               }
> >
> > -             if (r != sizeof(msg)) {
> > -                     pr_info("Read on uffd returned unexpected size: %d bytes", r);
> > -                     return NULL;
> > -             }
> > +             TEST_ASSERT(r == sizeof(msg),
> > +                                     "Read on uffd returned unexpected number of bytes (%d)", r);
> >
> >               if (!(msg.event & UFFD_EVENT_PAGEFAULT))
> >                       continue;
> > @@ -93,8 +89,8 @@ static void *uffd_handler_thread_fn(void *arg)
> >               if (reader_args->delay)
> >                       usleep(reader_args->delay);
> >               r = reader_args->handler(reader_args->uffd_mode, uffd, &msg);
> > -             if (r < 0)
> > -                     return NULL;
> > +             TEST_ASSERT(r >= 0,
> > +                                     "Reader thread handler fn returned negative value %d", r);
> >               pages++;
> >       }
> >
>