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

[Tanu Kaskinen]

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?

-- 
Tanu