[CF-Devel] Bug in stealing code

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

 >       esrv_send_inventory(op, op);
 >
 > +     op->contr->last_weight = 0;	/* set to incorrect weight, so 
esrv_update_item will send correct value  */
 > +     esrv_update_item(UPD_WEIGHT, op, op);Andreas Kirschbaum wrote:
>
     
      Mark Wedel wrote:
     
     >
     
     
     >>
     
     It might actually make more sense to treat the weight like any
     
     >>
     
     of the other stats (at least player weight), store the last
     
     >>
     
     value we sent, and send again if different instead of having
     
     >>
     
     these explicit calls.
     
     >
     
     
     >
     
     
     >
     
      Here is a diff that should do this.
     
     >
     
     
     >
     
      I declared the function pay_from_container() as "static" since it
     
     >
     
      is called from nowhere else. In addition I removed the parameter
     
     >
     
      "object *op" since it is unused.
     
     >
     
     
     >
     
      The use of the magic value "last_weight==(uint32)-1" was necessary
     
     >
     
      to delay sending "upditem" commands until the player object has
     
     >
     
      been sent ("player" and "item" commands).
     
     >
     
     
     >
     
      To check (and document) implicit assumptions, I use assert()
     
     >
     
      statements. Since these statements are basically not used
     
     >
     
      elsewhere, I'm not sure if I should remove these statements.
     
     
  A few notes:

common/item.c: is the get_weight() function really correct?  I note that it does 
not factor in the number of objects.  For example, if there are 30 arrows, 
get_weight returns the weight of just 1 arrow.  I'm not sure if that is 
intentional, but would perhaps seem confusion.  I do see it matches the WEIGHT() 
macro definition in socket/item.c, so maybe so, but now I'm curious why make 
that a function, compared to moving the macro to define.h or the like.

server/login.c: I'm not sure if the addition of a esrv_update_item is really 
necessary, since it immediately follows the esvr_new_player, and weight 
shouldn't have changed between those two calls.

  The use of assert doesn't follow with the current coding style, and it's usage 
is questionable.

  For the few uses, the question is 'if the assert is false, is this a 
significant error'.  If the answer is yes, then having it as an assert may do no 
good, because the asserts do nothing at all unless NDEBUG is defined, which 
currently doesn't happen anyplace.

  Thus also means that in normal running code, an error isn't even noted at such 
asserts.  So the usefulness in debugging is pretty limited.

  The current methodoly used in the code is to check those expressions manually 
(if ... { .. }) and LOG() in all such cases, with the priority of the log 
message (llevDebug, llWarn, llevError) based on how critical the error is.

  And if it is a critical error, to the extend that trying to continue will just 
cause it to core dump in the next line or two, might as well call abort right 
then and there.

  Other than that, it looks fine.


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