Re: [PATCH] tests/qtest/migration: Restore include for postcopy

[Fabiano Rosas]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, Dec 18, 2024 at 12:12:11PM -0300, Fabiano Rosas wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > On Tue, Dec 17, 2024 at 04:47:39PM -0500, Peter Xu wrote:
>> >> On Tue, Dec 17, 2024 at 06:22:01PM -0300, Fabiano Rosas wrote:
>> >> > Commit 124a3c58b8 ("tests/qtest/migration: Move ufd_version_check to
>> >> > utils") moved the ufd_version_check() function to another file but
>> >> > failed to bring along the <sys/syscall> include, which is necessary to
>> >> > pull in <asm/unistd.h> for __NR_userfaultd.
>> >> > 
>> >> > Restore the missing include.
>> >> 
>> >> Ohhhhhhh.. so postcopy tests will always be skipped as of now?  Maybe 
>> >> worth
>> >> explicit mention that in the commit message if so, only when you merge.
>> >> 
>> >> > 
>> >> > While here, remove the ifdef __linux__ that's redundant and fix a
>> >> > couple of typos.
>> >> > 
>> >> > Fixes: 124a3c58b8 ("tests/qtest/migration: Move ufd_version_check to 
>> >> > utils")
>> >> > Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> >> 
>> >> Reviewed-by: Peter Xu <peterx@redhat.com>
>> >> 
>> >> Maybe we don't need to be as careful on old kernels anymore especially in
>> >> tests, because userfaultfd syscall existed for ~10 years. So if we want we
>> >> can start requiring __NR_userfaultfd present for __linux__, then no way to
>> >> miss such spot next time.
>> >
>> > Yes, I think that check is obsolete, based on our supported platforms
>> > list. It would suffice to just check __linux__.
>> 
>> This breaks the cross builds. It seems the __NR_userfaultfd was actually
>> stopping several archs from reaching ufd_version_check(). Since
>> <sys/ioctl.h> is under HOST_X86_64, these new instances now fail to find
>> the 'ioctl' symbol:
>> 
>> https://gitlab.com/farosas/qemu/-/pipelines/1594332399
>> 
>> Of course I could just include <sys/ioctl.h> unconditionally, but the
>> fact that new code is not being built means the assumption that we can
>> imply __NR_userfaultfd from __linux__ alone is not correct.
>
> I think removing __NR_userfaultfd is still correct. The problem is that
> the ufd_version_check code is wrapped in a conditional that is not the
> same as the conditional that pulls in ioctl.h. That pre-existing bug
> should be fixed regardless, and once that's done, removing __NR_userfaultfd
> wouldn't be an issue.

Ah, the <sys/ioctl.h> include used to be under the userfaultfd ifdefs. I
got confused because kvm_dirty_ring_supported() also needs the ioctl and
it was introduced without moving the ioctl to a common ifdef.

>
> With regards,
> Daniel