Ticket #606 (new enhancement)

Opened 13 months ago

Last modified 2 months ago

[PATCH] pulseaudio systemwide is a pain

Reported by: Rudd-O Owned by: lennart
Milestone: Component: daemon
Keywords: Cc:

Description

as the offer of making patches stands here:

https://bugzilla.redhat.com/show_bug.cgi?id=461546

I am following up.

Attachments

0002-Skip-starting-PulseAudio-session-daemon-at-session-l.patch (2.2 kB) - added by Rudd-O 13 months ago.
patch that observes colin's complaint about the system-wide client.conf not being read if a client.conf file is present in the user profile

Change History

  Changed 13 months ago by coling

I think the patch should also check the user ~/.pulse/client.conf too :)

  Changed 13 months ago by Rudd-O

You are most definitely wrong, Colin. Let me explain.

If there is no system-wide server running or set up as part of the system administrator policy, then a local session server should be started anyway, regardless of whateve the user's client.conf says. Per-user configuration does not factor into this decision AT ALL.


If we were to follow your opinion, we would be introducing a bug into pulseaudio. Here is the bug:

- user uses padevchooser to choose a remote sink temporarily. this gets written into client.conf. - user logs off. - user logs on again later that day. no PA server is started, because start-pulseaudio-x11 did the thing you suggested. - user uses padevchooser to choose a local sink...ooops, no server is running on his session, no sinks appear, apps malfunction, it's a disaster - user writes a blog post about how much pulseaudio sucks


"A foolish consistency is the hobgoblin of little minds."

You may want PulseAudio to be "feature-complete" in a foolish way by adhering to every configuration file, but regrettably that would be a grave mistake in this case.

Most settings should be overridable by users, but some are not to be overridden by users because giving users that ability would make them inadvertently break their systems. This is one of those settings.

  Changed 13 months ago by coling

I'm afraid your analysis is wrong on a couple of points:

  1. padevchooser is obsolete and shouldn't be used (but that's more nitpicking).
  2. padevchooser does not edit client.conf. No tools edit client.conf, therefore if a user edits their client.conf to specify a default server then they do not want to use a local pulse server and pulse should not be started. padevchooser uses X11 Properties (xprop -root | grep PULSE) to override various defaults etc.
  3. Even if start-pulseaudio-x11 did not run at login and the user subsequently changed their client.conf, pulse would still autospawn. It just wouldn't have x11 properties set for handy SSHing.

If the user specifies a server in their personal client.conf, it's the same as specifying it in the system wide one.

What if the system wide client.conf specifies a default server, but the users specific one does not? start-pulseaudio-x11 will not be run with your patch. It should be.

The modifications relating to starting pulse based on client.conf should not be about "whether a system wide isntance is used." it should be about whether they will be connecting to a local server or a remote one. Using the default-server config option as an indicator that "system wide" is used (as per the comment in the patch) is an invalid assumption.

Of course you could say that if they set the default server to localhost or 127.0.0.1 or <insert IPV6 localhost here> then we *do* need to start a local server, but doing that sort of analysis is too much I think.

If your first comment in your analysis is true, then there should be no checks at all on any client.conf.

So I respectfully disagree with your analysis :)

follow-up: ↓ 6   Changed 13 months ago by Rudd-O

I'm afraid you have a deeper misunderstanding of the problem than I initially thought.


You say "What if the system wide client.conf specifies a default server, but the users specific one does not? start-pulseaudio-x11 will not be run with your patch. It should be."

That's the point of the patch. If the system-wide configuration points the system to a server, then by default a server should not be run, because that's what the administrator has decided system policy should be.


You say "Using the default-server config option as an indicator that "system wide" is used (as per the comment in the patch) is an invalid assumption."

