ghaerr / microwindows

The Nano-X Window System
Other
648 stars 91 forks source link

Question on keyboard event devices in linux #66

Open mik1234mc opened 2 years ago

mik1234mc commented 2 years ago

Hi all,

I have a custom keypad served with uinput (or now with libevdev) in linux-arm host. This keypad is not hooked to any tty, but it creates just /dev/input/eventX device. I checked the repo and kbd_event.c is now deprecated. My question is what is the recommended solution to my setup? Should I bring back kbd_event.c into the compilation or there is another way the event device can be somehow redirected to tty and then standard kbd_tty.c driver could be used?

I dont use X11.

Thank you for you suggestions, Michael

ghaerr commented 2 years ago

Hello @mik1234mc,

Thank you for your interest in MIcrowindows. I would suggest using kbd_event.c and moving it out of the deprecated/ folder and see whether it works for you unmodified, or with small changes. Let me know if you need additional help.

Than you!

mik1234mc commented 2 years ago

Thank you for fast reply, Greg. The initial bring-up was quite seamless. I just needed to delete some init code in the kbd_event.c. In the end, I migrated to specify the device via env variable. See my patch below if interested.

diff --git a/src/drivers/Objects.rules b/src/drivers/Objects.rules
index c5d0961..613ed0d 100644
--- a/src/drivers/Objects.rules
+++ b/src/drivers/Objects.rules
@@ -185,6 +185,10 @@ ifeq ($(KEYBOARD), SCANKBD)
 MW_CORE_OBJS += $(MW_DIR_OBJ)/drivers/kbd_ttyscan.o
 endif

+ifeq ($(KEYBOARD), EVENTKBD)
+MW_CORE_OBJS += $(MW_DIR_OBJ)/drivers/deprecated/kbd_event.o
+endif
+
 #
 # Other
 #
diff --git a/src/drivers/deprecated/kbd_event.c b/src/drivers/deprecated/kbd_event.c
index b52aaee..45fd583 100644
--- a/src/drivers/deprecated/kbd_event.c
+++ b/src/drivers/deprecated/kbd_event.c
@@ -23,34 +23,24 @@ static MWKEYMOD curmodif = 0, allmodif = 0;
 static int KBD_Open(KBDDEVICE *pkd)
 {
        int i, r;
-       char fname[64];
-       for (i = 0; i < 32; i++)
+       char* kbd;
+
+    // "/dev/input/eventX" string is expected
+    kbd = getenv("KBDEVENTDEVICE");
+
+       fd = open(kbd, O_RDONLY | O_NONBLOCK);
+       if (fd < 0) 
        {
-               sprintf(fname, "/sys/class/input/event%d/device/capabilities/ev", i);
-               fd = open(fname, O_RDONLY);
-               if (fd < 0)
-                       continue;
-               r = read(fd, fname, sizeof(fname));
-               close(fd);
-               if (r <= 0)
-                       continue;
-    fname[r - 1] = '\0';
-               if ((strtoul(fname, NULL, 16) & (1 << EV_MSC)) == 0)
-                       continue;
-    sprintf(fname, "/dev/input/event%d", i);
-               fd = open(fname, O_RDONLY | O_NONBLOCK);
-               if (fd < 0)
-                       continue;
-               curmodif = 0;
-               /* TODO: Assign allmodif */
-               allmodif = MWKMOD_LSHIFT | MWKMOD_RSHIFT| MWKMOD_LCTRL |
-                        MWKMOD_RCTRL | MWKMOD_LALT | MWKMOD_RALT |
-                       MWKMOD_LMETA | MWKMOD_RMETA | MWKMOD_NUM |
-                        MWKMOD_CAPS | MWKMOD_ALTGR | MWKMOD_SCR;
-    return fd;
+        EPRINTF("Error %d opening keyboard input device\n", errno);
+           return errno;
        }
-       EPRINTF("Error %d opening keyboard input device\n", errno);
-       return errno;
+       curmodif = 0;
+       /* TODO: Assign allmodif */
+       allmodif = MWKMOD_LSHIFT | MWKMOD_RSHIFT| MWKMOD_LCTRL |
+                                       MWKMOD_RCTRL | MWKMOD_LALT | MWKMOD_RALT |
+               MWKMOD_LMETA | MWKMOD_RMETA | MWKMOD_NUM |
+                                       MWKMOD_CAPS | MWKMOD_ALTGR | MWKMOD_SCR;
+    return fd;
 }

 /*
lpsantil commented 2 years ago
+    // "/dev/input/eventX" string is expected
+    kbd = getenv("KBDEVENTDEVICE");
+
+       fd = open(kbd, O_RDONLY | O_NONBLOCK);

This seems unsafe. You might want to check your input before attempting to open it.

mik1234mc commented 2 years ago

The open() call is followed by:

        if (fd < 0) 
    {
            EPRINTF("Error %d opening keyboard input device\n", errno);
        return errno;
    }

Isn't this sufficient check?

ghaerr commented 2 years ago

This seems unsafe. You might want to check your input before attempting to open it.

Isn't this sufficient check?

I think @lpsantil was referring to the potential security issue of opening a pathname sourced from an environment variable. I don't have a comment on that, other than the security issue would highly depend on where this code is used.

On another matter I just noticed, the folllowing code returns 'errno' on an error; it should return -1:

if (fd < 0) 
    {
            EPRINTF("Error %d opening keyboard input device\n", errno);
        return errno;
    }

The 'errno' return, always being positive, will be confused by Microwindows as having returned a valid keyboard file descriptor with the value of the positive errno value. All places in this keyboard driver that return errno should return -1.