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

[Jens Freimann <jfreimann@redhat.com>]

On Thu, Oct 26, 2017 at 02:53:12PM +0000, Michael S. Tsirkin wrote:
>On Thu, Oct 26, 2017 at 02:44:25PM +0200, Jens Freimann wrote:
>> From: Jens Freimann <jfreiman@redhat.com>
>>
>> Add a PXE testcase tunneling traffic through vhost-user-bridge process.
>> Create a vhost-user-bridge process and connect it to qemu via a socket.
>>
>> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
>> ---
>>  tests/Makefile.include |   4 +-
>>  tests/pxe-test.c       | 111 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 114 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/Makefile.include b/tests/Makefile.include
>> index 70dc711bca..6cdb783753 100644
>> --- a/tests/Makefile.include
>> +++ b/tests/Makefile.include
>> @@ -740,7 +740,8 @@ tests/boot-order-test$(EXESUF): tests/boot-order-test.o $(libqos-obj-y)
>>  tests/boot-serial-test$(EXESUF): tests/boot-serial-test.o $(libqos-obj-y)
>>  tests/bios-tables-test$(EXESUF): tests/bios-tables-test.o \
>>  	tests/boot-sector.o tests/acpi-utils.o $(libqos-obj-y)
>> -tests/pxe-test$(EXESUF): tests/pxe-test.o tests/boot-sector.o $(libqos-obj-y)
>> +tests/pxe-test$(EXESUF): tests/pxe-test.o tests/boot-sector.o \
>> +    tests/vhost-user-bridge$(EXESUF) $(libqos-obj-y)
>>  tests/tmp105-test$(EXESUF): tests/tmp105-test.o $(libqos-omap-obj-y)
>>  tests/ds1338-test$(EXESUF): tests/ds1338-test.o $(libqos-imx-obj-y)
>>  tests/m25p80-test$(EXESUF): tests/m25p80-test.o
>> @@ -849,6 +850,7 @@ $(patsubst %, check-qtest-%, $(QTEST_TARGETS)): check-qtest-%: subdir-%-softmmu
>>  	$(if $(CONFIG_GCOV),@rm -f *.gcda */*.gcda */*/*.gcda */*/*/*.gcda,)
>>  	$(call quiet-command,QTEST_QEMU_BINARY=$*-softmmu/qemu-system-$* \
>>  		QTEST_QEMU_IMG=qemu-img$(EXESUF) \
>> +		QTEST_VUBR_BINARY=./tests/vhost-user-bridge$(EXESUF) \
>>  		MALLOC_PERTURB_=$${MALLOC_PERTURB_:-$$(( $${RANDOM:-0} % 255 + 1))} \
>>  		gtester $(GTESTER_OPTIONS) -m=$(SPEED) $(check-qtest-$*-y) $(check-qtest-generic-y),"GTESTER","$@")
>>  	$(if $(CONFIG_GCOV),@for f in $(gcov-files-$*-y) $(gcov-files-generic-y); do \
>> diff --git a/tests/pxe-test.c b/tests/pxe-test.c
>> index 937f29e631..d2646ee86d 100644
>> --- a/tests/pxe-test.c
>> +++ b/tests/pxe-test.c
>> @@ -7,6 +7,7 @@
>>   *  Michael S. Tsirkin <mst@redhat.com>,
>>   *  Victor Kaplansky <victork@redhat.com>
>>   *  Thomas Huth <thuth@redhat.com>
>> + *  Jens Freimann <jfreiman@redhat.com>
>>   *
>>   * This work is licensed under the terms of the GNU GPL, version 2 or later.
>>   * See the COPYING file in the top-level directory.
>> @@ -14,14 +15,123 @@
>>
>>  #include "qemu/osdep.h"
>>  #include <glib/gstdio.h>
>> +#include <glib.h>
>>  #include "qemu-common.h"
>>  #include "libqtest.h"
>>  #include "boot-sector.h"
>>
>> +#define LPORT 5555
>
>Is this used anywhere?
>
>> +#define RPORT 4444
>
>Using a known port is problematic as you can't run
>many instances of the test then. I think you want an option
>to bind in the parent.

ok 

>>  #define NETNAME "net0"
>> +#define QEMU_CMD_MEM    "--enable-kvm -m %d "\
>
>enable-kvm won't work on boxes without kvm.
>Pls use same technique as other tests to only use kvm
>when available.

ok

>> +                        "-object memory-backend-file,id=mem,size=%dM,"\
>> +                        "mem-path=%s,share=on -numa node,memdev=mem -mem-prealloc "
>
>Do we really need prealloc? And why?

We don't need it, I'll remove it.

>> +#define QEMU_CMD_CHR    " -chardev socket,id=%s,path=%s"
>> +#define QEMU_CMD_NETDEV " -device virtio-net-pci,netdev=net0 "\
>> +                        " -netdev vhost-user,id=net0,chardev=%s,vhostforce "\
>> +                        " -netdev user,id=n0,tftp=./,bootfile=%s "\
>> +                        " -netdev socket,id=n1,fd=%d"
>> +#define QEMU_CMD_NET    " -device virtio-net-pci,netdev=n0 "\
>> +                        " -device virtio-net-pci,netdev=n1 "
>> +
>> +#define QEMU_CMD        QEMU_CMD_MEM QEMU_CMD_CHR \
>> +                        QEMU_CMD_NETDEV QEMU_CMD_NET
>> +
>> +#define VUBR_SOCK "vubr.sock"
>> +#define MEMSZ 1024
>>
>>  static char disk[] = "tests/pxe-test-disk-XXXXXX";
>>
>> +static int vubr_create_socket(struct sockaddr_in *si_remote, int rport)
>> +{
>> +    int sock;
>> +
>> +    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 vubr_watch(GPid pid, gint status, gpointer data)
>> +{
>> +    g_assert_cmpint(status, ==, 0);
>> +    g_spawn_close_pid(pid);
>> +}
>> +
>> +static void abrt_handler(void *data)
>> +{
>> +    int *pid = data;
>> +
>> +    g_spawn_close_pid(*pid);
>> +    kill(*pid, SIGTERM);
>> +    waitpid(*pid, NULL, 0);
>> +}
>> +
>> +static void test_pxe_vhost_user(void)
>> +{
>> +    char template[] = "/tmp/vhost-user-bridge-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 *tmpfs = NULL;
>> +    GError *error = NULL;
>> +    char *vubr_binary;
>> +    char *qemu_args;
>> +    GPid vubr_pid;
>> +    int sock = -1;
>> +
>> +    qtest_add_abrt_handler(abrt_handler, &vubr_pid);
>> +    tmpfs = mkdtemp(template);
>> +    if (!tmpfs) {
>> +        g_test_message("mkdtemp on path(%s): %s\n",
>> +                       template, strerror(errno));
>> +    }
>
>defer above until we know it's used (no hugetlbfs?)

yes, will do. Thanks for the review!

regards,
Jens