[CF-Devel] Questions about plugin code

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

I started to review the patch "Partial plugin pointer patch" from
sourceforge. Not all proposed changes are correct IMHO but after
checking the plugin code (server/plugin.c and plugin*/*.c) I found quiet
a lot other bugs.

For some of these bugs I have questions about how to fix them:

 - CFSetSkillExperience() calls HOOK_ADDEXP(..., uint64 *exp, ...) but
   CFWAddExp() expects "int *exp".

   I'd like to change the plugin interface to use "uint64 *". My
   question: is it acceptable to change the plugin interface? That is:
   do other plugin modules (outside the server repository) exist or is
   it sufficient to ensure that the server interface and the plugin
   implementations match?

 - CFForgetSpell() calls HOOK_FORGETSPELL(..., object *spell) but
   CFWDoForgetSpell expects "char *spell".

   In this case, I changed the Python api to use the spell name instead
   of the spell object. (That is, use PyArg_ParseTuple(..."ls"...)
   instead ..."li"...)

 - CFCreateObjectInside() contains some code I do not understand but
   assume it to be very broken:

        if (strcmp(query_name(myob),txt))
        {
            for(i=strlen(query_name(myob)); i>0;i--)
            {
  /*(1)*/       tmpname = (char *)(malloc(i+1));
                strncpy(tmpname,query_name(myob),i);
                tmpname[i] = 0x0;
  /*(2)*/       if (!strcmp(query_name(myob),tmpname))
                {
  /*(1)*/           free_string(tmpname);
                    tmpname = txt + i;
                    GCFP.Value[0] = (void *)(myob);
                    GCFP.Value[1] = (void *)(tmpname);
                    /*test = create_artifact(myob,tmpname); */
                    CFR = (PlugHooks[HOOK_CREATEARTIFACT])(&GCFP);
                    test = (object *)(CFR->Value[0]);
                    PyFreeMemory( CFR );
  /*(3)*/
                }
                else
                {
  /*(1)*/           free_string(tmpname);
                };
            };
        };

   This code

    - (1) uses malloc() but calls free_string()

    - (2) compares query_name(myob) with a prefix of itself

    - (3) is missing a "break" (I'm not not sure about this one because
      I do not understand what this code is intended to do but it feels
      not right for me to call HOOK_CREATEARTIFACT multiple times.)

    - calls malloc() repeatedly in the loop. This is not necessary
      because the first allocation of malloc(strlen(...)+1) is
      sufficient for all following uses.

    - calls query_name(myob) multiple times. Since query_cost() is quite
      expensive, there should be only one call like
      "char *myob_name = query_name(myob)"

    - does not check if malloc() was successful

   Said that, my question is: can someone tell me what this code is
   intended to do?

   Note that CFCreateObject() contains basically the same code.

 - CFSetCha() (and similar functions for other stats) do

     if (value>30) return NULL;
     if (value<-30) return NULL;

   to restrict the new value to valid values. I would like to change
   these statements to

     if (value>MAX_STAT) return NULL;
     if (value<MIN_STAT) return NULL;

   but MIN_STAT is not -30. Therefore: is my fix broken or is the value
   -30 incorrect?

 - CFWDumpObject() allocates a buffer with malloc() and returns it to
   the plugin implementation. This buffer is never freed.

   Should I use a static buffer or free it in the plugin implementation?

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