Re: [PATCH 08/22] tests/qtest/migration: Move ufd_version_check to utils

["Daniel P. Berrangé" <berrange@redhat.com>]

On Tue, Nov 05, 2024 at 03:08:23PM -0300, Fabiano Rosas wrote:
> Move ufd_version_check() to migration-util.c file. This is a helper
> function that is used during tests definition so it should be
> available outside of migration-test.c
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  tests/qtest/migration-test.c           | 47 ++------------------------
>  tests/qtest/migration/migration-util.c | 42 +++++++++++++++++++++++
>  tests/qtest/migration/migration-util.h | 11 ++++++
>  3 files changed, 55 insertions(+), 45 deletions(-)
> 
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index 308a50a988..825544ba5e 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -34,6 +34,7 @@
>  /* For dirty ring test; so far only x86_64 is supported */
>  #if defined(__linux__) && defined(HOST_X86_64)
>  #include "linux/kvm.h"
> +#include <sys/ioctl.h>
>  #endif
>  
>  unsigned start_address;
> @@ -87,50 +88,6 @@ typedef enum PostcopyRecoveryFailStage {
>  #include <sys/vfs.h>
>  #endif
>  
> -#if defined(__linux__) && defined(__NR_userfaultfd) && defined(CONFIG_EVENTFD)
> -#include <sys/eventfd.h>
> -#include <sys/ioctl.h>
> -#include "qemu/userfaultfd.h"
> -
> -static bool ufd_version_check(void)
> -{
> -    struct uffdio_api api_struct;
> -    uint64_t ioctl_mask;
> -
> -    int ufd = uffd_open(O_CLOEXEC);
> -
> -    if (ufd == -1) {
> -        g_test_message("Skipping test: userfaultfd not available");
> -        return false;
> -    }
> -
> -    api_struct.api = UFFD_API;
> -    api_struct.features = 0;
> -    if (ioctl(ufd, UFFDIO_API, &api_struct)) {
> -        g_test_message("Skipping test: UFFDIO_API failed");
> -        return false;
> -    }
> -    uffd_feature_thread_id = api_struct.features & UFFD_FEATURE_THREAD_ID;
> -
> -    ioctl_mask = (1ULL << _UFFDIO_REGISTER |
> -                  1ULL << _UFFDIO_UNREGISTER);
> -    if ((api_struct.ioctls & ioctl_mask) != ioctl_mask) {
> -        g_test_message("Skipping test: Missing userfault feature");
> -        return false;
> -    }
> -
> -    return true;
> -}
> -
> -#else
> -static bool ufd_version_check(void)
> -{
> -    g_test_message("Skipping test: Userfault not available (builtdtime)");
> -    return false;
> -}
> -
> -#endif
> -
>  static char *tmpfs;
>  static char *bootpath;
>  
> @@ -3522,7 +3479,7 @@ int main(int argc, char **argv)
>          return 0;
>      }
>  
> -    has_uffd = ufd_version_check();
> +    has_uffd = ufd_version_check(&uffd_feature_thread_id);
>      arch = qtest_get_arch();
>      is_x86 = !strcmp(arch, "i386") || !strcmp(arch, "x86_64");
>  
> diff --git a/tests/qtest/migration/migration-util.c b/tests/qtest/migration/migration-util.c
> index 8a974ded22..169b4884e6 100644
> --- a/tests/qtest/migration/migration-util.c
> +++ b/tests/qtest/migration/migration-util.c
> @@ -22,6 +22,13 @@
>  #include "migration/bootfile.h"
>  #include "migration/migration-util.h"
>  
> +/* for uffd_version_check() */
> +#if defined(__linux__) && defined(__NR_userfaultfd) && defined(CONFIG_EVENTFD)
> +#include <sys/eventfd.h>
> +#include "qemu/userfaultfd.h"
> +#endif
> +
> +
>  static char *SocketAddress_to_str(SocketAddress *addr)
>  {
>      switch (addr->type) {
> @@ -283,3 +290,38 @@ bool probe_o_direct_support(const char *tmpfs)
>      return true;
>  }
>  #endif
> +
> +#if defined(__linux__) && defined(__NR_userfaultfd) && defined(CONFIG_EVENTFD)
> +bool ufd_version_check(bool *uffd_feature_thread_id)
> +{
> +    struct uffdio_api api_struct;
> +    uint64_t ioctl_mask;
> +
> +    int ufd = uffd_open(O_CLOEXEC);
> +
> +    if (ufd == -1) {
> +        g_test_message("Skipping test: userfaultfd not available");
> +        return false;
> +    }
> +
> +    api_struct.api = UFFD_API;
> +    api_struct.features = 0;
> +    if (ioctl(ufd, UFFDIO_API, &api_struct)) {
> +        g_test_message("Skipping test: UFFDIO_API failed");
> +        return false;
> +    }
> +
> +    if (uffd_feature_thread_id) {
> +        *uffd_feature_thread_id = api_struct.features & UFFD_FEATURE_THREAD_ID;
> +    }
> +
> +    ioctl_mask = (1ULL << _UFFDIO_REGISTER |
> +                  1ULL << _UFFDIO_UNREGISTER);
> +    if ((api_struct.ioctls & ioctl_mask) != ioctl_mask) {
> +        g_test_message("Skipping test: Missing userfault feature");
> +        return false;
> +    }
> +
> +    return true;
> +}
> +#endif
> diff --git a/tests/qtest/migration/migration-util.h b/tests/qtest/migration/migration-util.h
> index d75c5db5f3..f4a1f4750e 100644
> --- a/tests/qtest/migration/migration-util.h
> +++ b/tests/qtest/migration/migration-util.h
> @@ -67,6 +67,17 @@ static inline bool probe_o_direct_support(const char *tmpfs)
>      return false;
>  }
>  #endif
> +
> +#if defined(__linux__) && defined(__NR_userfaultfd) && defined(CONFIG_EVENTFD)
> +bool ufd_version_check(bool *uffd_feature_thread_id);
> +#else
> +static inline bool ufd_version_check(bool *uffd_feature_thread_id)
> +{
> +    g_test_message("Skipping test: Userfault not available (builtdtime)");
> +    return false;
> +}
> +#endif

I wouldn't bother inlining this in the header, as it is not a performance
sensitive function. Having the complex #ifdef expression only in the .c
file would be a benefit IMHO

> +
>  void migration_test_add(const char *path, void (*fn)(void));
>  char *migrate_get_connect_uri(QTestState *who);
>  void migrate_set_ports(QTestState *to, QList *channel_list);
> -- 
> 2.35.3
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|