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

> +
> +    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>


_______________________________________________
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]