[Spice-devel] [PATCH v3 6/6] Allow to specify a filter for record output

[Frediano Ziglio]

> 
> > 
> > Hi
> > 
> > ----- Original Message -----> > ----- Original Message -----
> > > > > This allows compressions using external programs or any type
> > > > > of filters.
> > > > > 
> > > > 
> > > > The shell worked well for that already. Why do you need it?
> > > > 
> > > 
> > > How to use the shell in this case ?
> > > Make a fifo and pass it ? Who is going to delete the fifo?
> > > I did it as the same reason why there is the zlib code; I think
> > > compressing
> > > whole file is better.
> > 
> > Good question, I remember I did us xz for compression, and you can
> > see in further commit I disabled zlib. But not sure how now.
> > 
> > Fifo is a solution, I guess I was simply doing
> > SPICE_WORKER_RECORD_FILENAME=/dev/stdout | xz
> > 
> > Assuming that nothing else would go in stdout, imho that is a reasonable
> > assumption for a server code. debugging/log should go in stderr.
> > 
> > Imho, it's not necessary to add this.
> > 
> 
> No, stdout is quite risky. Unfortunately using printf as a debugging is too
> used.
> 
> I found a bash solution (at least supported in Linux):
> 
> SPICE_WORKER_RECORD_FILENAME=>(gzip > /tmp/spice-record.$$.gz) exec
> /usr/bin/qemu-system-x86_64.bin "$@"
> 
> yes... quite strange syntax but works and don't create a fifo but a pipe. I
> would ask how portable it is.
> Looks like one of the feature that is not supported by all bash. At least we
> should check if supported by
> all bash-es we support (RHEL5/6/7).
> 
> Well... found some time to try with RHEL 5.11, RHEL 6... the bash syntax
> works! So now that I master
> this new feature I can ditch my patch!

Mumble... no, the bash trick seems to work or at least it does not work in all
situations. I noted that after replacing qemu-system-x86_64 with a script it
worked but when I put in place the bash feature libvirtd daemon is not
working correctly :(
I checked and Qemu binary is still the direct child, but the trick does not
work for the daemon. After enabling again the filter environment everything
starts working again.
No clue what's the difference :(

Frediano

> I can change emulator from virsh and use a script to change the environment
> in this way.
> 
> Yes, zlib is disabled with #if, I would remove it entirely.
> 
> Frediano
> 
> > > 
> > > Frediano
> > > 
> > > > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > > > ---
> > > > >  server/red_record_qxl.c | 49
> > > > >  +++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  server/red_record_qxl.h |  2 ++
> > > > >  server/red_worker.c     |  2 +-
> > > > >  3 files changed, 52 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/server/red_record_qxl.c b/server/red_record_qxl.c
> > > > > index d96fb79..d6b837c 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 "red_memslots.h"
> > > > > @@ -825,3 +827,50 @@ void red_record_qxl_command(FILE *fd,
> > > > > RedMemSlotInfo
> > > > > *slots,
> > > > >          break;
> > > > >      }
> > > > >  }
> > > > > +
> > > > > +static void out_setup(gpointer user_data)
> > > > > +{
> > > > > +    int fd = GPOINTER_TO_INT(user_data);
> > > > > +
> > > > > +    dup2(fd, 1);
> > > > > +    close(fd);
> > > > > +    fcntl(1, F_SETFD, 0);
> > > > > +}
> > > > > +
> > > > > +FILE *red_record_open_file(const char *record_filename)
> > > > > +{
> > > > > +    const char *filter;
> > > > > +    FILE *f;
> > > > > +
> > > > > +    f = fopen(record_filename, "w+");
> > > > > +    if (!f)
> > > > > +        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;
> > > > > +
> > > > > +        if (!g_shell_parse_argv(filter, &argc, &argv, &error)) {
> > > > > +            fclose(f);
> > > > > +            return NULL;
> > > > > +        }
> > > > > +
> > > > > +        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) {
> > > > > +            fclose(f);
> > > > > +            return NULL;
> > > > > +        }
> > > > > +        dup2(fd_in, fileno(f));
> > > > > +        close(fd_in);
> > > > > +    }
> > > > > +    return f;
> > > > > +}
> > > > > diff --git a/server/red_record_qxl.h b/server/red_record_qxl.h
> > > > > index b737db8..0b74dc4 100644
> > > > > --- a/server/red_record_qxl.h
> > > > > +++ b/server/red_record_qxl.h
> > > > > @@ -31,4 +31,6 @@ void red_record_event(FILE *fd, int what, uint32_t
> > > > > type,
> > > > > unsigned long ts);
> > > > >  void red_record_qxl_command(FILE *fd, RedMemSlotInfo *slots,
> > > > >                              QXLCommandExt ext_cmd, unsigned long
> > > > >                              ts);
> > > > >  
> > > > > +FILE *red_record_open_file(const char *record_filename);
> > > > > +
> > > > >  #endif
> > > > > diff --git a/server/red_worker.c b/server/red_worker.c
> > > > > index 7d7858e..a80dc93 100644
> > > > > --- a/server/red_worker.c
> > > > > +++ b/server/red_worker.c
> > > > > @@ -12107,7 +12107,7 @@ static void red_init(RedWorker *worker,
> > > > > WorkerInitData *init_data)
> > > > >      if (record_filename) {
> > > > >          static const char header[] = "SPICE_REPLAY 1\n";
> > > > >  
> > > > > -        worker->record_fd = fopen(record_filename, "w+");
> > > > > +        worker->record_fd = red_record_open_file(record_filename);
> > > > >          if (worker->record_fd == NULL) {
> > > > >              spice_error("failed to open recording file %s\n",
> > > > >              record_filename);
> > > > >          }
> > > > > --
> > > > > 2.4.3
> > > > > 
> > > _______________________________________________
> > > Spice-devel mailing list
> > > Spice-devel at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> > > 
> > 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>