That is not exactly the idea behind checking client.conf. The idea behind checking client.conf is to know whether the administrator has set up a server specifically for the users. If a server has been set up specifically for the users (whether it's system-wide local to the machine, or remote), then the session server should not run.


The spirit of the patch is as follows:

- if a Pulse server has been set up by the administrator (either in the PULSE_SERVER environment variable or in a system-wide configuration file called client.conf), bail out - if a Pulse server is active locally and is running as system-wide, bail out - if neither of these conditions are true, run the session server

This is designed to support two scenarios:

- A local system-wide server is running. Do not start the session server because it might conflict with the system-wide one. As far as I recall, by default, pulse will attempt to connect to the system-wide instance if no session server is running, and if that fails, only then it autostarts a daemon. - The administrator has set a remote server as system policy in client.conf or PULSE_SERVER environment variable (terminal server machine). Do not start the session server because it is moronic and superfluous to start one server per user if you have 50 logged-in users and no sound hardware.

Since the second scenario can only be detected by checking client.conf, there is a check for client.conf's contents. Whatever the *user* has set up for a client has NOTHING to do with the spirit of the patch, which is to avoid the startup of the session server if the administrator has decided as policy that he is running a system-wide server, or a remote server.


It also doesn't matter that padevchooser is obsolete, or that it doesn't write to client.conf (according to you) -- all that matters is that, if the system administrator has decided on a global server that may be remote, or the system administrator has set up a local system-wide server, that decision should be followed by default, and session servers should not start.

If we remove the check for client.conf as you suggested, you will produce another, different bug. Namely, that a session server will be started on logon even in those cases where no system-wide server is running, but a remote server has been set as default in client.conf, which is an unnecessary step that wastes RAM in those scenarios, and may introduce other unforeseen problems. That is why the check for client.conf is needed besides the check for /var/lib/pulse/pid.

Oh, and autospawn was broken the last time I checked.


You may disagree very respectfully with my analysis, but it seems you haven't understood the intention of this work very well.

  Changed 13 months ago by Rudd-O

Frigging restructuredtext screwed up my formatting.

At any rate, there may be a case to be made for checking the user's client.conf, but you have not made any case for such a test, and since the patch *fulfills* its purpose as it is, I see no reason to add the check.

When you make a case for checking client.conf (with an obvious use case and grounded in technical data), I might add the check. But "I think..." is not a technical case at all.


"Using the default-server config option as an indicator that "system wide" is used (as per the comment in the patch) is an invalid assumption."

That is an assumption *you* are making, not me or the patch. The patch CLEARLY SAYS:

"# Exit if the system client.conf sets a default server (generally set as policy whenever system-wide daemons are run, or terminal services are deployed)"

Do you see the part about "or terminal services are deployed"? I trust you are familiar with boolean logic.

in reply to: ↑ 4   Changed 13 months ago by coling

I'm afraid you have a deeper misunderstanding of how the pulse client library works than I previously thought :p

Replying to Rudd-O:

You say "What if the system wide client.conf specifies a default server, but the users specific one does not? start-pulseaudio-x11 will not be run with your patch. It should be." That's the point of the patch. If the system-wide configuration points the system to a server, then by default a server should not be run, because that's what the administrator has decided system policy should be.

As stated before, this is an invalid assumption. This is *NOT* what the pulseaudio client libraries do. Your logic in this script *has* to follow the same rules for client.conf parsing as the pulse library does. Do you not agree?

The pulse library looks for a user specific client.conf and if it finds, one it uses it. If it does not find one it uses the system default one. If you want to change this behaviour to make the system client.conf have some kind of special meaning, editing start-pulseaudio-x11 is not sufficient.

With things as you propose, the following scenario would be possible.

  1. Default pulse install.
  2. No system wide daemon configured/running.
  3. Modified system wide client.conf to point at a remote server.
  4. User's own client.conf which does not set a default server.

When that user logs in, they user will have their own pulseaudio daemon autospawned, however this daemon will not be registered with the x11 session as start-pulseaudio-x11 will not have been run meaning that:

  • SSHing to another machine and playing sound will not work.
  • The daemon will die after a period of inactivity and be respawned again later. This is unneeded and pointless overhead.

You say "Using the default-server config option as an indicator that "system wide" is used (as per the comment in the patch) is an invalid assumption." That is not exactly the idea behind checking client.conf. The idea behind checking client.conf is to know whether the administrator has set up a server specifically for the users. If a server has been set up specifically for the users (whether it's system-wide local to the machine, or remote), then the session server should not run.

If this is your intention (and FWIW I disagree that this definition of what you are using the system wide client.conf's default-server setting is meant to do), then as I said previously you need to ensure that the logic in the library is changed to match the logic yo are imposing here.

The spirit of the patch is as follows: - if a Pulse server has been set up by the administrator (either in the PULSE_SERVER environment variable or in a system-wide configuration file called client.conf), bail out - if a Pulse server is active locally and is running as system-wide, bail out - if neither of these conditions are true, run the session server

That's a fine intention but (repeating myself) the client.conf parsing here does not match that done by the pulseaudio library. The two have to match.

This is designed to support two scenarios: - A local system-wide server is running. Do not start the session server because it might conflict with the system-wide one. As far as I recall, by default, pulse will attempt to connect to the system-wide instance if no session server is running, and if that fails, only then it autostarts a daemon. - The administrator has set a remote server as system policy in client.conf or PULSE_SERVER environment variable (terminal server machine). Do not start the session server because it is moronic and superfluous to start one server per user if you have 50 logged-in users and no sound hardware.

Yet again, (hopefully it's sinking in). the system wide client.conf is not for policy!!! It's just a fallback if the user does not have their own.

Since the second scenario can only be detected by checking client.conf, there is a check for client.conf's contents. Whatever the *user* has set up for a client has NOTHING to do with the spirit of the patch, which is to avoid the startup of the session server if the administrator has decided as policy that he is running a system-wide server, or a remote server.

As noted about six times now, the users client.conf will override the system one and thus even if you don't start the server, the user will have their own pulse autospawned anyway. If that doesn't work (e.g. autospawning is disabled) then pulse will fail to connect. Remember the library does not see the system client.conf at all. Doen't check for it, doesn't open it, doesn't read it. The contents are irrelevant.

------------------------------------------------ It also doesn't matter that padevchooser is obsolete, or that it doesn't write to client.conf (according to you)

Yes, but this is subjective.... programmers have often depated long into the night whether an app does or doesn't do something....

I'm not sure if you're trying deliberately to be awkward and condescending but either way it's working!

all that matters is that, if the system administrator has decided on a global server that may be remote, or the system administrator has set up a local system-wide server, that decision should be followed by default, and session servers should not start.

Yet again, this is not how the library works. You're basing you're whole argument of an invalid assumption.

If we remove the check for client.conf as you suggested, you will produce another, different bug. Namely, that a session server will be started on logon even in those cases where no system-wide server is running, but a remote server has been set as default in client.conf, which is an unnecessary step that wastes RAM in those scenarios, and may introduce other unforeseen problems. That is why the check for client.conf is needed besides the check for /var/lib/pulse/pid.

Well I only suggested removing it because your logic about it was fundamentally wrong. Do as I originally said, and the logic is OK and the check can stay.

Oh, and autospawn was broken the last time I checked.

Autospawn is enabled by default since about 0.9.11 and is working fine.

You may disagree very respectfully with my analysis, but it seems you haven't understood the intention of this work very well.

While you seem to be an expert at derision and condescension, those things only really work when you're actually right..... When you are wrong, you just end up sounding arrogant. I could comment about your other post which continues in the same vein, but I'll save my breath/fingers.

Please try to phrase your comments in a more community focused and collaborative way in the future.

If you create a patch wich is not logically broken and does not change the logic of the client.conf parsing, then I'll happily merge it into git master.

If you do want to change the client.conf logic, I'd rather consult with Lennart as to whether this is a wise plan.

  Changed 13 months ago by coling

Actually, thinking about this in more depth, you should also be checking other ENV vars that specify where to look for a client.conf file... So what I suggested at the beginning is not really enough. You have to go even further.

The logic of parsing client.conf is non trivial and probably warrants the creation of a pulseconf utility that can be run to print out the relevant value utilising the same logic as in the library. e.g. pulseconf --client default-server

follow-up: ↓ 9   Changed 13 months ago by Rudd-O

You are invited to write such a tool. In re meantime, the patch does what it was intended to do, so it should go in as-is until such time that you have written said tool.

in reply to: ↑ 8   Changed 13 months ago by coling

Replying to Rudd-O:

You are invited to write such a tool. In re meantime, the patch does what it was intended to do, so it should go in as-is until such time that you have written said tool.

Not sure if you read my longer comment before that one (the email didn't send for some reason), but I explained in quite some depth *why* this cannot go in right now due to the incorrect logic.

Using your patch as it stands *will* create issues and does not behave consistently to how the pulse client library works.

  Changed 13 months ago by Rudd-O

Excuse me if I appear rude but:

I haven't seen you come up with a SINGLE issue that this patch would create. You seem to be very keen on insisting that "this patch would create issues", but I have yet to see one, even one example of issues that this patch would create, with a technical justification or a test demonstrating your assertion is true.

All I see here is variations on this theme:

"but I explained in quite some depth *why* this cannot go in right now due to the incorrect logic. ... Using your patch as it stands *will* create issues."

Yet so far I really haven't seen ONE SINGLE EXAMPLE of these supposed issues my patch creates. So where is the example, and the test, that demonstrates my patch creates issues?

As for "does not behave consistently to how the pulse client library works"... are you going to reject my patch because you are unwilling to add a single line check in my patch for something that *you and only you* want to check? Why is it that I haven't seen you make a better patch? Perhaps an explicit mechanism to stop the pulseaudio session server from starting at logon?

Oh, that's right, I forgot... you weren't really making technical arguments and you don't really have an interest in listening to your users' concerns.

Prove me wrong.

  Changed 13 months ago by Rudd-O

Just so you know I'm not making baseless assertions here:

For example, you say:

"Default pulse install.

No system wide daemon configured/running.

Modified system wide client.conf to point at a remote server.

User's own client.conf which does not set a default server.

When that user logs in, they user will have their own pulseaudio daemon autospawned, however this daemon will not be registered with the x11 session as start-pulseaudio-x11 will not have been run meaning that:

SSHing to another machine and playing sound will not work. "

This first criticism is INVALID, since if the administrator has access to, and has defined a default-client in client.conf, it's clearly a machine under control of someone knowledgeable, clearly not a home machine we're talking about and the administrator can govern policy in the network and cope with this "breakage" which is really just an esoteric complaint.

"The daemon will die after a period of inactivity and be respawned again later. This is unneeded and pointless overhead."

Why would it be respawned, or even started in the first place, when clients HAVE BEEN INSTRUCTED TO CONTACT ANOTHER SERVER? Don't you read your own autospawning code?

Yet another invalid criticism on your part.

So, baseless complaints against my patch = 2.

  Changed 13 months ago by Rudd-O

Deconstructing this complaint more in detail:

"Default pulse install.

No system wide daemon configured/running.

Modified system wide client.conf to point at a remote server.

User's own client.conf which does not set a default server."

If the user went to the trouble of doing exactly that, then it is clear that when he logs on to the machine, he wants to play networked audio to a specific server. If he wanted to get network playback functionality when he SSHes into another machine, I'm sure he is knowledgeable enough to set up an environment variable or the remote client.conf correctly. Furthermore, this is not how pulseaudio ships by default, and this is not how pulseaudio is shipped in distributions either, so my patch does not create a problem at all.

  Changed 13 months ago by Rudd-O

Second complaint of yours addressed:

"then as I said previously you need to ensure that the logic in the library is changed to match the logic yo are imposing here."

I'm not imposing any logic here, because pulseaudio's default behavior as shipped in distros and in source won't change. If you consider that it is a worthwhile goal to make start-pulseaudio-x11 comply with the library logic fully so as to cater to an overwhelmingly tiny minority of users who already know what they are doing when editing a system-wide configuration file as root, THEN DO IT YOURSELF or document the behavior. I'm not your butler and I haven't seen you ask politely for a contribution.

  Changed 13 months ago by Rudd-O

"Yet again, (hopefully it's sinking in). the system wide client.conf is not for policy!!! It's just a fallback if the user does not have their own."

1. the library has fallbacks compiled in, and client.conf ships entirely commented. If the user does not have their own and the system-wide client.conf file is absent, pulseaudio works fine.

2. client.conf, a file that is installed mode 644 in a root-owned directory, owned by root, and whose configuration stanzas are applied system-wide, is *not for system-wide policy*? Are you serious?

  Changed 13 months ago by Rudd-O

"As noted about six times now, the users client.conf will override the system one and thus even if you don't start the server, the user will have their own pulse autospawned anyway."

Ah, so you say:


/etc/pulse/client.conf server = somemachine

~/.pulse/client.conf ; server = notconfigured_or_file_absent


Explain how exactly will a server be autospawned when the client library will SKIP the autospawning, contact somemachine and attempt to play audio there?

Really, please humor me. I tested this here -- IT DOES NOT HAPPEN. This claim of yours is FALSE.

  Changed 13 months ago by Rudd-O

"Remember the library does not see the system client.conf at all. Doen't check for it, doesn't open it, doesn't read it. The contents are irrelevant."

FALSE. The client.conf is checked by the library.


~/Projects/Third-party/pulseaudio@… α: strace -eopen paplay /usr/share/sounds/pop.wav open("/etc/ld.so.cache", O_RDONLY) = 3 open("/usr/lib64/libpulse.so.0", O_RDONLY) = 3 open("/usr/lib64/libpulsecommon-0.9.15.so", O_RDONLY) = 3 open("/usr/lib64/libX11.so.6", O_RDONLY) = 3 open("/usr/lib64/libSM.so.6", O_RDONLY) = 3 open("/usr/lib64/libICE.so.6", O_RDONLY) = 3 open("/usr/lib64/libXtst.so.6", O_RDONLY) = 3 open("/lib64/libwrap.so.0", O_RDONLY) = 3 open("/usr/lib64/libasyncns.so.0", O_RDONLY) = 3 open("/lib64/libdbus-1.so.3", O_RDONLY) = 3 open("/usr/lib64/libsndfile.so.1", O_RDONLY) = 3 open("/lib64/libcap.so.2", O_RDONLY) = 3 open("/usr/lib64/libgdbm.so.2", O_RDONLY) = 3 open("/lib64/librt.so.1", O_RDONLY) = 3 open("/lib64/libdl.so.2", O_RDONLY) = 3 open("/lib64/libm.so.6", O_RDONLY) = 3 open("/lib64/libpthread.so.0", O_RDONLY) = 3 open("/lib64/libc.so.6", O_RDONLY) = 3 open("/usr/lib64/libxcb.so.1", O_RDONLY) = 3 open("/lib64/libuuid.so.1", O_RDONLY) = 3 open("/usr/lib64/libXext.so.6", O_RDONLY) = 3 open("/lib64/libnsl.so.1", O_RDONLY) = 3 open("/lib64/libresolv.so.2", O_RDONLY) = 3 open("/usr/lib64/libFLAC.so.8", O_RDONLY) = 3 open("/usr/lib64/libogg.so.0", O_RDONLY) = 3 open("/lib64/libattr.so.1", O_RDONLY) = 3 open("/usr/lib64/libXau.so.6", O_RDONLY) = 3 open("/usr/lib/locale/locale-archive", O_RDONLY) = 3 open("/usr/share/sounds/pop.wav", O_RDONLY) = 3 open("/usr/lib64/gconv/gconv-modules.cache", O_RDONLY) = 4 open("/home/rudd-o/.pulse/client.conf", O_RDONLY) = -1 ENOENT (No such file or directory) open("/etc/pulse/client.conf", O_RDONLY) = 8


~/Projects/Third-party/pulseaudio@… α: strace -eopen aplay /usr/share/sounds/pop.wav open("/etc/ld.so.cache", O_RDONLY) = 3 open("/lib64/librt.so.1", O_RDONLY) = 3 open("/lib64/libasound.so.2", O_RDONLY) = 3 open("/lib64/libm.so.6", O_RDONLY) = 3 open("/lib64/libdl.so.2", O_RDONLY) = 3 open("/lib64/libpthread.so.0", O_RDONLY) = 3 open("/lib64/libc.so.6", O_RDONLY) = 3 open("/usr/lib/locale/locale-archive", O_RDONLY) = 3 open("/usr/share/alsa/alsa.conf", O_RDONLY) = 3 open("/etc/asound.conf", O_RDONLY) = 3 open("/etc/alsa/pcm/vdownmix.conf", O_RDONLY) = 3 open("/etc/alsa/pulse-default.conf", O_RDONLY) = 3 open("/usr/lib64/alsa-lib/libasound_module_pcm_pulse.so", O_RDONLY) = 3 open("/etc/ld.so.cache", O_RDONLY) = 3 open("/usr/lib64/libpulse.so.0", O_RDONLY) = 3 open("/usr/lib64/libpulsecommon-0.9.15.so", O_RDONLY) = 3 open("/usr/lib64/libX11.so.6", O_RDONLY) = 3 open("/usr/lib64/libSM.so.6", O_RDONLY) = 3 open("/usr/lib64/libICE.so.6", O_RDONLY) = 3 open("/usr/lib64/libXtst.so.6", O_RDONLY) = 3 open("/lib64/libwrap.so.0", O_RDONLY) = 3 open("/usr/lib64/libasyncns.so.0", O_RDONLY) = 3 open("/lib64/libdbus-1.so.3", O_RDONLY) = 3 open("/lib64/libcap.so.2", O_RDONLY) = 3 open("/usr/lib64/libgdbm.so.2", O_RDONLY) = 3 open("/usr/lib64/libxcb.so.1", O_RDONLY) = 3 open("/lib64/libuuid.so.1", O_RDONLY) = 3 open("/usr/lib64/libXext.so.6", O_RDONLY) = 3 open("/lib64/libnsl.so.1", O_RDONLY) = 3 open("/lib64/libresolv.so.2", O_RDONLY) = 3 open("/lib64/libattr.so.1", O_RDONLY) = 3 open("/usr/lib64/libXau.so.6", O_RDONLY) = 3 open("/home/rudd-o/.pulse/client.conf", O_RDONLY) = -1 ENOENT (No such file or directory) open("/etc/pulse/client.conf", O_RDONLY) = 7


Someone needs to learn how to use strace before making baseless claims.

  Changed 13 months ago by Rudd-O

"While you seem to be an expert at derision and condescension, those things only really work when you're actually right..... "

I have demonstrated with *strace logs* that you are factually mistaken and so your complaints have absolutely no ground. I really don't care about your personal feelings -- I care about improving the software. Your wrong-headed obstination is an OBSTACLE here to that goal, and your opinion on my ability to condescend you or your interpretation of my words has no place in a technical discussion.

I will say one thing though: by now, you do deserve condescension and derision because you are obstaculizing progress by refusing to even test your baseless theories. You could have proven me wrong with a simple strace log, you didn't. Every single complaint of yours has been without technical merit or justification, and I've had to go out of my way to justify a STUPID TWO-LINE PATCH to you, a person who has been wrong ALL ALONG.

This behavior of yours gets on my nerves and, you know what, it *should* get on my nerves. This is not proof of the existence of god that you're asked to provide -- this is just a simple test of your theories that you need to make and demonstrate to everybody else, in order for anyone to take your complaint seriously, and you should know better to do these tests instead of making claims that can be disproved by adding one line to your /etc/pulse/client.conf, and then issuing a simple:

strace -ff aplay 2>&1 | grep client.conf

  Changed 13 months ago by Rudd-O

Finally:

"Please try to phrase your comments in a more community focused and collaborative way in the future."

Pot, meet kettle. Where are your patches, or any other evidence of collaboration? All I see here from you is false claims that can be easily disproven.

If you can't get your own interactions straight, don't have the audacity to lecture me on how to talk to you.

  Changed 13 months ago by Rudd-O

However, just in case, I am now going to generate a SECOND patch that addresses your complaints by verifying if a client.conf file exists, and omitting its logic...

...OK?

That should at least stop the bickering about something that is pointless anyway. How's that for collaboration?

Changed 13 months ago by Rudd-O

patch that observes colin's complaint about the system-wide client.conf not being read if a client.conf file is present in the user profile

  Changed 13 months ago by Rudd-O

done. patch sent.

happy now?

  Changed 13 months ago by Rudd-O

I now retract one of my previous observations: if the user has a client.conf in his profile, the system client.conf is not read. This is what colin suggested originally.

However, this is a VERY UNUSUAL behavior. Regularly, apps and client libs first read the system configuration file, then read the user configuration file, and then merge the configurations, overriding system defaults with the user's custom configuration. I would have expected this to be true in the case of PulseAudio as well, and I am willing to bet most sysadmins would expect that too.

Examples of this user-overridability with defaults-taken-from-system-configuration-files can be found in:

bash openssh kde gnome

you name it, it generally operates in that way.

  Changed 13 months ago by Rudd-O

For the record, I am documenting here what happens with the client.conf's, and Colin is right. That, however, only means that the new patch will do what he expects.


~/Projects/Third-party/pulseaudio@… α: LANG=C ls -la /etc/pulse/client.conf ~/.pulse/client.conf ; strace -efile aplay /usr/share/sounds/pop.wav 2>&1 | grep client.conf ls: cannot access /home/rudd-o/.pulse/client.conf: No such file or directory -rw-r--r-- 1 root root 1227 Apr 3 09:48 /etc/pulse/client.conf open("/home/rudd-o/.pulse/client.conf", O_RDONLY) = -1 ENOENT (No such file or directory) open("/etc/pulse/client.conf", O_RDONLY) = 7

~/Projects/Third-party/pulseaudio@… α: touch ~/.pulse/client.conf

~/Projects/Third-party/pulseaudio@… α: LANG=C ls -la /etc/pulse/client.conf ~/.pulse/client.conf ; strace -efile aplay /usr/share/sounds/pop.wav 2>&1 | grep client.conf -rw-r--r-- 1 root root 1227 Apr 3 09:48 /etc/pulse/client.conf -rw-rw-r-- 1 rudd-o rudd-o 0 Jul 14 16:46 /home/rudd-o/.pulse/client.conf open("/home/rudd-o/.pulse/client.conf", O_RDONLY) = 7

~/Projects/Third-party/pulseaudio@… α:

  Changed 13 months ago by Rudd-O

in monospace. f*ck trac formatting -- they could have used markdown or wikipedia markup, but no, they had to use the one that nobody uses.

{{{~/Projects/Third-party/pulseaudio@… α: LANG=C ls -la /etc/pulse/client.conf ~/.pulse/client.conf ; strace -efile aplay /usr/share/sounds/pop.wav 2>&1 | grep client.conf ls: cannot access /home/rudd-o/.pulse/client.conf: No such file or directory -rw-r--r-- 1 root root 1227 Apr 3 09:48 /etc/pulse/client.conf open("/home/rudd-o/.pulse/client.conf", O_RDONLY) = -1 ENOENT (No such file or directory) open("/etc/pulse/client.conf", O_RDONLY) = 7

~/Projects/Third-party/pulseaudio@… α: touch ~/.pulse/client.conf

~/Projects/Third-party/pulseaudio@… α: LANG=C ls -la /etc/pulse/client.conf ~/.pulse/client.conf ; strace -efile aplay /usr/share/sounds/pop.wav 2>&1 | grep client.conf -rw-r--r-- 1 root root 1227 Apr 3 09:48 /etc/pulse/client.conf -rw-rw-r-- 1 rudd-o rudd-o 0 Jul 14 16:46 /home/rudd-o/.pulse/client.conf open("/home/rudd-o/.pulse/client.conf", O_RDONLY) = 7

~/Projects/Third-party/pulseaudio@… α:}}}

  Changed 13 months ago by Rudd-O

what's wrong with this thing!

~/Projects/Third-party/pulseaudio@karen.dragonfear α:
LANG=C ls -la /etc/pulse/client.conf  ~/.pulse/client.conf ; strace -efile aplay /usr/share/sounds/pop.wav 2>&1 | grep client.conf
ls: cannot access /home/rudd-o/.pulse/client.conf: No such file or directory
-rw-r--r-- 1 root root 1227 Apr  3 09:48 /etc/pulse/client.conf
open("/home/rudd-o/.pulse/client.conf", O_RDONLY) = -1 ENOENT (No such file or directory)
open("/etc/pulse/client.conf", O_RDONLY) = 7

~/Projects/Third-party/pulseaudio@karen.dragonfear α:
touch ~/.pulse/client.conf

~/Projects/Third-party/pulseaudio@karen.dragonfear α:
LANG=C ls -la /etc/pulse/client.conf  ~/.pulse/client.conf ; strace -efile aplay /usr/share/sounds/pop.wav 2>&1 | grep client.conf
-rw-r--r-- 1 root   root   1227 Apr  3 09:48 /etc/pulse/client.conf
-rw-rw-r-- 1 rudd-o rudd-o    0 Jul 14 16:46 /home/rudd-o/.pulse/client.conf
open("/home/rudd-o/.pulse/client.conf", O_RDONLY) = 7

~/Projects/Third-party/pulseaudio@karen.dragonfear α:

  Changed 13 months ago by Rudd-O

yes!

  Changed 12 months ago by lennart

Hmm, simply using 'pulseaudio --check --system' to check for a running system instance might be workable solution.

  Changed 12 months ago by lennart

But tbh I'd find it more appropriate if pulseaudio --start would do the checking itself, so that we don't have to rely on an kludgy shell script and don't have to run the pa binary twice.

  Changed 12 months ago by coling

Not sure I follow Lennart. pulseaudio --start should start a daemon surely? The checks here are to decide whether or not to start a daemon at all, not to prevent a second instance being started. Are you suggesting that pulseaudio --start becomes also a pulseaudio client, e.g. parsing the client.conf to determine whether it needs to be started or not? IMO this is not a good idea as there may be reasons you want to start a pulse server even if the current machine's client setup dictates that it's using a remote server.

That said, I guess I wouldn't be against a --if-needed argument e.g. pulseaudio --start --if-needed

  Changed 12 months ago by lennart

--start already does the --if-needed. --start only starts PA if it isn't runnning already in the session. What I am now thinking of is that it would not just check for PA instances in the session but also in the system.

  Changed 12 months ago by coling

But that's not the whole story... if you read the patch it's also checking to see whether a local daemon is needed at all due to client settings dictating a remote server e.g. via default-server = some-other-host.com in client.conf. This is why my comment was talking about client stuff and --if-needed being a trigger for the daemon to check client config options.

You're only thinking about daemons, not about client access logic. Now I'm not sure it's really needed at all anyway (who cares if a local deamon is started an never used??), but that's what this whole elongated thread has been about due to the additional checks added in the patch.

  Changed 12 months ago by lennart

Hmm, I think checking for alternative servers should not need to take place in the pa daemon binary. Also, the start-pulseaudio-x11 script also checks whether $PULSE_SERVER is set.

Configuration of non-local servers is completely orthogonal to running PA as system daemon, so I'd suggest fcussing on the latter only here. I think the checking in $PULSE_SERVER is completely adequate for checking if we run on thin clients.

In summary: what I'd like to see being done here is simply some patch that when PA is run as session daemon refuses to start if a system daemon is already present. That should be reasonably simple to add. Happy to take patches.

  Changed 12 months ago by Rudd-O

But in the meantime, my patch more than solves the problem, at least until that patch comes, right?

Why not commit it then?

  Changed 12 months ago by lennart

Sorry, that's not how I do those things. This is one of those cases which would never get fixed properly if I'd commit this hackish fix now. I am fine with commiting temporary fixes as long as it is important enough, time criticial and clear enough that a proper fix will then follow shortly. But uh, I am sorry, I don't see that this was the case here. Sorry.

follow-up: ↓ 35   Changed 12 months ago by Rudd-O

So,

We have a working fix to your specifications, that does not introduce any new defects, but you don't like it because it's in a shell script instead of in C code. Therefore, you elect to maintain the bug across every PulseAudio user, just so "you can remind yourself to fix it 'properly'".

"We have some cancer patients but I don't get them chemo because I am sure there is a better fix than chemo; it's just that I haven't implemented it yet and I am waiting for that option, so I keep them sick in order to remind myself that I need to find a chemo better than chemo.".

I love your logic.

in reply to: ↑ 34   Changed 12 months ago by lennart

Replying to Rudd-O:

So, We have a working fix to your specifications, that does not introduce any new defects, but you don't like it because it's in a shell script instead of in C code. Therefore, you elect to maintain the bug across every PulseAudio user, just so "you can remind yourself to fix it 'properly'".

Your patch is not according to my specifications. Which I tried to point out above.

"We have some cancer patients but I don't get them chemo because I am sure there is a better fix than chemo; it's just that I haven't implemented it yet and I am waiting for that option, so I keep them sick in order to remind myself that I need to find a chemo better than chemo.".

So PA is like cancer? Thanks for that enlighting bit of information. How about just keeping focused on prepping a patch that is good to merge?

  Changed 12 months ago by Rudd-O

Your patch is not according to my specifications. Which I tried to point out above.

Yes, because your specifications were arbitrarily redefined to exclude my patch, for reasons of personal preference (in vernacular: "nitpicks"). Not because the patch is useless or does not fulfill its intended goal.

So PA is like cancer?

Out of all the possible interpretations that one could make of the metaphor I used, this is the one you go with? Well, at least that provides further confirmation of your negative predisposition.

For future reference: if you want help with your project, the last thing that you want to do is antagonize the people who could potentially help you. This nugget of information, common sense as it is, has apparently escaped you. You know I'm not a bad person, you know I'm proficient, you know I mean well and I have both our best interests in mind, so there is absolutely no reason why you should be so dismissive and ungrateful with me.

Now that I've had enough antagonism thrown at me, I bail out. Much luck with re-coding my patch in C, or having to stand flames from people who can read the lame excuses you gave not to integrate the work you were gifted with. I, for one, consider myself served and so will many others who will find themselves in my situation and have the skills to integrate a patch, and it's enough for me that they will be able to use my work while you deny yourself and other people that. There's not much more I can do, and certainly nothing more I want to do.

  Changed 12 months ago by coling

Rudd-O, I don't want to stir up more problems, but if you take a detached read of this thread, you'll see that it started quite amicably with myself trying to offer advice and constructive feedback which you then decided to ignore and then ridicule me with an exceptionally arrogant and condescending reply. I tried my best to explain things to you clearly but again you replied with in an arrogant and condescending and, at times, down right rude reply.

Lennart was also very polite in explaining why he would not accept the patch. This is a perfectly reasonable decision for a project maintainer to take and in actual fact it's their JOB. If you've followed any open source development, you'll know this is a critical stance to take. Only with your bad tempered replies did any form of venom, annoyance or rudeness creep in.

If you want to be involved in open source projects, you sometimes have to rework your original patches. I've done that countless times on this particular project and on many others. That's how it works. You can't take it as a personal insult.

Now projects like pulseaudio always appreciate any extra help, but attitudes need to be left at the door. I hope you do want to work more on pulse and help make it better but you really need to learn how to collaborate properly. This whole thread, particularly the way you treated and interacted with me, shows precisely how not to interact with people. You have to listen to people who know more about the project than you do when they give you advice and guidance, not flame them or ridicule them. Read carefully the replies and do not jump to your own conclusions. Case in point: even with your revised patch it's clear you didn't actually appreciate what I said which related to which client.conf to parse, not whether or not to skip the check completely if the user has a client.conf.

That latter point is now moot anyway as Lennart, who has ultimate say, doesn't think this check is worth doing, which is his prerogative. As I said above, it's not really that important IMO if we start a server that ultimately remains unused (as client.conf has higher priority than x11 settings set during the server startup, avoiding said startup due to client.conf settings will not matter in the long run).

I hope you take a long, hard and detached view of this thread and how you've phrased your replies. I hope that you will learn that writing arrogant and condescending comments will not win favour nor make you any friends and that this experience will be a catalyst to you behaving differently when working in community projects in the future.

I genuinely wish you all the best and hope you do not take this as a personal insult - it's certainly not intended that way.

  Changed 12 months ago by lennart

Also see #582.

  Changed 12 months ago by lennart

  • type changed from defect to enhancement
  • component changed from clients to daemon

  Changed 12 months ago by Rudd-O

Thanks, colin. May I observe that I did rework the patch as per your suggestion, though the rework may not have been entirely to your satisfaction. I also do not interpret what went on here as personal, but I do interpret it as antagonistic. Anyway, good day.

follow-up: ↓ 42   Changed 7 months ago by lennart

PA in master now checks if some server is explicitly configured and if that is the case does not start itself if --start is passed.

What is missing now to close this bug is that it also checks whether a system wide daemon is running. Adding that should be relatively easy.

in reply to: ↑ 41   Changed 2 months ago by tanuk

Replying to lennart:

What is missing now to close this bug is that it also checks whether a system wide daemon is running. Adding that should be relatively easy.

What kind of logic did you have in your mind? Is checking for the default socket file existence enough? The system-wide daemon could be configured to listen on some different socket file, or only TCP connections could be allowed. A simple default socket file check might make sense anyway, but it seems to me that generally it's better to just clearly document somehow that if you're enabling the system-wide mode, then disable the start-pulseaudio-x11 script (or any other system for starting per-user daemons automatically).

Note: See TracTickets for help on using tickets.