Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm building a binary tree.

Example: key AAAA1.ETR, value 1.

I'm reading files with this structure:

DataLength Block-SequenceNummer FLag   Start Data       Ende
4 Bytes    8 Bytes              1 Byte S     Datalength E

Data can be compressed or uncompressed (this is saved in flag). Data contain more messages. Could you have a look and give me hints on what I can do better?

#include <stdint.h> 
#include <stdio.h>
#include <stdlib.h>
#include "zlib.h"
#include <errno.h>
#include <string.h>
#include <dirent.h>
#include <stdio.h>
#include <glib.h>
#include <sys/stat.h>
#define MYMAXSIZE 10000000
#define MAXFILESIZE 6000000
#define OBJNAME "8"
#define MSGTYPE "1"

const uint8_t broadcast[] = { 0x36, 0x33 };
const uint8_t status[] = { 0x34 };
const char dir0[]= "/home/frog/reader/feed.0/";
const char dir1[]= "/home/frog/reader/feed.1/";
const char dir2[]= "/home/frog/reader/feed.2/";
const char dir3[]= "/home/frog/reader/feed.3/";

typedef struct mtmheader_t
{
  unsigned char objName[20]; //8
  unsigned char msgType[3];     //1
}mtmheader_t;

typedef struct analyzer_t
{
  unsigned char *buff;
  size_t s;

}analyzer_t;


typedef struct analyzers_t
{
  analyzer_t anal1;
  analyzer_t anal2;
  analyzer_t anal3;
  unsigned char* seq;
}analyzers_t;

int readFilec(GTree *tree)
{
  FILE * fp = fopen("cfg/InstrumentList_FULL.csv", "rb" );
  char * line = NULL;
  size_t len = 0;
  ssize_t read;

  if (fp == NULL)
  exit(EXIT_FAILURE);
  while ((read = getline(&line, &len, fp)) != -1)
  {
    char *p1;
    int  *p2 = malloc(sizeof (int));
    printf("%s", line);
    p1 = strtok(line, "|");
    *p2 = atoi(strtok(NULL, "|"));
    g_tree_insert(tree, (gpointer)  g_strdup(p1), (gpointer)p2);
    //TrieAdd(&root, strdup(p1), p2);
    printf("-%s%d ", p1, *p2);

  }

  if (line)
  free(line);
  //exit(EXIT_SUCCESS);

}
int readFile( char *name,unsigned char *buffp)
{
  int fileSize = 0;
  //int n, i, j;
  printf("Opening file: \"%s\"\n", name);
  FILE *pFile = fopen(name, "rb");
  if(pFile == NULL)
  {
    printf("error: fopen()");
    return -1;
  }
  fseek(pFile, 0, SEEK_END);
  fileSize = ftell(pFile);
  printf ("%d\n", fileSize);
  rewind(pFile);
  //unsigned char *data = (unsigned char*) calloc(sizeof(unsigned char), fileSize + 20);
  int bytes = fread(buffp, 1, fileSize, pFile);
  if(ferror(pFile))
  {
    printf("error: fread()");
    return -1;
  }
  return bytes;
}

int processMTMHeader(unsigned char *datap, mtmheader_t *h,  unsigned char *endmmsgp)
{
  unsigned char *nextFsp;
  unsigned char *nextRsp;
  //nextRs
  size_t size = endmmsgp - datap;
  //Den Header komplett abarbeiten (bis GS (x1D) oder ETX (x03) kommt)
  //Byte lesen = RS (x1E)
  while ( (datap = (memchr(datap,  0x1E, size))) != NULL)//datap < endmmsgp)//=  strchr(datap,  '\x1E') != NULL )
  {
    datap++;

    //next Fs
    //Byte lesen = FS (x1C)
    nextFsp = memchr(datap, 0x1C,size);
    //Byte lesen = RS (x1C)
    nextRsp = memchr(datap, 0x1E, size);
    size = endmmsgp - nextRsp;
    if ( nextFsp - datap != 1)continue;
    if( *datap ==  0x38 )
    {
      //Objectnamen generieren() (Nur die ersten beiden Felder des Objectnamens verwenden)
      memcpy (h->objName, nextFsp+1, nextRsp - ( nextFsp+1)   );
      h->objName[ nextRsp - ( nextFsp+1)]='\0';
      //Abbruch der Schleife da ich jetzt den Objectnamen habe (Den Header komplett abarbeiten )
      return 1;
    }
    else if ( *datap == 0x31 )
    {
      memcpy (h->msgType, nextFsp+1, nextRsp - ( nextFsp+1)   );
      h->msgType[nextRsp - ( nextFsp+1)]='\0';
      if( (memcmp(h->msgType, broadcast, sizeof broadcast) != 0)  && (memcmp(h->msgType,status, sizeof( status))!=0 )) return 2;
    }
  }
  return -1;
}

int processData(unsigned char *datap, size_t size, GTree* t, analyzers_t *anals)
{
  int headerOk=0;
  unsigned char *nextmtmmsgp;
  unsigned char *endmmsgp   ;
  int * qsp;
  int s;

  //Solange alle MTM durcharbeiten bis keine mehr da sind
  while ( (datap = memchr (datap,  0x02, size))!= NULL )
  {
    //datap++;
    //printf("data\n%s", datap);
    endmmsgp = memchr (datap, 0x03, size);
    mtmheader_t h;
    //Alle HeaderFieldDataPairs abarbeiten
    headerOk = processMTMHeader(datap, &h, endmmsgp );
    int headersSize1 = 10;
    int headersSize2 = 10;
    int headersSize3 = 10;
    if (headerOk == 1)
    {
      //QOT-Status zu dem Objectnamen bestimmen
      if ((qsp = (int *) g_tree_lookup(t, h.objName)) != NULL )
      {
           s =  (endmmsgp - datap )+ 1;
           printf("found: %s %d\n", h.objName, *qsp);
          //Den Header inklusiv eventuell vorhanden Body dem ensprechenden Analyzer zuordnen
          //Dazu den kompletten Block von STX bit ETX auf den MTMcache des Analyzers kopieren
           switch (*qsp)
           {
             case 1:
               memcpy(anals->anal1.buff + anals->anal1.s +  headersSize1, datap, s);
               anals->anal1.s = s + headersSize1 + anals->anal1.s ;
               headersSize1=0;
               break;
             case 2:
               memcpy(anals->anal2.buff + anals->anal2.s + headersSize2, datap, s);
               anals->anal2.s= s + headersSize2 + anals->anal2.s ;
               headersSize2=0;
               break;
             case 3:
               memcpy(anals->anal3.buff + anals->anal1.s + headersSize3, datap, s);
               anals->anal3.s= s + headersSize3 + anals->anal3.s ;
               headersSize3=0;
               break;
        }
      }
      //Den Header inklusiv eventuell vorhanden Body ALLEN Analyzern weitermelden
      //Dazu den kompletten Block von STX bit ETX auf den MTMcache ALLER Analyzers kopieren
      else if (headerOk == 2)
      {
        memcpy(anals->anal1.buff + anals->anal1.s +  headersSize1, datap, s);
        anals->anal1.s = s + headersSize1 + anals->anal1.s ;
        headersSize1=0;

        memcpy(anals->anal2.buff + anals->anal2.s + headersSize2, datap, s);
        anals->anal2.s= s + headersSize2 + anals->anal2.s ;
        headersSize2=0;

        memcpy(anals->anal3.buff + anals->anal1.s + headersSize3, datap, s);
        anals->anal3.s= s + headersSize3 + anals->anal3.s ;
        headersSize3=0;
      }
    }
    datap++;
    printf("Name, type: %s %s\n",(char *) &h.objName,(char *) &h.msgType);
  }
  return -1;
}

gboolean iter_all(gpointer key, gpointer value, gpointer data) {
  int *s =  (int *)value;
  printf("\n%s%d \n", (char *)key, *s);
  return FALSE;
}

int writeFile(char *name,  unsigned char *buff, size_t *size, char *dir )
{
  FILE * pFile;
  char fullName[50];
  //fullName[0]='\0';
  //strcat(fullName, dir);
  //strcat(fullName, name);
  chdir (dir);  
  pFile = fopen ( name, "wb");
  //pFile = fopen (strcat ( strcat( strcat("feed.",anal), "/") , name ), "wb");
  fwrite (buff , sizeof(unsigned char), *size, pFile);
  fclose (pFile);
}


int writeFiles(char *name, analyzers_t *anals )
{
  if (anals->anal1.s > 10)writeFile(name, anals->anal1.buff, &anals->anal1.s, dir1);
  if (anals->anal2.s > 10)writeFile(name, anals->anal2.buff, &anals->anal2.s, dir2);
  if (anals->anal3.s > 10)writeFile(name, anals->anal3.buff, &anals->anal3.s, dir3);
}

int addHeader(size_t *start_size, size_t *end_size,unsigned char *buff, unsigned char *seq)
{
  *(buff+*start_size+ 9) ='S';
  uint32_t s = ((*end_size - *start_size) - 10);

  //size_t s_be = htonl(s);
  memcpy(buff+*start_size, &s, sizeof(s));
  memcpy(buff+4+*start_size, seq, 8 );
  buff[*start_size+8] = '\x00';
  *(buff + *end_size ) = 'E';

}
int key_compare_func (gconstpointer a, gconstpointer b) {
  return g_strcmp0 ((const char*) a, (const char*) b);
}
int main()
{
  analyzers_t anals;
  anals.anal1.buff = malloc(MYMAXSIZE);
  anals.anal2.buff = malloc(MYMAXSIZE);
  anals.anal3.buff = malloc(MYMAXSIZE);
  unsigned char *bytesp = malloc (MAXFILESIZE);
  unsigned char *datap = malloc (MYMAXSIZE);
  //char  dir[]     =   "feed.0/";

  DIR *dirp;
  char currDir[]=".";
  char parentDir[]="..";
  //unsigned char bytesp[MYMAXSIZE];
  GTree* t = g_tree_new_full (key_compare_func, NULL, g_free, NULL);
  readFilec(t);
  g_tree_foreach(t, (GTraverseFunc)iter_all, NULL);
  struct dirent *direntp;
  if ((dirp = opendir(dir0)) == NULL)
  {
    perror("opendir");
    return 0;
  }
  struct stat status;
  chdir(dir0);
  //Filesystem nach Dump-Files des Reader suchen
  int pos=0;
  uint32_t seq;
  size_t size;
  int flag;
  if (datap == NULL)return -1;
  unsigned char *datapor = datap;
  int compressOk=0;
  int loop =0;
  size_t oldSize1=0;
  size_t oldSize2=0;
  size_t oldSize3=0;
  //unsigned char *anal1or = anal.anal1;
  //unsigned char *anal2or = anal.anal2;
  //unsigned char *anal3or = anal.anal3;
  //Solange die BMB abarbeiten bis keine mehr da sind; Startflag ueberpruefen

  while( (direntp = readdir(dirp)) != NULL)
  {
    if(strcmp(direntp->d_name, currDir) == 0)continue;
    if(strcmp(direntp->d_name, parentDir) == 0)continue;
    lstat(direntp->d_name, &status);
    if(S_ISDIR(status.st_mode))continue;

    compressOk=0;
    loop =0;
    pos=0;
    anals.anal1.s = 0;
    anals.anal2.s = 0;
    anals.anal3.s = 0;
    oldSize1 = 0;
    oldSize2 = 0;
    oldSize3 = 0;
    anals.anal1.buff[0]='\0';//anal1or;
    anals.anal2.buff[0]='\0';//anal2or;
    anals.anal3.buff[0]='\0';//al3or;
   readFile(direntp->d_name, bytesp);
   while(bytesp[pos+9] == 'S')
   {
     datap = datapor;

     printf("----------------------------\n");
     printf("|   LOOP %d, POSITION %d   |\n", loop, pos);
     printf("----------------------------\n\n");

     //Datenlaenge des BMB bestimmen
     size =  ntohl(bytesp[pos+0]<<24| bytesp[pos+1]<<16| bytesp[pos+2]<<8| bytesp[pos+3]);
     //Endeflag E ueberpruefen
     if (bytesp[pos+10+ size]!='E')
     {
       pos = pos + 11 + size;
       continue;
     }
     //Block-Sequencenummer speichern
     seq = ntohl(bytesp[pos+4]<<24| bytesp[pos+5]<<16| bytesp[pos+6]<<8| bytesp[pos+7]);
     anals.seq = bytesp +pos+4;
     //Flag lesen
     flag = bytesp[pos+8];
     if( bytesp[pos+8]=='\x01')
     {
       size_t size_uncompress = MYMAXSIZE * sizeof(unsigned char);
       compressOk = uncompress(datap, &size_uncompress, &bytesp[pos+10], size);
       datap[size_uncompress]='\0';
       printf ("%zu %zu :\n", size, size_uncompress) ;
     }
     else
     {
       datap = &bytesp[pos+10];
       //datap[pos+10+ size] ='\0';
     }
     printf("---------------------------- \n");
     printf("|     Message as String:    |\n");
     printf("---------------------------- \n");
     printf("%s\n\n\n", datap);
     processData(datap, size, t, &anals);
     pos = pos + 11 + size;

     if (anals.anal1.s > oldSize1 )
     {
       addHeader( &oldSize1,&anals.anal1.s, anals.anal1.buff, anals.seq );
       anals.anal1.s++;
       oldSize1 = anals.anal1.s;

     }
     if (anals.anal2.s > oldSize2 )
     {
       addHeader( &oldSize2,&anals.anal2.s, anals.anal2.buff, anals.seq );
       anals.anal2.s++;
       oldSize2 = anals.anal2.s;

     }
     if (anals.anal3.s > oldSize3 )
     {
       addHeader( &oldSize3,&anals.anal1.s, anals.anal3.buff, anals.seq );
       anals.anal3.s++;
       oldSize3 = anals.anal1.s;

     }

     loop++;

   }
   writeFiles(direntp->d_name,  &anals);
 }
 free(datap);
 free(bytesp);
 free(anals.anal1.buff);
 free(anals.anal2.buff);
 free(anals.anal3.buff);
}
share|improve this question

1 Answer 1

Just a few tidbits (mainly about error handling):

  • Depending on the architecture, you may want to check whether your mallocs return NULL. It is generally not useful though.
  • In the function writeFile, you never check whether fopen succeded before using fwrite.
  • You could use perror more often to improve your error messages.
  • You made your program return 0 when then function opendir fails. Is it normal? Generally, you return any value, but not 0 on a failure.
  • Just to add some extra pedantry: you use uint8_t and uint32_t. That's generally fine, unless the architecture where your program runs has a char of more than 8 bits (less than 8 bits is not allowed by the standard). While it is extremely uncommon, C is often used for embedded software, so you might want to use uint_least8_t and uint_least32_t instead, whose presence is mandatory (as per the C99 standard).

Unfortunately, all of these are mere details and probably not the comprehensive answer your were expecting. Hope that helps you anyway :)

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.