Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[GunChleoc]

Let's get this in now. Thanks for the great change :)

@bunnybot merge
-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/net-boost-asio.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[GunChleoc]

Basically, that loop will give you the elements in the container, so you can do 
all the stuff with them that you would do if it was a simple variable.

I wasn't aware that close() will remove an element from the container. Best 
stay with the iterator after all then - adding a second loop + container would 
make the code unnecessarily complicated.

-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/net-boost-asio.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[Notabilis]

Oh, thanks for the hint! I wasn't aware that this loop allows access to the 
key. It does not solve the removal-problem, however. When I remove the client 
while in the loop the behavior of the rest of the loop will be undefined.
I could add a local std::set and store the to-be-removed ids in there, removing 
them when the loop is done. Do you prefer this to the iterator?

std::set to_remove;
for (auto& client: clients_) {
...
if (error) {
to_remove.add(client.first);
continue;
}
...
}
for (auto id: to_remove)
close(id);
-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/net-boost-asio.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[GunChleoc]

Well, this should work:

for (auto& client: clients_) {
...
ConnectionId id_to_remove = client.first;
...
client.second.deserializer.read_data(buffer, length);
...
}

You don't need an explicit iterator to access the first and second element of 
the pair.
-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/net-boost-asio.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[Notabilis]

Thanks for testing and the review. I fixed all your nits except the iterator. A 
foreach-loop would not work since I need it to get the key of the std::map and 
also since I am modifying the map while running through it. The temporary 
key-variable (for it->first) could be probably removed when I write code like 
"it++->first". But I at least prefer the better readable version with the 
variable.

I will give Bunnybot another chance to run, then this will go in.
-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/net-boost-asio.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[GunChleoc]

Review: Approve

Testing on Windows was successful with LAN + internet game.

Feel free to merge any time - don't forget to set a commit message on top of 
this page first though.
-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/net-boost-asio.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[GunChleoc]

I had another look at the code and added 9 small code style/English language 
nits. Fix as you see fit.

Diff comments:

