[Spice-devel] [PATCH 1/2 v2] replay: better record encapsulation
Frediano Ziglio
fziglio at redhat.com
Thu May 26 08:53:21 UTC 2016
>
> 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 at redhat.com>
> > ---
> > 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
>
As the caller is basically bailing out with a message on NULL I think
would be more consistent to move the error inside this function, this has
usually in spice-server functions does not return NULL.
Also is consistent with old behavior (failing).
> > +
> > + 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 at redhat.com>
>
>
>
Frediano
More information about the Spice-devel
mailing list