luke
/
magic
mirror of https://github.com/RTimothyEdwards/magic.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

txCommands.c txInputDevRec FD select() usage overhaul 

There are numerous concerns with the original code from a read through.

The #define TX_MAX_OPEN_FILES 20 is badly named, the txCommand.c uses
fd_set to hold active FD for each device, and makes no attempt to bounds
check the 'fd' or 'fd_set' of any TxAdd1InputDevice() is in range.

The real name of this variable should be TX_MAX_INPUT_DEVICES as it
related to the fixed compile time slots available for devices, each input
device must have at least one active FD and it can be in the range
that fd_set allows, which is 0..1023 on linux.

So based on this being the original intention, due to the way the code is
constructed and API calls made available, the file has been reworked
to allow all these considerations at the same time.

Now the design should be FD clean for FDs in the range 0..1023 instead of
being artificially clamped to the first 20 FDs being valid input devices.

Renaming of variable 'i' to 'fd' when it relates to an fd number, so all
uses of FD_XXXX() should use fd, this helps clarify the different domain
the number relates to.
When 'i' is used it relates to the index into the txInputDevRec array.
A large part of the concerns relate to trying to mix the two numbering
domains interchangeably, just using a different name really clears up
understanding to the reader.

Other items that look like errors TxDelete1InputDevice() will
shuffle-remove entries with no active FD, but it is iterating an array
it is also modifying, so it looks like it would have incorrectly skipped
the next item that should be inspected just after a shuffle-removal
occurred.

The various iterators that works from 0..TX_MAX_OPEN_FILES incorrectly
limited the visibility of the routines to the first 20 FDs instead of
the full FD_SETSIZE the TxAddInputDevice API call allows to be
registered.

Passing in TxAdd1InputDevice with an fd above 19 would have resulted in
everything looking successful until select() was used, then the kernel
would likely not sleep/wait because the input fd_set would look blank due
to being clipped by nfds=TX_MAX_OPEN_FILES (instead of that plus 1).

The use of TX_MAX_OPEN_FILES in select instead of TX_MAX_OPEN_FILES+1 for
the 'nfds' field would have meant a device with fd=19 would not work as
the design intended.

The define definition has been removed from the module public header,
I assume it was there for use by another module, but the incorrect
select() usage has been corrected over there in a previous commit.
So now the define can be maintained near the array it relates to.

While the code might looks less 'efficient' as it is now sweeping all
1024 FDs when input devices during add/remove (hopefully there maybe some
compiler auto-vectorization/tzcnt use there).  The main event loop is
slightly more 'efficient' (especially for the single device with input
fd=0 case) as it will only check the 1 FD each input event loop iteration,
not check all 20 FDs for data readyness everytime.
This commit is contained in:
Darryl L. Miles 2025-02-24 08:49:07 +00:00 committed by R. Timothy Edwards
parent 6d2d4353d3
commit 3c3ebcfd2b
2 changed files with 71 additions and 28 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
textio/textio.h
View File

@ -110,6 +110,4 @@ extern void TxInit(void);
extern void TxInitReadline(void);
#endif
#define TX_MAX_OPEN_FILES 20
#endif /* _TEXTIO_H */

97
textio/txCommands.c
View File

