Negative numbers

[zhiayang]

Help! I've recently implemented RTC in my OS. As you may know, however, not everyone lives in central time (duh), so I've implemented a way to set the timezone. Positives values work fine, however when I try to set a negative timezone, **** happens.

Code: Select all

		retnum *= -1;
		kprintf("%d", (signed long)-62);

I have this implemented this in my atoi function, where if my first character is a '-', it would execute this. The number itself works perfectly fine, the timezone gets offset by negative amounts. However, the kprintf function screws up. As you can see, I changed the variable into a constant and even cast it into a (signed), it doesn't work. kprintf will display random characters.

Here is the kprintf code:

Code: Select all

void kprintf(const char *str, ...)
{
	va_list listptr;
	va_start(listptr, str);
	
	short is_percent = 0;
	short colour_changed = 0;
	
	register int i = 0;
	for(; i < strlen(str); i++)
	{
		if(str[i] == '%')
		{
			is_percent = 1;
			continue;
		}	
		if(is_percent)
		{
			switch(str[i])
			{
				case '%':
					//Gotta print % itself
					putch('%');
					break;
				
				case 'i':
					putnum(va_arg(listptr, int));
					break;
					
				case 'd':
					putnum(va_arg(listptr, long));
					break;
					
				case 'c':
					putch((char)va_arg(listptr, int));
					break;					
							
				case 'x':
					puthex(va_arg(listptr, unsigned int));
					break;		
					
				case 's':
					puts(va_arg(listptr, char*));
					break;
					
				case 'u':
					putunum(va_arg(listptr, unsigned long));
					break;
					
				case 'f':
					putfloat(va_arg(listptr, double));
					break;
					
				case '~':
					
					//Start messy if-else...
					
					if(str[i + 1] == 'B' && str[i + 2] == 'L' && str[i + 3] == 'A')
					{
						set_text_colour(BLACK);
						colour_changed = 1;
					}
					else if(str[i + 1] == 'B' && str[i + 2] == 'L' && str[i + 3] == 'U')
					{
						set_text_colour(BLUE);
						colour_changed = 1;
					}
					else if(str[i + 1] == 'G' && str[i + 2] == 'R' && str[i + 3] == 'E')
					{
						set_text_colour(BLUE);
						colour_changed = 1;
					}
					else if(str[i + 1] == 'C' && str[i + 2] == 'Y' && str[i + 3] == 'A')
					{
						set_text_colour(CYAN);
						colour_changed = 1;
					}
					else if(str[i + 1] == 'R' && str[i + 2] == 'E' && str[i + 3] == 'D')
					{
						set_text_colour(RED);
						colour_changed = 1;
					}
					else if(str[i + 1] == 'M' && str[i + 2] == 'A' && str[i + 3] == 'G')
					{
						set_text_colour(MAGENTA);
						colour_changed = 1;
					}					
					else if(str[i + 1] == 'B' && str[i + 2] == 'R' && str[i + 3] == 'O')
					{
						set_text_colour(BROWN);
						colour_changed = 1;
					}
					else if(str[i + 1] == 'L' && str[i + 2] == 'G' && str[i + 3] == 'Y')
					{
						set_text_colour(LIGHTGREY);
						colour_changed = 1;
					}
					else if(str[i + 1] == 'D' && str[i + 2] == 'G' && str[i + 3] == 'Y')
					{
						set_text_colour(DARKGREY);
						colour_changed = 1;
					}
					else if(str[i + 1] == 'L' && str[i + 2] == 'B' && str[i + 3] == 'U')
					{
						set_text_colour(LIGHTBLUE);
						colour_changed = 1;
					}
					else if(str[i + 1] == 'L' && str[i + 2] == 'G' && str[i + 3] == 'E')
					{
						set_text_colour(LIGHTGREEN);
						colour_changed = 1;
					}
					else if(str[i + 1] == 'L' && str[i + 2] == 'C' && str[i + 3] == 'Y')
					{
						set_text_colour(LIGHTCYAN);
						colour_changed = 1;
					}
					else if(str[i + 1] == 'L' && str[i + 2] == 'R' && str[i + 3] == 'E')
					{
						set_text_colour(LIGHTRED);
						colour_changed = 1;
					}
					else if(str[i + 1] == 'L' && str[i + 2] == 'M' && str[i + 3] == 'A')
					{
						set_text_colour(LIGHTMAGENTA);
						colour_changed = 1;
					}
					else if(str[i + 1] == 'L' && str[i + 2] == 'B' && str[i + 3] == 'R')
					{
						set_text_colour(LIGHTBROWN);
						colour_changed = 1;
					}
					else if(str[i + 1] == 'W' && str[i + 2] == 'H' && str[i + 3] == 'I')
					{
						set_text_colour(WHITE);
						colour_changed = 1;
					}
					else
					{
						putchar(str[i]);
						break;
					}					
					i += 3;
					break;
					
					
				//Precision format handling
				default:
					if(is_number(str[i]) || str[i] == '.')
					{
						int tmp = i;
						while(is_number(str[tmp]) || str[tmp] == '.')
						{
							tmp++;
						}
						char tm[3] = { str[i], str[i + 1] };
						
						switch(str[tmp])
						{
							case 'i':								
							case 'd':
								putnum_precision(va_arg(listptr, int), atoi(tm));
								return;						
							
							case 'u':	
								putunum_precision(va_arg(listptr, unsigned long), atoi(tm));
								return;	
									
							case 'x':
								puthex_precision(va_arg(listptr, unsigned int), atoi(tm));
								return;
												
						}
						
						short precision = 0;
						tmp = i;
						while(str[tmp] != '.')
						{
							tmp++;
							if(is_letter(str[tmp]))
							{
								precision = 10;
								break;
							}									   
						}
						tmp++;
						char t[3] = "";
						
						if(!is_letter(str[tmp]))
						{
							t[0] = str[tmp];
						}
						else if(is_letter(str[tmp]))
						{
							precision = 10;
							goto print;
						}
						
						if(!is_letter(str[tmp + 1]))
						{
							t[1] = str[tmp + 1];
						}
						precision = atoi(t);
						
						print:
						if(is_letter(str[tmp + 1]))
						{
							tmp--;
						}
						switch(str[tmp + 2])
						{
							case 'f':
								putfloat_precision(va_arg(listptr, double), precision);
								break;
						}
						i = tmp + 3;
					}
					break;
			}
			is_percent = 0;
		}		
		else
		{
			putch(str[i]);
		}
	}
	if(colour_changed)
	{
		set_text_colour(WHITE);
	}
	va_end(listptr);
}

