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

[Jens Freimann <jfreimann@redhat.com>]

On Mon, Jul 24, 2017 at 02:42:43PM +0100, Stefan Hajnoczi wrote:
>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.

Yes, I think so. I might get rid of it init_hugepagefs altogether
though. 

>
>> +    } 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);

Yes, I'll change it. 

>> +    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.

I'll add an abort handler and make sure that the vubr process is
terminated. 

Thanks for the review!

regards,
Jens