> 
> === modified file 'src/network/netclient.cc'
> --- src/network/netclient.cc  2017-05-11 10:45:44 +
> +++ src/network/netclient.cc  2017-06-04 16:22:50 +
> @@ -38,42 +44,56 @@
>   return false;
>  
>   uint8_t buffer[512];
> - while (SDLNet_CheckSockets(sockset_, 0) > 0) {
> -
> - const int32_t bytes = SDLNet_TCP_Recv(sock_, buffer, 
> sizeof(buffer));
> - if (bytes <= 0) {
> - // Error while receiving
> - close();
> - return false;
> - }
> -
> - deserializer_.read_data(buffer, bytes);
> + boost::system::error_code ec;
> + size_t length = socket_.read_some(boost::asio::buffer(buffer, 512), ec);

constexpr size_t kNetworkBufferSize = 512;

We have this in nethost too, maybe it can be moved to a header file to avoid 
duplication.

> + if (!ec) {
> + assert(length > 0);
> + assert(length <= 512);
> + // Has read something
> + deserializer_.read_data(buffer, length);
> + }
> +
> + if (ec && ec != boost::asio::error::would_block) {
> + // Connection closed or some error, close the socket
> + log("[NetClient] Error when trying to receive some data: 
> %s.\n", ec.message().c_str());
> + close();
> + return false;
>   }
>   // Get one packet from the deserializer
>   return deserializer_.write_packet(packet);
>  }
>  
>  void NetClient::send(const SendPacket& packet) {
> - if (is_connected()) {
> - SDLNet_TCP_Send(sock_, packet.get_data(), packet.get_size());
> - }
> -}
> -
> -NetClient::NetClient(const std::string& ip_address, const uint16_t port)
> -   : sock_(nullptr), sockset_(nullptr), deserializer_() {
> -
> - IPaddress addr;
> - if (SDLNet_ResolveHost(, ip_address.c_str(), port) != 0) {
> - log("[Client]: Failed to resolve host address %s:%u.\n", 
> ip_address.c_str(), port);
> + if (!is_connected())
>   return;
> +
> + boost::system::error_code ec;
> + size_t written = boost::asio::write(socket_,
> + 
> boost::asio::buffer(packet.get_data(), packet.get_size()), ec);
> + // TODO(Notabilis): This one is an assertion of mine, I am not sure if 
> it will hold
> + // If it doesn't, set the socket to blocking before writing
> + // If it does, remove this comment after build 20
> + assert(ec != boost::asio::error::would_block);
> + assert(written == packet.get_size() || ec);
> + if (ec) {
> + log("[NetClient] Error when trying to send some data: %s.\n", 
> ec.message().c_str());
> + close();
>   }
> - log("[Client]: Trying to connect to %s:%u ... ", ip_address.c_str(), 
> port);
> - sock_ = SDLNet_TCP_Open();
> - if (is_connected()) {
> - log("success\n");
> - sockset_ = SDLNet_AllocSocketSet(1);
> - SDLNet_TCP_AddSocket(sockset_, sock_);
> +}
> +
> +NetClient::NetClient(const NetAddress& host)
> +   : io_service_(), socket_(io_service_), deserializer_() {
> +
> + assert(host.is_valid());
> + const boost::asio::ip::tcp::endpoint destination(host.ip, host.port);
> +
> + log("[NetClient]: Trying to connect to %s:%u ... ", 
> host.ip.to_string().c_str(), host.port);
> + boost::system::error_code ec;
> + socket_.connect(destination, ec);
> + if (!ec && is_connected()) {
> + log("success.\n");
> + socket_.non_blocking(true);
>   } else {
> - log("failed\n");
> + log("failed.\n");
>   }
>  }
> 
> === modified file 'src/network/netclient.h'
> --- src/network/netclient.h   2017-05-11 10:45:44 +
> +++ src/network/netclient.h   2017-06-04 16:22:50 +
> @@ -22,23 +22,23 @@
>  
>  #include 
>  
> -#include 
> -
>  #include "network/network.h"
>  
>  /**
>   * NetClient manages the network connection for a network game in which this 
> computer
>   * participates as a client.
> + * This class only tries to create a single socket, either for IPv4 and IPv6.
> + * Which is used depends on what kind of address is given on call to 
> connect().
>   */
>  class NetClient {
>  public:
> +
>   /**
>* Tries to establish a connection to the given host.
> -  * @param ip_address A hostname or an IPv4 address as string.
> -  * @param port The port to connect to.
> -  * @return A pointer to a connected \c NetClient object or a nullptr if 
> the connection failed.
> +  * \param host The host to connect to.
> +  * \return A pointer to a connected \c NetClient object or an invalid 
> pointer if the connection failed.

Can we make this nullptr instead of an invalid pointer? There's a difference... 
nullptr is well-defined, an invalid 

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[Notabilis]

Today it does not happen for me anymore. Really strange effect, no idea what 
happened there.

So, if no-one has any requests or suggestions left, lets merge this?
-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/net-boost-asio.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[Notabilis]

Pushed a fix for an assert(state_ == OFFLINE) bug which I encountered. The 
problem was that on crash of a hosted game the logic jumped back to the main 
menu without closing the connection to the metaserver. So when the player tries 
to connect again, he already is connected to the metaserver (and not offline).

To reproduce the bug:
- Login to the metaserver
- Cut your wifi cable
- Try to host a game. Should fail and jump you back into the main menu
- Try to connect to the metaserver again (I did this one without network)
- Game crashes due to the assert
-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/net-boost-asio.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[Notabilis]

Thanks for the explanation and fixing Bunnybot! Good to see that it is working 
again.

I did a short test with two network interfaces on Linux. With the current code 
one packet is sent on both interfaces, so the kernel (or so) is already dealing 
with duplicating them. With the apple-code, two packets with the same contents 
are sent on both interfaces. So as long as it doesn't make any problems, I 
would keep the current code.

Removing/Adding the network cable did not change anything but some time later I 
also got the exception. The problem is/was that the metaserver-connection gets 
disconnected, so the client does a DNS lookup to get its IP. When that fails 
(i.e. currently no network) an exception is thrown. In the handler of this 
exception the metaserver is disconnected which crashes since we aren't 
connected anyway (and so have no socket). I pushed a simple fix.

But... is it my system or is trunk (and this branch) currently broken? When 
ingame the view is permanently scrolling down, as if the arrow-down key is 
pressed.
-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/net-boost-asio.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[Klaus Halfmann]

Review: Approve compile, test, small review

One nit inline - no need to fix this.

We should get this in now, otherwise we will never get enouht testing coverage.

Notablilis: try pulling the cable at several stepts in the game,
maybe there is one error left, I am no 100% sure.

FAPP tthis is OK for me.

Diff comments:

> 
> === modified file 'src/network/network_lan_promotion.cc'
> --- src/network/network_lan_promotion.cc  2017-01-25 18:55:59 +
> +++ src/network/network_lan_promotion.cc  2017-06-03 05:35:52 +
> @@ -19,116 +19,349 @@
>  
>  #include "network/network_lan_promotion.h"
>  
> -#include 
> -#include 
> +#ifndef _WIN32
> +#include 
> +#endif
>  
> +#include "base/i18n.h"
>  #include "base/log.h"
> -#include "base/macros.h"
> +#include "base/warning.h"
>  #include "build_info.h"
>  #include "network/constants.h"
>  
> +namespace {
> +
> + /**
> +  * Returns the IP version.
> +  * \param addr The address object to get the IP version for.
> +  * \return Either 4 or 6, depending on the version of the given address.
> +  */
> + int get_ip_version(const boost::asio::ip::address& addr) {
> + assert(!addr.is_unspecified());
> + if (addr.is_v4()) {
> + return 4;
> + } else {
> + assert(addr.is_v6());
> + return 6;
> + }
> + }
> +
> + /**
> +  * Returns the IP version.
> +  * \param version A whatever object to get the IP version for.
> +  * \return Either 4 or 6, depending on the version of the given address.
> +  */
> + int get_ip_version(const boost::asio::ip::udp& version) {
> + if (version == boost::asio::ip::udp::v4()) {
> + return 4;
> + } else {
> + assert(version == boost::asio::ip::udp::v6());
> + return 6;
> + }
> + }
> +}
> +
>  /*** class LanBase ***/
> -
> -LanBase::LanBase() {
> -
> - sock = socket(PF_INET, SOCK_DGRAM, IPPROTO_UDP);  //  open the socket
> -
> - int32_t opt = 1;
> - //  the cast to char* is because microsoft wants it that way
> - setsockopt(sock, SOL_SOCKET, SO_BROADCAST, 
> reinterpret_cast(), sizeof(opt));
> +/**
> + * \internal
> + * In an ideal world, we would use the same code with boost asio for all 
> three operating systems.
> + * Unfortunately, it isn't that easy and we need some platform specific code.
> + * For IPv4, windows needs a special case: For Linux and Apple we have to 
> iterate over all assigned IPv4
> + * addresses (e.g. 192.168.1.68), transform them to broadcast addresses 
> (e.g. 192.168.1.255) and send our
> + * packets to those addresses. For windows, we simply can sent to 
> 255.255.255.255.
> + * For IPv6, Apple requires special handling. On the other two operating 
> systems we can send to the multicast
> + * address ff02::1 (kind of a local broadcast) without specifying over which 
> interface we want to send.
> + * On Apple we have to specify the interface, forcing us to send our message 
> over all interfaces we can find.
> + */
> +LanBase::LanBase(uint16_t port)
> + : io_service(), socket_v4(io_service), socket_v6(io_service) {
>  
>  #ifndef _WIN32
> -
> - //  get a list of all local broadcast addresses
> - struct if_nameindex* ifnames = if_nameindex();
> - struct ifreq ifr;
> -
> - for (int32_t i = 0; ifnames[i].if_index; ++i) {
> - strncpy(ifr.ifr_name, ifnames[i].if_name, IFNAMSIZ);
> -
> - DIAG_OFF("-Wold-style-cast")
> - if (ioctl(sock, SIOCGIFFLAGS, ) < 0)
> - continue;
> -
> - if (!(ifr.ifr_flags & IFF_BROADCAST))
> - continue;
> -
> - if (ioctl(sock, SIOCGIFBRDADDR, ) < 0)
> - continue;
> - DIAG_ON("-Wold-style-cast")
> -
> - broadcast_addresses.push_back(
> -
> reinterpret_cast(_broadaddr)->sin_addr.s_addr);
> - }
> -
> - if_freenameindex(ifnames);
> + // Iterate over all interfaces. If they support IPv4, store the 
> broadcast-address
> + // of the interface and try to start the socket. If they support IPv6, 
> just start
> + // the socket. There is one fixed broadcast-address for IPv6 (well, 
> actually multicast)
> +
> + // Adapted example out of "man getifaddrs"
> + // TODO(Notabilis): I don't like this part. But boost is not able to 
> iterate over
> + // the local IPs and interfaces at this time. If they ever add it, 
> replace this code
> + struct ifaddrs *ifaddr, *ifa;
> + int s, n;
> + char host[NI_MAXHOST];
> + if (getifaddrs() == -1) {
> + perror("getifaddrs");
> + exit(EXIT_FAILURE);
> + }
> + for (ifa = ifaddr, n = 0; ifa != nullptr; ifa = ifa->ifa_next, n++) {
> + if (ifa->ifa_addr == nullptr)
> + continue;
> + 

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[SirVer]

Sorry for that. Bunnybot did not restart after a computer reboot and I did not 
even notice the computer rebooted. Should work now again. 
-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/net-boost-asio.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[Tino]

It seems the Bazaar->Github-Bridge is broken. Both Appveyor und Travis do only 
build on new commits to the Github repo.
-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/net-boost-asio.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[Notabilis]

See the positive side: You were able to compile the branch! ;-)

Seems like some kind of memory corruption though I am not sure where it is 
coming from. The line in GameHost indicates that the server crashed. What were 
you doing when it happened? Could it be that your network went offline (just a 
wild guess)?

Which bug in trunk do you mean? Random crashes or something network related? 
(The asio service does not exist in trunk but a similar problem could probably 
happen there, too.)


An unrelated question to whomever: I am a bit worried that Appveyor isn't 
saying anything. Is it just slow or does that mean that the code is still 
broken?
-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/net-boost-asio.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[Klaus Halfmann]

After playing quite a qhile (> 1hour) I now got a crash:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   widelands   0x00010a7c2d9c 
boost::asio::basic_io_object::get_service() const + 12 (basic_io_object.hpp:214)
1   widelands   0x00010a7c03ef 
boost::asio::basic_socket::is_open() const + 
31 (basic_socket.hpp:338)
2   widelands   0x00010a7bceb9 
NetClient::is_connected() const + 25 (netclient.cc:24)
3   widelands   0x00010a7bd609 
NetClient::send(SendPacket const&) + 41 (netclient.cc:67)
4   widelands   0x00010a7526c1 
InternetGaming::logout(std::__1::basic_string const&) + 161 
(internet_gaming.cc:225)
5   widelands   0x00010a795f1c GameHost::run() + 
9932 (gamehost.cc:749)


So the interal asio service object was broken :-(

should this be our long searched for bug in trunk?
-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/net-boost-asio.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[Notabilis]

Uh uh, my fault. This should have actually been an #ifdef. Is fixed now.
-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/net-boost-asio.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[Klaus Halfmann]

Oops, now this does not compile on OSX:

net-boost-asio/src/network/network_lan_promotion.cc:78:29: error: member 
initializer
  'interface_indices_v6' does not name a non-static data member or base 
class
broadcast_addresses_v4(), interface_indices_v6()

in the .h files this is:

#ifndef __APPLE__
/// Apple forces us to define which interface to broadcast through.
std::set interface_indices_v6;
#endif // __APPLE__

-> need another #ifdef then...




-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/net-boost-asio.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[Tino]

Review: Resubmit

Windows.h includes winsock.h, which clashes with winsock2 and boost asio.
The additional define WIN32_LEAN_AND_MEAN avoids including winsock.h and some 
other headers.
-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/net-boost-asio.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[GunChleoc]

Sounds like a plan - AppVeyor builds are broken though, so we still need to 
make Windows happy.
-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/net-boost-asio.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[Notabilis]

So, code is cleaned up. A review of the new apple specific parts and maybe 
another short test, then it can go?
-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/net-boost-asio.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[Notabilis]

Fine by me, I will try to do so.
Thanks a lot for all the testing, it was a great help!
-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/net-boost-asio.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[GunChleoc]

Don't worry, RL always has to come first. Thanks for all this extensive testing!
-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/net-boost-asio.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[Klaus Halfmann]

Will not have much time the next 3 weeks I guess.
I aon call duty for my job and the weekends are stuffed
with other thing I must care for.

Lets fix the ugly apple code and then lets get this in.
I like having smaller steps and the history of this branch
is long enought ...

sorry
-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/net-boost-asio.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[GunChleoc]

Exactly, codecheck is only run with debug builds. So, it's failing all the 
debug builds, but not the release builds.
-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/net-boost-asio.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[Notabilis]

Travis is only complaining about my coding style, which I have to agree with. 
The current apple-test-code is totally ugly. ;-)
The failing builds are the debug builds, seems like release builds aren't doing 
codechecks.

When I understood it right, the new code worked on your system, or? If you want 
to try with only IPv6 enabled, add "close_socket(_v4);" at the end of 
LanBase::LanBase(uint16_t port).
If the code is working, I will keep it in and clean it up.
-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/net-boost-asio.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[Klaus Halfmann]

Codecheck from travis fails, but for some compilers?
grep '^[/_.a-zA-Z]\+:[0-9]\+:' codecheck.out

/home/travis/build/widelands/widelands/src/network/network_lan_promotion.cc:225:
 Use log() from base/log.h instead of printf.

/home/travis/build/widelands/widelands/src/network/network_lan_promotion.cc:238:
 Comments need to start with a space.

/home/travis/build/widelands/widelands/src/network/network_lan_promotion.cc:261:
 Old C-Style cast. Change to static_cast(var) or similar!

/home/travis/build/widelands/widelands/src/network/network_lan_promotion.cc:264:
 Use log() from base/log.h instead of printf.

/home/travis/build/widelands/widelands/src/network/network_lan_promotion.cc:271:
 Use log() from base/log.h instead of printf.

/home/travis/build/widelands/widelands/src/network/network_lan_promotion.cc:275:
 Comments need to start with a space.

/home/travis/build/widelands/widelands/src/network/network_lan_promotion.cc:280:
 Old C-Style cast. Change to static_cast(var) or similar!

/home/travis/build/widelands/widelands/src/network/network_lan_promotion.cc:283:
 Use log() from base/log.h instead of printf.

+echo 'You have codecheck warnings (see above) Please fix.'

You have codecheck warnings (see above) Please fix.


-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/net-boost-asio.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[Notabilis]

Here, I have something more to play with for you. ;-)

When I understand it right, with the current version you are always getting the 
log message "closing IPv6 socket" when entering the LAN lobby, right? With my 
newest push this message hopefully does not appear any longer. And if it works 
really good, two computers should be able to find their games over the LAN with 
IPv6.

When trying, make sure the __APPLE__ branch of the code is compiled. It should 
also print a comment on console when entering the LAN lobby. But if you don't 
have to better don't look at the code, its very ugly.
-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/net-boost-asio.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[Klaus Halfmann]

Review: Approve code review, compile

Code looks much better, no more 4/6 duplicates.
Will now play a bit, again.
-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/net-boost-asio.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[Notabilis]

Review is dealt with. The code looks prettier now, thanks.

I have to do some more reading about IPv6 and OSX. I fear we will need some 
platform specific code there. I will try to push something for testing in the 
next days.
-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/net-boost-asio.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[Notabilis]

Thanks for the review, I will address most of it.
I commented some of your comments, on two points I disagree with you and would 
prefer the way it currently is. Feel free to insist if you think I am wrong.

Diff comments:

> 
> === modified file 'src/network/netclient.cc'
> --- src/network/netclient.cc  2017-05-11 10:45:44 +
> +++ src/network/netclient.cc  2017-05-26 20:56:15 +
> @@ -17,20 +18,25 @@
>  NetClient::~NetClient() {
>   if (is_connected())
>   close();
> - if (sockset_ != nullptr)
> - SDLNet_FreeSocketSet(sockset_);
>  }
>  
>  bool NetClient::is_connected() const {
> - return sock_ != nullptr;
> + return socket_.is_open();
>  }
>  
>  void NetClient::close() {
>   if (!is_connected())
>   return;
> - SDLNet_TCP_DelSocket(sockset_, sock_);
> - SDLNet_TCP_Close(sock_);
> - sock_ = nullptr;
> + boost::system::error_code ec;

That's not actually resolving it, but getting some internal data. The error 
shouldn't occur at this place as far as I know.
We aren't storing the NetAddress, so it is not available here.

> + boost::asio::ip::tcp::endpoint remote = socket_.remote_endpoint(ec);
> + if (!ec) {
> + log("[NetClient] Closing network socket connected to %s:%i.\n",
> + remote.address().to_string().c_str(), remote.port());
> + } else {
> + log("[NetClient] Closing network socket.\n");
> + }
> + socket_.shutdown(boost::asio::ip::tcp::socket::shutdown_both, ec);
> + socket_.close(ec);
>  }
>  
>  bool NetClient::try_receive(RecvPacket* packet) {
> 
> === modified file 'src/network/nethost.cc'
> --- src/network/nethost.cc2017-05-11 10:45:44 +
> +++ src/network/nethost.cc2017-05-26 20:56:15 +
> @@ -47,68 +53,150 @@
>   // Not connected anyway
>   return;
>   }
> - SDLNet_TCP_DelSocket(sockset_, iter_client->second.socket);
> - SDLNet_TCP_Close(iter_client->second.socket);
> + boost::system::error_code ec;
> + boost::asio::ip::tcp::endpoint remote = 
> iter_client->second.socket.remote_endpoint(ec);
> + if (!ec) {
> + log("[NetHost] Closing network connection to %s:%i.\n",
> + remote.address().to_string().c_str(), remote.port());
> + } else {
> + log("[NetHost] Closing network connection to some client.\n");
> + }
> + 
> iter_client->second.socket.shutdown(boost::asio::ip::tcp::socket::shutdown_both,
>  ec);
> + iter_client->second.socket.close(ec);
>   clients_.erase(iter_client);
>  }
>  
>  bool NetHost::try_accept(ConnectionId* new_id) {
>   if (!is_listening())
>   return false;
> + boost::system::error_code ec;
> + boost::asio::ip::tcp::socket socket(io_service_);

Yes, but we would have to take care of freeing it ourselves. Is there a problem 
with move() ?

> + if (acceptor_v4_.is_open()) {
> + acceptor_v4_.accept(socket, ec);
> + if (ec == boost::asio::error::would_block) {
> + // No client wants to connect
> + // New socket don't need to be closed since it isn't 
> open yet
> + } else if (ec) {
> + // Some other error, close the acceptor
> + log("[NetHost] No longer listening for IPv4 connections 
> due to error: %s.\n",
> + ec.message().c_str());
> + acceptor_v4_.close(ec);
> + } else {
> + log("[NetHost]: Accepting IPv4 connection from %s.\n",
> + 
> socket.remote_endpoint().address().to_string().c_str());
> + }
> + }
> + if (acceptor_v6_.is_open() && !socket.is_open()) {
> + // IPv4 did not get a connection
> + acceptor_v6_.accept(socket, ec);
> + if (ec == boost::asio::error::would_block) {
> + // No client wants to connect
> + } else if (ec) {
> + log("[NetHost] No longer listening for IPv6 connections 
> due to error: %s.\n",
> + ec.message().c_str());
> + acceptor_v6_.close(ec);
> + } else {
> + log("[NetHost]: Accepting IPv6 connection from %s.\n",
> + 
> socket.remote_endpoint().address().to_string().c_str());
> + }
> + }
>  
> - TCPsocket sock = SDLNet_TCP_Accept(svrsock_);
> - // No client wants to connect
> - if (sock == nullptr)
> + if (!socket.is_open()) {
> + // No new connection
>   return false;
> - SDLNet_TCP_AddSocket(sockset_, sock);
> + }
> +
> + socket.non_blocking(true);
> +
>   ConnectionId id = next_id_++;
>   assert(id > 0);
>   assert(clients_.count(id) == 0);
> - clients_.insert(std::make_pair(id, Client(sock)));
> + 

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[GunChleoc]

Re Travis: Yes, that was just Travis unable to get a package. So, it doesn't 
count as an error for our code.
-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/net-boost-asio.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[Klaus Halfmann]

Travis has Problme with just CLANG_VERSION="4.0" BUILD_TYPE="Debug"
all others are ok?

Looks like some transient error, llvm.or had a bad time?

sudo apt-get install -qq --force-yes -y clang-4.0
E: Failed to fetch 
http://llvm.org/apt/trusty/pool/main/l/llvm-toolchain-4.0/libllvm4.0_4.0~svn303688-1~exp1_amd64.deb
  404  Not Found [IP: 151.101.128.204 80]
-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/net-boost-asio.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[Klaus Halfmann]

* Hmm, here is the IPv6 Error:
[LAN] Started an IPv6 socket on UDP port 7394.
...
[LAN] Will broadcast for IPv6.
[LAN] Error when broadcasting on IPv6 socket, closing it: No route to host.
[LAN] Closing an IPv6 socket.

Must use debugger/consult docs what this is about. 
Looks Broadcast adresses in IPv6 need some differetn aproach.

* Testing lcoal network (OSX)
  server <- client
  asio   <- asio game is not visible in the local network browser (but is fast)
but can connect via localhost
  old<- asio game is visible in the local network browser (but slow setting 
this up)
connection works as expected
  asio   <- old  game is visible in the local network browser 
connection works as expected

Now I am puzzeld an will need some debugging to find this.
(And we will need some Linux, OSX, Windows ... LAN Party for the final checks 
:-)

Notabilis please give some hint where to check.

As its is bsiaclly working I would like to see some > 30 min Internet game,
for stabilit, we then vould write seom bugs for our findings.

I will start the code review now, while wating for some victim in the lobby for 
testing :-)

-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/net-boost-asio.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[Klaus Halfmann]

* Hello Notabalis: I will proceed as follows:
 * Try "normal" network games with another instance as host and vice versa
 * Try local network gaming (assuming its IPv4)
 * Find out what wrong with IPv6 on this Mac, I have seen the error code
   (I hope you log it), So i can ivestigate there.
  * Either my Firewall needs tweaking
  * Or IPv6 Broadcasting on Darvin/OSX is different
  * or what else...

* Local Broadcasting
 - Pollutes the local network with unneeded traffic
 + Allows local computers to connect not knowing the server IP
 * it should not make normal network games fail :-)
* I think the Errror after a failed connect is already adressed in some other 
bug/branch
  https://bugs.launchpad.net/widelands/+bug/1690649 but this was merged to trunk
  so maybe your merge fixed it.

Lest go compiling 
-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/net-boost-asio.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[Notabilis]

Review: Resubmit

Thanks a lot for testing! Sorry, somehow missed this yesterday.
- Thanks for fixing the find-broadcast code, it compiles and works on my 
system, too.
- Trunk is merged, luckily without conflicts.
- I went through the boost history, seems like version 1.48 is good enough. A 
lot of fixes but no changes that should be relevant for us.
- Fixed the too early abort when only one broadcast failed. Does your system 
has an IPv6 (address)? Its strange that earlier the code detected IPv6 on your 
system but later on is unable to send using it.
- Whether to remove the broadcast for internet games is up to discussion. It 
was already in the code before I worked on it. Since it allows LAN clients to 
connect to our game I don't see any harm in it.
- Regarding the failed assertion: You connected to the metaserver, clicked 
"back" and connected again without restarting the game? Have there been any 
error messages before the assert? I am not sure whether this is fixed, when it 
happens again let me know.
-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/net-boost-asio.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[GunChleoc]

I ran a LAN game with Windows host and Linux client, no problem.

Will do more testing when the issues raised by Klaus have been addressed.
-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/net-boost-asio.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[Klaus Halfmann]

Review: Needs Fixing compile, test

* I can connect to the metaserver, so networking basically works. 

* With a second try I now died with:

Assertion failed: (state_ == OFFLINE), function login, 
file 
/Users/klaus/develop/widelands-repo/net-boost-asio/src/network/internet_gaming.cc,
 line 118.
But I think this is already adressed in some other fix.

* Hmm, You use 
   d->promoter = new LanGamePromoter() 
with a BroadcastAddr even for an internet game.

And this then fails here:

boost::asio::ip::udp::endpoint 
destination(boost::asio::ip::address::from_string("ff02::1"), port);
socket_v6.send_to(boost::asio::buffer(buf, len), destination, 0, ec);

please fix:
 * no Brodcast for a pure Internet game.
 * in case one of IVP4 _or_ IPV6 broadcast can be used, 
   do not fail but log a warning only.

I will not try a local network game until this is fixed.
I assume it will fail in the same way.

Thanks for your work nonetheless, you may find me in the Lobby between 
18:00 and 20:00 CET some times, so we may further discuss my findings
-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/net-boost-asio.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[Klaus Halfmann]

Trying an itenratgame now gives me:

InternetGaming: Client opened a game with the name BugHasi.
[NetHost]: Opening a listening IPv4 socket on TCP port 7396
[NetHost]: Opening a listening IPv6 socket on TCP port 7396
[LAN] Started an IPv6 socket on UDP port 7395
[LAN] Started an IPv4 socket on UDP port 7395
[LAN] Will broadcast to 192.168.254.255
[LAN] Will broadcast for IPv6
[LAN] Closing an IPv6 socket.
[NetHost]: Closing a listening IPv4 socket
[NetHost]: Closing a listening IPv6 socket
[LAN] Closing an IPv4 socket.

Warning: Failed to use the local network!
Widelands was unable to use the local network. Maybe some other process is 
already running a server on port 7394, 7395 or 7396 or your network setup is 
broken.

Any idea what this is about? 
> netstat -a | fgrep 139 
does not show me any such port. going to debug this now

-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/net-boost-asio into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[GunChleoc]

> I found Asio 1.10.6 / Boost 1.58 as part of the boost library 
> provided by MacPorts (https://www.macports.org/)
> 
> most recent is Asio 1.10.9 / Boost 1.64

>From CMakelists.txt:

find_package(Boost 1.48
  COMPONENTS
unit_test_framework
regex
  REQUIRED
system)

So, we require version 1.48 as our lowest boost version. If we need a
higher version for the ASIO stuff, we will need to change that line.

The lowest Ubuntu version that we support is Trusty (14.04), which
already comes with Boost version 1.54. Travis is also on Boost 1.54

AppVeyor is pulling Boost version 1.63.

We should go over
http://www.boost.org/doc/libs/1_64_0/doc/html/boost_asio/history.html to
see if we need to require a higher Boost version.

-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/net-boost-asio into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[Klaus Halfmann]

OK you use some struct ifaddrs from 
$FreeBSD: src/include/ifaddrs.h,v 1.3.32.1.4.1 2010/06/14 02:09:06 kensmith Exp 
$ 
As OSX is bases on Freebsd no surprise here.
man getifaddr tells me 

 The ifaddrs structure contains at least the following entries:

 struct ifaddrs   *ifa_next; /* Pointer to next struct */
 char *ifa_name; /* Interface name */
 u_int ifa_flags;/* Interface flags */
 struct sockaddr  *ifa_addr; /* Interface address */
 struct sockaddr  *ifa_netmask;  /* Interface netmask */
 struct sockaddr  *ifa_dstaddr;  /* P2P interface destination */
 void *ifa_data; /* Address specific data */

...
Note that as a convenience, ifa_broadaddr is defined by a compiler 
#define directive to be the same as ifa_dstaddr.

On linux this is:

 struct ifaddrs  *ifa_next;/* Next item in list */
 char*ifa_name;/* Name of interface */
 unsigned int ifa_flags;   /* Flags from SIOCGIFFLAGS */
 struct sockaddr *ifa_addr;/* Address of interface */
 struct sockaddr *ifa_netmask; /* Netmask of interface */
 union {
 struct sockaddr *ifu_broadaddr;
  /* Broadcast address of interface */
 struct sockaddr *ifu_dstaddr;
  /* Point-to-point destination address */
 } ifa_ifu;
 #define  ifa_broadaddr ifa_ifu.ifu_broadaddr
 #define  ifa_dstaddr   ifa_ifu.ifu_dstaddr
 void*ifa_data;/* Address-specific data */

No ifa_ifu here, but as of the Note for BSD this should be ifa_broadaddr ...

Ok, this did the trick, will cehck in this chanage and some struct/class 
confusions my compiler complains about.

Notabilis: will you merge with trunk as Gun has commited some major change 
today?



-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/net-boost-asio into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[Klaus Halfmann]

OK checked some things,

I found Asio 1.10.6 / Boost 1.58 as part of the boost library 
provided by MacPorts (https://www.macports.org/)

most recent is Asio 1.10.9 / Boost 1.64

I assume you are on Ubuntu, so I assume you are somehwere in the middle of 
these versions.

Can anybody tell use the windows boost versions we use?
-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/net-boost-asio into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[Klaus Halfmann]

tried to compile this an it failed me with:

net-boost-asio/src/network/network_lan_promotion.cc:90:26: error: no member 
named
  'ifa_ifu' in 'ifaddrs'
s = getnameinfo(ifa->ifa_ifu.ifu_broadaddr, sizeof(struct sockaddr_in),

So Do _you_ use some implementation detail, or do I have to chnange something?

Can you give my some link to what this Boost.Asio is?

Ill as the usal suspects in the internet meanwhile.
-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/net-boost-asio into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[Notabilis]

Seems like its building on Windows (or at least on Appveyor) now. A 
confirmation and a short test would be appreciated, though. Also, building and 
testing on MacOS is still missing.
-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/net-boost-asio into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[Notabilis]

Thanks for the review! I included all your suggestions, even though the "prefer 
IPv6" part is somewhere else in the code.
-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/net-boost-asio into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

[GunChleoc]

I have done a code review anyway. Since I don't know that much about 
networking, it's for code style only.

I have also given it a quick spin and found that IPv4 is preferred over IPv6, I 
think it should be the other way around if possible, since v6 is the more 
modern protocol.

Diff comments:

> 
> === modified file 'src/network/gameclient.cc'
> --- src/network/gameclient.cc 2017-05-11 10:45:44 +
> +++ src/network/gameclient.cc 2017-05-20 19:06:48 +
> @@ -89,17 +88,13 @@
>   std::vector chatmessages;
>  };
>  
> -GameClient::GameClient(const std::string& host,
> -   const uint16_t port,
> -   const std::string& playername,
> -   bool internet)
> +GameClient::GameClient(const NetAddress& host, const std::string& 
> playername, bool internet)
> : d(new GameClientImpl), internet_(internet) {
> - d->net = NetClient::connect(host, port);
> + d->net = NetClient::connect(host);
>   if (!d->net || !d->net->is_connected()) {
>   throw WLWarning(_("Could not establish connection to host"),
> - _("Widelands could not establish a connection 
> to the given "
> -   "address.\n"
> -   "Either no Widelands server was running at 
> the supposed port or\n"
> + _("Widelands could not establish a connection 
> to the given address. "

Note: In general, do not change translatable strings without good reason, 
because it will break all the translations. I do agree with this particular 
change though.

> +   "Either no Widelands server was running at 
> the supposed port or "
> "the server shut down as you tried to 
> connect."));
>   }
>  
> 
> === modified file 'src/network/nethost.cc'
> --- src/network/nethost.cc2017-05-11 10:45:44 +
> +++ src/network/nethost.cc2017-05-20 19:06:48 +
> @@ -47,68 +51,136 @@
>   // Not connected anyway
>   return;
>   }
> - SDLNet_TCP_DelSocket(sockset_, iter_client->second.socket);
> - SDLNet_TCP_Close(iter_client->second.socket);
> + boost::system::error_code ec;
> + 
> iter_client->second.socket.shutdown(boost::asio::ip::tcp::socket::shutdown_both,
>  ec);
> + iter_client->second.socket.close(ec);
>   clients_.erase(iter_client);
>  }
>  
>  bool NetHost::try_accept(ConnectionId* new_id) {
>   if (!is_listening())
>   return false;
> + boost::system::error_code ec;
> + boost::asio::ip::tcp::socket socket(io_service_);
> + if (acceptor_v4_.is_open()) {

I think that we should swap these and prefer IPv6 if we can get it. This will 
give us more testing exposure for IPv6. e.g. my network can handle both, so I'd 
always end up with IPv4 on a LAN connection.

> + acceptor_v4_.accept(socket, ec);
> + if (ec == boost::asio::error::would_block) {
> + // No client wants to connect
> + // New socket don't need to be closed since it isn't 
> open yet
> + } else if (ec) {
> + // Some other error, close the acceptor
> + acceptor_v4_.close(ec);
> + } else {
> + log("[NetHost]: Accepting IPv4 connection from %s\n",
> + 
> socket.remote_endpoint().address().to_string().c_str());
> + }
> + }
> + if (acceptor_v6_.is_open() && !socket.is_open()) {
> + // IPv4 did not get a connection
> + acceptor_v6_.accept(socket, ec);
> + if (ec == boost::asio::error::would_block) {
> + ;
> + } else if (ec) {
> + acceptor_v6_.close(ec);
> + } else {
> + log("[NetHost]: Accepting IPv6 connection from %s\n",
> + 
> socket.remote_endpoint().address().to_string().c_str());
> + }
> + }
>  
> - TCPsocket sock = SDLNet_TCP_Accept(svrsock_);
> - // No client wants to connect
> - if (sock == nullptr)
> + if (!socket.is_open()) {
> + // No new connection
>   return false;
> - SDLNet_TCP_AddSocket(sockset_, sock);
> + }
> +
> + socket.non_blocking(true);
> +
>   ConnectionId id = next_id_++;
>   assert(id > 0);
>   assert(clients_.count(id) == 0);
> - clients_.insert(std::make_pair(id, Client(sock)));
> + clients_.insert(std::make_pair(id, Client(std::move(socket;
>   assert(clients_.count(id) == 1);
>   *new_id = id;
>   return true;
>  }
>  
>  bool NetHost::try_receive(const ConnectionId id, RecvPacket* packet) {
> -
>   // Always read all available data into buffers
>   uint8_t buffer[512];
> - while (SDLNet_CheckSockets(sockset_, 0) > 0) {
> - for (auto& e : clients_) {
> - if