@ -64,11 +64,17 @@ bool TxDebug = FALSE;
/* A mask of the file descriptors for all input devices.
*/
static fd_set txInputDescriptors;
/* Used for the 'nfds' field for select() syscall, the highest
* fd number that is set in the txInputDescriptors bitmask.
* Don't forget to plus 1 for select().
*/
static int txInputDescriptors_nfds;
#define TX_MAX_INPUT_DEVICES 20
/* A table of all input devices.
*/
static txInputDevRec txInputDevice[TX_MAX_OPEN_FILES];
static int txLastInputEntry = -1;
static txInputDevRec txInputDevice[TX_MAX_INPUT_DEVICES];
static int txLastInputEntry;
/* The current point -- reset by the 'setpoint' command and for each
* interactive command. Calls to TxClearPoint clear previous setpoints,
@ -130,6 +136,9 @@ static TxCommand *lisp_cur_cmd = NULL;
*
* FD_IsZero --
* FD_OrSet --
* FD_MaxFd --
* Returns the highest 'fd' in the mask for select(2) nfds argument, or
* -1 when 'fdmask' is empty.
*
* Routines for manipulating set of file descriptors.
*
@ -140,9 +149,9 @@ bool
FD_IsZero(
const fd_set *fdmask)
{
int i;
for (i = 0; i <= TX_MAX_OPEN_FILES; i++)
if (FD_ISSET(i, fdmask)) return FALSE;
int fd;
for (fd = 0; fd < FD_SETSIZE; fd++)
if (FD_ISSET(fd, fdmask)) return FALSE;
return TRUE;
}
@ -151,9 +160,30 @@ FD_OrSet(
const fd_set *fdmask,
fd_set *dst)
{
int i;
for (i = 0; i <= TX_MAX_OPEN_FILES; i++)
if (FD_ISSET(i, fdmask)) FD_SET(i, dst);
int fd;
for (fd = 0; fd < FD_SETSIZE; fd++)
if (FD_ISSET(fd, fdmask)) FD_SET(fd, dst);
}
/* A bitmask find max bit set operation */
int
FD_MaxFd(fdmask)
const fd_set *fdmask;
{
int fd;
for (fd = FD_SETSIZE-1; fd >= 0; fd--) /* backwards */
if (FD_ISSET(fd, fdmask)) return fd;
return -1;
}
/* update txInputDescriptors_nfds with the correct value
* call this everytime txInputDescriptors is modified
*/
static void
TxInputDescriptorsRecalc(void)
{
int nfds = FD_MaxFd(&txInputDescriptors);
txInputDescriptors_nfds = (nfds >= 0) ? nfds : 0;
}
/*
@ -468,18 +498,20 @@ TxAddInputDevice(
* it is called.
*/
{
int i;
TxDeleteInputDevice(fdmask);
if (txLastInputEntry + 1 == TX_MAX_OPEN_FILES)
if (txLastInputEntry == TX_MAX_INPUT_DEVICES)
{
TxError("Too many input devices.\n");
return;
}
txLastInputEntry++;
txInputDevice[txLastInputEntry].tx_fdmask = *fdmask;
txInputDevice[txLastInputEntry].tx_inputProc = inputProc;
txInputDevice[txLastInputEntry].tx_cdata = cdata;
txLastInputEntry++;
FD_OrSet(fdmask, &txInputDescriptors);
TxInputDescriptorsRecalc();
}
void
@ -520,10 +552,9 @@ TxDeleteInputDevice(
* no longer active.
*/
{
int i;
for (i = 0; i <= TX_MAX_OPEN_FILES; i++)
if (FD_ISSET(i, fdmask)) TxDelete1InputDevice(i);
int fd;
for (fd = 0; fd < FD_SETSIZE; fd++)
if (FD_ISSET(fd, fdmask)) TxDelete1InputDevice(fd);
}
void
@ -537,18 +568,30 @@ TxDelete1InputDevice(
return; /* allowing things to continue is UB */
}
int i, j;
for (i = 0; i <= txLastInputEntry; i++)
restart:
{
FD_CLR(fd, &(txInputDevice[i].tx_fdmask));
if (FD_IsZero(&txInputDevice[i].tx_fdmask))
int i;
for (i = 0; i < txLastInputEntry; i++)
{
for (j = i+1; j <= txLastInputEntry; j++)
txInputDevice[j-1] = txInputDevice[j];
txLastInputEntry--;
FD_CLR(fd, &txInputDevice[i].tx_fdmask);
if (FD_IsZero(&txInputDevice[i].tx_fdmask))
{
int j;
for (j = i+1; j < txLastInputEntry; j++)
txInputDevice[j-1] = txInputDevice[j];
txLastInputEntry--;
/* we shuffled entries down one, so txInputDevice[i] now
* looks at potential next one to inspect, but we're about
* to i++ (if we continue) and that will incorrectly skip
* inspection of it.
* lets just start over
*/
goto restart;
}
}
}
FD_CLR(fd, &txInputDescriptors);
TxInputDescriptorsRecalc();
}
@ -968,7 +1011,7 @@ TxGetInputEvent(
{
if (returnOnSigWinch && SigGotSigWinch) return gotSome;
inputs = txInputDescriptors;
numReady = select(TX_MAX_OPEN_FILES, &inputs, (fd_set *)NULL,
numReady = select(txInputDescriptors_nfds + 1, &inputs, (fd_set *)NULL,
(fd_set *)NULL, waitTime);
if (numReady <= 0) FD_ZERO(&inputs); /* no fd is ready */
} while ((numReady <= 0) && (errno == EINTR));
@ -978,20 +1021,21 @@ TxGetInputEvent(
perror("magic");
}
for (i = 0; i <= txLastInputEntry; i++)
for (i = 0; numReady && i < txLastInputEntry; i++)
{
/* This device has data on its file descriptor, call
* it so that it can add events to the input queue.
*/
for (fd = 0; fd < TX_MAX_OPEN_FILES; fd++) {
for (fd = 0; fd <= txInputDescriptors_nfds; fd++) {
if (FD_ISSET(fd, &inputs) &&
FD_ISSET(fd, &(txInputDevice[i].tx_fdmask))) {
FD_ISSET(fd, &txInputDevice[i].tx_fdmask)) {
lastNum = txNumInputEvents;
(*(txInputDevice[i].tx_inputProc))
(fd, txInputDevice[i].tx_cdata); /** @invoke cb_textio_input_t */
FD_CLR(fd, &inputs);
/* Did this driver choose to add an event? */
if (txNumInputEvents != lastNum) gotSome = TRUE;
numReady--;
}
}
}
@ -1711,6 +1755,7 @@ txCommandsInit(void)
txZeroTime.tv_sec = 0;
txZeroTime.tv_usec = 0;
FD_ZERO(&txInputDescriptors);
txInputDescriptors_nfds = 0;
DQInit(&txInputEvents, 4);
DQInit(&txFreeEvents, 4);
DQInit(&txFreeCommands, 4);