gets() works with global buffer but not with a local buffer

[earlz]

This code has a very strange problem
this works perfect with a global buffer but when it is allocated on the stack(local) it prints letters perfect but the buffer has different characters(same length)
so what the crap is wrong with this:

the weird part is that the ascii key is copied to the buffer and then putchar(buffer) is done so it at one point is in the buffer right

Code: Select all

extern char getkey(char *buf);
char *gets(char *buf){ //get a string from the keyboard
     int x,y;
   char tmp[1];
   x=cwin->curx;
     for(;;){
        getkey(tmp);
        //tmp[1] is asci code; tmp[0] is scancode
        if(tmp[1]==0){
        switch(tmp[0]){
        }
        }else{
        switch(tmp[1]){
           //case 27:
           //buf--;
           //break;
           case '\n':
           //enter/return
           *buf=0;
           return buf;
           break;
           case '\b':
           if (x<cwin->curx){
           buf--;
           *buf=0;
           cwin->curx--;
           putchar(' ');
           cwin->curx--;
           
           
           }
           break;
           

               
           default:
               //*buf=tmp[1];
               *buf=tmp[1];
               putchar(*buf);
               buf++;
               break;
        }
        }
     }
     //buf--;
     //*buf=0;
}

[Candy]

There's only one byte in tmp, so tmp[1] doesn't point to anything. This in reality won't show because due to alignment the buffer is automatically "sized up" to 4 bytes.

How do you allocate the buffer that's sent to the function? is it actually big enough? If not, you're overwriting it by doing a function call. Do you have both the calling function and the getkey-function for us? They would probably help.

[earlz]

ok but i must warn you to be able to understand getkey i must show you basically all my kbd driver

Code: Select all

void call_test(){
char *tmpxy[32];
gets(tmpxy);
printf(tmpxy);
}

//kbd driver
volatile unsigned char keys[128];
char getkey(char *buf){ 
   //char tmp[1]; error! allocated on stack
   
   while(keycount==0){}
   get_kbd_buffer(&buf[0],&buf[1]);
   return buf[1];
}

unsigned char get_kbd_buffer(unsigned char *scan_buf,unsigned char *asc_buf){
   keycount--;
     *scan_buf=keys[keycount];
   keycount--;
   *asc_buf=keys[keycount];
   return *asc_buf;
}

void put_kbd_buffer(unsigned char scan,unsigned char chr){
   keys[keycount]=chr;
   keycount++;
   keys[keycount]=scan;
     keycount++;
}

void kbd_handler(r){
   unsigned char scancode;unsigned char tmp;
   if (keycount==128){return 0;}
     scancode = inportb(0x60);
     if (scancode & 0x80) { //this i thought meant that the key was released
          switch (scancode) {
               case 42|0x80:
               shifts.shift=0;
               break;
               case 54|0x80:
               shifts.shift=0;
               break;
               case 29|0x80:
               shifts.ctrl=0;
               break;
               case 56|0x80:
               shifts.alt=0;
               break;
          }
     }else{

     switch (scancode){
        case 42: 
        shifts.shift=1;
        break;
        case 54:
        shifts.shift=1;
        break;
        case 29:
        shifts.ctrl=1;
        break;
        case 56:
          shifts.alt=1;
          break;
          case 58:
          shifts.caps=shifts.caps ^ 1;
          break;
          case 69:
          shifts.num=shifts.num ^ 1;
          break;
          case 70:
          shifts.scroll=shifts.scroll ^ 1;
          break;
          
     }
     if (TextUI==1){ //ignore this
        switch(scancode){
           case 0x40: //f6
           cwin=def_console;
           break;
           case 0x41: //f7
           cwin=&mcon;
           break;
           case 0x42:
           
           break;
           case 0x43:
           
           break;
           case 0x44:
           
           break;
        }
        //call_evh(*cwin,7,(scancode<<8)|scan2asc(scancode)); //modification for ring3?
        //return 0;
        
     }
          
     tmp=scan2asc(scancode);
     put_kbd_buffer(scancode,tmp);


     }
     
}


unsigned char scan2asc(unsigned char scan){
     if (shifts.caps^shifts.shift==1) {
          scan=kbduscaps[scan];
     }else{
        scan=kbdus[scan];
     }
     return scan;
}

[Candy]

Code: Select all

unsigned char get_kbd_buffer(unsigned char *scan_buf,unsigned char *asc_buf){
   keycount--;
     *scan_buf=keys[keycount];
   keycount--;
   *asc_buf=keys[keycount];
   return *asc_buf;
}

void put_kbd_buffer(unsigned char scan,unsigned char chr){
   keys[keycount]=chr;
   keycount++;
   keys[keycount]=scan;
     keycount++;
}

Even though I've been drinking (it's late at night here) this is a race condition, if these threads are preemptible and separate. I assume they're separate (the put would be from the interrupt handler I guess and the get would be from the normal code). If the interrupt handler came in halfway through the get, you'd get a mess.

