Re: [Qemu-devel] [PATCH 4/4] tests/pxe-test: add testcase using vhost-user-bridge

[Stefan Hajnoczi]

On Fri, Jul 21, 2017 at 11:55:53AM +0200, Jens Freimann wrote:
> +static const char *init_hugepagefs(const char *path)
> +{
> +    struct statfs fs;
> +    int ret;
> +
> +    if (access(path, R_OK | W_OK | X_OK)) {
> +        g_test_message("access on path (%s): %s\n", path, strerror(errno));
> +        return NULL;
> +    }
> +
> +    do {
> +        ret = statfs(path, &fs);

This system-call and HUGETLBFS_MAGIC are Linux-specific but I don't see
anything in the makefile or this test code that restricts it to Linux.
Will this break non-Linux hosts?

Perhaps #ifdef __linux__ is needed.

> +    } while (ret != 0 && errno == EINTR);
> +
> +    if (ret != 0) {
> +        g_test_message("statfs on path (%s): %s\n", path, strerror(errno));
> +        return NULL;
> +    }
> +
> +    if (fs.f_type != HUGETLBFS_MAGIC) {
> +        g_test_message("Warning: path not on HugeTLBFS: %s\n", path);
> +        return NULL;
> +    }
> +
> +    return path;
> +}
> +
> +static int vubr_create_socket(struct sockaddr_in *si_remote, int rport)
> +{
> +    int sock;
> +
> +    if (inet_aton("127.0.0.1", (struct in_addr *) 
> &si_remote->sin_addr.s_addr)
> +            == 0) {
> +        g_test_message("inet_aton failed\n");
> +        return -1;
> +    }

Or:

  si_remote->sin_addr.s_addr = htonl(INADDR_LOOPBACK);

> +    sock = socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP);
> +    if (sock == -1) {
> +        g_test_message("socket creation failed\n");
> +        return -1;
> +    }
> +    if (connect(sock, (struct sockaddr *) si_remote, sizeof(*si_remote))) {
> +        g_test_message("connect failed: %s", strerror(errno));
> +        return -1;
> +    }
> +
> +    return sock;
> +}
> +
> +static void test_pxe_vhost_user(void)
> +{
> +    char template[] = "/tmp/vhost-user-bridge-XXXXXX";
> +    char template2[] = "/tmp/hugepages-XXXXXX";
> +    gchar * vubr_args[] = {NULL, NULL, NULL, NULL};
> +    struct sockaddr_in si_remote = {
> +        .sin_family = AF_INET,
> +        .sin_port = htons(RPORT),
> +    };
> +    const char *hugefs = NULL;
> +    const char *tmpfs2 = NULL;
> +    const char *tmpfs = NULL;
> +    const char *root = NULL;
> +    GError *error = NULL;
> +    char *vubr_binary;
> +    char *qemu_args;
> +    GPid vubr_pid;
> +    int sock = -1;
> +
> +    tmpfs = mkdtemp(template);
> +    if (!tmpfs) {
> +        g_test_message("mkdtemp on path(%s): %s\n",
> +                       template, strerror(errno));
> +    }
> +    vubr_binary = getenv("QTEST_VUBR_BINARY");
> +    g_assert(vubr_binary);
> +    vubr_args[0] = g_strdup_printf("%s", vubr_binary);
> +    vubr_args[1] = g_strdup_printf("-u");
> +    vubr_args[2] = g_strdup_printf("%s/%s", tmpfs, VUBR_SOCK);
> +    g_spawn_async(NULL, vubr_args, NULL,
> +                  G_SPAWN_SEARCH_PATH_FROM_ENVP |
> +                  G_SPAWN_SEARCH_PATH,
> +                  NULL, NULL, &vubr_pid, &error);

I think a few things are missing for test cleanup (especially on
failure).

From 
https://developer.gnome.org/glib/stable/glib-Spawning-Processes.html#g-spawn-async-with-pipes:

  If child_pid is not NULL and an error does not occur then the returned
  process reference must be closed using g_spawn_close_pid().

G_SPAWN_DO_NOT_REAP_CHILD wasn't specified so glib will automatically
reap the child but the test case will not know when the vubr process has
terminated.

A qtest_add_abrt_handler() is also missing to kill the vubr child if the
test case terminates.

signature.asc
Description: PGP signature