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
