You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Steve Huston <sh...@riverace.com> on 2008/03/20 21:00:14 UTC
Question on preferences for platform diffs in C++ code
Hi,
I'm working on porting the qpid C++ code to Windows. I've got changes
for 1 file to get it to build, cpp/src/qpid/Url.cpp. This required
Windows-specific code:
Index: Url.cpp
===================================================================
--- Url.cpp (revision 638488)
+++ Url.cpp (working copy)
@@ -24,10 +24,17 @@
#include <boost/spirit.hpp>
#include <boost/spirit/actor.hpp>
-#include <sys/ioctl.h>
-#include <net/if.h>
-#include <unistd.h>
-#include <arpa/inet.h>
+#ifdef _WINDOWS
+# include <ws2tcpip.h>
+# ifndef HOST_NAME_MAX
+# define HOST_NAME_MAX 256
+# endif
+#else
+# include <sys/ioctl.h>
+# include <net/if.h>
+# include <unistd.h>
+# include <arpa/inet.h>
+#endif /* _WINDOWS */
#include <stdio.h>
#include <errno.h>
@@ -53,6 +60,33 @@
Url Url::getIpAddressesUrl(uint16_t port) {
Url url;
+
+#ifdef _WINDOWS
+ enum { MAX_URL_INTERFACES = 100 };
+ SOCKET s = socket (PF_INET, SOCK_STREAM, 0);
+ if (s != INVALID_SOCKET) {
+ INTERFACE_INFO interfaces[MAX_URL_INTERFACES];
+ DWORD filledBytes = 0;
+ WSAIoctl (s,
+ SIO_GET_INTERFACE_LIST,
+ 0,
+ 0,
+ interfaces,
+ sizeof (interfaces),
+ &filledBytes,
+ 0,
+ 0);
+ unsigned int interfaceCount = filledBytes / sizeof
(INTERFACE_INFO);
+ for (unsigned int i = 0; i < interfaceCount; ++i) {
+ if (interfaces[i].iiFlags & IFF_UP) {
+ string
addr(inet_ntoa(interfaces[i].iiAddress.AddressIn.sin_add
r));
+ if (addr != LOCALHOST)
+ url.push_back(TcpAddress(addr, port));
+ }
+ }
+ closesocket (s);
+ }
+#else
int s = socket (PF_INET, SOCK_STREAM, 0);
for (int i=1;;i++) {
struct ifreq ifr;
@@ -68,6 +102,8 @@
url.push_back(TcpAddress(addr, port));
}
close (s);
+#endif /* _WINDOWS */
+
return url;
}
I'm continuing on with compile errors and the next one is from
cpp/src/qpid/sys/SystemInfo.cpp. It has a sysconf() call with a
comment that the sysconf value is Linux-specific. This leads me to two
questions for you all:
1. Is the above diff along the lines of what's acceptable? Any major
snafus I should fix before continuing? When this type of thing comes
up, do you prefer it be done with conditional compiles like this, or
refactor things so they're easily split? For example, create a
class/function that returns a vector of strings with ip addresses, and
have Url call it then build the url.
2. Some stuff, such as the SystemInfo.cpp sysconf() call, could be set
up with autoconf-discovered information. Although autoconf is used,
was/is it your intention to take care of all the platform specifics
with it? This won't help with Windows, but I imagine it will help if
there are other configurable ports attempted in the future.
Thanks,
-Steve
Re: Question on preferences for platform diffs in C++ code
Posted by Carl Trieloff <cc...@redhat.com>.
in-line
Steve Huston wrote:
> Hi,
>
> I'm working on porting the qpid C++ code to Windows. I've got changes
> for 1 file to get it to build, cpp/src/qpid/Url.cpp. This required
> Windows-specific code:
>
> Index: Url.cpp
> ===================================================================
> --- Url.cpp (revision 638488)
> +++ Url.cpp (working copy)
> @@ -24,10 +24,17 @@
> #include <boost/spirit.hpp>
> #include <boost/spirit/actor.hpp>
>
> -#include <sys/ioctl.h>
> -#include <net/if.h>
> -#include <unistd.h>
> -#include <arpa/inet.h>
> +#ifdef _WINDOWS
> +# include <ws2tcpip.h>
> +# ifndef HOST_NAME_MAX
> +# define HOST_NAME_MAX 256
> +# endif
> +#else
> +# include <sys/ioctl.h>
> +# include <net/if.h>
> +# include <unistd.h>
> +# include <arpa/inet.h>
> +#endif /* _WINDOWS */
> #include <stdio.h>
> #include <errno.h>
>
> @@ -53,6 +60,33 @@
>
> Url Url::getIpAddressesUrl(uint16_t port) {
> Url url;
> +
> +#ifdef _WINDOWS
> + enum { MAX_URL_INTERFACES = 100 };
> + SOCKET s = socket (PF_INET, SOCK_STREAM, 0);
> + if (s != INVALID_SOCKET) {
> + INTERFACE_INFO interfaces[MAX_URL_INTERFACES];
> + DWORD filledBytes = 0;
> + WSAIoctl (s,
> + SIO_GET_INTERFACE_LIST,
> + 0,
> + 0,
> + interfaces,
> + sizeof (interfaces),
> + &filledBytes,
> + 0,
> + 0);
> + unsigned int interfaceCount = filledBytes / sizeof
> (INTERFACE_INFO);
> + for (unsigned int i = 0; i < interfaceCount; ++i) {
> + if (interfaces[i].iiFlags & IFF_UP) {
> + string
> addr(inet_ntoa(interfaces[i].iiAddress.AddressIn.sin_add
> r));
> + if (addr != LOCALHOST)
> + url.push_back(TcpAddress(addr, port));
> + }
> + }
> + closesocket (s);
> + }
> +#else
> int s = socket (PF_INET, SOCK_STREAM, 0);
> for (int i=1;;i++) {
> struct ifreq ifr;
> @@ -68,6 +102,8 @@
> url.push_back(TcpAddress(addr, port));
> }
> close (s);
> +#endif /* _WINDOWS */
> +
> return url;
> }
>
> I'm continuing on with compile errors and the next one is from
> cpp/src/qpid/sys/SystemInfo.cpp. It has a sysconf() call with a
> comment that the sysconf value is Linux-specific. This leads me to two
> questions for you all:
>
> 1. Is the above diff along the lines of what's acceptable? Any major
> snafus I should fix before continuing? When this type of thing comes
> up, do you prefer it be done with conditional compiles like this, or
> refactor things so they're easily split? For example, create a
> class/function that returns a vector of strings with ip addresses, and
> have Url call it then build the url.
>
I think that is fine for URL , when you get to the IO files, I think it
might be better to
create another directory at the same level as posix, epoll and put the
Windows specific
IO in that to avoid #ifdef thought the IO code.
> 2. Some stuff, such as the SystemInfo.cpp sysconf() call, could be set
> up with autoconf-discovered information. Although autoconf is used,
> was/is it your intention to take care of all the platform specifics
> with it? This won't help with Windows, but I imagine it will help if
> there are other configurable ports attempted in the future.
>
>
yes autoconf could work well for that, sysconf is used to set the
initial thread count
for the broker. If you only want to do windows client, you can put a
'assert' and
message in for windows as it will not be used by the client. Then when
/if a broker
port is done it will be found.
Carl.
> Thanks,
> -Steve
>
>
>
Re: Question on preferences for platform diffs in C++ code
Posted by Alan Conway <ac...@redhat.com>.
Steve Huston wrote:
> Hi,
>
> I'm working on porting the qpid C++ code to Windows. I've got changes
> for 1 file to get it to build, cpp/src/qpid/Url.cpp. This required
> Windows-specific code:
>
> Index: Url.cpp
> ===================================================================
> --- Url.cpp (revision 638488)
> +++ Url.cpp (working copy)
> @@ -24,10 +24,17 @@
> #include <boost/spirit.hpp>
> #include <boost/spirit/actor.hpp>
>
> -#include <sys/ioctl.h>
> -#include <net/if.h>
> -#include <unistd.h>
> -#include <arpa/inet.h>
> +#ifdef _WINDOWS
> +# include <ws2tcpip.h>
> +# ifndef HOST_NAME_MAX
> +# define HOST_NAME_MAX 256
> +# endif
> +#else
> +# include <sys/ioctl.h>
> +# include <net/if.h>
> +# include <unistd.h>
> +# include <arpa/inet.h>
> +#endif /* _WINDOWS */
> #include <stdio.h>
> #include <errno.h>
>
> @@ -53,6 +60,33 @@
>
> Url Url::getIpAddressesUrl(uint16_t port) {
> Url url;
> +
> +#ifdef _WINDOWS
> + enum { MAX_URL_INTERFACES = 100 };
> + SOCKET s = socket (PF_INET, SOCK_STREAM, 0);
> + if (s != INVALID_SOCKET) {
> + INTERFACE_INFO interfaces[MAX_URL_INTERFACES];
> + DWORD filledBytes = 0;
> + WSAIoctl (s,
> + SIO_GET_INTERFACE_LIST,
> + 0,
> + 0,
> + interfaces,
> + sizeof (interfaces),
> + &filledBytes,
> + 0,
> + 0);
> + unsigned int interfaceCount = filledBytes / sizeof
> (INTERFACE_INFO);
> + for (unsigned int i = 0; i < interfaceCount; ++i) {
> + if (interfaces[i].iiFlags & IFF_UP) {
> + string
> addr(inet_ntoa(interfaces[i].iiAddress.AddressIn.sin_add
> r));
> + if (addr != LOCALHOST)
> + url.push_back(TcpAddress(addr, port));
> + }
> + }
> + closesocket (s);
> + }
> +#else
> int s = socket (PF_INET, SOCK_STREAM, 0);
> for (int i=1;;i++) {
> struct ifreq ifr;
> @@ -68,6 +102,8 @@
> url.push_back(TcpAddress(addr, port));
> }
> close (s);
> +#endif /* _WINDOWS */
> +
> return url;
> }
>
> I'm continuing on with compile errors and the next one is from
> cpp/src/qpid/sys/SystemInfo.cpp. It has a sysconf() call with a
> comment that the sysconf value is Linux-specific. This leads me to two
> questions for you all:
>
> 1. Is the above diff along the lines of what's acceptable? Any major
> snafus I should fix before continuing? When this type of thing comes
> up, do you prefer it be done with conditional compiles like this, or
> refactor things so they're easily split? For example, create a
> class/function that returns a vector of strings with ip addresses, and
> have Url call it then build the url.
I prefer the latter - separate .cpp files for stuff that's platform dependent.
> 2. Some stuff, such as the SystemInfo.cpp sysconf() call, could be set
> up with autoconf-discovered information. Although autoconf is used,
> was/is it your intention to take care of all the platform specifics
> with it? This won't help with Windows, but I imagine it will help if
> there are other configurable ports attempted in the future.
This is only needed for the broker, you could move it to the broker lib so you
can do a client-only build without tripping over it. The SystemInfo class name
is overly general, I thought at the time it might be a placeholder for other
system stuff but we haven't come up with any so far...