Re: [mpd-devel] PATCH: Add support for configuration specified optional headers in HTTP output plugins
On 2014/12/03 23:09, X Ryl wrote: > Actually, it's useful to clients but not only. In my use case, MPD is > on a headless server. Without it, it's not possible to receive this > stream in Javascript context due to Cross Origin protection in > browsers. That prevents using HTTPD plugin as a audio source for a web > page, unless you have the Allow Origin header (ditto for > Icecast/Shoutcast mode). An option for generating that one header sounds acceptable. But what you wrote is hard to understand, hard to configure and error prone. ___ mpd-devel mailing list mpd-devel@musicpd.org http://mailman.blarg.de/listinfo/mpd-devel
Re: [mpd-devel] PATCH: Add support for configuration specified optional headers in HTTP output plugins
> How is this change related to your patch description? > > -# encoder "vorbis"# optional, vorbis or lame > +# encoder "vorbis"# optional, vorbis or lame or > any encoder (use mpd --verbose to get a list) It's not. The comment is wrong, and is 2 lines above my change. I fixed it since I was here, can revert it, but I'm not going to make a patch for this one (false) line. > > Still no documentation, and still no reason why this feature is > useful. I don't like it at all, and the commit message needs to > convince me. Actually, it's useful to clients but not only. In my use case, MPD is on a headless server. Without it, it's not possible to receive this stream in Javascript context due to Cross Origin protection in browsers. That prevents using HTTPD plugin as a audio source for a web page, unless you have the Allow Origin header (ditto for Icecast/Shoutcast mode). The usual workaround, is to serve the music library out of MPD via the same port as the webserver, but it's bad because: 1) It duplicates the effort (what is the point of a music daemon if we have to use another software doing 80% of the same work ?) 2) No synchronization of "meta action". If I create a playlist in MPD it's not in the second software so I have to redo it. 3) Need to redo in Javascript what MPD does very well (like crossfading, replay gain, mixramp etc...) 4) Some feature will never be doable in Javascript/HTML, like input streaming from any sources. Also, the feature can be used for reverse proxy mode, identifying the source MPD when numerous are being used. Cheers, Cyril ___ mpd-devel mailing list mpd-devel@musicpd.org http://mailman.blarg.de/listinfo/mpd-devel
Re: [mpd-devel] systemd and "zeroconf: No global port, disabling zeroconf"
Thanks Max,
Thus, MPD currently does not enable zeroconf when socket activation is
> used, because MPD does not know what port to announce.
>
I understand this now.
> systemd would activate the socket on behalf of the service because you
> told it to. It is optional, and you enabled it ("systemctl enable
> mpd.socket" instead of "systemctl enable mpd.service").
>
So to make zeroconf works under systemd, I need to "systemctl disable
mpd.socket" then "systemctl enable mpd.service" and "systemctl restart
mpd.service". Zeroconf works again.
--
--
Regards,
Kim-man "Punky" Tse
* Open Source Embedded Solutions and Systems
- Voyage Linux (http://linux.voyage.hk)
- Voyage MPD (http://linux.voyage.hk/voyage-mpd)
- Voyage MuBox (http://mubox.voyage.hk)
* Voyage Store (http://store.voyage.hk)
___
mpd-devel mailing list
mpd-devel@musicpd.org
http://mailman.blarg.de/listinfo/mpd-devel
Re: [mpd-devel] systemd and "zeroconf: No global port, disabling zeroconf"
On Wed, Dec 03, 2014 at 20:25:55 +0100, Max Kellermann wrote: > But combining socket activation with zeroconf would be a rather > pointless exercise. You would only be able to find MPD after you > already connected to MPD. Ah, indeed, that is a chicken-and-egg. I guess documentation would be the best fix here then. --Ben ___ mpd-devel mailing list mpd-devel@musicpd.org http://mailman.blarg.de/listinfo/mpd-devel
Re: [mpd-devel] PATCH: Add support for configuration specified optional headers in HTTP output plugins
On 2014/12/03 22:00, X Ryl wrote: > Please find attached a new patch, with your other remarks fixed. How is this change related to your patch description? -# encoder "vorbis"# optional, vorbis or lame +# encoder "vorbis"# optional, vorbis or lame or any encoder (use mpd --verbose to get a list) This is the very first thing I see in your patch file. Still no documentation, and still no reason why this feature is useful. I don't like it at all, and the commit message needs to convince me. ___ mpd-devel mailing list mpd-devel@musicpd.org http://mailman.blarg.de/listinfo/mpd-devel
Re: [mpd-devel] PATCH: Add support for configuration specified optional headers in HTTP output plugins
Ok. My intention was not to start a flamewar, but to contribute back a feature that's missing in the daemon. Actually you criticize that there was no documentation, but I do include the documentation, since I've added it in the first file (and by the way, I fixed the wrong comments in the file)! All the added members in classes have their doxygen documentation too. The "space" change you've spot is used to align vertically the documentation improvement I've added in the conf example file. I'll remove it, but it'll be likely ugly due to the new option name length that's 1 char larger than the previous ones... For verification, I see no code in the current code base for testing the other parameters from the HTTPOutput plugin, if there was some, I would have appended mine here. I'm probably wrong, so please spot where I should improve the testing code. I see test_ files in Test folder, but none of them test the tokenizer. Please find attached a new patch, with your other remarks fixed. Cheers, Cyril 2014-12-03 21:23 GMT+01:00 Max Kellermann : > On 2014/12/03 17:00, X Ryl wrote: >> I don't see a single whitespace change in this patch. > > I do. Proof: > > -# type"httpd" > +# type "httpd" > >> Also, I didn't know what were the project policies, but the content of >> the initial email is summed up in the commit message on top of the >> patch, >> Maybe you want me to send the patch as embedded text in this mail, or >> with attachment is enough ? > > No, I just want NO EXPLANATION in the email. Explanation outside of > the commit message as an indication for a bad commit message. And > indeed it is bad; it contains everything in one huge subject line. > > There is no documentation, and there is no verification. Your > indentation is wrong (spaces instead of tabs). 0001-plugins-httpd-Add-support-for-optional-headers-in-HT.patch Description: Binary data ___ mpd-devel mailing list mpd-devel@musicpd.org http://mailman.blarg.de/listinfo/mpd-devel
Re: [mpd-devel] [PATCHv4 1/2] settings: support service names in MPD_PORT variable
> On 2014/12/03 21:13, Michal Nazarewicz wrote:
>> If $MPD_PORT is not a number, i.e. does not start with a digit,
>> attempt to resolve it using getservbyname, i.e. by reading the
>> /etc/services database.
On Wed, Dec 03 2014, Max Kellermann wrote:
> This now passes the build test, but I don't like how this adds
> unnecessary overhead to the non-TCP build.
>
> Instead of just "atoi(str)", the !ENABLE_TCP build does:
>
>> +if (!*str)
>> +return 0;
>> +if (isdigit(str[0]))
>> +return atoi(str);
>> +
>> +return 0;
>
> Now if you move the #ifdef, this gets eliminated easily.
I take this would be better received:
+static unsigned
+mpd_parse_port(const char *str)
+{
+#ifdef ENABLE_TCP
+ if (*str && !isdigit(*str)) {
+ struct servent *servent = getservbyname(str, "tcp");
+ if (servent)
+ return ntohs(servent->s_port);
+ }
+#endif
+ return atoi(str);
+}
On Wed, Dec 03 2014, Max Kellermann wrote:
> One more thing: I'd like to know what is the point of this patch.
> I mean, what is the practical use of being able to say
> "MPD_PORT=telnet"?
steve suggested authinfo parsing should accept named ports, so for
consistency I made $MPD_PORT accept them as well. I don't care about
this feature to be honest (either in $MPD_PORT or in authinfo file) and
am happy to do whatever you prefer.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--
___
mpd-devel mailing list
mpd-devel@musicpd.org
http://mailman.blarg.de/listinfo/mpd-devel
Re: [mpd-devel] [PATCHv4 1/2] settings: support service names in MPD_PORT variable
On 2014/12/03 21:13, Michal Nazarewicz wrote: > From: Michal Nazarewicz > > If $MPD_PORT is not a number, i.e. does not start with a digit, > attempt to resolve it using getservbyname, i.e. by reading the > /etc/services database. One more thing: I'd like to know what is the point of this patch. I mean, what is the practical use of being able to say "MPD_PORT=telnet"? ___ mpd-devel mailing list mpd-devel@musicpd.org http://mailman.blarg.de/listinfo/mpd-devel
Re: [mpd-devel] [PATCHv4 1/2] settings: support service names in MPD_PORT variable
On 2014/12/03 21:13, Michal Nazarewicz wrote: > From: Michal Nazarewicz > > If $MPD_PORT is not a number, i.e. does not start with a digit, > attempt to resolve it using getservbyname, i.e. by reading the > /etc/services database. This now passes the build test, but I don't like how this adds unnecessary overhead to the non-TCP build. Instead of just "atoi(str)", the !ENABLE_TCP build does: > + if (!*str) > + return 0; > + if (isdigit(str[0])) > + return atoi(str); > + > + return 0; Now if you move the #ifdef, this gets eliminated easily. Further improvement (of existing code): remove the whole MPD_PORT parser if !ENABLE_TCP. ___ mpd-devel mailing list mpd-devel@musicpd.org http://mailman.blarg.de/listinfo/mpd-devel
Re: [mpd-devel] PATCH: Add support for configuration specified optional headers in HTTP output plugins
On 2014/12/03 17:00, X Ryl wrote: > I don't see a single whitespace change in this patch. I do. Proof: -# type"httpd" +# type "httpd" > Also, I didn't know what were the project policies, but the content of > the initial email is summed up in the commit message on top of the > patch, > Maybe you want me to send the patch as embedded text in this mail, or > with attachment is enough ? No, I just want NO EXPLANATION in the email. Explanation outside of the commit message as an indication for a bad commit message. And indeed it is bad; it contains everything in one huge subject line. There is no documentation, and there is no verification. Your indentation is wrong (spaces instead of tabs). ___ mpd-devel mailing list mpd-devel@musicpd.org http://mailman.blarg.de/listinfo/mpd-devel
Re: [mpd-devel] PATCH: Add support for configuration specified optional headers in HTTP output plugins
Hi, I'm sorry, but I don't see a single whitespace change in this patch. Also, I didn't know what were the project policies, but the content of the initial email is summed up in the commit message on top of the patch, Maybe you want me to send the patch as embedded text in this mail, or with attachment is enough ? Regards, Cyril 2014-12-02 21:40 GMT+01:00 Max Kellermann : > On 2014/12/02 14:05, X Ryl wrote: >> As the subject says, this patch adds support for optional headers for >> the HTTP output plugins. These headers are required for allowing cross >> origin resource sharing (for one, but it can be used in reverse proxy >> mode to identify the server sources). >> This makes possible to receiving the HTTP stream asynchronously in >> Javascript, and as such to lower the latency when playing the stream >> in a web browser / javascript based audio player. >> >> It also fix a bug with escaped chars not processed as described in the >> config file's tokenizer. > > I find it too difficult to read your patch, due to many pointless > whitespace changes. Please strip those out. > > Oh, and a patch submission should not need an explanation email. If > you need email text, then your commit message is too short. ___ mpd-devel mailing list mpd-devel@musicpd.org http://mailman.blarg.de/listinfo/mpd-devel
[mpd-devel] [PATCHv4 1/2] settings: support service names in MPD_PORT variable
From: Michal Nazarewicz
If $MPD_PORT is not a number, i.e. does not start with a digit,
attempt to resolve it using getservbyname, i.e. by reading the
/etc/services database.
---
src/settings.c | 33 -
1 file changed, 32 insertions(+), 1 deletion(-)
diff --git a/src/settings.c b/src/settings.c
index f2799df..f153452 100644
--- a/src/settings.c
+++ b/src/settings.c
@@ -30,9 +30,20 @@
#include "config.h"
#include
+#include
#include
#include
+#ifdef ENABLE_TCP
+# ifdef WIN32
+#include
+# else
+#include
+#include
+#include
+# endif
+#endif
+
struct mpd_settings {
char *host;
@@ -100,6 +111,26 @@ mpd_check_host(const char *host, char **password_r)
}
/**
+ * Parse port number (which can be a service name).
+ */
+static unsigned
+mpd_parse_port(const char *str)
+{
+ if (!*str)
+ return 0;
+ if (isdigit(str[0]))
+ return atoi(str);
+
+#ifdef ENABLE_TCP
+ struct servent *servent = getservbyname(str, "tcp");
+ if (servent)
+ return ntohs(servent->s_port);
+#endif
+
+ return 0;
+}
+
+/**
* Parses the port specification. If not specified (0), it attempts
* to load it from the environment variable MPD_PORT.
*/
@@ -109,7 +140,7 @@ mpd_check_port(unsigned port)
if (port == 0) {
const char *env_port = getenv("MPD_PORT");
if (env_port != NULL)
- port = atoi(env_port);
+ port = mpd_parse_port(env_port);
}
return port;
--
2.2.0.rc0.207.ga3a616c
___
mpd-devel mailing list
mpd-devel@musicpd.org
http://mailman.blarg.de/listinfo/mpd-devel
[mpd-devel] [PATCHv4 2/2] settings: add support for reading ~/.authinfo file
From: Michal Nazarewicz
Instead of storing password in MPD_HOST environment variable (which
is passed around everywhere) allow saving password in an ~/.authinfo
file. This is especially useful if MPD is listening on default
host:port, i.e. localhost:6600, in which case all one needs to do is
to put line like
machine localhost port 6600 password some-password
to make MPD clients use “some-password” when connecting to it.
---
src/fd_util.c | 23
src/fd_util.h | 8
src/settings.c | 117 +
3 files changed, 148 insertions(+)
diff --git a/src/fd_util.c b/src/fd_util.c
index 09373e3..3a21ce5 100644
--- a/src/fd_util.c
+++ b/src/fd_util.c
@@ -38,6 +38,8 @@
#include
#include
#include
+#include
+#include
#ifdef WIN32
#include
@@ -117,3 +119,24 @@ socket_cloexec_nonblock(int domain, int type, int protocol)
return fd;
}
+
+FILE *
+mpd_fopen(const char *path)
+{
+ int fd = -1;
+
+#ifdef O_CLOEXEC
+ fd = open(path, O_RDONLY | O_CLOEXEC);
+ if (fd < 0 && errno != EINVAL)
+ return NULL;
+#endif
+
+ if (fd < 0) {
+ fd = open(path, O_RDONLY);
+ if (fd == -1)
+ return NULL;
+ fd_set_cloexec(fd, true);
+ }
+
+ return fdopen(fd, "r");
+}
diff --git a/src/fd_util.h b/src/fd_util.h
index f0c13c9..858896d 100644
--- a/src/fd_util.h
+++ b/src/fd_util.h
@@ -36,6 +36,8 @@
#ifndef FD_UTIL_H
#define FD_UTIL_H
+#include
+
/**
* Wrapper for socket(), which sets the CLOEXEC and the NONBLOCK flag
* (atomically if supported by the OS).
@@ -43,4 +45,10 @@
int
socket_cloexec_nonblock(int domain, int type, int protocol);
+/**
+ * Opens FILE for reading (mode="r") setting O_CLOEXEC flag.
+ */
+FILE *
+mpd_fopen(const char *path);
+
#endif
diff --git a/src/settings.c b/src/settings.c
index f153452..5a27dc7 100644
--- a/src/settings.c
+++ b/src/settings.c
@@ -29,10 +29,13 @@
#include
#include "config.h"
+#include "fd_util.h"
+
#include
#include
#include
#include
+#include
#ifdef ENABLE_TCP
# ifdef WIN32
@@ -146,6 +149,115 @@ mpd_check_port(unsigned port)
return port;
}
+#ifdef ENABLE_TCP
+
+static char *
+mpd_parse_authinfo_line(const char *host, unsigned port, char *line)
+{
+ /* If port is zero (e.g. if socket is used), allow authinfo lines w/o
+* a port. */
+ bool got_host = false, got_port = !port;
+ char *pwd = NULL, *saveptr, *kw, *val;
+
+ while ((kw = strtok_r(line, " \t\n", &saveptr))) {
+ val = strtok_r(NULL, " \t\n", &saveptr);
+ /* Ignore line if there's a keyword w/o value. */
+ if (!val)
+ return NULL;
+ line = NULL;
+
+ if (!strcmp(kw, "machine")) {
+ /* Ignore line if hosts differ. */
+ if (strcmp(val, host))
+ return NULL;
+ got_host = true;
+
+ } else if (!strcmp(kw, "port")) {
+ /* Ignore line if ports differ. */
+ if (!port || mpd_parse_port(val) != port)
+ return NULL;
+ got_port = true;
+
+ } else if (!strcmp(kw, "password")) {
+ pwd = val;
+
+ } else {
+ /* Unknown keyword present, ignore line. */
+ return NULL;
+ }
+ }
+
+ return got_host && got_port ? pwd : NULL;
+}
+
+static char *
+mpd_parse_authinfo(const char *host, unsigned port, FILE *fd)
+{
+ char line[1024], *pwd = NULL;
+
+ do {
+ if (!fgets(line, sizeof line, fd)) {
+ return NULL;
+ }
+
+ if (!strchr(line, '\n')) {
+ /* Discard partial lines. TODO: handle lines of
+* arbitrary length */
+ while (fgets(line, sizeof line, fd) &&
+ !strchr(line, '\n')) { }
+ } else {
+ pwd = mpd_parse_authinfo_line(host, port, line);
+ }
+ } while (!pwd);
+
+ return strdup(pwd);
+}
+
+static char *
+mpd_read_authinfo_password(const char *host, unsigned port)
+{
+ static const char *const filenames[] = {
+ /* Code assumes the first entry is the longest. */
+ ".authinfo", ".netrc"
+ };
+
+ char *str = getenv("HOME");
+ if (!str) {
+ return NULL;
+ }
+
+ size_t i = strlen(str);
+ char *path = malloc(i + strlen(filenames[0]) + 2);
+ memcpy(path, str, i);
+ path[i] = '/';
+ str = path + i + 1;
+
+ char *pwd = NULL;
+ for (i = 0; !pwd && i < sizeof filenames / sizeof *filenames; ++i) {
+ strcpy(str, filenames[i]);
+
Re: [mpd-devel] systemd and "zeroconf: No global port, disabling zeroconf"
On 2014/12/03 16:49, Ben Boeckel wrote: > On Wed, Dec 03, 2014 at 08:17:39 +0100, Max Kellermann wrote: > > Thus, MPD currently does not enable zeroconf when socket activation is > > used, because MPD does not know what port to announce. > > Would getsockname(2) help? Yes, that could be used to find out, under certain circumstances. But combining socket activation with zeroconf would be a rather pointless exercise. You would only be able to find MPD after you already connected to MPD. ___ mpd-devel mailing list mpd-devel@musicpd.org http://mailman.blarg.de/listinfo/mpd-devel
Re: [mpd-devel] systemd and "zeroconf: No global port, disabling zeroconf"
On Wed, Dec 03, 2014 at 08:17:39 +0100, Max Kellermann wrote: > Thus, MPD currently does not enable zeroconf when socket activation is > used, because MPD does not know what port to announce. Would getsockname(2) help? --Ben ___ mpd-devel mailing list mpd-devel@musicpd.org http://mailman.blarg.de/listinfo/mpd-devel
