[Freeciv-Dev] [patch #1252] cleanup of load_ruleset_game()

[Matthias Pfafferodt]

Follow-up Comment #1, patch #1252 (project freeciv):

version 2:

* add missing change from game.rgame to game.server.rgame


(file #6478)
___

Additional Item Attachment:

File name: version2-cleanup-of-load_ruleset_game.patch Size:28 KB


___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [patch #1252] cleanup of load_ruleset_game()

[Matthias Pfafferodt]

URL:
  

 Summary: cleanup of load_ruleset_game()
 Project: Freeciv
Submitted by: syntron
Submitted on: Dienstag 18.08.2009 um 12:23
Category: general
Priority: 3 - Low
  Status: None
 Privacy: Public
 Assigned to: None
Originator Email: 
 Open/Closed: Open
 Discussion Lock: Any
 Planned Release: None

___

Details:

cleanup of load_ruleset_game()

* define min/max/default values for most ruleset settings
* define the standard values for all ruleset settings in game.h using 'RS_*'
* add secfile_lookup_int_default_min_max() function which uses the min/max
value if the ruleset setting is out of range

(see also patch 1203)

Marko said:

> 3) I'm against secfile_lookup_int_default_min_max() that would directly
exit() the server. We should change ruleset loading failure to act nicely in
general and proposed secfile_lookup_int_default_min_max() goes to opposite
direction. It's even worse than rest of the current ruleset loading code in
that it doesn't send failure messages to client that spawned the server. 

So this changed version uses the min/max value if the ruleset setting is out
of range. This is reported as LOG_ERROR. 
 
How should the functions in registry.c be changed to send a message to the
clients? An extra argument 'char *err_msg' could be added to each of these
functions. It would be NULL if there is no error, else it would contain the
error message which will be send to the client.

If there are default values for all ruleset settings one could start with an
empty (/default/)game.ruleset file ...





___

File Attachments:


---
Date: Dienstag 18.08.2009 um 12:23  Name:
0004-cleanup-of-load_ruleset_game.patch  Size: 29kB   By: syntron



___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [patch #1252] cleanup of load_ruleset_game()

[Marko Lindqvist]

Follow-up Comment #2, patch #1252 (project freeciv):

> How should the functions in registry.c be changed to send a
> message to the clients? An extra argument 'char *err_msg' could
> be added to each of these functions. It would be NULL if there
> is no error, else it would contain the error message which will
> be send to the client.

That would mean this patch only replaces need to check min/max values (at
calling side) with need to check for returned error message.
I would use callback function that handles error messages. In case of ruleset
loading ruleset_error() could be passed as such a callback function.


___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [patch #1252] cleanup of load_ruleset_game()

[Matthias Pfafferodt]

Follow-up Comment #3, patch #1252 (project freeciv):

> That would mean this patch only replaces need to check min/max values (at
calling side) with need to check for returned error message. 
> I would use callback function that handles error messages. In case of
ruleset loading ruleset_error() could be passed as such a callback function.


I added such a callback function for the new function proposed by me. If this
is the right way to go, I will do this for all secfile_*() functions. The
attached patch is only an example. If I code this, it will be a new ticket
...

Furthermore, there will be a function registry_error(error_handle, ...) which
will be the main error handling function within registry.c (code is nearly
done)

(file #6487)
___

Additional Item Attachment:

File name: 0001-add-error-handle-for-registry-functions.patch Size:15 KB


___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [patch #1252] cleanup of load_ruleset_game()

[Marko Lindqvist]

Update of patch #1252 (project freeciv):

 Assigned to:None => cazfi  
 Planned Release:None => 2.2.0  

___

Follow-up Comment #4:

> I added such a callback function for the new function proposed
> by me. If this is the right way to go, I will do this for all
> secfile_*() functions. The attached patch is only an example. If
> I code this, it will be a new ticket ... 

Sorry for late reply.

At least this 'example' error callback for min/max error checking seems fine.
I'm not so sure (not checked related code) if changing other secfile_*()
functions make similar sense. Let's finish this min/default/max values patch
first. If you can make updated and combined patch, I'd like to commit it
before branching S2_2 (which may happen next weekend, if I find time for
it...)

___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [patch #1252] cleanup of load_ruleset_game()

[Matthias Pfafferodt]

Follow-up Comment #5, patch #1252 (project freeciv):

as requested I created an updated patch:

cleanup of load_ruleset_game()

* define min/max/default values for most ruleset settings
* define the standard values for all ruleset settings in
  game.h using 'RS_*'
* add secfile_lookup_int_default_min_max() function which
  uses the min/max value if the ruleset setting is out of
  range
* move definition of ruleset_error to the head of the
  ruleset.c file
* repair definition of SLOW_INVASION
* add error handle for registry functions

Please consider to delete line 302 in game.c:

>  game.info.slow_invasions= RS_DEFAULT_SLOW_INVASIONS;

I will follow up with a patch to use a generalized error
handling function within registry.c

The min/max values are only estimated by myself. Please
check these values.

(file #6550)
___

Additional Item Attachment:

File name: 0001-cleanup-of-load_ruleset_game.patch Size:31 KB


___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [patch #1252] cleanup of load_ruleset_game()

[Matthias Pfafferodt]

Follow-up Comment #6, patch #1252 (project freeciv):

the function ruleset_error has to be changed to send a
notification to all clients ...

___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [patch #1252] cleanup of load_ruleset_game()

[Marko Lindqvist]

Follow-up Comment #7, patch #1252 (project freeciv):

> the function ruleset_error has to be changed to send a
> notification to all clients ...

No. It would be potential security issue if remote clients get error messages
quite internal to server.

___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [patch #1252] cleanup of load_ruleset_game()

[Marko Lindqvist]

Follow-up Comment #8, patch #1252 (project freeciv):

- I guess "xxx_BRIDE_COST" should be "xxx_BRIBE_COST" :-)

In general min/max checks should be only against illegal values, and not
restricting too much what ruleset can do. So I'd set some max values to
ridiculously high values to make sure nobody hits it except by typo.

- MAX_ILLNESS_BASE_FACTOR is only 4x default value
- MAX_ILLNESS_TRADE_INFECTION_PCT is only 4x default value
- MAX_ILLNESS_POLLUTION_PCT is only 4x default value
- MAX_BORDER_SIZE_EFFECT should be higher
- MAX_INCITEBASE_COST is only 5x default value
- MAX_INCITE_IMPROVEMENT_FCT should be higher
- MAX_INCITE_UNIT_FCT should be higher
- MAX_INCITE_TOTAL_FCT is only 5x default value
- MAX_CITIES_MIN_DIST should probably be higher (only one city/island
scenario games)
- MAX_RANSOM_GOLD should be higher
- MAX_UPGRADE_VETERAN_LOSS should be lower. It's certainly a mistake if
someone tries to set this to more than 100 veterancy levels (default ruleset
has 5 and IIRC code supports up to 32)


- secfile_lookup_int_default_min_max() returns minval when val > maxval.
Should be maxval

___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [patch #1252] cleanup of load_ruleset_game()

[Matthias Pfafferodt]

Follow-up Comment #9, patch #1252 (project freeciv):

> - I guess "xxx_BRIDE_COST" should be "xxx_BRIBE_COST" :-)

ups; you are correct ... 

> In general min/max checks should be only against illegal values, and not
> restricting too much what ruleset can do. So I'd set some max values to
> ridiculously high values to make sure nobody hits it except by typo.
>
> - MAX_ILLNESS_BASE_FACTOR is only 4x default value

increased

> - MAX_ILLNESS_TRADE_INFECTION_PCT is only 4x default value
> - MAX_ILLNESS_POLLUTION_PCT is only 4x default value

both are percent values; as the illness is clipped to (0, 100) much higher
percent values are not needed; increased to 500 (factor 10 to the default
value)

> - MAX_BORDER_SIZE_EFFECT should be higher
> - MAX_INCITEBASE_COST is only 5x default value
> - MAX_INCITE_IMPROVEMENT_FCT should be higher
> - MAX_INCITE_UNIT_FCT should be higher
> - MAX_INCITE_TOTAL_FCT is only 5x default value
> - MAX_CITIES_MIN_DIST should probably be higher (only one city/island
> scenario games)
> - MAX_RANSOM_GOLD should be higher

values changed

> - MAX_UPGRADE_VETERAN_LOSS should be lower. It's certainly a mistake if
> someone tries to set this to more than 100 veterancy levels (default
> ruleset has 5 and IIRC code supports up to 32)

in civ1 / civ2 rulesets a value of 255 is used (all veterancy levels lost?);
I now set it to this value ...

> - secfile_lookup_int_default_min_max() returns minval when val > maxval.
> Should be maxval

corrected

The add-on (file #6487) adds a generalized error function.

(file #6589)
___

Additional Item Attachment:

File name: 0001-cleanup-of-load_ruleset_game.patch.diff Size:31 KB


___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [patch #1252] cleanup of load_ruleset_game()

[Marko Lindqvist]

Follow-up Comment #10, patch #1252 (project freeciv):

>> - MAX_UPGRADE_VETERAN_LOSS should be lower. It's certainly
>> a mistake if
>> someone tries to set this to more than 100 veterancy levels
>>  (default ruleset has 5 and IIRC code supports up to 32)

I recalled wrongly. I now checked this and max supported by code is 10. But
of course you should use macro MAX_VET_LEVELS instead of magic 10.

> in civ1 / civ2 rulesets a value of 255 is used (all veterancy
> levels lost?); I now set it to this value ...

civ1 / civ2 rulesets give error messages when loaded because of this. They
should be fixed.

___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [patch #1252] cleanup of load_ruleset_game()

[Matthias Pfafferodt]

Follow-up Comment #11, patch #1252 (project freeciv):

updated patch attached

changes:

* set RS_MAX_UPGRADE_VETERAN_LOSS to  MAX_VET_LEVELS (= 10)
* adapted civ1/civ2 ruleset

(file #6678)
___

Additional Item Attachment:

File name: 0001-cleanup-of-load_ruleset_game.diff Size:33 KB


___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [patch #1252] cleanup of load_ruleset_game()

[Marko Lindqvist]

Update of patch #1252 (project freeciv):

  Status:None => Done   
 Open/Closed:Open => Closed 


___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev