Re: [PATCH 2/7] dpwsockx: Implementation of SPInit
2009/9/1 Ismael Barros <razielmine(a)gmail.com>:
+typedef struct tagDPSP_MSG_HEADER +{ + DWORD size; /* size & 0x000FFFFF, token & 0xFFF00000 */ + SOCKADDR_IN SockAddr; +} DPSP_MSG_HEADER, *LPDPSP_MSG_HEADER; +typedef const DPSP_MSG_HEADER* LPCDPSP_MSG_HEADER; + It seems you're sending this over the network, so you should make sure it's correctly packed, and take the byte order into account.
On Tue, Sep 1, 2009 at 11:00 AM, Henri Verbeet<hverbeet(a)gmail.com> wrote:
2009/9/1 Ismael Barros <razielmine(a)gmail.com>:
+typedef struct tagDPSP_MSG_HEADER +{ + DWORD size; /* size & 0x000FFFFF, token & 0xFFF00000 */ + SOCKADDR_IN SockAddr; +} DPSP_MSG_HEADER, *LPDPSP_MSG_HEADER; +typedef const DPSP_MSG_HEADER* LPCDPSP_MSG_HEADER; + It seems you're sending this over the network, so you should make sure it's correctly packed, and take the byte order into account.
I tested it with a wireshark dissector and against the original implementation, and it seems to work nicely.
2009/9/1 Ismael Barros² <razielmine(a)gmail.com>:
On Tue, Sep 1, 2009 at 11:00 AM, Henri Verbeet<hverbeet(a)gmail.com> wrote:
2009/9/1 Ismael Barros <razielmine(a)gmail.com>:
+typedef struct tagDPSP_MSG_HEADER +{ + DWORD size; /* size & 0x000FFFFF, token & 0xFFF00000 */ + SOCKADDR_IN SockAddr; +} DPSP_MSG_HEADER, *LPDPSP_MSG_HEADER; +typedef const DPSP_MSG_HEADER* LPCDPSP_MSG_HEADER; + It seems you're sending this over the network, so you should make sure it's correctly packed, and take the byte order into account.
I tested it with a wireshark dissector and against the original implementation, and it seems to work nicely.
Probably, but that depends on how the compiler packs the structure, and on the host byte order.
On Tue, Sep 1, 2009 at 12:43 PM, Henri Verbeet<hverbeet(a)gmail.com> wrote:
2009/9/1 Ismael Barros² <razielmine(a)gmail.com>:
Is there any standard way to deal with this cases? Any example in the codebase?
For packing, look for "#include <pshpack1.h>" and "#include <poppack.h>" in some of the headers. For byte ordering look at how htonl etc. are defined in include/winsock.h, but you'll want the opposite since the data uses x86 byte ordering.
I've been looking into iphlpapi/ip.h (just learned bit fields exist...); would this implementation be fine? #include "pshpack1.h" typedef struct tagDPSP_MSG_HEADER { #ifdef WORDS_BIGENDIAN DWORD size:20; DWORD token:12; #else DWORD token:12; DWORD size:20; #endif SOCKADDR_IN SockAddr; } DPSP_MSG_HEADER, *LPDPSP_MSG_HEADER; typedef const DPSP_MSG_HEADER* LPCDPSP_MSG_HEADER; /* DPSP_MSG_HEADER->token */ #define DPSP_MSG_TOKEN_REMOTE 0xFAB #define DPSP_MSG_TOKEN_FORWARDED 0xCAB #define DPSP_MSG_TOKEN_SERVER 0xBAB #include "poppack.h"
2009/9/4 Ismael Barros² <razielmine(a)gmail.com>:
I've been looking into iphlpapi/ip.h (just learned bit fields exist...); would this implementation be fine?
#include "pshpack1.h"
typedef struct tagDPSP_MSG_HEADER { #ifdef WORDS_BIGENDIAN DWORD size:20; DWORD token:12; #else DWORD token:12; DWORD size:20; #endif SOCKADDR_IN SockAddr; } DPSP_MSG_HEADER, *LPDPSP_MSG_HEADER;
Probably not. I'm not sure iphlpapi/ip.h does things right in the first place, it seems to confuse bitfield ordering with machine byte ordering. As a general rule, I think it's best to avoid bitfields for things like writing data to a file or sending data over the network, it's just a pain. Aside from that, using the bitfields in this way messes with the bit ordering, but doesn't change the byte order. For the bitfields in iphlpapi/ip.h that's not an issue because they fit in a single byte. Just store the data in a DWORD with the appropriate masks and shifts, and byte swap that DWORD when reading/writing it.
On Fri, Sep 4, 2009 at 12:12 PM, Henri Verbeet<hverbeet(a)gmail.com> wrote:
2009/9/4 Ismael Barros² <razielmine(a)gmail.com>:
I've been looking into iphlpapi/ip.h (just learned bit fields exist...); would this implementation be fine?
#include "pshpack1.h"
typedef struct tagDPSP_MSG_HEADER { #ifdef WORDS_BIGENDIAN DWORD size:20; DWORD token:12; #else DWORD token:12; DWORD size:20; #endif SOCKADDR_IN SockAddr; } DPSP_MSG_HEADER, *LPDPSP_MSG_HEADER;
Probably not. I'm not sure iphlpapi/ip.h does things right in the first place, it seems to confuse bitfield ordering with machine byte ordering. As a general rule, I think it's best to avoid bitfields for things like writing data to a file or sending data over the network, it's just a pain. Aside from that, using the bitfields in this way messes with the bit ordering, but doesn't change the byte order. For the bitfields in iphlpapi/ip.h that's not an issue because they fit in a single byte. Just store the data in a DWORD with the appropriate masks and shifts, and byte swap that DWORD when reading/writing it.
Here is the next try, how does it look? It seems to behave correctly in some quick tests (byte-swapping parameters to emulate big-endian). Thanks a lot for the feedback, really appreciated. ------------- #ifdef WORDS_BIGENDIAN static inline u_short __dpws_ushort_swap(u_short s) { return (s >> 8) | (s << 8); } static inline u_long __dpws_ulong_swap(u_long l) { return ((u_long)__dpws_ushort_swap((u_short)l) << 16) | __dpws_ushort_swap((u_short)(l >> 16)); } #define dpws_letohl(l) __dpws_ulong_swap(l) #define dpws_letohs(s) __dpws_ushort_swap(s) #define dpws_htolel(l) __dpws_ulong_swap(l) #define dpws_htoles(s) __dpws_ushort_swap(s) #else /* WORDS_BIGENDIAN */ #define dpws_letohl(l) ((u_long)(l)) #define dpws_letohs(s) ((u_short)(s)) #define dpws_htolel(l) ((u_long)(l)) #define dpws_htoles(s) ((u_short)(s)) #endif /* WORDS_BIGENDIAN */ #include "pshpack1.h" typedef struct tagDPSP_MSG_HEADER { DWORD mixed; SOCKADDR_IN SockAddr; } DPSP_MSG_HEADER, *LPDPSP_MSG_HEADER; typedef const DPSP_MSG_HEADER* LPCDPSP_MSG_HEADER; #include "poppack.h" #define DPSP_MSG_TOKEN_REMOTE 0xFAB00000 #define DPSP_MSG_TOKEN_FORWARDED 0xCAB00000 #define DPSP_MSG_TOKEN_SERVER 0xBAB00000 #define DPSP_MSG_MAKE_MIXED(s,t) dpws_htolel((s) | (t)) #define DPSP_MSG_SIZE(m) dpws_letohl((m) & 0x000FFFFF) #define DPSP_MSG_TOKEN(m) ((m) & 0xFFF00000)
2009/9/9 Ismael Barros² <razielmine(a)gmail.com>:
-------------
#ifdef WORDS_BIGENDIAN
static inline u_short __dpws_ushort_swap(u_short s) { return (s >> 8) | (s << 8); } static inline u_long __dpws_ulong_swap(u_long l) { return ((u_long)__dpws_ushort_swap((u_short)l) << 16) | __dpws_ushort_swap((u_short)(l >> 16)); }
#define dpws_letohl(l) __dpws_ulong_swap(l) #define dpws_letohs(s) __dpws_ushort_swap(s) #define dpws_htolel(l) __dpws_ulong_swap(l) #define dpws_htoles(s) __dpws_ushort_swap(s)
#else /* WORDS_BIGENDIAN */
#define dpws_letohl(l) ((u_long)(l)) #define dpws_letohs(s) ((u_short)(s)) #define dpws_htolel(l) ((u_long)(l)) #define dpws_htoles(s) ((u_short)(s))
#endif /* WORDS_BIGENDIAN */
#include "pshpack1.h"
typedef struct tagDPSP_MSG_HEADER { DWORD mixed; SOCKADDR_IN SockAddr; } DPSP_MSG_HEADER, *LPDPSP_MSG_HEADER; typedef const DPSP_MSG_HEADER* LPCDPSP_MSG_HEADER;
#include "poppack.h"
#define DPSP_MSG_TOKEN_REMOTE 0xFAB00000 #define DPSP_MSG_TOKEN_FORWARDED 0xCAB00000 #define DPSP_MSG_TOKEN_SERVER 0xBAB00000
#define DPSP_MSG_MAKE_MIXED(s,t) dpws_htolel((s) | (t)) #define DPSP_MSG_SIZE(m) dpws_letohl((m) & 0x000FFFFF) #define DPSP_MSG_TOKEN(m) ((m) & 0xFFF00000)
I think it should be like this: #define DPSP_MSG_SIZE(m) (dpws_letohl(m) & 0x000FFFFF) #define DPSP_MSG_TOKEN(m) (dpws_letohl(m) & 0xFFF00000) I.e., swap before masking. Perhaps you just want to do this though: #define DPSP_MSG_MAKE_MIXED(s,t) ((s) | (t)) #define DPSP_MSG_SIZE(m) ((m) & 0x000fffff) #define DPSP_MSG_TOKEN(m) ((m) & 0xfff00000) ... m = DPSP_MSG_MAKE_MIXED(s, t); header.mixed = dpws_htolel(m); ... m = dpws_letohl(header.mixed); s = DPSP_MSG_SIZE(m); t = DPSP_MSG_SIZE(m);
On Tuesday 01 September 2009 11:00:37 Henri Verbeet wrote: I realize I'm a bit late for the party, but anyway:
2009/9/1 Ismael Barros <razielmine(a)gmail.com>:
+typedef struct tagDPSP_MSG_HEADER +{ + DWORD size; /* size & 0x000FFFFF, token & 0xFFF00000 */ + SOCKADDR_IN SockAddr; +} DPSP_MSG_HEADER, *LPDPSP_MSG_HEADER; +typedef const DPSP_MSG_HEADER* LPCDPSP_MSG_HEADER; +
It seems you're sending this over the network, so you should make sure it's correctly packed, and take the byte order into account.
Wine runs on architectures with different byte order? I'm not sure if we need to be too concerned here. DirectPlay basically sets up a couple of structs for managing multiplayer games and pushes the structs over the network. It's a pretty common way for Microsoft to define "network protocols". I've even seen pointers in the structures being passed, of course they're completely meaningless to the other peers. Of course the packing needs to match what MS is sending over the network. Cheers, Kai -- Kai Blin WorldForge developer http://www.worldforge.org/ Wine developer http://wiki.winehq.org/KaiBlin Samba team member http://www.samba.org/samba/team/ -- Will code for cotton.
2009/10/10 Kai Blin <kai.blin(a)gmail.com>:
Wine runs on architectures with different byte order? I'm not sure if we need to be too concerned here. DirectPlay basically sets up a couple of structs We have at least some level of support for PowerPC, SPARC, Alpha and ARM. Probably not something to be overly concerned about, no, but I don't think maintaining the difference between network and host byte order is a bad idea in general.
On Saturday 10 October 2009 12:08:32 Henri Verbeet wrote:
2009/10/10 Kai Blin <kai.blin(a)gmail.com>:
Wine runs on architectures with different byte order? I'm not sure if we need to be too concerned here. DirectPlay basically sets up a couple of structs
We have at least some level of support for PowerPC, SPARC, Alpha and ARM. Probably not something to be overly concerned about, no, but I don't think maintaining the difference between network and host byte order is a bad idea in general.
But I highly doubt DirectPlay works on any of those. It's always in intel byteorder. I don't disagree in general, but for DirectPlay, it's just not an issue whatsoever. Cheers, Kai -- Kai Blin WorldForge developer http://www.worldforge.org/ Wine developer http://wiki.winehq.org/KaiBlin Samba team member http://www.samba.org/samba/team/ -- Will code for cotton.
2009/10/10 Kai Blin <kai.blin(a)gmail.com>:
But I highly doubt DirectPlay works on any of those. That would be a bug then, although perhaps not the most important one.
It's always in intel byteorder. That just means the network byte order is little endian.
I agree this isn't the most important thing in a practical sense, but as a principle I think network protocol implementations should maintain that distinction. The same applies for reading/writing data from/to disk or any other external source/destination.
On Saturday 10 October 2009 18:18:30 Henri Verbeet wrote:
2009/10/10 Kai Blin <kai.blin(a)gmail.com>:
But I highly doubt DirectPlay works on any of those.
That would be a bug then, although perhaps not the most important one.
It's always in intel byteorder.
That just means the network byte order is little endian.
I agree this isn't the most important thing in a practical sense, but as a principle I think network protocol implementations should maintain that distinction.
I'd agree if DirectPlay was a networking protocol. It's not. It only works over the network by accident.
The same applies for reading/writing data from/to disk or any other external source/destination.
I just don't see why we need to be more compatible than native in this case, that's all. Cheers, Kai -- Kai Blin WorldForge developer http://www.worldforge.org/ Wine developer http://wiki.winehq.org/KaiBlin Samba team member http://www.samba.org/samba/team/ -- Will code for cotton.
participants (3)
-
Henri Verbeet -
Ismael Barros² -
Kai Blin