Thread: Hmm... Anything wrong with this?

I use the function below for logging in my program. All it does is write the line I input into an array, and shift the oldest line out. I thought it was working fine, but this morning I wrote some code to handle serial input and things went haywire. What I was doing was just calling the logit function every time I received a complete packet like this LogIt("Packet Received [%s], packet_data"); Every time I ran the program, the program would randomly freeze-up after a random amount of packets... I checked, and double checked the code, and then went as far as taking out the LogIt function... and then it worked.

I've gone back and looked at the code, but I can't find anything wrong with it -- except that it is messy =) All the '\0' positions & lengths seem to be ok, but am I over looking something?

Code:

// --------------
// LOG FUNCTION
// --------------
LogIt( char *newline, ... ) {
	va_list arg_ptr;
	va_start(arg_ptr, newline);
	char newlinef[88]; // 88 because on the log line there 
// will be a 12 character time string added on (see below) for a 
// total of 100

	vsprintf(newlinef, newline, arg_ptr);
	va_end(arg_ptr);

	time_t t = time(0);
	char *date = ctime(&t);
	date[19] = '\0';
	char *ampm = "AM";
	char hour[3] = "";
	int inthr;
	hour[0] = date[11];
	hour[1] = date[12];
	inthr = atoi( hour );
	if( inthr == 12 ) {
		strcpy( ampm, "PM" );
	} else if( inthr > 12 ) {
		inthr -= 12;
		strcpy( ampm, "PM" );
	} else if( inthr == 0 ) {
		inthr += 12;
	}

	itoa( inthr, hour, 10 );
	if( inthr > 9 ) {
		date[11] = hour[0];
		date[12] = hour[1];
	} else {
		date[11] = ' ';
		date[12] = hour[0];
	}
	char msg[13]; // Time should come out to be in this 
// format "00:00:00AM: " -- 12 characters
	strcpy( msg, &date[11] );
	strcat( msg, ampm );
	strcat( msg, ": " );
	char sepnewline[100];
	strcpy(sepnewline, msg);
	strcat( sepnewline, newlinef );
	char templine[100];
	int i;
	for(i=0;i<50;i++)	{
//save current string
		strcpy(templine,LogLines[i].Line); 
//replace with new one
		strcpy(LogLines[i].Line,sepnewline);  
//reset new one to former one
		strcpy(sepnewline,templine);       
	}
	return 1;
}

Thanks,

Jon Scott

>>char *ampm = "AM";
>>strcpy( ampm, "PM" );
This is not legal practice. You are trying to overwrite a string literal.

Code:

char *ampm;

if (hour < 12)
    ampm = "AM";
else
    ampm = "PM";