widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

I have replied to some comments.

I have had an idea for more streamlining, so there will be an additional commit to this branch.

Diff comments:

> 
> === modified file 'src/logic/player.cc'
> --- src/logic/player.cc	2018-07-12 04:41:20 +0000
> +++ src/logic/player.cc	2018-07-13 13:41:35 +0000
> @@ -1357,25 +1358,45 @@
>   *
>   * \param fr source stream
>   */
> -void Player::read_statistics(FileRead& fr) {
> +void Player::read_statistics(FileRead& fr, const uint16_t packet_version) {
>  	uint16_t nr_wares = fr.unsigned_16();
> -	uint16_t nr_entries = fr.unsigned_16();
> +	size_t nr_entries = fr.unsigned_16();
> +
> +	// Stats are saved as a single string to reduce number of hard disk write operations
> +	const auto parse_stats = [nr_entries](std::vector<std::vector<uint32_t>>* stats, const DescriptionIndex ware_index, const std::string& stats_string, const std::string& description) {
> +		if (!stats_string.empty()) {
> +			std::vector<std::string> stats_vector;
> +			boost::split(stats_vector, stats_string, boost::is_any_of("|"));
> +			if (stats_vector.size() != nr_entries) {
> +				throw GameDataError("wrong number of %s statistics - expected %" PRIuS " but got %" PRIuS, description.c_str(), nr_entries, stats_vector.size());

If they're not the same size, the savegame is corrupt and loading will fail anyway, because we'll be parsing packets into the wrong spot. Even if the statistics should load something wonky, the next packet will fail (bob packet). I have tested this with an unknown ware now and will promote the log entry to an exception as well, because it just hides the real reason for the problem.

> +			}
> +			for (size_t j = 0; j < nr_entries; ++j) {
> +				stats->at(ware_index)[j] = static_cast<unsigned int>(atoi(stats_vector.at(j).c_str()));
> +			}
> +		} else if (nr_entries > 0) {
> +			throw GameDataError("wrong number of %s statistics - expected %" PRIuS " but got 0", description.c_str(), nr_entries);
> +		}
> +	};
>  
>  	for (uint32_t i = 0; i < current_produced_statistics_.size(); ++i)
>  		ware_productions_[i].resize(nr_entries);
>  
>  	for (uint16_t i = 0; i < nr_wares; ++i) {
> -		std::string name = fr.c_string();
> -		DescriptionIndex idx = egbase().tribes().ware_index(name);
> +		const std::string name = fr.c_string();
> +		const DescriptionIndex idx = egbase().tribes().ware_index(name);
>  		if (!egbase().tribes().ware_exists(idx)) {
>  			log("Player %u statistics: unknown ware name %s", player_number(), name.c_str());
>  			continue;
>  		}
>  
>  		current_produced_statistics_[idx] = fr.unsigned_32();
> -
> -		for (uint32_t j = 0; j < nr_entries; ++j)
> -			ware_productions_[idx][j] = fr.unsigned_32();
> +		if (packet_version < 22) {

The packet version is defined in another class, so that doesn't really work here. We handle things the same way in the MapObjects' load functions.

> +			for (uint32_t j = 0; j < nr_entries; ++j) {
> +				ware_productions_[idx][j] = fr.unsigned_32();
> +			}
> +		} else {
> +			parse_stats(&ware_productions_, idx, fr.c_string(), "produced");
> +		}
>  	}
>  
>  	// Read consumption statistics


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1678987-save-handler/+merge/349096
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1678987-save-handler.