On Wed, 2017-03-01 at 12:23 -0500, Frediano Ziglio wrote:
> >
> > 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.
sure
> 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?
good question. I suspect that nobody but developers really use this
option, so I'm not sure it's all that important. I wouldn't try to
complicate the documentation by listing obsolete names for things like
this. We could also just leave it as SPICE_WORKER_RECORD_FILENAME I
suppose, even though it's no longer strictly accurate. It's not the
most important thing int he world...
>
> 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");
> }
> }
Yes, of course, that makes much more sense.
>
> > > + 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