Advertisement

Problem with Select

Started by April 13, 2005 12:29 PM
4 comments, last by Hexxagonal 19 years, 9 months ago
Linux (Debian), GCC. I just tried a modified version John's isReadable function (from a different thread, great function thanks for letting us see it) code for a class project I'm doing where I keep sending out packets, and then instead of a while, I used an if on the isReadable (my version) and then if isReadable == 1 then I would recvfrom the data. I did this and I my data was somewhat corrupted as opposed to simply doing a sendto and then blocking with a recvfrom. Any ideas (I'm checking the actual bytes and they are different btw). Here's my function, thanks in advance if you guys have any ideas. I'm stumped.

// isReadable
//   Checks to see if the Socket has something to be read
int isReadable( int sockfd, int usec ) {
  fd_set socketReadSet;
  FD_ZERO(&socketReadSet);
  FD_SET(sockfd, &socketReadSet);

  // Setup timeout
  struct timeval tv;
  if( usec ) {
    tv.tv_sec   = usec / 1000000;
    tv.tv_usec = (usec % 1000000);
  } else {
    tv.tv_sec  = 0;
    tv.tv_usec = 0;
  } // end if



  // Select on our Socket Description
  if( select(sockfd+1,&socketReadSet,0,0,&tv) == -1 ) {
    perror("select");
    exit(1);
  }

  return FD_ISSET(sockfd,&socketReadSet) != 0;

} // end isReadable
Here's is a simplified version of how I do it without my checking etc. Notice I am faking dropping packets, which makes the code a little more complicated. Here it is.
int s2( struct hostent *he, short int port, char* filename, int l, int tpacket )
{
  unsigned char  temp;   // used for reading in characters
  unsigned char* data;   // holds the current data
  unsigned char* buf;
  unsigned char ackdata[9];
  FILE*          pFile;  // File pointer
  int sockfd;
  int numbytes;
  int filelen;
  int totpackets;
  struct sockaddr_in their_addr;
  struct datapack* packets;
  unsigned char* pArray;
  unsigned char* pVar;
  struct timeval tb;
  struct timezone tz;
  int seq_num, timeof;
  char typeofp;

  // Open File
  pFile = fopen( filename , "rb" );
  if( pFile == NULL ) { fprintf( stderr , "Error: Unable to open file.\n" ); return 1; }

  // Get size of file
  fseek(pFile, 0L, SEEK_END);
  filelen = ftell(pFile);
  rewind(pFile);

  // Detemine how many packets are needed
  totpackets = filelen/l;
  if( filelen%l != 0 ) { totpackets++; }

  // Make room for the packets
  packets = malloc( sizeof(struct datapack)*totpackets );
  if( packets == NULL ) { fprintf( stderr , "Unable to make packets" ); exit(1); }
  
  // Creates Packets/Grab Data
  int i = 0;
  int j;
  for( i = 0 ; feof(pFile) == 0 && i < totpackets ; i++ ) {

    data = malloc( l );

    // Grab data from file for this packet
    for( j = 0 ; j < l && feof(pFile) == 0 ; j++ ) {
      fread(&temp, sizeof( unsigned char), 1, pFile );
      data[j] = temp;
    }

    // Create packet
    packets.type = 0x00;
    packets.time = 0;
    packets.seq_num = htonl(i);
    packets.data = data;
    
  } // end for

  // Last packet set as last
  packets[totpackets-1].type = 0x08;

  // Create Socket
  if( (sockfd = socket(AF_INET, SOCK_DGRAM, 0 )) == -1 ) {
    perror("socket");
    exit(1);
  }

  // Fill out their information
  their_addr.sin_family = AF_INET;
  their_addr.sin_port = htons(port);
  their_addr.sin_addr = *((struct in_addr *)he->h_addr);
  memset(&(their_addr.sin_zero), '\0', 8);

  printf("Total Packets: %d\n",totpackets);

  // Scott's Algorithm
  int  avgtime = 5;
  int  maxhold = avgtime*1.2;
  int  minhold = avgtime*0.4;
  int  curwait = avgtime;
  int  done = 0;
  int  totokpacks = 0;
  int* okpacks;

  // Setup Ok Packets Array
  okpacks = malloc( totpackets*sizeof(int) );  // array for whether they came back
  i = 0;
  for( i = 0 ; i < totpackets ; i++ ) { okpacks=0; }  

  // Loop until done
  for( i = 0 ; !done && i < totpackets ; i++ ) {

    // Look for new messages to handle
    while( isReadable( sockfd , curwait ) == 1 ) {

      printf("ACK back\n");

      // Recieve
      addr_len = sizeof(struct sockaddr);
      if((numbytes=recvfrom( sockfd , ackdata , 9 , 0 , (struct sockaddr *)&their_addr , &addr_len )) == -1 ) {
	perror("recvfrom");
	exit(1);
      } // end if

    } // end if

    // Check to see if every packet sent
    if( totokpacks == totpackets ) {
      done = 1;
    } // end if

    // Check to see if last packet can be sent
    if( totokpacks == totpackets - 1 ) {
      i = totpackets-1;
      usleep(avgtime+avgtime*1.5);
    }

    // Restart if last packet, not ready for ye
    else if( i == totpackets-1 ) {
      i = -1;
      continue;
    }

    // Check if this packet sent ok already
    if( okpacks == 1 ) {
      continue;
    } // end if

    if( rand()%100 > 60 ) {

      // Set timestamp of packet being sent
      gettimeofday( &tb, &tz );
      packets.time = htonl(tb.tv_usec);

      // Create data to send across
      data = malloc( l+9 );
      pVar = (unsigned char*)&packets.type;
      memcpy( data, pVar, sizeof(unsigned char) );
      pArray = (unsigned char*)&data[1];
      pVar = (unsigned char*)&packets.time;
      memcpy( pArray , pVar , sizeof(int));
      pArray += sizeof(int);
      pVar = (unsigned char*)&packets.seq_num;
      memcpy( pArray , pVar , sizeof(int) );
      pArray += sizeof(int);
      pVar = packets.data;
      memcpy( pArray , pVar , l );

      // Send it
      if( (numbytes=sendto(sockfd, data, l+9, 0, (struct sockaddr *)&their_addr, sizeof(struct sockaddr))) == -1 ) {
	perror("sendto");
	exit(1);
      } // end if

      okpacks = 1;
      totokpacks++;
    
      printf("Packet %d sent     Tot Sent: %d\n",i,totokpacks);

    } // end if
    else {
      printf("Packet %d skipped\n",i);

    }

    // Out of packets in this iteration, restart
    if( i == totpackets-1 ) { i = -1; }


  } // end for[/source lang="c"]Thanks if you guys can think of anything.  ALso, the reason I don't send the last packet until the end is because it has a special header if you can see.
Here are the byte values of the Example output from the one selecting. Packet #s are the last 4 bytes (numbers).
Total Packets: 13Packet 5 - RECEIVED   0  0  5  192  224  117  20  64  160Packet 7 - RECEIVED   0  0  5  237  224  117  20  64  160Packet 7 - RECEIVED   0  0  6  53  224  117  20  64  160Packet 7 - RECEIVED   0  0  6  92  224  117  20  64  160Packet 11 - RECEIVED   0  0  7  70  224  117  20  64  160Packet 0 - RECEIVED   0  0  9  184  224  117  20  64  160Packet 2 - RECEIVED   0  0  10  45  224  117  20  64  160Packet 1 - RECEIVED   0  0  12  41  224  117  20  64  160Packet 10 - RECEIVED   0  0  13  215  224  117  20  64  160Packet 3 - RECEIVED   0  0  15  54  224  117  20  64  160Packet 8 - RECEIVED   0  0  3  1  224  117  20  64  160Packet 12 - RECEIVED   0  0  5  75  224  117  20  64  160

Actual byte values from the sender
Packet 4 - OK  0  0  5  192  123  0  0  0  4Packet 5 - OK  0  0  5  237  88  0  0  0  5Packet 6 - OK  0  0  6  53  109  0  0  0  6Packet 7 - OK  0  0  6  92  224  0  0  0  7Packet 11 - OK  0  0  7  70  198  0  0  0  11Packet 0 - OK  0  0  9  184  80  0  0  0  0Packet 2 - OK  0  0  10  45  6  0  0  0  2Packet 1 - OK  0  0  12  41  55  0  0  0  1Packet 10 - OK  0  0  13  215  84  0  0  0  10Packet 3 - OK  0  0  15  54  34  0  0  0  3Packet 8 - OK  0  0  3  1  151  0  0  0  8Packet 9 - OK  0  0  5  75  40  0  0  0  9Packet 12 - OK  8  0  5  195  152  0  0  0  12
As you can see it's messing up quite a bit on what it is recieving. And like I said before, if I don't do the select and only use recvfrom it gets good output.
Advertisement
A comparision to != 0 isn't guaranteed to return 1, just to return non-0. Your code compares to == 1 where you cal isReadable().

However, using something like isReadable() is usually just a receipe for poor performance. If you only want to check a single socket, make that socket non-blocking and just call recv() or recvfrom() on it.

Now, regarding your code, you don't show the code that SENDS the packets at all, so it's impossible for us to tell what, if anything is supposed to be messed up in the results you post.
enum Bool { True, False, FileNotFound };
I have verified on Win32 that a non-blocking recvfrom() with a WSAGetLastError() is over 2 times faster than calling select() and recvfrom(). It could be even faster on *nix as errno is checked instead of a library call. Thus, if you don't need a timeout, and the socket never needs to block (and you won't be switching between blocking and non-blocking modes), unblock the socket:

  bool setBlocking(bool enable) {    unsigned long arg = !enable; // when arg is 1, non-blocking mode is turned on.    int res = ::ioctlsocket(s,FIONBIO,&arg);    return res != SOCKET_ERROR;  } // setBlocking// ...setBlocking(false);// ...


Then call recvfrom() and call WSAGetLastError() each time recvfrom() returns an error (most of the time it will be (WSA)EWOULDBLOCK, which can be safely ignored).
For *nix, check the global errno after every socket error.

The above applies to recv() as well.
Actually, I can make this prediction, even though you didn't post your sending code, using Highly Advanced Remote Code Reading Technology (one of the benefits of working for a defense contractor :-)

Your sending code is passing the wrong value for number-of-bytes when sending. In fact, you are passing sizeof(pointer) (which is 4) when you want to pass some larger number, like the actual number of bytes you want to send.

Further, I can see from your code that you aren't actually checking whether you got the number of bytes you expected to get from recv(); not checking return codes is sloppy programming.
enum Bool { True, False, FileNotFound };
yeah i forgot to check the number of bytes that i had recieved, so i was recieving before the everything was recieved.

i actually sent not based on a sizeof but in actual bytes, since packet sizes varied and my structure for the packet had dead space in it to make sure the variables are word aligned. Instead of sending the structure I sent a byte buffer.

thanks for the help guys, I meant to say something earlier, but was a little busy lately and kept forgeting to come on and say thanks for all of ya'lls help.

This topic is closed to new replies.

Advertisement