[pulseaudio-discuss] [PATCH 2/2] context: add cookie loaders + setter

[Tanu Kaskinen]

On Tue, 2013-08-20 at 16:13 +0300, Tanu Kaskinen wrote:
> On Tue, 2013-08-13 at 11:41 +0200, Alexander Couzens wrote:
> > I'm not sure if these 3 seperate calls for cookie loading are better than creating a pa_context_get_conf() call and
> > operating on it.
> 
> I think it's better to keep pa_client_conf as a private interface.
> 
> > 
> > Signed-off-by: Alexander Couzens <lynxis at fe80.eu>
> > ---
> >  src/pulse/context.c | 15 +++++++++++++++
> >  src/pulse/context.h |  9 +++++++++
> >  2 files changed, 24 insertions(+)
> > 
> > diff --git a/src/pulse/context.c b/src/pulse/context.c
> > index 1ba2672..654284e 100644
> > --- a/src/pulse/context.c
> > +++ b/src/pulse/context.c
> > @@ -1448,3 +1448,18 @@ size_t pa_context_get_tile_size(pa_context *c, const pa_sample_spec *ss) {
> >      mbs = PA_ROUND_DOWN(pa_mempool_block_size_max(c->mempool), fs);
> >      return PA_MAX(mbs, fs);
> >  }
> > +
> > +int pa_context_set_cookie(pa_context *c, uint8_t *cookie, size_t cookie_size) {
> 
> The pointers should be checked with pa_assert(). And there should be
> some sanity checks:
> 
>     PA_CHECK_VALIDITY(c, !pa_detect_fork(), PA_ERR_FORKED);
>     PA_CHECK_VALIDITY(c, c->state == PA_CONTEXT_UNCONNECTED, PA_ERR_BADSTATE);
> 
> (The same goes for the other functions.)
> 
> > +    if(cookie_size != PA_NATIVE_COOKIE_LENGTH)
> 
> Missing space after "if".
> 
> > +        return -PA_ERR_INVALID;
> > +    memcpy(c->conf->cookie, cookie, cookie_size);
> 
> c->conf->cookie_valid should be set to true. Or actually, there should
> be pa_client_conf_set_cookie() that sets the cookie_valid field -
> modifying the pa_client_conf state should be done from client-conf.c
> only.
> 
> > +    return 0;
> > +}
> > +
> > +int pa_context_load_cookie_from_hex(pa_context *c, const char *cookie_as_hex) {
> > +    return pa_client_conf_load_cookie_from_hex(c->conf, cookie_as_hex);
> > +}
> > +
> > +int pa_context_load_cookie_file(pa_context *c, const char *cookie_file_path) {
> > +    return pa_client_conf_load_cookie_from_file(c->conf, cookie_file_path);
> > +}
> > diff --git a/src/pulse/context.h b/src/pulse/context.h
> > index 6e615f6..20fa1ff 100644
> > --- a/src/pulse/context.h
> > +++ b/src/pulse/context.h
> > @@ -280,6 +280,15 @@ void pa_context_rttime_restart(pa_context *c, pa_time_event *e, pa_usec_t usec);
> >   * pa_stream_get_sample_spec(ss)); \since 0.9.20 */
> >  size_t pa_context_get_tile_size(pa_context *c, const pa_sample_spec *ss);
> >  
> > +/** Set the authentication cookie */
> 
> The documentation is missing a \since 5.0 tag, and explanation about
> what the cookie size should be.
> 
> We need to expose the cookie size to clients somehow. A simple #define
> should be good enough. The problem with a #define in the public API is
> that it can't be changed without breaking compatibility, but I don't
> think we can change the cookie size anyway without breaking
> compatibility between old server and new libpulse or vice versa.

Actually, having the cookie_size parameter seems a bit silly, since only
one value will be accepted. On the other hand, it's nice to have some
way of forcing the client to think about the buffer size... I have
trouble deciding what I want the interface to look like. The most
pleasant way forward would be to avoid the decision and drop
pa_context_set_cookie() and pa_context_load_cookie_from_hex()
altogether. AFAIK you don't need those functions anyway. If we need the
functionality in the future (pretty unlikely), it can be added later.

A couple of additional notes:

Shouldn't the function name be pa_context_load_cookie_from_file()
instead of pa_context_load_cookie_file(), to be consistent with the
client-conf API?

I think the documentation should explain what the authentication cookie
is, and explain why anyone would want to call the function (it should be
noted that most applications don't need to care about the cookie).

-- 
Tanu