[Spice-devel] [PATCH 2/2 v2] replay: allows to specify a filter for record output

[Jonathon Jongsma]

On Wed, 2016-05-25 at 16:35 +0100, Frediano Ziglio wrote:
> This allows compressions using external programs or any type

compressions -> compression

> of filters.
> 
> To use it set SPICE_WORKER_RECORD_FILTER environment to the
> filter command you want to use. The command is executed with
> g_spawn_async_with_pipes (which uses execve) so is not a shell
> command although the command is parsed using g_shell_parse_argv
> which split arguments as shell does.
> 
> One easy way to use it is to just use a compressor like gzip with
> 
>   export SPICE_WORKER_RECORD_FILENAME=/tmp/qemu_record.gz
>   export SPICE_WORKER_RECORD_FILTER=gzip
>   qemu ...
> 
> The filter will receive the recording on standard input and is
> supposed to write in output filename (which is the standard output).
> Nothing forbid to close and delete the file and do something else
> use additional argument in SPICE_WORKER_RECORD_FILTER to specify
> for instance compression level. 

I don't quite understand this last sentence.

> Note however that even if you don't
> use the output file is always opened

what does this mean "the output file is always opened"?

>  and SPICE_WORKER_RECORD_FILENAME
> have to be defined to enable recording.
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/red-record-qxl.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> Changes from v1:
> - rebased.
> 
> diff --git a/server/red-record-qxl.c b/server/red-record-qxl.c
> index a4e0cc0..778ee1e 100644
> --- a/server/red-record-qxl.c
> +++ b/server/red-record-qxl.c
> @@ -21,6 +21,8 @@
>  
>  #include <stdbool.h>
>  #include <inttypes.h>
> +#include <fcntl.h>
> +#include <glib.h>
>  #include "red-worker.h"
>  #include "red-common.h"
>  #include "memslot.h"
> @@ -834,10 +836,21 @@ void red_record_qxl_command(RedRecord *record,
> RedMemSlotInfo *slots,
>      }
>  }
>  
> +static void out_setup(gpointer user_data)

How about child_output_setup()? It would help me understand the context a little
better. Also, a couple comments in this function would probably help me in the
future since I don't use these functions very often.

> +{
> +    int fd = GPOINTER_TO_INT(user_data);
> +
> +    while (dup2(fd, 1) < 0 && errno == EINTR)

What about using STDOUT_FILENO here instead of a bare constant? 

> +        continue;
> +    close(fd);
> +    fcntl(1, F_SETFD, 0);

and here too? 

> +}
> +
>  RedRecord *red_record_new(const char *filename)
>  {
>      static const char header[] = "SPICE_REPLAY 1\n";
>  
> +    const char *filter;
>      FILE *f;
>      RedRecord *record;
>  
> @@ -847,6 +860,33 @@ RedRecord *red_record_new(const char *filename)
>          return NULL;
>      }
>  
> +    filter = getenv("SPICE_WORKER_RECORD_FILTER");
> +    if (filter) {
> +        gint argc;
> +        gchar **argv = NULL;
> +        GError *error = NULL;
> +        GPid child_pid;
> +        gboolean ret;
> +        gint fd_in;
> +
> +        ret = g_shell_parse_argv(filter, &argc, &argv, &error);
> +
> +        if (ret)
> +            ret = g_spawn_async_with_pipes(NULL, argv, NULL,
> G_SPAWN_SEARCH_PATH,
> +                               out_setup, GINT_TO_POINTER(fileno(f)),
> &child_pid,
> +                               &fd_in, NULL, NULL, &error);
> +
> +        g_strfreev(argv);
> +        if (!ret) {
> +            g_error_free(error);
> +            fclose(f);
> +            spice_error("failed to setup filter for replay");
> +        }
> +        while (dup2(fd_in, fileno(f)) < 0 && errno == EINTR)
> +            continue;
> +        close(fd_in);
> +    }
> +
>      if (fwrite(header, sizeof(header)-1, 1, f) != 1) {
>          spice_error("failed to write replay header");
>      }


Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>