[Spice-devel] [PATCH 1/2] replay: better record encapsulation
Jonathon Jongsma
jjongsma at redhat.com
Wed May 25 15:06:10 UTC 2016
I vaguely remembered reviewing something like this earlier and found an earlier
thread where we were discussing some changes to this patch:
https://lists.freedesktop.org/archives/spice-devel/2016-January/026206.html
It doesn't look like those changes were incorporated. Did you decide against
them in the end or just forget about them?
Also: it doesn't appear that red_record_free() is ever called.
Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>
On Wed, 2016-05-25 at 13:46 +0100, Frediano Ziglio wrote:
> Remove global/static from red_record_qxl.c.
> Defined a structure and use it to hold record state.
>
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
> server/red-record-qxl.c | 58 +++++++++++++++++++++++++++++++++++++++++++-----
> -
> server/red-record-qxl.h | 12 +++++++---
> server/red-worker.c | 30 ++++++++-----------------
> 3 files changed, 69 insertions(+), 31 deletions(-)
>
> diff --git a/server/red-record-qxl.c b/server/red-record-qxl.c
> index aa2f3e5..4821222 100644
> --- a/server/red-record-qxl.c
> +++ b/server/red-record-qxl.c
> @@ -26,6 +26,12 @@
> #include "memslot.h"
> #include "red-parse-qxl.h"
> #include "zlib-encoder.h"
> +#include "red-record-qxl.h"
> +
> +struct RedRecord {
> + FILE *fd;
> + unsigned int counter;
> +};
>
> #if 0
> static void hexdump_qxl(RedMemSlotInfo *slots, int group_id,
> @@ -782,9 +788,11 @@ void red_record_cursor_cmd(FILE *fd, RedMemSlotInfo
> *slots, int group_id,
> }
> }
>
> -void red_record_dev_input_primary_surface_create(FILE *fd,
> +void red_record_dev_input_primary_surface_create(RedRecord *record,
> QXLDevSurfaceCreate* surface, uint8_t *line_0)
> {
> + FILE *fd = record->fd;
> +
> fprintf(fd, "%d %d %d %d\n", surface->width, surface->height,
> surface->stride, surface->format);
> fprintf(fd, "%d %d %d %d\n", surface->position, surface->mouse_mode,
> @@ -793,22 +801,22 @@ void red_record_dev_input_primary_surface_create(FILE
> *fd,
> line_0);
> }
>
> -void red_record_event(FILE *fd, int what, uint32_t type, unsigned long ts)
> +void red_record_event(RedRecord *record, int what, uint32_t type, unsigned
> long ts)
> {
> - static int counter = 0;
> -
> // TODO: record the size of the packet in the header. This would make
> // navigating it much faster (well, I can add an index while I'm at it..)
> // and make it trivial to get a histogram from a file.
> // But to implement that I would need some temporary buffer for each
> event.
> // (that can be up to VGA_FRAMEBUFFER large)
> - fprintf(fd, "event %d %d %u %lu\n", counter++, what, type, ts);
> + fprintf(record->fd, "event %u %d %u %lu\n", record->counter++, what,
> type, ts);
> }
>
> -void red_record_qxl_command(FILE *fd, RedMemSlotInfo *slots,
> +void red_record_qxl_command(RedRecord *record, RedMemSlotInfo *slots,
> QXLCommandExt ext_cmd, unsigned long ts)
> {
> - red_record_event(fd, 0, ext_cmd.cmd.type, ts);
> + FILE *fd = record->fd;
> +
> + red_record_event(record, 0, ext_cmd.cmd.type, ts);
>
> switch (ext_cmd.cmd.type) {
> case QXL_CMD_DRAW:
> @@ -825,3 +833,39 @@ void red_record_qxl_command(FILE *fd, RedMemSlotInfo
> *slots,
> break;
> }
> }
> +
> +RedRecord *red_record_new(void)
> +{
> + static const char header[] = "SPICE_REPLAY 1\n";
> +
> + const char *filename;
> + FILE *f;
> + RedRecord *record;
> +
> + filename = getenv("SPICE_WORKER_RECORD_FILENAME");
> + if (!filename)
> + return NULL;
> +
> + f = fopen(filename, "w+");
> + if (!f) {
> + spice_warning("failed to open recording file %s\n", filename);
> + return NULL;
> + }
> +
> + if (fwrite(header, sizeof(header)-1, 1, f) != 1) {
> + spice_error("failed to write replay header");
> + }
> +
> + record = g_new(RedRecord, 1);
> + record->fd = f;
> + record->counter = 0;
> + return record;
> +}
> +
> +void red_record_free(RedRecord *record)
> +{
> + if (record) {
> + fclose(record->fd);
> + g_free(record);
> + }
> +}
> diff --git a/server/red-record-qxl.h b/server/red-record-qxl.h
> index d5eaaa0..b5da31e 100644
> --- a/server/red-record-qxl.h
> +++ b/server/red-record-qxl.h
> @@ -24,12 +24,18 @@
> #include "red-common.h"
> #include "memslot.h"
>
> +typedef struct RedRecord RedRecord;
> +
> +RedRecord* red_record_new(void);
> +
> +void red_record_free(RedRecord *record);
> +
> void red_record_dev_input_primary_surface_create(
> - FILE *fd, QXLDevSurfaceCreate *surface, uint8_t
> *line_0);
> + RedRecord *record, QXLDevSurfaceCreate *surface, uint8_t *line_0);
>
> -void red_record_event(FILE *fd, int what, uint32_t type, unsigned long ts);
> +void red_record_event(RedRecord *record, int what, uint32_t type, unsigned
> long ts);
>
> -void red_record_qxl_command(FILE *fd, RedMemSlotInfo *slots,
> +void red_record_qxl_command(RedRecord *record, RedMemSlotInfo *slots,
> QXLCommandExt ext_cmd, unsigned long ts);
>
> #endif
> diff --git a/server/red-worker.c b/server/red-worker.c
> index 0c945c1..b06bfe1 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -88,7 +88,7 @@ struct RedWorker {
>
> int driver_cap_monitors_config;
>
> - FILE *record_fd;
> + RedRecord *record;
> };
>
> static RedsState* red_worker_get_server(RedWorker *worker);
> @@ -215,8 +215,8 @@ static int red_process_display(RedWorker *worker, int
> *ring_is_empty)
> return n;
> }
>
> - if (worker->record_fd)
> - red_record_qxl_command(worker->record_fd, &worker->mem_slots,
> ext_cmd,
> + if (worker->record)
> + red_record_qxl_command(worker->record, &worker->mem_slots,
> ext_cmd,
> stat_now(CLOCK_MONOTONIC));
>
> stat_inc_counter(reds, worker->command_counter, 1);
> @@ -687,9 +687,9 @@ static void dev_create_primary_surface(RedWorker *worker,
> uint32_t surface_id,
> if (error) {
> return;
> }
> - if (worker->record_fd) {
> - red_record_dev_input_primary_surface_create(worker->record_fd,
> - &surface, line_0);
> + if (worker->record) {
> + red_record_dev_input_primary_surface_create(worker->record,
> + &surface, line_0);
> }
>
> if (surface.stride < 0) {
> @@ -1200,7 +1200,7 @@ static void worker_dispatcher_record(void *opaque,
> uint32_t message_type, void *
> {
> RedWorker *worker = opaque;
>
> - red_record_event(worker->record_fd, 1, message_type,
> stat_now(CLOCK_MONOTONIC));
> + red_record_event(worker->record, 1, message_type,
> stat_now(CLOCK_MONOTONIC));
> }
>
> static void register_callbacks(Dispatcher *dispatcher)
> @@ -1468,7 +1468,6 @@ RedWorker* red_worker_new(QXLInstance *qxl,
> QXLDevInitInfo init_info;
> RedWorker *worker;
> Dispatcher *dispatcher;
> - const char *record_filename;
> RedsState *reds = red_qxl_get_server(qxl->st);
> RedChannel *channel;
>
> @@ -1478,24 +1477,13 @@ RedWorker* red_worker_new(QXLInstance *qxl,
> worker->core = event_loop_core;
> worker->core.main_context = g_main_context_new();
>
> - record_filename = getenv("SPICE_WORKER_RECORD_FILENAME");
> - if (record_filename) {
> - static const char header[] = "SPICE_REPLAY 1\n";
> -
> - worker->record_fd = fopen(record_filename, "w+");
> - if (worker->record_fd == NULL) {
> - spice_error("failed to open recording file %s\n",
> record_filename);
> - }
> - if (fwrite(header, sizeof(header)-1, 1, worker->record_fd) != 1) {
> - spice_error("failed to write replay header");
> - }
> - }
> + worker->record = red_record_new();
> dispatcher = red_qxl_get_dispatcher(qxl);
> dispatcher_set_opaque(dispatcher, worker);
>
> worker->qxl = qxl;
> register_callbacks(dispatcher);
> - if (worker->record_fd) {
> + if (worker->record) {
> dispatcher_register_universal_handler(dispatcher,
> worker_dispatcher_record);
> }
>
More information about the Spice-devel
mailing list