Re: [Musicpd-dev-team] comments on "changes to make mpd work on win32 using gcc"

[Gunnar Roth]

Hello,

Please keep the mailing list on Cc, so all developers can participate.

Done.

commit 6c1d9df29ad22c6ef13528718a3d91fcff34819f
Author: Gunnar Roth <gun...@garmedia.(none)>

That email address is invalid.

The comment  of the editor told me that # lines will not be in the comit,
but that menat probably only # i added myself. i did not edit the mail
adress because i thought this was just informational for me.

The author field is public, and should contain useful information.

I see.

It's "-Werror" (or "--enable-werror" on configure), and enabling it is
a good thing.  Especially because your code was wrong.  A "gsize"
cannot be negative, and thus your check could never succeed.  That's a
good example which proves why "-Werror" is good - if you had enabled
it, you would have noticed your mistake.

Well "--enable-werror  maybe it should be default.
Problem on windows vs 2008  is that i get a lot other warnings, so cannot
make warnings to errors.

No uppercase letters in file names.

Ok its no problem on windows, but i accept this as a policy.

It's not just a "policy", it's to make sure that cross builds work.

Oh yes you are right, did never crosscompile for windows,
so i hadnt this in mind.

True, but you removed  xClose() in git, and i dint want to add it again.
So how should this wrapper be called? or just a #define closesocket(x)
close(x)
for non win32?

xclose() does not have anything to do with this.

Well before i did only change xClose but then it was removed so i changed all the other places

and took as example this code from the git version:
#ifdef WIN32
ret = _pipe(event_pipe, 512, _O_BINARY);
#else
ret = pipe(event_pipe);
#endif

Also: don't code in CPP when you can code in C.  There are many
advantages in inline functions compared to macros.

Of course you are right.
So i made a inline function
inline closesocket(int fd)
{
close(fd);
}
in utils.h ( oscompat.h was removed from git)

Yes, you could wrap g_io_channel_win32_new_socket() /
g_io_channel_unix_new() in some OS dependent inline function, if you
want.

added g_io_channel_new_socket and g_io_channel_new_fd inline func in utils.h

Why yet another #ifdef hack, when you could just replace read() with
recv()?  On POSIX compliant systems, both is allowed, while Windows
only allows recv().

recv() has one more parameter than read(). (note the ,0)

I know that.  So?

I just misunderstood what you meant by replace, because i didnt know that recv is available on linux too.

so i simply removed the read and the ifdefs.

 if (bytesRead > 0)
 return client_input_received(client, bytesRead);
+#ifdef _WIN32
+ else if (bytesRead < 0 && WSAGetLastError() == WSAEINTR)
+#else
 else if (bytesRead < 0 && errno == EINTR)
