On Wed, 2016-05-25 at 16:35 +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@xxxxxxxxxx>
> ---
> server/red-record-qxl.c | 53 ++++++++++++++++++++++++++++++++++++++++++------
> -
> server/red-record-qxl.h | 15 ++++++++++----
> server/red-worker.c | 25 ++++++++++-------------
> 3 files changed, 67 insertions(+), 26 deletions(-)
>
> Changes from v1:
> - read SPICE_WORKER_RECORD_FILENAME in red-worker.c;
> - minor indentation fixed in header.
>
> diff --git a/server/red-record-qxl.c b/server/red-record-qxl.c
> index aa2f3e5..a4e0cc0 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,34 @@ void red_record_qxl_command(FILE *fd, RedMemSlotInfo
> *slots,
> break;
> }
> }
> +
> +RedRecord *red_record_new(const char *filename)
> +{
> + static const char header[] = "SPICE_REPLAY 1\n";
> +
> + FILE *f;
> + RedRecord *record;
> +
> + 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");
> + }
It seems a little bit inconsistent that failing to open the file would result in
only a warning and return NULL, but succeeding to open the file and then failing
to write would result in an error and abort. Maybe it's fine though
> +
> + 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..3a9e8df 100644
> --- a/server/red-record-qxl.h
> +++ b/server/red-record-qxl.h
> @@ -24,12 +24,19 @@
> #include "red-common.h"
> #include "memslot.h"
>
> -void red_record_dev_input_primary_surface_create(
> - FILE *fd, QXLDevSurfaceCreate *surface, uint8_t
> *line_0);
> +typedef struct RedRecord RedRecord;
>
> -void red_record_event(FILE *fd, int what, uint32_t type, unsigned long ts);
> +RedRecord* red_record_new(const char *filename);
>
> -void red_record_qxl_command(FILE *fd, RedMemSlotInfo *slots,
> +void red_record_free(RedRecord *record);
> +
> +void red_record_dev_input_primary_surface_create(RedRecord *record,
> + QXLDevSurfaceCreate
> *surface,
> + uint8_t *line_0);
> +
> +void red_record_event(RedRecord *record, int what, uint32_t type, unsigned
> long ts);
> +
> +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..83e7b0a 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)
> @@ -1480,22 +1480,17 @@ RedWorker* red_worker_new(QXLInstance *qxl,
>
> 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) {
> + worker->record = red_record_new(record_filename);
> + if (worker->record == 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");
> - }
> }
> 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);
> }
>
Looks fine overall.
Acked-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel