strcmp() fooling me? :o

Discussion in 'C Programming' started by Ron Eggler, Jun 5, 2008.

  1. Ron Eggler

    Ron Eggler Guest

    Hi,

    I got a problem: I have a function that allocates new memory to hold another
    element of my struct array if it's not existing already.
    Code:
    /** My Structures **/
    typedef struct {
            char BusID[6];
            int SeqNr;
            unsigned short Msg;
            } MsgStruct;
    typedef struct{
            unsigned char  app;
            unsigned char  pri;
            unsigned char  act;
            int            seq;
            char   id[6];   
    }io_t;
    /* My function */
    int CheckStruct(MsgStruct **SumStruct_ptr, io_t *CurrentBus)
    {       
      int i=0;
      if (StructCount==0){
    /* allocating memory to hold the 1st element */
      }
      /* loop thru all elements in the struct array and check if
         busID and seq# matches with an existing element
         if not, create new element */
      for (i=0; i < StructCount; i++){
        if((strcmp((*SumStruct_ptr)[i].BusID, CurrentBus->id)==0) &&
          ((*SumStruct_ptr)[i].SeqNr== CurrentBus->seq)){
          return i;
        } /*if we're at the end of the array and nothing matched -  extend
    memory*/ 
        else if( i == StructCount - 1){
          (*SumStruct_ptr) = realloc((*SumStruct_ptr),
          (StructCount+1)*sizeof(MsgStruct ));
          StructCount++;
          strncpy((*SumStruct_ptr)[i+1].BusID, CurrentBus->id, 6);
          (*SumStruct_ptr)[i+1].SeqNr=CurrentBus->seq;
          (*SumStruct_ptr)[i+1].Msg=0;
          prs_log(LOG_NOTICE,"***CheckStruct() - Added new element[%d]: BusID:%s
    seqNr:%d - Count: %d", StructCount-1, (*SumStruct_ptr)[i+1].BusID,
    (*SumStruct_ptr)[i+1].SeqNr, StructCount);
          return i+1;
        }
      }
    }
    MsgStruct *
    ClearElement(MsgStruct ** SumStruct_ptr,
                 int         Position)
    {
        /* Check for invalid 'Position' value */
        prs_log(LOG_NOTICE,"***ClearElement() - Will remove element [%d]: BusID
    %s seqNr:%d - Count: %d", Position,(*SumStruct_ptr)[Position].BusID,
    (*SumStruct_ptr)[Position].SeqNr, StructCount);
        if ( Position>= StructCount )
             return (*SumStruct_ptr);
    
        /* If 'Position' doesn't index the very last element move all the
           ones following it to a position with an index smaller by 1,
           thereby overwriting the element at 'Position' */
    
        if ( Position != StructCount - 1 )
            memmove( SumStruct_ptr + Position, SumStruct_ptr + Position + 1,
                     ( StructCount - Position - 1 ) * sizeof *SumStruct_ptr );
    
        /* The last element isn't used anymore, so give back the memory
           used for it. */
        StructCount--;
        return (*SumStruct_ptr) = realloc( (*SumStruct_ptr), StructCount *
    sizeof *SumStruct_ptr );
    
    
    And this is what happened (copy/paste from my syslog - where prs_log() is
    writing to):
    "CheckStruct() - Added new element[2]: BusID:T9998 seqNr:9 - Count: 3"
    "ClearElement() - Will remove element [0]: BusID:T1111 seqNr:627 - Count: 2"
    "CheckStruct() - Added new element[2]: BusID:T9998 seqNr:9 - Count: 3"
    Now... Why did it create a second entry tor T9998, SEQ#9?
    Shouldn't it have recognized the one that's there already? Held on position
    1 after the element 0 has been cleared or do you see some mistake in my
    clearing function? :eek: I get pretty stuck here...
    Thanks for everybody's hints and suggestions!
    Ron
    --
    weeks of software enineering safe hours of planing ;)
    Ron Eggler, Jun 5, 2008
    #1
    1. Advertising

  2. On Jun 5, 6:18 pm, Ron Eggler <> wrote:

    > Thanks for everybody's hints and suggestions!


    I recommend that you move *SumStruct_ptr into a local variable as
    early as possible. This will make your code much more readable and the
    bug in your code very obvious. Also, it seems quite weird that you
    pass a pointer that is modified by address, and use another related
    variable as a global variable. If you had a single struct containing
    the pointer and the count, the code would be much cleaner and again,
    the error would become obvious.
    christian.bau, Jun 5, 2008
    #2
    1. Advertising

  3. Another recommendation: Your combination of using strncpy to fill an
    array of char and strcmp to compare the contents is dangerous. Either
    your strings are limited to five characters and fit, then use strcpy.
    Or they can be longer than five characters, then the call to strcmp is
    dangerous and can give the wrong result.
    christian.bau, Jun 5, 2008
    #3
  4. Ron Eggler

    Ron Eggler Guest

    Eric Sosman wrote:

    > Ron Eggler wrote:
    >> [...]
    >> MsgStruct *
    >> ClearElement(MsgStruct ** SumStruct_ptr,
    >> int Position)
    >> {
    >> [...]
    >> if ( Position != StructCount - 1 )
    >> memmove( SumStruct_ptr + Position, SumStruct_ptr + Position + 1,
    >> ( StructCount - Position - 1 ) * sizeof *SumStruct_ptr
    >> );

    >
    > You've forgotten one level of indirection:
    >
    > memmove( (*SumStruct_ptr) + Position,
    > (*SumStruct_ptr) + Position + 1,
    > (StructCount - Position - 1) * sizeof **SumStruct_ptr);


    Exactly, great - thank you :)

    --
    weeks of software enineering safe hours of planing ;)
    Ron Eggler, Jun 5, 2008
    #4
  5. Ron Eggler

    Ron Eggler Guest

    christian.bau wrote:

    > On Jun 5, 6:18 pm, Ron Eggler <> wrote:
    >
    >> Thanks for everybody's hints and suggestions!

    >
    > I recommend that you move *SumStruct_ptr into a local variable as
    > early as possible. This will make your code much more readable and the
    > bug in your code very obvious. Also, it seems quite weird that you
    > pass a pointer that is modified by address, and use another related
    > variable as a global variable. If you had a single struct containing
    > the pointer and the count, the code would be much cleaner and again,
    > the error would become obvious.


    Well the reason for me using a double pointer is, because i want to use the
    array elements from my main loop and this specific struct variable (in the
    array) should keep its value from time of allocation until it is destroyed
    again in ClearElement().
    Does this make more sense now?

    --
    weeks of software enineering safe hours of planing ;)
    Ron Eggler, Jun 5, 2008
    #5
  6. Ron Eggler

    Ron Eggler Guest

    christian.bau wrote:

    > Another recommendation: Your combination of using strncpy to fill an
    > array of char and strcmp to compare the contents is dangerous. Either
    > your strings are limited to five characters and fit, then use strcpy.
    > Or they can be longer than five characters, then the call to strcmp is
    > dangerous and can give the wrong result.


    Yes i know this but BusID will always be 5 characters only. It'll always be
    in the form "X1234" so I should be safe with strncpy() and strcmp(). But
    thanks for pointing this out!

    Ron
    --
    weeks of software enineering safe hours of planing ;)
    Ron Eggler, Jun 5, 2008
    #6
  7. Ron Eggler <> writes:
    > christian.bau wrote:
    >> Another recommendation: Your combination of using strncpy to fill an
    >> array of char and strcmp to compare the contents is dangerous. Either
    >> your strings are limited to five characters and fit, then use strcpy.
    >> Or they can be longer than five characters, then the call to strcmp is
    >> dangerous and can give the wrong result.

    >
    > Yes i know this but BusID will always be 5 characters only. It'll always be
    > in the form "X1234" so I should be safe with strncpy() and strcmp(). But
    > thanks for pointing this out!


    strncpy() is rarely what you want. It isn't *really* a string
    function. A string is an arbitrary sequence of characters, terminated
    by and including a trailing '\0'. The data structure that strncpy()
    deals with (at least in its target) is a fixed-length sequence of
    characters ending in zero or more '\0's.

    You probably might as well use strcpy() (or perhaps memcpy()).

    --
    Keith Thompson (The_Other_Keith) <http://www.ghoti.net/~kst>
    Nokia
    "We must do something. This is something. Therefore, we must do this."
    -- Antony Jay and Jonathan Lynn, "Yes Minister"
    Keith Thompson, Jun 5, 2008
    #7
  8. On Jun 5, 7:54 pm, Ron Eggler <> wrote:

    > Well the reason for me using a double pointer is, because i want to use the
    > array elements from my main loop and this specific struct variable (in the
    > array) should keep its value from time of allocation until it is destroyed
    > again in ClearElement().
    > Does this make more sense now?


    No. Your data is based on two values: One is a variable "StructCount",
    one is a malloc'd pointer to that number of MsgStruct structures. This
    two values are logically inseparable, but you are separating them.
    Why? You not only keep them separate, you pass them to your two
    functions in different ways: One as a global variable (apparently),
    and one by passing it's address. Either use two globals, or pass the
    address of StructCount, or put StructCount and the pointer together
    into one struct.
    christian.bau, Jun 5, 2008
    #8
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Ike

    fooling for a repaint

    Ike, Feb 13, 2004, in forum: HTML
    Replies:
    9
    Views:
    2,965
    Beauregard T. Shagnasty
    Feb 14, 2004
  2. Andreas Thiele

    fooling XP with visual styles?

    Andreas Thiele, Jun 29, 2004, in forum: C++
    Replies:
    2
    Views:
    360
    Howard
    Jun 29, 2004
  3. karthikbalaguru

    Fooling C Compiler

    karthikbalaguru, Sep 3, 2007, in forum: C Programming
    Replies:
    5
    Views:
    363
    Keith Thompson
    Sep 3, 2007
  4. Replies:
    2
    Views:
    81
  5. Jorge

    Fooling the debugger.

    Jorge, Sep 25, 2009, in forum: Javascript
    Replies:
    11
    Views:
    205
    Jorge
    Sep 27, 2009
Loading...

Share This Page