[Spice-devel] [PATCH v2 1/3] replay: better record encapsulation
Jonathon Jongsma
jjongsma at redhat.com
Fri Jan 29 09:35:00 PST 2016
On Fri, 2016-01-29 at 10:57 -0500, Frediano Ziglio wrote:
> >
> > 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.
> >
>
> It costs mainly nothing, I can go with _try_new.
>
> > Also, I still think that squashing the following patch into this one would
> > be
>
> Ok, as you agree with the change I'll squash before merging.
>
> > slightly nicer. I know that we're never likely to instantiate two instances
> > of
>
> I'm sorry, you are wrong. Each card on Windows has a different red worker
> thread.
Indeed, I had forgotten about that case.
>
> > 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.
> >
>
> The reason why I moved the getenv to red_record_open (now spice_record_new) is
> to
> add support for filter. Filter is using an environment too. However I don't
> want all
> the burden of subprocessing and redirecting in red worker.
>
> About the environment and multiple files. What about following some syntax
> like
> core_pattern (http://man7.org/linux/man-pages/man5/core.5.html) ?
> So for instance we could set
>
> SPICE_WORKER_RECORD_FILENAME='/tmp/record.%p_%i_%t.spice'
>
> In this case a file with file name composed by time, PID and TID will be used
> while with
>
> SPICE_WORKER_RECORD_FILENAME='|/path/to/my_filter /tmp/record.%p_%i_%t.spice'
>
> a pipe will be used to execute /path/to/my_filter passing a parameter similar
> to
> previous example.
> I still however won't like all the burden to parse and setup file/pipe/filter
> in red worker.
>
> Frediano
So we discussed this briefly on IRC, so I thought I'd reply here for the record.
My personal feeling is that supporting a filename pattern like this makes things
overly complex. I'm afraid that every time I need to record a session, I'll need
to go look up documentation for how to format the pattern, etc. So I suggested
making things simpler and maybe not even let the user specify the filename. We
could use a filename pattern internally such as 'spice-record-$PID-$TID'. Then
the spice-server could print the name of the file(s) that were generated to
stdout so that the user can find them.
Ideally you could just do something simple like this:
SPICE_WORKER_RECORD=1 $SPICE_SERVER_COMMAND
I also suggested always compressing with xz to remove the necessity of
specifying a filter with an environment variable. But Frediano objected that
this could slow down the host too much.
It seems clear that we need to support multiple workers somehow, though
(including in the spice-replay utility).
>
> > 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