[pulseaudio-discuss] [PATCH] pipe-sink: new option "use_existing_fifo"

[Samo Pogačnik]

Dne 10.01.2018 (sre) ob 08:36 +0100 je Georg Chini napisal(a):
> On 09.01.2018 22:24, Samo Pogačnik wrote:
> > 
> > Allow usage of an already existing fifo (named pipe) upon module
> > load. Also, the fifo is not going to be removed upon module unload,
> > if "use_existing_fifo" option has been enabled.
> I wonder if we really need a new option for this. Can't you just
> check
> if the file exists and use it if it is present? You could set a 
> do_not_unlink
> flag if it exists to avoid removing an already existing file on exit.
I'd be glad to make this change without additional option. My concern
was that there might be some existing use case, where an exclusive fifo
creation would have been required by the sink in some benificial way
(like starting with an empty fifo, however this is not guarantied
anyway).

> 
> > 
> > ---
> >   src/modules/module-pipe-sink.c | 17 ++++++++++++++---
> >   1 file changed, 14 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/modules/module-pipe-sink.c b/src/modules/module-
> > pipe-sink.c
> > index fe2af70..eafc3f7 100644
> > --- a/src/modules/module-pipe-sink.c
> > +++ b/src/modules/module-pipe-sink.c
> > @@ -62,6 +62,7 @@ PA_MODULE_USAGE(
> >           "channels=<number of channels> "
> >           "channel_map=<channel map> "
> >           "use_system_clock_for_timing=<yes or no> "
> > +        "use_existing_fifo=<yes or no> "
> >   );
> >   
> >   #define DEFAULT_FILE_NAME "fifo_output"
> > @@ -91,6 +92,7 @@ struct userdata {
> >       pa_usec_t timestamp;
> >   
> >       bool use_system_clock_for_timing;
> > +    bool use_existing_fifo;
> >   };
> >   
> >   static const char* const valid_modargs[] = {
> > @@ -102,6 +104,7 @@ static const char* const valid_modargs[] = {
> >       "channels",
> >       "channel_map",
> >       "use_system_clock_for_timing",
> > +    "use_existing_fifo",
> >       NULL
> >   };
> >   
> > @@ -442,6 +445,11 @@ int pa__init(pa_module *m) {
> >           goto fail;
> >       }
> >   
> > +    if (pa_modargs_get_value_boolean(ma, "use_existing_fifo", &u-
> > >use_existing_fifo) < 0) {
> > +        pa_log("Failed to parse use_existing_fifo argument.");
> > +        goto fail;
> > +    }
> > +
> >       if (pa_thread_mq_init(&u->thread_mq, m->core->mainloop, u-
> > >rtpoll) < 0) {
> >           pa_log("pa_thread_mq_init() failed.");
> >           goto fail;
> > @@ -452,8 +460,10 @@ int pa__init(pa_module *m) {
> >       u->filename = pa_runtime_path(pa_modargs_get_value(ma,
> > "file", DEFAULT_FILE_NAME));
> >   
> >       if (mkfifo(u->filename, 0666) < 0) {
> > -        pa_log("mkfifo('%s'): %s", u->filename,
> > pa_cstrerror(errno));
> > -        goto fail;
> > +        int errno_save = errno;
> > +        pa_log("mkfifo('%s'): %s", u->filename,
> > pa_cstrerror(errno_save));
> > +        if (!u->use_existing_fifo || errno_save != EEXIST)
> > +            goto fail;
> >       }
> The log line should be within the if condition. Also, should we not
> make 
> sure that
> the file is a pipe if it exists?
I'll move the log line and the "ISFIFO" check is already present in the
original code a few lines further.
> 
> > 
> >       if ((u->fd = pa_open_cloexec(u->filename, O_RDWR, 0)) < 0) {
> >           pa_log("open('%s'): %s", u->filename,
> > pa_cstrerror(errno));
> > @@ -584,7 +594,8 @@ void pa__done(pa_module *m) {
> >           pa_rtpoll_free(u->rtpoll);
> >   
> >       if (u->filename) {
> > -        unlink(u->filename);
> > +        if (!u->use_existing_fifo)
> > +            unlink(u->filename);
> >           pa_xfree(u->filename);
> >       }
> >   
> 
regards, Samo