Subject: Assn 5: C language reminders ---- Reminder mid term exam is Tuesday --------- (This message is being sent to all CPSC 360 students) A student program contains the following lines which demonstrate some basic problems in understanding how C operates. The failure occurs in lines 219 - 221 but I include the rest to provide context. 191 // rps_write 192 int rps_write(rps_sock_t *rpsock, char *data, int len) 193 { 194 rps_buf_t *buf; 195 rps_hdr_t *hdr; 196 int slot; 198 // Verify that the rpsock is in the RPS_CONNECTED state 199 if(rpsock->state != RPS_CONNECTED) { 200 fprintf(stderr, "rps_write: must be connected before write \n"); 201 exit(1); 202 } 203 dprintf("rps_write: state %d \n", rpsock->state); 204 205 // Verify length is positive and not larger than MAX_APP_SIZE 206 if(!rps_between(len, 0, MAX_APP_SIZE)) { 207 fprintf(stderr, "rps_write: len must be between 0 and MAX_APP_SIZE \n"); 208 exit(1); 209 } 210 dprintf("rps_write: len = %d \n", len); 211 212 // Ask ring_get_slot() for the index of the next slot 213 dprintf("rps_write before slot wait with slots %d \n", \ 214 (&rpsock->sndring)->slots); 215 slot = ring_get_slot(&rpsock->sndring); 216 dprintf("rps_write after slot wait with slot %d \n", slot); 217 ----> 218 // Copy data from user buf to ring buf and save length of data in hdr len 219 *buf->data = data; 220 (&rpsock->sndring)->bufs[slot] = *buf; 221 hdr->len = len; rpsapi.c: In function 'rps_write': rpsapi.c:219: warning: assignment makes integer from pointer without a cast It looks to me that lines 219 and 220 represent an attempt to copy data from the user buffer to the send ring buffer indexed by slot. It looks like a "two stage" copy was attempted in which the *buf declared on line 194 was to be used as the destination for the first stage. Line 220 (which contains a complex construction (&rpsock->sndring)->bufs[slot] that I think often represents "wishful programming" effects the second stage... ----------------------------------------------------------------------- The basic misunderstanding here is the conception that a C assignment statement can be used to copy the contents of an array of characters (the data being written) from one location to another. C assignments can be used to copy basic types (int, char, float, pointer) and even instances of a structure but NEVER an array. I'm not a java expert but I would think this would be true of Java as well. The main reasons that array copies don't make sense (a) source and target arrays might not have the same size and (b) you might not want to copy the whole array. There are two ways to copy and array: (1) write a loop and move the data one byte at a time (2) use the memcpy() function which is designed to perform exactly this task. Because this example illustrates several problems that I see repeatedly I will analyze each line in some detail. You should endeavor to understand this analysis. ------------------------------------------------------------------------- Line 219 generates a warning because the right side "data" is a pointer but the left side (I think) is a way to refer to the first byte of the data component of the buffer to which "buf" points. Unfortunately "buf" is an UNINITIALIZED POINTER so this assignment will either segfault or overwrite some other data. IF buffer had been declared rps_buf_t buf; then buf.data[0] = *data; would copy THE FIRST BYTE ONLY. ------------------------------------------------------------------------- Line 220 (is actually fairly ingenious but a bad idea (&rpsock->sndring)->bufs[slot] = *buf; Here the left side (I think) evaluates to the CORRECT instance of the rps_buf_t in the sender ring. And the right side evaluates to an instance of the rps_buf_t pointed to by the uninitialized pointer "buf". So if, as in line 219, buf had been declared rps_buf_t buf; then (&rpsock->sndring)->bufs[slot] = buf; would be a structure assignment which WOULD COPY the entire contents of the local buffer buf to the sender ring!! 47 typedef struct rps_buf_type 48 { 49 double ts; /* Transmit time stamp */ 50 rps_hdr_t hdr; /* rps hdr */ 51 unsigned char data[MAX_APP_SIZE]; /* the app data */ 52 } rps_buf_t; The reason that is STILL a bad idea is that the structure has size 1480 bytes. So even if the sender is trying to send 10 byte messages, that approach would necessitate a 1480 byte copy for each send :-( ----------------------------------------------------------------------- Line 221 suffers from two problems. Problem 1 is that "hdr" is also another uninitialized pointer :-( I have recommended and still do that whenever you declare a pointer you initialize it to 0. rps_hdr_t *hdr = 0; This will absolutely prevent loose pointers being used to overwrite random data, and I think it will help you all be "less wishful" in that it forces you to think: This is just like (0)->len = len; and that cant work! Line 221 also contains a true "networking" error. Suppose I am sending from little endian to a big endian receiver. The receiver can't know I'm little endian and convert the len on reception. Just like with UDP and IP it is MANDATORY that all multibyte ints be sent in NETWORK BYTE ORDER and be decoded by the receiver. ---------------------------------------------------------------------- So the best plan is to just use the approach used in prodcom.c with memcpy() replacing the I/O statements that appear there. This reduces the copy problem to 255 int rps_write( 256 rps_sock_t *rpsock, 257 char *data, 258 int len) 259 { 260 rps_buf_t *buf; 261 rps_hdr_t *hdr; 262 int slot; 286 slot = ring_get_slot(&rpsock->sndring); 287 dprintf( "rps_write after slot wait with slot %d \n", 288 slot); 289 290 buf = rpsock->sndring.bufs + slot; 291 hdr = &buf->hdr; 292 memcpy(buf->data, data, len); 293 hdr->len = htons(len); This same basic approach can be used successfully in rps_recvdata() and rps_read()