>
> I vaguely remembered reviewing something like this earlier and found an
> earlier
> thread where we were discussing some changes to this patch:
> https://lists.freedesktop.org/archives/spice-devel/2016-January/026206.html
>
> It doesn't look like those changes were incorporated. Did you decide against
> them in the end or just forget about them?
>
I don't remember that much.
I agree with the _try_new.
About the FILENAME I remember we had some question about the next patch.
It would end up with the caller having to handle an environment variable
(the FILENAME) and the called managing the FILTER (actually maybe make
sense as the FILTER is likely to be unchanged)
We also discuss about having a single FILENAME variable with a given syntax
(like "|/path/send_to_this_executable parameters") but you quite
disagreed as too complicated.
About the squashing first patch was using Red prefix while the second
Spice but lately we decide for Spice so the second were removed.
About the multiple monitors we discussed but was not a regression,
simply a thing to consider.
> Also: it doesn't appear that red_record_free() is ever called.
>
Well... as any display/cursor/red-worker stuff :(
Luckily exit() will flush last data in the FILE*.
> Reviewed-by: Jonathon Jongsma <jjong...@redhat.com>
>
I'll send an update.
Frediano
>
> On Wed, 2016-05-25 at 13:46 +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 <fzig...@redhat.com>
> > ---
> > server/red-record-qxl.c | 58
> > +++++++++++++++++++++++++++++++++++++++++++-----
> > -
> > server/red-record-qxl.h | 12 +++++++---
> > server/red-worker.c | 30 ++++++++-----------------
> > 3 files changed, 69 insertions(+), 31 deletions(-)
> >
> > diff --git a/server/red-record-qxl.c b/server/red-record-qxl.c
> > index aa2f3e5..4821222 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,39 @@ void red_record_qxl_command(FILE *fd, RedMemSlotInfo
> > *slots,
> > break;
> > }
> > }
> > +
> > +RedRecord *red_record_new(void)
> > +{
> > + static const char header[] = "SPICE_REPLAY 1\n";
> > +
> > + const char *filename;
> > + FILE *f;
> > + RedRecord *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(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..b5da31e 100644
> > --- a/server/red-record-qxl.h
> > +++ b/server/red-record-qxl.h
> > @@ -24,12 +24,18 @@
> > #include "red-common.h"
> > #include "memslot.h"
> >
> > +typedef struct RedRecord RedRecord;
> > +
> > +RedRecord* red_record_new(void);
> > +
> > +void red_record_free(RedRecord *record);
> > +
> > void red_record_dev_input_primary_surface_create(
> > - FILE *fd, QXLDevSurfaceCreate *surface, uint8_t
> > *line_0);
> > + RedRecord *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(RedRecord *record, int what, uint32_t type, unsigned
> > long 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);
> >
> > #endif
> > diff --git a/server/red-worker.c b/server/red-worker.c
> > index 0c945c1..b06bfe1 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)
> > @@ -1468,7 +1468,6 @@ RedWorker* red_worker_new(QXLInstance *qxl,
> > QXLDevInitInfo init_info;
> > RedWorker *worker;
> > Dispatcher *dispatcher;
> > - const char *record_filename;
> > RedsState *reds = red_qxl_get_server(qxl->st);
> > RedChannel *channel;
> >
> > @@ -1478,24 +1477,13 @@ RedWorker* red_worker_new(QXLInstance *qxl,
> > 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_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);
> > }
> >
>
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
