[Spice-devel] [PATCH v2 1/3] replay: better record encapsulation
Frediano Ziglio
fziglio at redhat.com
Wed Feb 10 11:43:07 UTC 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.
>
Me too... I just realized much later!
>
> >
> > > 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'.
I think there are very different cases that require to change at list the
directory output. For instance we are not sure (oVirt, RHEV, Qemu) where
the current directory is at the moment file is created and if the file can
be created in that directory. An administrator could decide to turn the logging
on for a specific machine in order to send us the capture and wants to make sure
output is in a specific volume/directory to prevent filling the disk.
> 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.
>
During last week in Brno we captured a 5 minutes streaming (not compressed).
To compression of that captured file took more than 40 minutes with xz!
> It seems clear that we need to support multiple workers somehow, though
> (including in the spice-replay utility).
>
One way to make it simple to capture with multiple card would be to use a
single output file for all card adding lines like
worker <id>
when id change and we record an event/command for a given worker.
Potentially this serialization will slow down more but also make easier to
just post a single file and keep the command in sync instead of having to
open multiple files and syncing all the commands.
Note that neither record nor replay code actually support having multiple
cards failing the record/replay.
This would make possible to use
SPICE_WORKER_RECORD_FILENAME="/tmp/machine.spice"
or
SPICE_WORKER_RECORD_FILENAME="|/my/path/to/script machine.spice"
possible. The syntax extension in this case is not that terrible
and avoids to add an extra environment. In the simplest case
SPICE_WORKER_RECORD_FILENAME is just a filename, not much to
say about in the documentation.
I like the subprocess way as in less that 100 lines is possible to use
whatever compressor with whatever options/buffering the user wants
without having to deal with different algorithms by ourselves.
Imagine for instance that user wants to record large logs and use rotation
distributing the chunks and compress them using different machines.
>
> >
> > > 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);
> > >
> > >
>
Frediano
More information about the Spice-devel
mailing list