[CF-Devel] I have a few patches for you

[crossfire-devel at archives.real-time.com]

  Took a quick look - a few comments:

Munteanu Eduard Gabriel wrote:

>
     
      2) settings.diff
     
     >
     
      - now we can modify the behavior of stats rolling, by changing the 
     
     >
     
      minimum and maximum sum of stats that can be rolled during character 
     
     >
     
      creation, which can be achieved by editing the settings file 
     
     >
     
      (min_initial_stats_sum, max_initial_stats_sum).
     
     
  Seems reasonable.  The comment about it could possibly take a large number of 
rolls to get valid stats is reasonable, but in most cases, shouldn't be an issue.

  The problem probably really occurs if the min stats is set to some value close 
to the max threshold - if it is set to 133 for example, that means the average 
stat roll for all the stats would 19.

  That could take a very long time - I think it may be reasonable that if the 
stat total is above some percentage of maximum allowed stats (90%?) to at least 
print a warning.

  Also, instead of hardcoding the value '7' in the load_settings for min values, 
you should use the NUM_STATS value instead.  Unlikely new stats will be added, 
but you never no.

>
     
      - when the server starts and loads settings, if the settings file is 
     
     >
     
      misconfigured, the default behavior is to die by calling LOG(logLevel, 
     
     >
     
      ..., ...), where logLevel is llevFatal by default (but can be changed by 
     
     >
     
      modifying INIT_LOAD_SETTINGS_LOGLEVEL).
     
     
  I don't think I like that as a default.

  The real case here is that different errors have different priorities.  As 
mentioned above, something like the stat average being too high might be a 
warning, something like invalid values might be a warning (because it just falls 
back to built in defaults), but there can certainly be cases where something 
should really be fatal.

  One real concern I have here is the potential for values currently in the 
settings files to be obsoleted.  This would mean a player using their old 
settings file would find that there server no longer starts - this would be very 
obnoxious for those servers that automatically restart on failure.

  I think the default should be to log the errors, but not actually halt.

>
     
      - janitorial work
     
     >
     
      3) sighup_reload_settings.diff
     
     >
     
      - this is highly experimental: the server now behaves differently when 
     
     >
     
      it recieves a SIGHUP signal, by reloading the settings file (if there 
     
     >
     
      are errors in the settings file, the server does _not_ die; here, 
     
     >
     
      logLevel is llevError)
     
     
  Don't like that as is - sighup right now is the only signal one can send to 
the server to get it to shut down cleanly.  Changing that behaviour is likely to 
cause confusion.

  Using another signal for the reload of data (sigusr1 perhaps) is more 
reasonable.  However, I'm not sure how much I like the idea - certainly, it is 
convenient to be able to reload those values.  However, at the same time, I'm 
fearful that people will use it and get strange side effects and file bugs 
against them, of which there may be no way to fix them - if you make a function 
available, people then expect to be able to use it.

  If sufficient disclaimers are placed perhaps it is then reasonable.

>
     
     
     >
     
      I hope my future patches will include both client and server side 
     
     >
     
      changes that will allow, if the server administrator permits it, the 
     
     >
     
      clients to manually setup their character's stats.
     
     >
     
      Please tell me if you liked this patch.
     
     
  Other than those minor notes, it looks good.


_______________________________________________
crossfire-devel mailing list
     
     crossfire-devel at lists.real-time.com
     
     
     https://mailman.real-time.com/mailman/listinfo/crossfire-devel