Re: [PATCH 1/2 v2] replay: better record encapsulation

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 
> On Wed, 2016-05-25 at 16:35 +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 <fziglio@xxxxxxxxxx>
> > ---
> >  server/red-record-qxl.c | 53
> >  ++++++++++++++++++++++++++++++++++++++++++------
> > -
> >  server/red-record-qxl.h | 15 ++++++++++----
> >  server/red-worker.c     | 25 ++++++++++-------------
> >  3 files changed, 67 insertions(+), 26 deletions(-)
> > 
> > Changes from v1:
> > - read SPICE_WORKER_RECORD_FILENAME in red-worker.c;
> > - minor indentation fixed in header.
> > 
> > diff --git a/server/red-record-qxl.c b/server/red-record-qxl.c
> > index aa2f3e5..a4e0cc0 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,34 @@ void red_record_qxl_command(FILE *fd, RedMemSlotInfo
> > *slots,
> >          break;
> >      }
> >  }
> > +
> > +RedRecord *red_record_new(const char *filename)
> > +{
> > +    static const char header[] = "SPICE_REPLAY 1\n";
> > +
> > +    FILE *f;
> > +    RedRecord *record;
> > +
> > +    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");
> > +    }
> 
> It seems a little bit inconsistent that failing to open the file would result
> in
> only a warning and return NULL, but succeeding to open the file and then
> failing
> to write would result in an error and abort. Maybe it's fine though
> 

As the caller is basically bailing out with a message on NULL I think
would be more consistent to move the error inside this function, this has
usually in spice-server functions does not return NULL.
Also is consistent with old behavior (failing).

> > +
> > +    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..3a9e8df 100644
> > --- a/server/red-record-qxl.h
> > +++ b/server/red-record-qxl.h
> > @@ -24,12 +24,19 @@
> >  #include "red-common.h"
> >  #include "memslot.h"
> >  
> > -void red_record_dev_input_primary_surface_create(
> > -                           FILE *fd, QXLDevSurfaceCreate *surface, uint8_t
> > *line_0);
> > +typedef struct RedRecord RedRecord;
> >  
> > -void red_record_event(FILE *fd, int what, uint32_t type, unsigned long
> > ts);
> > +RedRecord* red_record_new(const char *filename);
> >  
> > -void red_record_qxl_command(FILE *fd, RedMemSlotInfo *slots,
> > +void red_record_free(RedRecord *record);
> > +
> > +void red_record_dev_input_primary_surface_create(RedRecord *record,
> > +                                                 QXLDevSurfaceCreate
> > *surface,
> > +                                                 uint8_t *line_0);
> > +
> > +void red_record_event(RedRecord *record, int what, uint32_t type, unsigned
> > long ts);
> > +
> > +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..83e7b0a 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)
> > @@ -1480,22 +1480,17 @@ RedWorker* red_worker_new(QXLInstance *qxl,
> >  
> >      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) {
> > +        worker->record = red_record_new(record_filename);
> > +        if (worker->record == 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");
> > -        }
> >      }
> >      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);
> >      }
> >  
> 
> Looks fine overall.
> 
> Acked-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> 
> 
> 
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]