[Spice-devel] [PATCH 1/2] replay: better record encapsulation
Frediano Ziglio
fziglio at redhat.com
Wed Jan 27 10:09:04 PST 2016
>
> 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
Sounds fair
> - functions should be named spice_record_*() instead of red_record_*()?
Well, I tried to keep patch small, I can do it in a separate one.
> - red_worker_new() should continue handling the environment variable to
> determine whether we should be recording or not
This I don't agree.
> - Replace red_record_open() with red_record_new(const char *filename)
I would use a spice_record_new(void) which returns NULL if cannot
initialize the recording. I would put just a return NULL in case there is no
environment and a spice_warning + return NULL if cannot initialize for some
reasons. So users can see the problem in the log but machine is starting
(for instance qemu has no permission to write in that directory).
> - 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.
>
The non-NULL check sounds good. I want red-worker to consume less cpu possible
if recording is disabled so a test like
if (G_UNLIKELY(worker->record))
spice_record_whatever(worker->record, ...);
would be great.
> If you disagree, that's fine. Just a matter of preference. I could ack this
> approach too.
>
> Jonathon
>
Well, instead of one brain or the other I think two brains are better :)
Frediano
>
>
> 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 at redhat.com>
> > ---
> > 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;
>
More information about the Spice-devel
mailing list