+#endif
 /* try again later, after select() */
 return 0;
 else


 static gboolean
 client_in_event(G_GNUC_UNUSED GIOChannel *source,
- G_GNUC_UNUSED GIOCondition condition,
+ GIOCondition condition,
 gpointer data)
 {
 struct client *client = data;
 int ret;

 assert(!client_is_expired(client));
+ if (condition != G_IO_IN) {
+ client_set_expired(client);
+ return false;
+ }
+

 client->lastTime = time(NULL);

@@ -542,7 +570,7 @@ client_out_event(G_GNUC_UNUSED GIOChannel *source,
 if (g_queue_is_empty(client->deferred_send)) {
 /* done sending deferred buffers exist: schedule
    read */
- client->source_id = g_io_add_watch(client->channel, G_IO_IN,
+ client->source_id = g_io_add_watch(client->channel,
G_IO_IN|G_IO_NVAL | G_IO_HUP | G_IO_ERR,
    client_in_event, client);
 return false;
 }

@@ -483,13 +506,18 @@ client_out_event(G_GNUC_UNUSED GIOChannel *source,

Care to explain this one?  On Unix, it's enough to listen for G_IO_IN,
since the socket becomes readable when the peer closes the connection.

I dont know what happen on unix.
on windows the connect list run full ,when i was in the debugger  and
stepped.

Trial'n'error is the wrong way to develop software.  Do some research
instead of trying until it succeeds.  This leads to fragile code.

i was debbuging some other stuuf ( better see how the code flows

) but due to debugger stops mpd did not answer to the client, which opened more conections then.

I cannot see code in client_in_event which handles connection close when
no close command is issued before by the client.

See client.c:476:

       else
               /* peer disconnected or I/O error */
               return COMMAND_RETURN_CLOSE;

This is executed when read() returns a non-positive value (except for
EINTR).  This is known to work well on POSIX compliant systems.

So it does not work with glib on windows it seems.

send also has one more parameter than write()

I know that, too.  So?

so same case as recv above

diff --git a/src/cmdline.c b/src/cmdline.c
old mode 100644
new mode 100755
index 48c1e87..ef7dd2c
--- a/src/cmdline.c
+++ b/src/cmdline.c
@@ -32,7 +32,11 @@
 #include <stdio.h>
 #include <stdlib.h>

+#ifdef _WIN32
+#define SYSTEM_CONFIG_FILE_LOCATION "c:/.mpd/mpd.conf"
+#else
 #define SYSTEM_CONFIG_FILE_LOCATION "/etc/mpd.conf"
+#endif
 #define USER_CONFIG_FILE_LOCATION ".mpdconf"

Is that actually a good default location for win32?  This looks like
it violates Microsoft's policies.

the patch uses  HOMEDIR\.mpdconf as default.
SYSTEM_CONFIG_FILE_LOCATION is just for testing.
but better is to use ALLUSERSPROFILE env var

Don't submit "just for testing" code, it won't be merged.

Even better would be to follow the Microsoft policy.  I'm not sure if
ALLUSERSPROFILE is compliant.  Find out, and add references to the
documentation to the patch description and to source code comments.

hmm ok, wxwidgets uses this:
// return the directory with system config files:
   // /etc under Unix, c:\Documents and Settings\All Users\Application Data
   // under Windows, /Library/Preferences for Mac
   virtual wxString GetConfigDir() const = 0;

but to get this,  its not just reading a env var.

( need to call SHGetSpecialFolderLocation with CSIDL_COMMON_APPDATA, feéd item list result to SHGetPathFromIDListto get actual string, call SHGetMalloc to free the item list)

I disabled this code on Win32 for a good reason, which can be read in
the according patch.  Please explain this hunk.

well ok, i thought this is disabled because no uid checks on windows,
but i wanted to be able to test this feature.

Again, don't submit "just for testing" code.

maybe it should be configurable, if the files must belong to the mpd user.

as this is a rather hard restrinction, because no files of me would
belong to this user.

Why not use the same semantics as MPD on Unix?

I meant unix. ( i use mpd on linux actually)

For windows i thought this feature could be open as a start.
because setting access right and creating user is rather complicated
for a user without a setup program.

--- a/src/event_pipe.c
+++ b/src/event_pipe.c
@@ -53,15 +53,16 @@ event_pipe_invoke(enum pipe_event event)
 }

 static gboolean
-main_notify_event(G_GNUC_UNUSED GIOChannel *source,
+main_notify_event(GIOChannel *source,
   G_GNUC_UNUSED GIOCondition condition,
   G_GNUC_UNUSED gpointer data)
 {
 char buffer[256];
- ssize_t r = read(event_pipe[0], buffer, sizeof(buffer));
+ gsize bytes_read = 0;
+ GIOStatus status =
g_io_channel_read_chars(source,buffer,1,&bytes_read,NULL);
 bool events[PIPE_EVENT_MAX];

- if (r < 0 && errno != EAGAIN && errno != EINTR)
+ if (bytes_read < 0 && status != G_IO_STATUS_NORMAL && status !=
G_IO_STATUS_AGAIN)
 g_error("error reading from pipe: %s", strerror(errno));

 g_mutex_lock(event_pipe_mutex);

This hunk is wrong and breaks the Linux build, and should also break
the Windows build.

you are right, but as mentioned did not break here.
i replacethe line by
if(status == G_IO_STATUS_AGAIN)
 return true;
else if (status != G_IO_STATUS_NORMAL )
 g_error("error reading from pipe: %s", strerror(errno));

It looked like it didn't break, because you didn't trigger the error
condition which is checked here, did you?

I emant it did not break the compile. i already said , i see it is wrong code.

@@ -76,10 +77,9 @@ main_notify_event(G_GNUC_UNUSED GIOChannel *source,

 return true;
 }
-
+static GIOChannel *channel;
 void event_pipe_init(void)
 {
- GIOChannel *channel;
 int ret;

 #ifdef WIN32
@@ -89,14 +89,16 @@ void event_pipe_init(void)
 #endif
 if (ret < 0)
 g_error("Couldn't open pipe: %s", strerror(errno));
+#ifdef _WIN32
+ channel = g_io_channel_win32_new_fd(event_pipe[0]);
+#else
 if (set_nonblocking(event_pipe[1]) < 0)
 g_error("Couldn't set non-blocking I/O: %s", strerror(errno));
-
 channel = g_io_channel_unix_new(event_pipe[0]);
+#endif
 event_pipe_source_id = g_io_add_watch(channel, G_IO_IN,
       main_notify_event, NULL);
- g_io_channel_unref(channel);
-
+
 event_pipe_mutex = g_mutex_new();
 }

@@ -105,8 +107,11 @@ void event_pipe_deinit(void)
 g_mutex_free(event_pipe_mutex);

 g_source_remove(event_pipe_source_id);
-
- close(event_pipe[0]);
+
+ g_io_channel_shutdown(channel,false,NULL);
+ g_io_channel_unref(channel);
+ channel = NULL;
+
 close(event_pipe[1]);
 }

The whole event_pipe.c patch requires explanation

ok first this from g lib header
/* Create an IO channel for C runtime (emulated Unix-like) file
* descriptors. After calling g_io_add_watch() on a IO channel
* returned by this function, you shouldn't call read() on the file
* descriptor. This is because adding polling for a file descriptor is
* implemented on Win32 by starting a thread that sits blocked in a
* read() from the file descriptor most of the time. All reads from
* the file descriptor should be done by this internal GLib
* thread. Your code should call only g_io_channel_read_chars().
*/
GIOChannel* g_io_channel_win32_new_fd (gint         fd);

so i have to use  g_io_channel_read_chars()
as i/o would block, i only read 1 byte on incoming event. that will not
block,
because on event at least one byte is there.

I believe you misunderstood non-blocking I/O here.  A read does not
block if at least 1 byte is *available*, not if exactly one byte is
*requested*.  You can request any number of bytes, but read() will
not block and return what's available.

blocks, because it would only return on eof or if 256 bytes can be read.

You didnt understand me. First a pipe creaste with _pipe() is always blocking on windows. So if a byte is written to the pipe , the event callback is called and the 256 byte read call

i also need to use g_io_channel_shutdown()
because else the internal glib thread would not be stopped.

i also think that imho  if giochnnel is used for a fd ( socket)
giochannel functions should be used for read/write/close.
( would also remove closescoket/close recv/read send/write problems) but
i thought that could be done later.

Proper patch welcome.  Don't wait until "later", don't write
half-finished code with improper solutions.

recv send and closesocket only used now

@@ -228,23 +239,6 @@ parseListenConfigParam(G_GNUC_UNUSED unsigned
int port, ConfigParam * param)
 BINDERROR();

 freeaddrinfo(ai);
-#else /* WIN32 */
- const struct hostent *he;
-
- g_debug("binding to address for %s", param->value);
-
- he = gethostbyname(param->value);
- if (he == NULL)
- g_error("can't lookup host \"%s\" at line %i",
- param->value, param->line);
-
- if (he->h_addrtype != AF_INET)
- g_error("IPv4 address expected for host \"%s\" at line %i",
- param->value, param->line);
-
- if (establishListen(AF_INET, he->h_addr, he->h_length) < 0)
- BINDERROR();
-#endif /* !WIN32 */

Please explain.

Well the i removed that because the other code does also compile on windows.

What other code?

You removed the whole host lookup code!  That breaks the
bind_to_address setting.

i removed the win32 version

diff --git a/src/song.h b/src/song.h
old mode 100644
new mode 100755
index a4c72cf..e717c07
--- a/src/song.h
+++ b/src/song.h
@@ -21,7 +21,7 @@

 #include <stddef.h>
 #include <stdbool.h>
-#include <sys/time.h>
+#include <time.h>

 #define SONG_BEGIN "songList begin"
 #define SONG_END "songList end"
diff --git a/src/tag.c b/src/tag.c
old mode 100644
new mode 100755
index e7ce51c..d2f02cb
--- a/src/tag.c
+++ b/src/tag.c
@@ -152,8 +152,11 @@ struct tag *tag_ape_load(const char *file)
 TAG_ITEM_TRACK,
 TAG_ITEM_DATE,
 };
-
+#ifdef _Win32
+ fp = fopen(file, "rb");
+#else
 fp = fopen(file, "r");
+#endif
 if (!fp)
 return NULL;

The "b" flag is portable, and the #ifdef can be removed

ok, i just wasnt sure, because i thought O_BINARY were also portable,
but was not. so i chaged al "rb" to be in #ifdef _WIN32
but will change it back

Another incidence of trial'n'error engineering: instead of looking up
the fopen() documentation, you made wrong assumptions, and made the
code more a mess than it could be.

yes should have read the man page

 #include <pwd.h>
 #endif
-
 #ifdef HAVE_IPV6
+#ifndef _WIN32
 #include <sys/socket.h>
 #endif
+#endif

 #ifdef WIN32
 #include <windows.h>
@@ -56,7 +59,14 @@ void my_usleep(long usec)

 char *parsePath(char *path)
 {
-#ifndef WIN32
+
+#ifdef _WIN32
+ if (path[1] != ':' && path[0] != '/') {
+ g_warning("\"%s\" is not an absolute path", path);
+ return NULL;
+ }
+
+#else

This causes a crash if path is an empty string.

replaced it by if (path && strlen(path) > 0 && path[1] != ':' && path[0]
!= '/') {

Why the "path != NULL" check?  It's illegal to pass NULL here, and it
should crash then.  Maybe add an assertion.

strlen() can be a huge overhead, because strlen() reads the whole
string.  Why not just check "*path != 0", which is essentially the
same as "strlen(path) > 0"?

ok

 int set_nonblocking(int fd)
 {
-#ifdef WIN32
- u_long val = 0;
-
+#ifdef _WIN32
+ u_long val = 1;
+
 return ioctlsocket(fd, FIONBIO, &val) == 0 ? 0 : -1;
 #else
 int ret, flags;

Please explain.

from msdn example code
/ Set the socket I/O mode: In this case FIONBIO
// enables or disables the blocking mode for the
// socket based on the numerical value of iMode.
// If iMode = 0, blocking is enabled;
// If iMode != 0, non-blocking mode is enabled.
u_long iMode = 0;
ioctlsocket(m_socket, FIONBIO, &iMode);

so to enable non-blocking you  set param to be NOT 0

That description belongs to the patch documentation (commit message).
It's not obvious what you were trying to achieve, and it's not obvious
why the old code was wrong.

hmm i thought it was obvious.

diff --git a/src/volume.c b/src/volume.c
old mode 100644
new mode 100755
index 5eb0bd6..ac2add0
--- a/src/volume.c
+++ b/src/volume.c
@@ -40,7 +40,11 @@
 #define SW_VOLUME_STATE                         "sw_volume: "

 const struct audio_output_plugin *default_mixer;
+#ifdef _WIN32
+static int volume_mixer_type = VOLUME_MIXER_TYPE_SOFTWARE; /*
hardware mixer on windows not implemented yet!*/
+#else
 static int volume_mixer_type = VOLUME_MIXER_TYPE_HARDWARE;
+#endif
 static int volume_software_set = 100;

 void volume_finish(void)

This is wrong.  MPD should always attempt to find a hardware mixer.
If none is implemented, MPD should automatically fall back to
software.  This is a non-solution.

Ok i didnt know, but sounds right. fallback just does not seem work.
so to make it work for windows it did that can remove it of course.

Another "trial'n'error" fix: instead of fixing the root cause of the
problem (fallback not working), you changed a global constant.

i didnt know about a fallback, so didnt know behaviour wasnt correct.

removd the change and use conf setting to force software mixer in my mpd.conf

diff --git a/src/zeroconf.c b/src/zeroconf.c
old mode 100644
new mode 100755
index dd9790a..4f40419
--- a/src/zeroconf.c
+++ b/src/zeroconf.c
@@ -22,7 +22,7 @@
 #include "config.h"

 #include <glib.h>
-
+#ifdef HAVE_ZEROCONF
 /* The default service name to publish
  * (overridden by 'zeroconf_name' config parameter)
  */
@@ -71,3 +71,4 @@ void finishZeroconf(void)
 bonjour_finish();
 #endif
 }
+#endif /*HAVE_ZEROCONF*/
\ No newline at end of file

This hunk is wrong, since zeroconf.c shouldn't be compiled at all if
HAVE_ZEROCONF is not defined.

ok didnt know that it is not compiled at all on unix.
as i use vcproj it was easier for me this way,
but i remove this change, no problem.

You didn't know, maybe because you didn't look at Makefile.am:

if HAVE_ZEROCONF
mpd_SOURCES += zeroconf.c
[...]
endif

I suggest you just use autoconf/automake for building, instead of
proprietary Visual Studio files.  This way, you don't have to
duplicate all the code in Makefile.am.

Sorry but one of MY main goals is to be able to use vs2008 for building mpd.

No, that would mean that I have to merge your first huge patch, and
then more patches which undo lots of its changes.  Won't happen, I
will only merge individual clean patches.

I recommend that you use "stgit" for creating smaller patches.  The
MPD wiki has some information on it:

http://mpd.wikia.com/wiki/git

In your case, I'd "uncommit" (stg uncommit) the huge patch, and pop it
off the stack (stg pop), and re-create lots of smaller patches from it
("stg-fold-files-from" might be helpful).  Either throw away the big
patch at the end, or "push" it on top after each new patch, and fix
all the merge conflicts manually.

Puh i didnt manage to understand whats going on, did uncommit and pop,
but somehow i dont get it. as i am not at home for some days,




regards,
gunnar

i will have no time in the next few days to continue. Hopefully can continue on weekend. i attach a git diff file as a reference, there are not many changes left anymore just 747 lines.

mpd_git.diff
Description: Binary data

------------------------------------------------------------------------------
This SF.net email is sponsored by:
SourcForge Community
SourceForge wants to tell your story.
http://p.sf.net/sfu/sf-spreadtheword
_______________________________________________
Musicpd-dev-team mailing list
Musicpd-dev-team@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/musicpd-dev-team