From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with archive (Exim 4.43) id 1MppMi-0001PN-J2 for mharc-grub-devel@gnu.org; Mon, 21 Sep 2009 16:20:04 -0400 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MppMh-0001NS-7Q for grub-devel@gnu.org; Mon, 21 Sep 2009 16:20:03 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MppMc-0001Ek-Hg for grub-devel@gnu.org; Mon, 21 Sep 2009 16:20:02 -0400 Received: from [199.232.76.173] (port=47028 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MppMc-0001Ea-9U for grub-devel@gnu.org; Mon, 21 Sep 2009 16:19:58 -0400 Received: from xvm-190-8.ghst.net ([217.70.190.8]:35598 helo=aybabtu.com) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1MppMb-0003Ky-M5 for grub-devel@gnu.org; Mon, 21 Sep 2009 16:19:58 -0400 Received: from [192.168.10.10] (helo=thorin) by aybabtu.com with esmtp (Exim 4.69) (envelope-from ) id 1MppMV-0002fM-Lc for grub-devel@gnu.org; Mon, 21 Sep 2009 22:19:51 +0200 Received: from rmh by thorin with local (Exim 4.69) (envelope-from ) id 1MppMT-0001CY-Uw for grub-devel@gnu.org; Mon, 21 Sep 2009 22:19:49 +0200 Date: Mon, 21 Sep 2009 22:19:49 +0200 From: Robert Millan To: grub-devel@gnu.org Message-ID: <20090921201949.GA4485@thorin> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="HlL+5n6rz5pIUxbD" Content-Disposition: inline Organization: free as in freedom X-Message-Flag: Worried about Outlook viruses? Switch to Thunderbird! www.mozilla.com/thunderbird X-Debbugs-No-Ack: true User-Agent: Mutt/1.5.18 (2008-05-17) X-detected-operating-system: by monty-python.gnu.org: GNU/Linux 2.6 (newer, 3) Subject: at_keyboard and checkkey() X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: The development of GRUB 2 List-Id: The development of GRUB 2 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 21 Sep 2009 20:20:03 -0000 --HlL+5n6rz5pIUxbD Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Hi, at_keyboard's checkkey() implementation is buggy. It will flush the controller's input buffer, but checkkey()'s semantics require that it's not flushed. This results in some keys being lost when using at_keyboard. A proper fix that adjusts to this API is complicated. I implemented one [1], only to realize afterwards that maybe this mess isn't really necessary. A quick check at grub_checkkey() users reveals that none of them care about the actual key, even if grub_checkkey returns it. They just want to know whether there's a key available for read at all. If we removed this constraint from the checkkey() semantics, perhaps at_keyboard can get a checkkey() with less hassle. However, when at_keyboard reads an event, it needs to determine whether it's a MAKE (key hit) or BREAK (key unhit) event. Which AFAICT can't be done without reading KEYBOARD_REG_DATA. But maybe I'm missing something. Any bright ideas? [1] See attachment. This made me implement queues as well, perhaps some of this can be reused. -- Robert Millan The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and how) you may access your data; but nobody's threatening your freedom: we still allow you to remove your data and not access it at all." --HlL+5n6rz5pIUxbD Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="at_keyboard_queue.diff" Index: conf/i386.rmk =================================================================== --- conf/i386.rmk (revision 2607) +++ conf/i386.rmk (working copy) @@ -6,7 +6,7 @@ cpuid_mod_CFLAGS = $(COMMON_CFLAGS) cpuid_mod_LDFLAGS = $(COMMON_LDFLAGS) pkglib_MODULES += at_keyboard.mod -at_keyboard_mod_SOURCES = term/i386/pc/at_keyboard.c +at_keyboard_mod_SOURCES = term/i386/pc/at_keyboard.c kern/queue.c at_keyboard_mod_CFLAGS = $(COMMON_CFLAGS) at_keyboard_mod_LDFLAGS = $(COMMON_LDFLAGS) Index: kern/queue.c =================================================================== --- kern/queue.c (revision 0) +++ kern/queue.c (revision 0) @@ -0,0 +1,53 @@ +/* queue.c - grub queue functions */ +/* + * GRUB -- GRand Unified Bootloader + * Copyright (C) 2009 Free Software Foundation, Inc. + * + * GRUB is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * GRUB is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with GRUB. If not, see . + */ + +#include +#include +#include + +void +grub_queue_push (grub_queue_t queue, grub_queue_item_t item) +{ + item->next = queue->head; + item->prev = NULL; + if (queue->head) + queue->head->prev = item; + queue->head = item; + + if (! queue->tail) + /* Empty queue becomes non-empty. */ + queue->tail = item; +} + +void * +grub_queue_pop (grub_queue_t queue) +{ + grub_queue_item_t item; + + item = queue->tail; + if (queue->tail) + queue->tail = item->prev; + if (queue->tail) + queue->tail->next = NULL; + else + /* Non-empty queue becomes empty. */ + queue->head = NULL; + + return item; +} Index: include/grub/queue.h =================================================================== --- include/grub/queue.h (revision 0) +++ include/grub/queue.h (revision 0) @@ -0,0 +1,52 @@ +/* queue.h - header for grub queue */ +/* + * GRUB -- GRand Unified Bootloader + * Copyright (C) 2009 Free Software Foundation, Inc. + * + * GRUB is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * GRUB is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with GRUB. If not, see . + */ + +#ifndef GRUB_QUEUE_HEADER +#define GRUB_QUEUE_HEADER 1 + +#include +#include +#include /* GRUB_FIELD_MATCH */ + +struct grub_queue_item +{ + struct grub_queue_item *next, *prev; +}; +typedef struct grub_queue_item *grub_queue_item_t; + +struct grub_queue +{ + grub_queue_item_t head, tail; +}; +typedef struct grub_queue *grub_queue_t; + +void EXPORT_FUNC(grub_queue_push) (grub_queue_t queue, grub_queue_item_t item); +void * EXPORT_FUNC(grub_queue_pop) (grub_queue_t queue); + +extern void* grub_assert_fail (void); + +#define GRUB_AS_QUEUE_ITEM(ptr) \ + (GRUB_FIELD_MATCH (ptr, grub_queue_item_t, next) && GRUB_FIELD_MATCH (ptr, grub_queue_item_t, prev) ? \ + (grub_queue_item_t) ptr : grub_assert_fail ()) + +#define GRUB_AS_QUEUE_ITEM_P(pptr) \ + (GRUB_FIELD_MATCH (*pptr, grub_queue_item_t, next) && GRUB_FIELD_MATCH (*pptr, grub_queue_item_t, prev) ? \ + (grub_queue_item_t *) (void *) pptr : grub_assert_fail ()) + +#endif /* ! GRUB_QUEUE_HEADER */ Index: term/i386/pc/at_keyboard.c =================================================================== --- term/i386/pc/at_keyboard.c (revision 2607) +++ term/i386/pc/at_keyboard.c (working copy) @@ -1,6 +1,6 @@ /* * GRUB -- GRand Unified Bootloader - * Copyright (C) 2007,2008 Free Software Foundation, Inc. + * Copyright (C) 2007,2008,2009 Free Software Foundation, Inc. * * GRUB is free software: you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -22,7 +22,52 @@ #include #include #include +#include +#include +struct grub_at_keyboard_queue_item +{ + struct grub_at_keyboard_queue_item *next, *prev; + int key; +}; +typedef struct grub_at_keyboard_queue_item *grub_at_keyboard_queue_item_t; + +static struct grub_queue grub_at_keyboard_queue; + +static void +grub_at_keyboard_queue_push (int key) +{ + grub_at_keyboard_queue_item_t item; + item = grub_malloc (sizeof (*item)); + item->key = key; + grub_queue_push (&grub_at_keyboard_queue, GRUB_AS_QUEUE_ITEM (item)); +} + +static int +grub_at_keyboard_queue_pop (void) +{ + grub_at_keyboard_queue_item_t item; + int key; + item = grub_queue_pop (&grub_at_keyboard_queue); + if (item) + { + key = item->key; + grub_free (item); + } + else + key = -1; + return key; +} + +static int +grub_at_keyboard_queue_last (void) +{ + if (grub_at_keyboard_queue.tail) + return ((grub_at_keyboard_queue_item_t) grub_at_keyboard_queue.tail)->key; + else + return -1; +} + static short at_keyboard_status = 0; #define KEYBOARD_STATUS_SHIFT_L (1 << 0) @@ -146,7 +191,7 @@ grub_keyboard_getkey (void) /* If there is a character pending, return it; otherwise return -1. */ static int -grub_at_keyboard_checkkey (void) +grub_at_keyboard_getkey_noblock (void) { int code, key; code = grub_keyboard_getkey (); @@ -186,10 +231,24 @@ static int key += 'a' - 'A'; } } - return (int) key; + return key; } static int +grub_at_keyboard_checkkey (void) +{ + int key; + key = grub_at_keyboard_getkey_noblock (); + + /* If there was a key, queue it. */ + if (key != -1) + grub_at_keyboard_queue_push (key); + + /* Return last key from queue. */ + return grub_at_keyboard_queue_last (); +} + +static int grub_at_keyboard_getkey (void) { int key; @@ -197,6 +256,10 @@ grub_at_keyboard_getkey (void) { key = grub_at_keyboard_checkkey (); } while (key == -1); + + /* Remove it from the queue. */ + (void) grub_at_keyboard_queue_pop (); + return key; } --HlL+5n6rz5pIUxbD--