>
> 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.
>
Make sense.
> > {
> > 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");
> }
>
Well.. I would have this change in a follow up, it's a user level
change.
Surely I would update also the manual.txt file.
How to deal with online information (like https://www.spice-space.org/docs/manual/manual.chunked/ar01s14.html)?
Should we have a comment on the old option so users reading it and having an
old version will understand the issue?
Or just better to add a comment in the code stating that this is maintained for
backward compatibility?
I would check the new environment first and give a warning if old
is find without the new, kind of
record_filename = getenv("SPICE_SERVER_RECORD_FILENAME");
if (!record_filename) {
record_filename = getenv("SPICE_WORKER_RECORD_FILENAME");
if (record_filename) {
g_warning("SPICE_WORKER_RECORD_FILENAME is deprecated. use 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,
>
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel