[Spice-devel] [PATCH v2 1/3] replay: better record encapsulation
Frediano Ziglio
fziglio at redhat.com
Fri Jan 29 07:57:11 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.
>
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.
> 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
> 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