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