What's wrong?

Thanks.

[JamesM]

The important code to see would be the code for putnum.

[gerryg400]

Code: Select all

            case 'd':
               putnum(va_arg(listptr, long));
               break;

'd' means int not long.

[zhiayang]

The important code to see would be the code for putnum.

silly me, i overlooked that part. Fixed it now, thanks for jerking my brain.

[brain]

You do know this code may do all manner of messed up things if you end a string with ~ as your colour one assumes that ~ is always followed by 3 letters of colour code, if they are not there it reads random value from the stack... Right now this is ok but there may be problems with this later especially if you let userland code use this function. Just letting you know!

Edit: a precision string on the end of the format without a dot will also cause the pointer to leave the bounds of the string potentially looping forever, from what I can see?

[bluemoon]

kprintf() should not be exported to user anyway, they should use printf(), which do the formatting in userland and call kernel with write(...).

[brain]

True, but even in userland this function is exploitable and dangerous...

[bluemoon]

I agree. So I recommend things like newlib or solar's pdclib, which has certain degree of stability, conform to standards, a rich set of features.
Rolling your own printf() proven to be a boring job and provide no excitement, and may not even met the standard.
Most people only need a minimum subset as kprintf() for debugging purpose.

[sandras]

Just putting out a somewhat related thought. After trying to implement my equivalent to a variadic printf(), and reading one of Brendan's posts somewhere on the forum, I tried going back to separate prnt_char(), prnt_str(), prnt_uint(), and prnt_sint(), and because of the separation:
*) I saved about 200 bytes;
*) didn't have to use a variadic funtion, implementation of which, I think, is not portable across compilers;
*) complexity of individual functions decreased.

Also, note that I only use print_uint(my_uint)/prnt_sint(my_sint), which internally use prnt_char() for printing, instead of doing prnt_str(atoi(my_int)). This also simplifies things. After all, I do not have the need to use strings converted from integers for anything else than printing.

You have to tell your kernel what to print one way or the other, but why do it in some roundabout way, having a (relatively complex) function, that decodes a string and finds out what you want to print, instead of telling it what to print directly.

[Solar]

*) didn't have to use a variadic funtion, implementation of which, I think, is not portable across compilers;

It's perfectly possible.

You have to tell your kernel what to print one way or the other, but why do it in some roundabout way, having a (relatively complex) function, that decodes a string and finds out what you want to print, instead of telling it what to print directly.

You do it "in some roundabout way" because of the principle of least surprise. Every C programmer knows how to use printf(). A function named "kprintf()" sends a clear message on how it should be used. But what your prnt_*() functions do is to place the burden of formatting the output on the user.