Re: [PATCH 5/7] server/tests/replay: introduce

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, 2015-08-17 at 04:45 -0400, Frediano Ziglio wrote:
> > 
> > On Thu, 2015-08-13 at 16:25 +0100, Frediano Ziglio wrote:
> > > From: Alon Levy <alon@xxxxxxxxx>
> > > 
> > > usage: replay <cmdfile> <port> <client command line>
> > 
> > This usage doesn't really match the implementation below. It should be
> > something more like:
> > 
> > replay -p <port> -c <client command line> <cmdfile>
> > 
> > While we're on the subject, though, can we use a more specific name?
> > e.g. spice-replay
> > 
> 
> Already taken :)
> 
> http://cgit.freedesktop.org/~fziglio/spice-replay/
> 

Sorry, It seems that maybe you misunderstood my suggestion. I wasn't
referring to the branch name (although that's fine as well). I was
actually talking about the executable name for this replay utility. I
think it would a bit nicer if it was like

spice-replay -p <port> ....

This executable is not actually installed, so it's not a huge problem,
but I still feel like it should use a less-generic name. 


> > > 
> > > will run the commands from cmdfile ignoring timestamps, right after a
> > > connection is established from the client, and will SIGINT the client
> > > on end of cmdfile, and exit itself after waiting for the client.
> > > 
> > > spicy-stats from spice-gtk is useful for testing, it prints the summary
> > > of the traffic on each channel.
> > > 
> > > You can also run with no client by doing:
> > > replay <cmdfile>
> > > 
> > > For example, the 300 MB file (compressed to 4 MB with xz -9) available
> > > at [1] produces the following output:
> > > 
> > > spicy-stats total bytes read:
> > > total bytes read:
> > > inputs: 214
> > > display: 1968983
> > > cursor: 390
> > > main: 256373
> > > 
> > > You could run it directly like so:
> > > curl http://annarchy.freedesktop.org/~alon/win7_boot_shutdown.cmd.xz |
> > > xzcat | server/tests/replay - 12345 `which spicy-stats` -h localhost -p
> > > 12345
> > 
> > Again, command above doesn't seem to match the implementation.
> > 
> 
> How to handle such changes?
> Should we just change the commit and add the signed-off-by for authorship?
> 
> > > 
> > > Known Problems:
> > > * Implementation is wrong. Should do a single device->host conversion
> > >  (i.e. get_virt), and then marshall/demarshall that (i.e. RedDrawable).
> > > * segfault on file read done resulting in the above spicy-stats not
> > >  being reproducable (well, up to 1% yes).
> > > 
> > > [1] http://annarchy.freedesktop.org/~alon/win7_boot_shutdown.cmd.xz
> > > 
> > > Now based on glib including using an asyncqueue for reading the playback
> > > file, and proper freeing of the allocated commands, with --slow,
> > > --compression and a progress timer, and doesn't use more then nsurfaces.3
> > 
> > This last paragraph seems to be addressing differences to a previous
> > version of the patch. We can probably incorporate this into the above
> > commit log or just drop it.
> > 
> > 
> > > ---
> > >  server/red_replay_qxl.c  |   7 +-
> > >  server/red_replay_qxl.h  |  17 +++
> > >  server/tests/Makefile.am |   9 ++
> > >  server/tests/replay.c    | 346
> > >  +++++++++++++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 377 insertions(+), 2 deletions(-)
> > >  create mode 100644 server/tests/replay.c
> > > 
> > > diff --git a/server/red_replay_qxl.c b/server/red_replay_qxl.c
> > > index 663c6ed..e52ee9c 100644
> > > --- a/server/red_replay_qxl.c
> > > +++ b/server/red_replay_qxl.c
> > > @@ -186,13 +186,15 @@ static replay_t read_binary(SpiceReplay *replay,
> > > const char *prefix, size_t *siz
> > >                              **buf, size_t base_size)
> > >  {
> > >      char template[1024];
> > > -    int with_zlib;
> > > +    int with_zlib = -1;
> > >      int zlib_size;
> > >      uint8_t *zlib_buffer;
> > >      z_stream strm;
> > >  
> > >      snprintf(template, sizeof(template), "binary %%d %s %%ld:", prefix);
> > > -    replay_fscanf(replay, template, &with_zlib, size);
> > > +    if (replay_fscanf(replay, template, &with_zlib, size) == REPLAY_EOF)
> > > +        return REPLAY_EOF;
> > > +
> > >      if (*buf == NULL) {
> > >          *buf = malloc(*size + base_size);
> > >          if (*buf == NULL) {
> > > @@ -207,6 +209,7 @@ static replay_t read_binary(SpiceReplay *replay, const
> > > char *prefix, size_t *siz
> > >          hexdump(*buf + base_size, *size);
> > >      }
> > >  #else
> > > +    spice_return_val_if_fail(with_zlib != -1, REPLAY_EOF);
> > >      if (with_zlib) {
> > >          int ret;
> > >  
> > > diff --git a/server/red_replay_qxl.h b/server/red_replay_qxl.h
> > > index 77a37e0..a89b3d4 100644
> > > --- a/server/red_replay_qxl.h
> > > +++ b/server/red_replay_qxl.h
> > > @@ -1,3 +1,20 @@
> > > +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> > > +/*
> > > +   Copyright (C) 2009-2015 Red Hat, Inc.
> > > +
> > > +   This library is free software; you can redistribute it and/or
> > > +   modify it under the terms of the GNU Lesser General Public
> > > +   License as published by the Free Software Foundation; either
> > > +   version 2.1 of the License, or (at your option) any later version.
> > > +
> > > +   This library is distributed in the hope that it will be useful,
> > > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > > +   Lesser General Public License for more details.
> > > +
> > > +   You should have received a copy of the GNU Lesser General Public
> > > +   License along with this library; if not, see
> > > <http://www.gnu.org/licenses/>.
> > > +*/
> > >  #ifndef RED_REPLAY_QXL_H
> > >  #define RED_REPLAY_QXL_H
> > >  
> > > diff --git a/server/tests/Makefile.am b/server/tests/Makefile.am
> > > index 32c97ce..4fa03fe 100644
> > > --- a/server/tests/Makefile.am
> > > +++ b/server/tests/Makefile.am
> > > @@ -40,6 +40,7 @@ noinst_PROGRAMS =				\
> > >  	test_two_servers			\
> > >  	test_vdagent				\
> > >  	test_display_width_stride		\
> > > +	replay					\
> > >  	$(NULL)
> > >  
> > >  test_vdagent_SOURCES =		\
> > > @@ -101,3 +102,11 @@ test_display_width_stride_SOURCES =		\
> > >  	test_display_base.h			\
> > >  	test_display_width_stride.c 		\
> > >  	$(NULL)
> > > +
> > > +replay_SOURCES = 		\
> > > +	replay.c			\
> > > +	test_display_base.h	\
> > > +	basic_event_loop.c	\
> > > +	basic_event_loop.h	\
> > > +	test_util.h			\
> > > +	$(NULL)
> > > diff --git a/server/tests/replay.c b/server/tests/replay.c
> > > new file mode 100644
> > > index 0000000..f1f1098
> > > --- /dev/null
> > > +++ b/server/tests/replay.c
> > > @@ -0,0 +1,346 @@
> > > +/* Replay a previously recorded file (via SPICE_WORKER_RECORD_FILENAME)
> > > + */
> > > +
> > > +#include <string.h>
> > > +#include <stdlib.h>
> > > +#include <stdio.h>
> > > +#include <strings.h>
> > > +#include <sys/types.h>
> > > +#include <signal.h>
> > > +#include <unistd.h>
> > > +#include <pthread.h>
> > > +#include <sys/wait.h>
> > > +#include <fcntl.h>
> > > +#include <glib.h>
> > > +
> > > +#include <spice/macros.h>
> > > +#include "red_replay_qxl.h"
> > > +#include "test_display_base.h"
> > > +#include "common/log.h"
> > > +
> > > +static SpiceCoreInterface *core;
> > > +static SpiceServer *server;
> > > +static SpiceReplay *replay;
> > > +static QXLWorker *qxl_worker = NULL;
> > > +static gboolean started = FALSE;
> > > +static QXLInstance display_sin = { 0, };
> > > +static gint slow = 0;
> > > +static pid_t client_pid;
> > > +static GMainLoop *loop = NULL;
> > > +static GAsyncQueue *aqueue = NULL;
> > > +static long total_size;
> > > +
> > > +static GMutex mutex;
> > > +static guint fill_source_id = 0;
> > > +
> > > +
> > > +#define MEM_SLOT_GROUP_ID 0
> > > +
> > > +/* Parts cribbed from spice-display.h/.c/qxl.c */
> > > +
> > > +static QXLDevMemSlot slot = {
> > > +.slot_group_id = MEM_SLOT_GROUP_ID,
> > > +.slot_id = 0,
> > > +.generation = 0,
> > > +.virt_start = 0,
> > > +.virt_end = ~0,
> > > +.addr_delta = 0,
> > > +.qxl_ram_size = ~0,
> > > +};
> > > +
> > > +static void attach_worker(QXLInstance *qin, QXLWorker *_qxl_worker)
> > > +{
> > > +    static int count = 0;
> > > +    if (++count > 1) {
> > > +        printf("%s ignored\n", __func__);
> > 
> > g_warning()?
> > 
> > > +        return;
> > > +    }
> > > +    printf("%s\n", __func__);
> > 
> > Is this necessary? perhaps use g_debug() so it doesn't print by default.
> > 
> > > +    qxl_worker = _qxl_worker;
> > > +    spice_qxl_add_memslot(qin, &slot);
> > > +    spice_server_vm_start(server);
> > > +}
> > > +
> > > +static void set_compression_level(QXLInstance *qin, int level)
> > > +{
> > > +    printf("%s\n", __func__);
> > 
> > g_debug()?
> > 
> > > +}
> > > +
> > > +static void set_mm_time(QXLInstance *qin, uint32_t mm_time)
> > > +{
> > > +}
> > > +
> > > +// same as qemu/ui/spice-display.h
> > > +#define MAX_SURFACE_NUM 1024
> > > +
> > > +static void get_init_info(QXLInstance *qin, QXLDevInitInfo *info)
> > > +{
> > > +    bzero(info, sizeof(*info));
> > > +    info->num_memslots = 1;
> > > +    info->num_memslots_groups = 1;
> > > +    info->memslot_id_bits = 1;
> > > +    info->memslot_gen_bits = 1;
> > > +    info->n_surfaces = MAX_SURFACE_NUM;
> > > +}
> > > +
> > > +static gboolean fill_queue_idle(gpointer user_data)
> > > +{
> > > +    gboolean keep = FALSE;
> > > +
> > > +    while (g_async_queue_length(aqueue) < 50) {
> > > +        QXLCommandExt *cmd = spice_replay_next_cmd(replay, qxl_worker);
> > > +        if (!cmd) {
> > > +            g_async_queue_push(aqueue, GINT_TO_POINTER(-1));
> > > +            goto end;
> > > +        }
> > > +
> > > +        if (slow)
> > > +            g_usleep(slow);
> > > +
> > > +        g_async_queue_push(aqueue, cmd);
> > > +    }
> > > +
> > > +end:
> > > +    if (!keep) {
> > > +        g_mutex_lock(&mutex);
> > > +        fill_source_id = 0;
> > > +        g_mutex_unlock(&mutex);
> > > +    }
> > > +    spice_qxl_wakeup(&display_sin);
> > > +
> > > +    return keep;
> > > +}
> > > +
> > > +static void fill_queue(void)
> > > +{
> > > +    g_mutex_lock(&mutex);
> > > +
> > > +    if (!started)
> > > +        goto end;
> > > +
> > > +    if (fill_source_id != 0)
> > > +        goto end;
> > > +
> > > +    fill_source_id = g_idle_add(fill_queue_idle, NULL);
> > > +
> > > +end:
> > > +    g_mutex_unlock(&mutex);
> > > +}
> > > +
> > > +
> > > +// called from spice_server thread (i.e. red_worker thread)
> > > +static int get_command(QXLInstance *qin, QXLCommandExt *ext)
> > > +{
> > > +    QXLCommandExt *cmd;
> > > +
> > > +    if (g_async_queue_length(aqueue) == 0) {
> > > +        /* could use a gcondition ? */
> > > +        fill_queue();
> > > +        return FALSE;
> > > +    }
> > > +
> > > +    cmd = g_async_queue_try_pop(aqueue);
> > > +    if (GPOINTER_TO_INT(cmd) == -1) {
> > > +        g_main_loop_quit(loop);
> > > +        return FALSE;
> > > +    }
> > > +
> > > +    *ext = *cmd;
> > > +
> > > +    return TRUE;
> > > +}
> > > +
> > > +static int req_cmd_notification(QXLInstance *qin)
> > > +{
> > > +    if (!started)
> > > +        return TRUE;
> > > +
> > > +    spice_printerr("id: %d, queue length: %d",
> > > +                   fill_source_id, g_async_queue_length(aqueue));
> > 
> > I'd personally prefer to use glib functions in new code: ->
> > g_printerr()?
> > 
> > > +
> > > +    return TRUE;
> > > +}
> > > +
> > > +static void end_replay(void)
> > > +{
> > > +    int child_status;
> > > +
> > > +    /* FIXME: wait threads and end cleanly */
> > > +    spice_replay_free(replay);
> > > +
> > > +    if (client_pid) {
> > > +        g_debug("kill %d", client_pid);
> > > +        kill(client_pid, SIGINT);
> > > +        waitpid(client_pid, &child_status, 0);
> > > +    }
> > > +    exit(0);
> > > +}
> > > +
> > > +static void release_resource(QXLInstance *qin, struct QXLReleaseInfoExt
> > > release_info)
> > > +{
> > > +    spice_replay_free_cmd(replay, (QXLCommandExt *)release_info.info->id);
> > > +}
> > > +
> > > +static int get_cursor_command(QXLInstance *qin, struct QXLCommandExt *ext)
> > > +{
> > > +    return FALSE;
> > > +}
> > > +
> > > +static int req_cursor_notification(QXLInstance *qin)
> > > +{
> > > +    return TRUE;
> > > +}
> > > +
> > > +static void notify_update(QXLInstance *qin, uint32_t update_id)
> > > +{
> > > +}
> > > +
> > > +static int flush_resources(QXLInstance *qin)
> > > +{
> > > +    return TRUE;
> > > +}
> > > +
> > > +static QXLInterface display_sif = {
> > > +    .base = {
> > > +        .type = SPICE_INTERFACE_QXL,
> > > +        .description = "replay",
> > > +        .major_version = SPICE_INTERFACE_QXL_MAJOR,
> > > +        .minor_version = SPICE_INTERFACE_QXL_MINOR
> > > +    },
> > > +    .attache_worker = attach_worker,
> > > +    .set_compression_level = set_compression_level,
> > > +    .set_mm_time = set_mm_time,
> > > +    .get_init_info = get_init_info,
> > > +    .get_command = get_command,
> > > +    .req_cmd_notification = req_cmd_notification,
> > > +    .release_resource = release_resource,
> > > +    .get_cursor_command = get_cursor_command,
> > > +    .req_cursor_notification = req_cursor_notification,
> > > +    .notify_update = notify_update,
> > > +    .flush_resources = flush_resources,
> > > +};
> > > +
> > > +static void replay_channel_event(int event, SpiceChannelEventInfo *info)
> > > +{
> > > +    spice_printerr("");
> > 
> > g_printerr()
> > 
> > > +
> > > +    if (info->type == SPICE_CHANNEL_DISPLAY &&
> > > +        event == SPICE_CHANNEL_EVENT_INITIALIZED) {
> > > +        started = TRUE;
> > > +    }
> > > +}
> > > +
> > > +static gboolean start_client(gchar *cmd, GError **error)
> > > +{
> > > +    gboolean retval;
> > > +    gint argc;
> > > +    gchar **argv = NULL;
> > > +
> > > +
> > > +    if (!g_shell_parse_argv(cmd, &argc, &argv, error))
> > > +        return FALSE;
> > > +
> > > +    retval = g_spawn_async(NULL, argv, NULL, G_SPAWN_SEARCH_PATH,
> > > +                           NULL, NULL, &client_pid, error);
> > > +    g_strfreev(argv);
> > > +
> > > +    return retval;
> > > +}
> > > +
> > > +static gboolean progress_timer(gpointer user_data)
> > > +{
> > > +    FILE *fd = user_data;
> > > +    /* it seems somehow thread safe, move to worker thread? */
> > > +    double pos = (double)ftell(fd);
> > > +
> > > +    g_debug("%.2f%%", pos/total_size * 100);
> > > +    return TRUE;
> > > +}
> > > +
> > > +int main(int argc, char **argv)
> > > +{
> > > +    GError *error = NULL;
> > > +    GOptionContext *context = NULL;
> > > +    gchar *client = NULL, **file = NULL;
> > > +    gint port = 5000, compression = SPICE_IMAGE_COMPRESSION_AUTO_GLZ;
> > > +    gboolean wait = FALSE;
> > > +    FILE *fd;
> > > +
> > > +    GOptionEntry entries[] = {
> > > +        { "client", 'c', 0, G_OPTION_ARG_STRING, &client, "Client", "CMD"
> > > },
> > > +        { "compression", 'C', 0, G_OPTION_ARG_INT, &compression,
> > > "Compression (default 2)", "INT" },
> > > +        { "port", 'p', 0, G_OPTION_ARG_INT, &port, "Server port (default
> > > 5000)", "PORT" },
> > > +        { "wait", 'w', 0, G_OPTION_ARG_NONE, &wait, "Wait for client",
> > > NULL },
> > > +        { "slow", 's', 0, G_OPTION_ARG_INT, &slow, "Slow down replay",
> > > NULL },
> > > +        { G_OPTION_REMAINING, 0, 0, G_OPTION_ARG_FILENAME_ARRAY, &file,
> > > "replay file", "FILE" },
> > 
> > This last option specifies an array of files, but the code below only
> > opens the first one.
> > 
> > > +        { NULL }
> > > +    };
> > > +
> > > +    context = g_option_context_new("- replay spice server recording");
> > 
> > This is a common use of this "parameter_string" argument (and it's even
> > mentioned as an alternative in the API documentation), but I really
> > dislike it and find it confusing. I would prefer to use
> > g_option_context_set_summary() for the description.
> > 
> > > +    g_option_context_add_main_entries(context, entries, NULL);
> > > +    if (!g_option_context_parse(context, &argc, &argv, &error)) {
> > > +        g_printerr("Option parsing failed: %s\n", error->message);
> > > +        exit(1);
> > > +    }
> > > +    if (!file) {
> > > +        g_printerr(g_option_context_get_help(context, TRUE, NULL));
> > > +        exit(1);
> > > +    }
> > > +
> > > +    if (strncmp(file[0], "-", 1) == 0) {
> > > +        fd = stdin;
> > > +    } else {
> > > +        fd = fopen(file[0], "r");
> > > +    }
> > > +    if (fd == NULL) {
> > > +        g_printerr("error opening %s\n", argv[1]);
> > > +        return 1;
> > > +    }
> > > +    if (fcntl(fileno(fd), FD_CLOEXEC) < 0) {
> > > +        perror("fcntl failed");
> > > +        exit(1);
> > > +    }
> > > +    fseek(fd, 0L, SEEK_END);
> > > +    total_size = ftell(fd);
> > > +    fseek(fd, 0L, SEEK_SET);
> > > +    if (total_size > 0)
> > > +        g_timeout_add_seconds(1, progress_timer, fd);
> > > +    replay = spice_replay_new(fd, MAX_SURFACE_NUM);
> > > +
> > > +    aqueue = g_async_queue_new();
> > > +    core = basic_event_loop_init();
> > > +    core->channel_event = replay_channel_event;
> > > +
> > > +    SpiceServer* server = spice_server_new();
> > > +    spice_server_set_image_compression(server, compression);
> > > +    spice_server_set_port(server, port);
> > > +    spice_server_set_noauth(server);
> > > +
> > > +    g_print("listening on port %d (unsecure)\n", port);
> > 
> > "unsecure" should be "insecure"
> > 
> > > +    spice_server_init(server, core);
> > > +
> > > +    display_sin.base.sif = &display_sif.base;
> > > +    spice_server_add_interface(server, &display_sin.base);
> > > +
> > > +    if (client) {
> > > +        start_client(client, &error);
> > > +        wait = TRUE;
> > > +    }
> > > +
> > > +    if (!wait) {
> > > +        started = TRUE;
> > > +        fill_queue();
> > > +    }
> > > +
> > > +    loop = g_main_loop_new(NULL, FALSE);
> > > +    g_main_loop_run(loop);
> > > +
> > > +    end_replay();
> > > +    g_async_queue_unref(aqueue);
> > > +
> > > +    /* FIXME: there should be a way to join server threads before:
> > > +     * g_main_loop_unref(loop);
> > > +     */
> > > +
> > > +    return 0;
> > > +}
> > 
> > 
> > 
> 
> Frediano


_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]