widelands

Merge lp:~widelands-dev/widelands/bug-1826669-mp-map-b20 into lp:widelands/build20

  1. bug-1826669-mp-map-b20
  2. Merge into build20
Proposed by GunChleoc
Status: Merged
Merged at revision: 9065
Proposed branch: lp:~widelands-dev/widelands/bug-1826669-mp-map-b20
Merge into: lp:widelands/build20
Diff against target: 128 lines (+15/-17)
5 files modified
src/network/gameclient.cc (+2/-6)
src/network/gameclient.h (+3/-1)
src/network/gamehost.cc (+3/-9)
src/network/gamehost.h (+3/-1)
src/network/network.h (+4/-0)
To merge this branch: bzr merge lp:~widelands-dev/widelands/bug-1826669-mp-map-b20
Reviewer Review Type Date Requested Status
Klaus Halfmann compile / test Approve
Review via email: mp+366619@code.launchpad.net

Commit message

Fix crash in NetClient when host changes their mind about a custom map in mid-transfer.

To post a comment you must log in.
Revision history for this message
Klaus Halfmann (klaus-halfmann) wrote :

Admitted, you fix is more elegant.

One nit inline.

Wil still compile and test this

Revision history for this message
Klaus Halfmann (klaus-halfmann) wrote :

Your soultion prevents the crahs but makes a difference:
 - The new Map is not announced at the GUI
 + The new Map appears in the logs
 - No Transfer of any Map happens any longer.

from the code I see no diecrt difference.

eventually ther is some == comparison for file_ which we must consider?

lets check that code, what is done with that file_?

review: Needs Fixing (compile test)
Revision history for this message
Klaus Halfmann (klaus-halfmann) wrote :

I found one:
  file_ = nullptr;
and
  !file_ // this should be save.

in gameclient.cc

I used a Windows Server wit build20_rc1 as host so I could not test gamehost.cc this way.

Now I am out of Ideas...

Revision history for this message
GunChleoc (gunchleoc) wrote :

When I tested I still had a file from the previous attempt with the crashy version. I had to delete that for the transfer to be initiated. Maybe that's it?

Revision history for this message
Klaus Halfmann (klaus-halfmann) wrote :

Yep, found and deleted it, will try again tomorrow.
Playing against WorldSavior and the-X makes me tired.

Thats another Bug for the Review in R21 then ...

Revision history for this message
Klaus Halfmann (klaus-halfmann) wrote :

OK, tested this again, works as intended.

@bunnybot merge

review: Approve (compile / test)
Revision history for this message
GunChleoc (gunchleoc) wrote :

Excellent, thanks for testing!

I'll port the fix to trunk.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/network/gameclient.cc'
2--- src/network/gameclient.cc 2019-02-23 11:00:49 +0000
3+++ src/network/gameclient.cc 2019-04-28 15:08:41 +0000
4@@ -586,8 +586,7 @@
5 d->settings.mapfilename.c_str());
6
7 // New map was set, so we clean up the buffer of a previously requested file
8- if (file_)
9- delete file_;
10+ file_.reset(nullptr);
11 break;
12 }
13
14@@ -637,10 +636,7 @@
15 s.unsigned_8(NETCMD_NEW_FILE_AVAILABLE);
16 d->net->send(s);
17
18- if (file_)
19- delete file_;
20-
21- file_ = new NetTransferFile();
22+ file_.reset(new NetTransferFile());
23 file_->bytes = bytes;
24 file_->filename = path;
25 file_->md5sum = md5;
26
27=== modified file 'src/network/gameclient.h'
28--- src/network/gameclient.h 2019-02-23 11:00:49 +0000
29+++ src/network/gameclient.h 2019-04-28 15:08:41 +0000
30@@ -20,6 +20,8 @@
31 #ifndef WL_NETWORK_GAMECLIENT_H
32 #define WL_NETWORK_GAMECLIENT_H
33
34+#include <memory>
35+
36 #include "chat/chat.h"
37 #include "logic/game_controller.h"
38 #include "logic/game_settings.h"
39@@ -120,7 +122,7 @@
40 bool sendreason = true,
41 bool showmsg = true);
42
43- NetTransferFile* file_;
44+ std::unique_ptr<NetTransferFile> file_;
45 GameClientImpl* d;
46 bool internet_;
47 };
48
49=== modified file 'src/network/gamehost.cc'
50--- src/network/gamehost.cc 2019-02-23 11:00:49 +0000
51+++ src/network/gamehost.cc 2019-04-28 15:08:41 +0000
52@@ -524,7 +524,7 @@
53 hostuser.position = UserSettings::none();
54 hostuser.ready = true;
55 d->settings.users.push_back(hostuser);
56- file_ = nullptr; // Initialize as 0 pointer - unfortunately needed in struct.
57+ file_.reset(nullptr); // Initialize as 0 pointer - unfortunately needed in struct.
58 }
59
60 GameHost::~GameHost() {
61@@ -539,7 +539,6 @@
62 d->net.reset();
63 d->promoter.reset();
64 delete d;
65- delete file_;
66 }
67
68 const std::string& GameHost::get_local_playername() const {
69@@ -1076,9 +1075,7 @@
70 // Read in the file
71 FileRead fr;
72 fr.open(*g_fs, mapfilename);
73- if (file_)
74- delete file_;
75- file_ = new NetTransferFile();
76+ file_.reset(new NetTransferFile());
77 file_->filename = mapfilename;
78 uint32_t leftparts = file_->bytes = fr.get_size();
79 while (leftparts > 0) {
80@@ -1097,10 +1094,7 @@
81 file_->md5sum = md5sum.get_checksum().str();
82 } else {
83 // reset previously offered map / saved game
84- if (file_) {
85- delete file_;
86- file_ = nullptr;
87- }
88+ file_.reset(nullptr);
89 }
90
91 packet.reset();
92
93=== modified file 'src/network/gamehost.h'
94--- src/network/gamehost.h 2019-02-23 11:00:49 +0000
95+++ src/network/gamehost.h 2019-04-28 15:08:41 +0000
96@@ -20,6 +20,8 @@
97 #ifndef WL_NETWORK_GAMEHOST_H
98 #define WL_NETWORK_GAMEHOST_H
99
100+#include <memory>
101+
102 #include "logic/game_controller.h"
103 #include "logic/game_settings.h"
104 #include "logic/player_end_result.h"
105@@ -156,7 +158,7 @@
106 const std::string& arg = "");
107 void reaper();
108
109- NetTransferFile* file_;
110+ std::unique_ptr<NetTransferFile> file_;
111 GameHostImpl* d;
112 bool internet_;
113 bool forced_pause_;
114
115=== modified file 'src/network/network.h'
116--- src/network/network.h 2019-02-23 11:00:49 +0000
117+++ src/network/network.h 2019-04-28 15:08:41 +0000
118@@ -181,6 +181,10 @@
119 };
120
121 struct NetTransferFile {
122+ NetTransferFile() : bytes(0), filename(""), md5sum("") {
123+ }
124+ ~NetTransferFile() = default;
125+
126 uint32_t bytes;
127 std::string filename;
128 std::string md5sum;

Subscribers

People subscribed via source and target branches

to all changes: