[Spice-devel] [PATCH v2 1/3] replay: better record encapsulation
Jonathon Jongsma
jjongsma at redhat.com
Fri Jan 29 07:23:19 PST 2016
On Fri, 2016-01-29 at 13:28 +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 at redhat.com>
> ---
> server/red-record-qxl.c | 58 +++++++++++++++++++++++++++++++++++++++++++-----
> -
> server/red-record-qxl.h | 12 +++++++---
> server/red-worker.c | 28 +++++++-----------------
> 3 files changed, 68 insertions(+), 30 deletions(-)
>
> diff --git a/server/red-record-qxl.c b/server/red-record-qxl.c
> index 9c9dd62..9add176 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 SpiceRecord {
> + 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(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 +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(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 +833,39 @@ void red_record_qxl_command(FILE *fd, RedMemSlotInfo
> *slots,
> break;
> }
> }
> +
> +SpiceRecord *red_record_new(void)
> +{
> + static const char header[] = "SPICE_REPLAY 1\n";
> +
> + const char *filename;
> + FILE *f;
> + SpiceRecord *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(SpiceRecord, 1);
> + record->fd = f;
> + record->counter = 0;
> + return record;
> +}
> +
> +void red_record_free(SpiceRecord *record)
> +{
> + if (record) {
> + fclose(record->fd);
> + g_free(record);
> + }
> +}
> diff --git a/server/red-record-qxl.h b/server/red-record-qxl.h
> index 21f0bc9..ad77962 100644
> --- a/server/red-record-qxl.h
> +++ b/server/red-record-qxl.h
> @@ -23,12 +23,18 @@
> #include "red-common.h"
> #include "memslot.h"
>
> +typedef struct SpiceRecord SpiceRecord;
> +
> +SpiceRecord* red_record_new(void);
> +
> +void red_record_free(SpiceRecord *record);
> +
> 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 560d172..c2e1a19 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;
> + SpiceRecord *record;
> };
>
> QXLInstance* red_worker_get_qxl(RedWorker *worker)
> @@ -238,8 +238,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 (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);
> @@ -723,8 +723,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 (worker->record) {
> + red_record_dev_input_primary_surface_create(worker->record,
> &surface, line_0);
> }
>
> @@ -1222,7 +1222,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)
> @@ -1476,7 +1476,6 @@ RedWorker* red_worker_new(QXLInstance *qxl,
> RedDispatcher *red_dispatcher)
> QXLDevInitInfo init_info;
> RedWorker *worker;
> Dispatcher *dispatcher;
> - const char *record_filename;
>
> qxl->st->qif->get_init_info(qxl, &init_info);
>
> @@ -1484,25 +1483,14 @@ RedWorker* red_worker_new(QXLInstance *qxl,
> RedDispatcher *red_dispatcher)
> 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_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 (worker->record) {
> dispatcher_register_universal_handler(dispatcher,
> worker_dispatcher_record);
> }
> worker->image_compression = image_compression;
The thought just occurred to me: since red_record_new() has a very high
possibility of failure (since usually the environment variable is not
specified), would it be better to name it something like red_record_try_new()?
Maybe that's unnecessary and annoying.
Also, I still think that squashing the following patch into this one would be
slightly nicer. I know that we're never likely to instantiate two instances of
SpiceRecord. But if we did, they would both try to write to the same filename. T
hat suggests to me that the design is less than ideal. So I still think that the
responsibility of choosing the filename should belong to the caller. But this is
nitpicking, so I leave the choice up to you.
Acked-by: Jonathon Jongsma <jjongsma at redhat.com>
diff --git a/server/red-record-qxl.c b/server/red-record-qxl.c
index 9add176..281f5cd 100644
--- a/server/red-record-qxl.c
+++ b/server/red-record-qxl.c
@@ -834,15 +834,13 @@ void red_record_qxl_command(SpiceRecord *record,
RedMemSlotInfo *slots,
}
}
-SpiceRecord *red_record_new(void)
+SpiceRecord *red_record_new(const char *filename)
{
static const char header[] = "SPICE_REPLAY 1\n";
- const char *filename;
FILE *f;
SpiceRecord *record;
- filename = getenv("SPICE_WORKER_RECORD_FILENAME");
if (!filename)
return NULL;
diff --git a/server/red-record-qxl.h b/server/red-record-qxl.h
index ad77962..6685636 100644
--- a/server/red-record-qxl.h
+++ b/server/red-record-qxl.h
@@ -25,7 +25,7 @@
typedef struct SpiceRecord SpiceRecord;
-SpiceRecord* red_record_new(void);
+SpiceRecord* red_record_new(const char *filename);
void red_record_free(SpiceRecord *record);
diff --git a/server/red-worker.c b/server/red-worker.c
index a94ded2..4b2be74 100644
--- a/server/red-worker.c
+++ b/server/red-worker.c
@@ -1499,6 +1499,7 @@ RedWorker* red_worker_new(QXLInstance *qxl, RedDispatcher
*red_dispatcher)
QXLDevInitInfo init_info;
RedWorker *worker;
Dispatcher *dispatcher;
+ const char *record_filename;
qxl->st->qif->get_init_info(qxl, &init_info);
@@ -1506,7 +1507,8 @@ RedWorker* red_worker_new(QXLInstance *qxl, RedDispatcher
*red_dispatcher)
worker->core = event_loop_core;
worker->core.main_context = g_main_context_new();
- worker->record = red_record_new();
+ record_filename = getenv("SPICE_WORKER_RECORD_FILENAME");
+ worker->record = red_record_new(record_filename);
dispatcher = red_dispatcher_get_dispatcher(red_dispatcher);
dispatcher_set_opaque(dispatcher, worker);
More information about the Spice-devel
mailing list