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

[Jens Freimann]

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