[pulseaudio-discuss] [PATCH] core: Fallbacks for machine-id in filesystem

[Tanu Kaskinen]

On Mon, 2015-07-13 at 15:25 +0300, Tanu Kaskinen wrote:
> On Tue, 2015-06-09 at 21:56 +0200, Peter Meerwald wrote:
> > see https://bugs.freedesktop.org/show_bug.cgi?id=88834
> > 
> > Signed-off-by: Peter Meerwald <pmeerw at pmeerw.net>
> > ---
> >  src/pulsecore/core-util.c | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> Thanks for addressing this! I have a couple of minor comments.
> 
> > diff --git a/src/pulsecore/core-util.c b/src/pulsecore/core-util.c
> > index ad5b2d2..edb9e38 100644
> > --- a/src/pulsecore/core-util.c
> > +++ b/src/pulsecore/core-util.c
> > @@ -3059,12 +3059,21 @@ char *pa_machine_id(void) {
> >      /* The returned value is supposed be some kind of ascii 
> > identifier
> >       * that is unique and stable across reboots. */
> >  
> > -    /* First we try the /etc/machine-id, which is the best option 
> > we
> > -     * have, since it fits perfectly our needs and is not as 
> > volatile
> > +    /* First we try ${sysconfdir}/etc/machine-id, with fallbacks 
> > to
> > +     * ${localstatedir}/lib/dbus/machine-id, /etc/machine-id and
> > +     * /var/lib/dbus/machine-id, which are the best option we
> > +     * have, since they fit perfectly our needs and are not as 
> > volatile
> >       * as the hostname which might be set from dhcp. */
> 
> The new version of the comment doesn't sound very good to me. 
> "...which
> is the best option" in the original version really referred to the 
> idea
> of a machine-id file. Changing it to "...which are the best option"
> referring to all the fallbacks sounds awkward. I don't think the
> comment needs any modifications. If you still want to say something
> about the fallbacks, I think that would be better done in a separate
> sentence.
> 
> There's also a typo in one of the paths: "${sysconfdir}/etc/machine
> -id"
> should be "${sysconfdir}/machine-id".
> 
> >      if ((f = pa_fopen_cloexec(PA_MACHINE_ID, "r")) ||
> > -        (f = pa_fopen_cloexec(PA_MACHINE_ID_FALLBACK, "r"))) {
> > +        (f = pa_fopen_cloexec(PA_MACHINE_ID_FALLBACK, "r")) ||
> > +#if !defined(OS_IS_WIN32)
> > +        (f = pa_fopen_cloexec("/etc/machine-id", "r")) ||
> > +        (f = pa_fopen_cloexec("/var/lib/dbus/machine-id", "r"))
> > +#else
> > +        false
> > +#endif
> > +        ) {
> 
> I wondered why you added the OS_IS_WIN32 check only around the two 
> last
> paths and not the whole machine-id section, but I guess it makes 
> sense:
> those last two paths will not exist in any sane configuration on
> Windows, so no need to try them, but ${sysconfdir}/etc/machine-id or
> ${localstatedir}/lib/dbus/machine-id might exist at least in theory, 
> if
> libdbus is installed in the same directory hierarchy as PulseAudio.
> However, PA_MACHINE_ID and PA_MACHINE_ID_FALLBACK contain forward
> slashes, while Windows uses backslashes, so I wonder if the code
> actually works as intended?

I pushed your patch now to the "next" branch. I'll write a separate
patch that addresses the complaint I had about the comment.

-- 
Tanu