if (scancode & 0x80) { //this i thought meant that the key was released

Yes, but not for AT extended keys. If you hit a key that's on the AT keyboard only, it'll first give a 0xE0 for a single-code key and 0xE1 for a two-code key (only one). You need to filter those out or ban them from use (where the second is nigh impossible globally but possible if you're just the only tester).

Code: Select all

void call_test(){
char *tmpxy[32];
gets(tmpxy);
printf(tmpxy);
}

Uhm... this allocates an array of 32 char pointers... You then pass that to printf which should give a warning or error, I think error. You should be creating an array of char (char tmpxy[32]). Depending on your implementation, this might only give big warnings.

Code: Select all

unsigned char scan2asc(unsigned char scan){
     if (shifts.caps^shifts.shift==1) {
          scan=kbduscaps[scan];
     }else{
        scan=kbdus[scan];
     }
     return scan;
}

ugh.. hardcoded tables... Still, if you can't load them at runtime...

Code: Select all

//kbd driver
volatile unsigned char keys[128];

void kbd_handler(r){
   unsigned char scancode;unsigned char tmp;
   if (keycount==128){return 0;}
     scancode = inportb(0x60);
<snip shift/ctrl/alt/caps/num/scroll stuff and some ugly hack for ring3 stuff>
     tmp=scan2asc(scancode);
     put_kbd_buffer(scancode,tmp);
     }
     
}

That should work... You use the keyboard buffer as a stack however, while the common usage is a fifo. If you press two keys before the keyboard handler can process them they'll be processed in inverse order.

Does the problem still occur after fixing these three things?

[Schol-R-LEA]

bangs head on desk
<rant mode="tangent">
The whole programming world would owe the C and C++ standards committees a debt of gratitude if they would just deprecate [tt]gets()[/tt] once and for all. It's a loose cannon that everyone agrees was a mistake from the start. I know that it is widely used, but that's just all the more reason to ditch it for good, IMAO.

(Also, since this seems to be a version internal to your kernel and not the application-level library version, you may want to rename it as kgets() or something similar, to avoid confusion.)
</rant>

Going back to your code... first off, just as a stylistic note, you might want to wrap the access to the cursor up in a set of functions (or at least macros), both to make it clearer what your doing with them, and to localize the variables:

Code: Select all

inline int xpos() 
{
   return cwin->curx;
}

inline int ypos() 
{
   return cwin->cury;
}


inline void backchar() 
{
   cwin->curx--;
}

inline void nextchar() 
{
   cwin->curx++;
}

... and so on.

[earlz]

the array of chars in calling function was a typo(i typed that from the function not copy and paste)

[Schol-R-LEA]

I've got to stop editing things like that once I've posted them...

Anyway... it occurs to me that localizing the cursor access is more than just a stylistic preference: there's a possible race condition here, too, unless you can ensure that the function has exclusive access to the cursor. I'm surprised I didn't notice that before, epecially after reading Candy's earlier posting. That probably means that not only would you need to limit access, using the [tt]inline[/tt] type modifier probably wouldn't be so wise after all.

In any case, the parts for [tt]cury[/tt] are probably inadequate, since they don't handle the case where the cursor is at the top or bottom lines already. I could be wrong, of course. Now, I'm not entirely certain how you are handling the cursor, but from the look of things, you're treating the screen as a two-dimensional array, with [tt]curx[/tt] holding the horizontal position and [tt]cury[/tt] holding the vertical. If so, then then top and bottom lines present (literal) edge cases. You would need to keep existing value of [tt]cury[/tt], but scroll the existing buffer up or down one line as appropriate (and in the case if the top line, you would need to have retained the previous screen entries in a separate buffer, or else not allow scrolling up at all).

This brings up a possible problem that hasn't been mentioned yet, probably because you seem to have only tested single lines so far:

Code: Select all

          case '\n':
          //enter/return
          *buf=0;
          return buf;
          break;

Nothing is being done to update the cursor, here. Since it doesn't look like the [tt]getkey()[/tt] function or the rest of the code you posted does anything for that; the only references to [tt]cwin[/tt] in the second post are with what seem to be code for switching virtual terminals (and this is probably far too low-level a point for that!). The result is that the cursor won't get updated after a newline at all. For that matter, it doesn't seem that the cursor gets advanced at all, just reversed in the case of ... unless this is being done automatically elsewhere, this is certain to cause problems.

It's been a while since I've looked into any of these issues, so I may be completely off. YMMV.

Finally, I would recommend putting the character-decoding code into a separate single-character function such as [tt]decode_ch()[/tt] or something similar. This would allow you to debug the character-decoding code separate from the actual read loop, and will probably be needed later on anyway. Indeed, you will probably need a separate [tt]kgetchar()[/tt] at some point, and for that matter, you may at some point need to allow for multiple character decodings.

[Solar]

<rant mode="tangent">
The whole programming world would owe the C and C++ standards committees a debt of gratitude if they would just deprecate [tt]gets()[/tt] once and for all. It's a loose cannon that everyone agrees was a mistake from the start. I know that it is widely used, but that's just all the more reason to ditch it for good, IMAO.

They won't break compatibility. I can think of a dozen functions in the C lib alone right away that could do with some serious fixing, but that's what you get when a language gets popular before its lib gets "standard".