Nice. I think I would prefer a slightly different approach. In essence:
- SpiceRecord definition should be in the .c file and only typedef in the header
- functions should be named spice_record_*() instead of red_record_*()?
- red_worker_new() should continue handling the environment variable to
determine whether we should be recording or not
- Replace red_record_open() with red_record_new(const char *filename)
- Replace red_record_close() with red_record_free()
- delete red_record_opened() and replace with a check to see whether worker
->record is non-NULL.
If you disagree, that's fine. Just a matter of preference. I could ack this
approach too.
Jonathon
On Wed, 2016-01-27 at 13:05 +0000, 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 | 45 ++++++++++++++++++++++++++++++++++++++-------
> server/red-record-qxl.h | 24 +++++++++++++++++++++---
> server/red-worker.c | 28 ++++++++--------------------
> 3 files changed, 67 insertions(+), 30 deletions(-)
>
> diff --git a/server/red-record-qxl.c b/server/red-record-qxl.c
> index 9c9dd62..a0607a0 100644
> --- a/server/red-record-qxl.c
> +++ b/server/red-record-qxl.c
> @@ -26,6 +26,7 @@
> #include "memslot.h"
> #include "red-parse-qxl.h"
> #include "zlib-encoder.h"
> +#include "red-record-qxl.h"
>
> #if 0
> static void hexdump_qxl(RedMemSlotInfo *slots, int group_id,
> @@ -782,9 +783,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(SpiceRecord *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 +796,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(SpiceRecord *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(SpiceRecord *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 +828,31 @@ void red_record_qxl_command(FILE *fd, RedMemSlotInfo
> *slots,
> break;
> }
> }
> +
> +bool red_record_open(SpiceRecord *record)
> +{
> + static const char header[] = "SPICE_REPLAY 1\n";
> +
> + const char *filename;
> + FILE *f;
> +
> + filename = getenv("SPICE_WORKER_RECORD_FILENAME");
> + if (!filename)
> + return false;
> +
> + f = fopen(filename, "w+");
> + if (!f)
> + goto error;
> +
> + if (fwrite(header, sizeof(header)-1, 1, f) != 1) {
> + spice_error("failed to write replay header");
> + }
> +
> + record->fd = f;
> + record->counter = 0;
> + return true;
> +
> +error:
> + spice_error("failed to open recording file %s\n", filename);
> + return false;
> +}
> diff --git a/server/red-record-qxl.h b/server/red-record-qxl.h
> index 21f0bc9..2c8d8dd 100644
> --- a/server/red-record-qxl.h
> +++ b/server/red-record-qxl.h
> @@ -23,12 +23,30 @@
> #include "red-common.h"
> #include "memslot.h"
>
> +typedef struct SpiceRecord {
> + FILE *fd;
> + unsigned int counter;
> +} SpiceRecord;
> +
> +bool red_record_open(SpiceRecord *record);
> +
> +static inline void red_record_close(SpiceRecord *record)
> +{
> + fclose(record->fd);
> + record->fd = NULL;
> +}
> +
> +static inline bool red_record_opened(SpiceRecord *record)
> +{
> + return record->fd != NULL;
> +}
> +
> void red_record_dev_input_primary_surface_create(
> - FILE *fd, QXLDevSurfaceCreate *surface, uint8_t
> *line_0);
> + SpiceRecord *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(SpiceRecord *record, int what, uint32_t type, unsigned
> long ts);
>
> -void red_record_qxl_command(FILE *fd, RedMemSlotInfo *slots,
> +void red_record_qxl_command(SpiceRecord *record, RedMemSlotInfo *slots,
> QXLCommandExt ext_cmd, unsigned long ts);
>
> #endif
> diff --git a/server/red-worker.c b/server/red-worker.c
> index 6196682..d9c9c86 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -97,7 +97,7 @@ struct RedWorker {
>
> int driver_cap_monitors_config;
>
> - FILE *record_fd;
> + SpiceRecord record;
> };
>
> QXLInstance* red_worker_get_qxl(RedWorker *worker)
> @@ -255,8 +255,8 @@ static int red_process_display(RedWorker *worker, int
> *ring_is_empty)
> continue;
> }
>
> - if (worker->record_fd)
> - red_record_qxl_command(worker->record_fd, &worker->mem_slots,
> ext_cmd,
> + if (red_record_opened(&worker->record))
> + red_record_qxl_command(&worker->record, &worker->mem_slots,
> ext_cmd,
> stat_now(CLOCK_THREAD_CPUTIME_ID));
>
> stat_inc_counter(worker->command_counter, 1);
> @@ -845,8 +845,8 @@ 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,
> + if (red_record_opened(&worker->record)) {
> + red_record_dev_input_primary_surface_create(&worker->record,
> &surface, line_0);
> }
>
> @@ -1344,7 +1344,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_THREAD_CPUTIME_ID));
> + red_record_event(&worker->record, 1, message_type,
> stat_now(CLOCK_THREAD_CPUTIME_ID));
> }
>
> static void register_callbacks(Dispatcher *dispatcher)
> @@ -1536,32 +1536,20 @@ RedWorker* red_worker_new(QXLInstance *qxl,
> RedDispatcher *red_dispatcher)
> RedWorker *worker;
> Dispatcher *dispatcher;
> int i;
> - const char *record_filename;
>
> qxl->st->qif->get_init_info(qxl, &init_info);
>
> worker = spice_new0(RedWorker, 1);
> worker->core = worker_core_initializer;
>
> - 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");
> - }
> - }
> + red_record_open(&worker->record);
> dispatcher = red_dispatcher_get_dispatcher(red_dispatcher);
> dispatcher_set_opaque(dispatcher, worker);
>
> worker->red_dispatcher = red_dispatcher;
> worker->qxl = qxl;
> register_callbacks(dispatcher);
> - if (worker->record_fd) {
> + if (red_record_opened(&worker->record)) {
> dispatcher_register_universal_handler(dispatcher,
> worker_dispatcher_record);
> }
> worker->image_compression = image_compression;
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel