Personally, I think that this would be better as two smaller commits.
One changing the RedRecord implementation itself (adding refcounting,
mutex locking, etc), and one changing which object allocates and owns
the RedRecord instance. The patch looks pretty good, but I have a few
additional comments below.
On Tue, 2017-02-28 at 10:41 +0000, Frediano Ziglio wrote:
> Allows to use recording function for multiple purposes.
> This will allow to register multiple screen VM or recording
> additional stuff like sound.
> The mutex code added to red-record-qxl.c is required to avoid
> mixing writing from multiple threads.
>
> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> ---
> server/red-record-qxl.c | 35 +++++++++++++++++++++++++++++------
> server/red-record-qxl.h | 3 ++-
> server/red-worker.c | 8 ++------
> server/reds-private.h | 2 ++
> server/reds.c | 16 ++++++++++++++++
> server/reds.h | 5 +++++
> 6 files changed, 56 insertions(+), 13 deletions(-)
>
> This patch was previously acked but changed slightly
> the structure creation.
>
> diff --git a/server/red-record-qxl.c b/server/red-record-qxl.c
> index ee22236..ef6de9f 100644
> --- a/server/red-record-qxl.c
> +++ b/server/red-record-qxl.c
> @@ -30,7 +30,9 @@
>
> struct RedRecord {
> FILE *fd;
> + pthread_mutex_t lock;
> unsigned int counter;
> + gint refs;
> };
>
> #if 0
> @@ -794,15 +796,17 @@ void
> red_record_primary_surface_create(RedRecord *record,
> {
> FILE *fd = record->fd;
>
> + pthread_mutex_lock(&record->lock);
> 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,
> surface->flags, surface->type);
> write_binary(fd, "data", line_0 ? abs(surface->stride)*surface-
> >height : 0,
> line_0);
> + pthread_mutex_unlock(&record->lock);
> }
>
> -void red_record_event(RedRecord *record, int what, uint32_t type)
> +static void red_record_event_start(RedRecord *record, int what,
> uint32_t type)
I don't quite understand the justification for the _start() name here.
When I first read through the patch, I was expecting that there would
also be a red_record_event_end() to match this function. But really it
just is a version of red_record_event() that doesn't lock the mutex. So
I think red_record_event_unlocked() would probably be a better choice
here.
> {
> red_time_t ts = spice_get_monotonic_time_ns();
> // TODO: record the size of the packet in the header. This would
> make
> @@ -813,12 +817,20 @@ void red_record_event(RedRecord *record, int
> what, uint32_t type)
> fprintf(record->fd, "event %u %d %u %"PRIu64"\n", record-
> >counter++, what, type, ts);
> }
>
> +void red_record_event(RedRecord *record, int what, uint32_t type)
> +{
> + pthread_mutex_lock(&record->lock);
> + red_record_event_start(record, what, type);
> + pthread_mutex_unlock(&record->lock);
> +}
> +
> void red_record_qxl_command(RedRecord *record, RedMemSlotInfo
> *slots,
> QXLCommandExt ext_cmd)
> {
> FILE *fd = record->fd;
>
> - red_record_event(record, 0, ext_cmd.cmd.type);
> + pthread_mutex_lock(&record->lock);
> + red_record_event_start(record, 0, ext_cmd.cmd.type);
>
> switch (ext_cmd.cmd.type) {
> case QXL_CMD_DRAW:
> @@ -837,6 +849,7 @@ void red_record_qxl_command(RedRecord *record,
> RedMemSlotInfo *slots,
> red_record_cursor_cmd(fd, slots, ext_cmd.group_id,
> ext_cmd.cmd.data);
> break;
> }
> + pthread_mutex_unlock(&record->lock);
> }
>
> /**
> @@ -898,15 +911,25 @@ RedRecord *red_record_new(const char *filename)
> }
>
> record = g_new(RedRecord, 1);
> + record->refs = 1;
> record->fd = f;
> record->counter = 0;
> + pthread_mutex_init(&record->lock, NULL);
> return record;
> }
>
> -void red_record_free(RedRecord *record)
> +RedRecord *red_record_ref(RedRecord *record)
> {
> - if (record) {
> - fclose(record->fd);
> - g_free(record);
> + g_atomic_int_inc(&record->refs);
> + return record;
> +}
> +
> +void red_record_unref(RedRecord *record)
> +{
> + if (!record || !g_atomic_int_dec_and_test(&record->refs)) {
> + return;
> }
> + fclose(record->fd);
> + pthread_mutex_destroy(&record->lock);
> + g_free(record);
> }
> diff --git a/server/red-record-qxl.h b/server/red-record-qxl.h
> index 0685393..293e24a 100644
> --- a/server/red-record-qxl.h
> +++ b/server/red-record-qxl.h
> @@ -33,7 +33,8 @@ typedef struct RedRecord RedRecord;
> */
> RedRecord* red_record_new(const char *filename);
>
> -void red_record_free(RedRecord *record);
> +RedRecord *red_record_ref(RedRecord *record);
> +void red_record_unref(RedRecord *record);
>
> void red_record_primary_surface_create(RedRecord *record,
> QXLDevSurfaceCreate *surface,
> diff --git a/server/red-worker.c b/server/red-worker.c
> index e5adbaa..497a0da 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -1312,7 +1312,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;
>
> @@ -1322,10 +1321,7 @@ 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) {
> - worker->record = red_record_new(record_filename);
> - }
> + worker->record = reds_get_record(reds);
> dispatcher = red_qxl_get_dispatcher(qxl);
> dispatcher_set_opaque(dispatcher, worker);
>
> @@ -1461,7 +1457,7 @@ void red_worker_free(RedWorker *worker)
> g_main_context_unref(worker->core.main_context);
>
> if (worker->record) {
> - red_record_free(worker->record);
> + red_record_unref(worker->record);
> }
> memslot_info_destroy(&worker->mem_slots);
> free(worker);
> diff --git a/server/reds-private.h b/server/reds-private.h
> index e7e9ad7..07b7b38 100644
> --- a/server/reds-private.h
> +++ b/server/reds-private.h
> @@ -25,6 +25,7 @@
> #include "main-channel.h"
> #include "inputs-channel.h"
> #include "stat-file.h"
> +#include "red-record-qxl.h"
>
> #define MIGRATE_TIMEOUT (MSEC_PER_SEC * 10)
> #define MM_TIME_DELTA 400 /*ms*/
> @@ -135,6 +136,7 @@ struct RedsState {
> SpiceCoreInterfaceInternal core;
> GList *qxl_instances;
> MainDispatcher *main_dispatcher;
> + RedRecord *record;
> };
>
> #define FOREACH_QXL_INSTANCE(_reds, _iter, _qxl) \
> diff --git a/server/reds.c b/server/reds.c
> index 39a7a31..08a391e 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -3455,7 +3455,9 @@ static const char default_video_codecs[] =
> "spice:mjpeg;" GSTREAMER_CODECS;
> /* new interface */
> SPICE_GNUC_VISIBLE SpiceServer *spice_server_new(void)
> {
> + const char *record_filename;
> RedsState *reds = spice_new0(RedsState, 1);
> +
> reds->config = spice_new0(RedServerConfig, 1);
> reds->config->default_channel_security =
> SPICE_CHANNEL_SECURITY_NONE | SPICE_CHANNEL_SECURITY_SSL;
> @@ -3484,6 +3486,11 @@ SPICE_GNUC_VISIBLE SpiceServer
> *spice_server_new(void)
> #ifdef RED_STATISTICS
> reds->stat_file = stat_file_new(REDS_MAX_STAT_NODES);
> #endif
> +
> + record_filename = getenv("SPICE_WORKER_RECORD_FILENAME");
This environment variable name is now a little bit out-of-date if it's
no longer owned by the worker. Perhaps something like this?
record_filename = getenv("SPICE_WORKER_RECORD_FILENAME");
if (record_filename) {
g_warning("SPICE_WORKER_RECORD_FILENAME is deprecated. use
SPICE_SERVER_RECORD_FILENAME");
} else {
record_filename = getenv("SPICE_SERVER_RECORD_FILENAME");
}
> + if (record_filename) {
> + reds->record = red_record_new(record_filename);
> + }
> return reds;
> }
>
> @@ -4494,3 +4501,12 @@ static RedCharDeviceVDIPort
> *red_char_device_vdi_port_new(RedsState *reds)
> "self-tokens",
> (guint64)REDS_NUM_INTERNAL_AGENT_MESSAGES,
> NULL);
> }
> +
> +RedRecord *reds_get_record(RedsState *reds)
> +{
> + if (reds->record) {
> + return red_record_ref(reds->record);
> + }
> +
> + return NULL;
> +}
> diff --git a/server/reds.h b/server/reds.h
> index 28e3444..2748102 100644
> --- a/server/reds.h
> +++ b/server/reds.h
> @@ -101,6 +101,11 @@ SpiceCoreInterfaceInternal*
> reds_get_core_interface(RedsState *reds);
> void reds_update_client_mouse_allowed(RedsState *reds);
> MainDispatcher* reds_get_main_dispatcher(RedsState *reds);
>
> +/* Get the recording object stored in RedsState.
> + * You should free with red_record_unref.
> + */
> +struct RedRecord *reds_get_record(RedsState *reds);
> +
> /* fd watches/timers */
> SpiceWatch *reds_core_watch_add(RedsState *reds,
> int fd, int event_mask,
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel