termios - Add Capability to Generate SIGINTR and SIGQUIT

[Joel Sherrill]

Currently the RTEMS termios implementation does not examine the ISIG setting in the termios attributes and thus does not examine input for the INTR (ctl-C) and QUIT (ctl-\) characters. As a consequence, it cannot return -1/EINTR.

The proposed solution implements a point at which a default handler can do nothing like currently or the application can use the following new method which allows them to register the RTEMS provided method rtems_termios_posix_isig_handler().

rtems_termios_isig_status_code rtems_termios_register_isig_handler(
  rtems_termios_isig_handler handler
);

The method rtems_termios_posix_isig_handler() is provided and has the POSIX compliant behavior of generating SIGINTR for the VINTR character and SIGQUIT for the VQUIT character.

The user also can register rtems_termios_default_isig_handler() to return to the default behavior.

The tests termios10 (polled IO) and termios11 (interrupt driven IO) are added to exercise this behavior.

[Joel Sherrill <joel@…>]

In 667501a/rtems:

termios: Add Capability to Generate SIGINTR and SIGQUIT

This patch adds the ability for termios to send SIGINTR on receipt
of VINTR and SIGQUIT for VKILL and return -1/EINTR from read() on
a termios channel. Importantly, this patch does not alter the default
behavior or force POSIX signal code in just because termios is used.
The application must explicitly enable the POSIX behavior of generating
a signal upon receipt of these characters. This is discussed in the
POSIX standard:

​https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap11.html

Closes #3800.

[Sebastian Huber]

The implementation breaks the canonical input mode as shown by test case termios09.

[Sebastian Huber]

The implementation introduced a new enum:

/**
 * @brief Type returned by all input processing (isig) methods
 */ 
typedef enum {
  /**
   * This indicates that the input character was processed
   * and possibly placed into the buffer.
   */
  RTEMS_TERMIOS_ISIG_WAS_PROCESSED,
  /**
   * This indicates that the input character was not processed and
   * subsequent processing is required.
   */
  RTEMS_TERMIOS_ISIG_WAS_NOT_PROCESSED,
  /**
   * This indicates that the character was processed and determined
   * to be one that requires a signal to be raised (e.g. VINTR or
   * VKILL). The tty must be in the right termios mode for this to
   * occur. There is no further processing of this character required and
   * the pending read() operation should be interrupted.
   */
  RTEMS_TERMIOS_ISIG_INTERRUPT_READ
} rtems_termios_isig_status_code;

What was the rationale for the two status codes RTEMS_TERMIOS_ISIG_WAS_PROCESSED and RTEMS_TERMIOS_ISIG_WAS_NOT_PROCESSED?

What should a handler do if it returns RTEMS_TERMIOS_ISIG_WAS_PROCESSED? The default handler just returns RTEMS_TERMIOS_ISIG_WAS_NOT_PROCESSED.

[Sebastian Huber <sebastian.huber@…>]

In 90f11edd/rtems:

termios: Fix input canonical mode

Canonical input processing was broken by
667501a314ba75f80f1c13c6b43dd35d0a00efd1 as shown by test case
termios09.

Update #3800.

[Joel Sherrill]

Sebastian can this be closed now?

[Sebastian Huber]

Replying to Joel Sherrill:

Sebastian can this be closed now?

See comment 4.

[Chris Johns]

Ping?

[Sebastian Huber]

Joel, I have no idea what you had in mind with the rtems_termios_isig_status_code, see my comment #4. Is this part of the API which applications may implement? The

rtems_termios_isig_status_code rtems_termios_posix_isig_handler(
  unsigned char             c,
  struct rtems_termios_tty *tty
)
{
  if (c == tty->termios.c_cc[VINTR]) {
    raise(SIGINT);
    return RTEMS_TERMIOS_ISIG_INTERRUPT_READ;
  }

  if (c == tty->termios.c_cc[VQUIT]) {
    raise(SIGQUIT);
    return RTEMS_TERMIOS_ISIG_INTERRUPT_READ;
  }

  return RTEMS_TERMIOS_ISIG_WAS_NOT_PROCESSED;
}

doesn't use RTEMS_TERMIOS_ISIG_WAS_PROCESSED for example. The "unsigned char" should be probably cc_t.

[Joel Sherrill]

I added the enum for readability. This is a very odd path and having a real name versus a boolean seemed to me to make the code clearer. We could add a debug assert before the return RTEMS_TERMIOS_IPROC_CONTINUE and then the enum RTEMS_TERMIOS_ISIG_WAS_NOT_PROCESSED would be used.

I was leaning to improving the readability with the enum names. It was also not 100% clear to be that there would not end up ever being a third case.

static rtems_termios_iproc_status_code
iproc (unsigned char c, struct rtems_termios_tty *tty)
{
  /*
   * If signals are enabled, then allow possibility of VINTR causing
   * SIGINTR and VQUIT causing SIGQUIT. Invoke user provided isig handler.
   */
  if ((tty->termios.c_lflag & ISIG)) {
    if ((c == tty->termios.c_cc[VINTR]) || (c == tty->termios.c_cc[VQUIT])) {
      rtems_termios_isig_status_code rc;
      rc = (*termios_isig_handler)(c, tty);
      if (rc == RTEMS_TERMIOS_ISIG_INTERRUPT_READ) {
         return RTEMS_TERMIOS_IPROC_INTERRUPT;
      }

      return RTEMS_TERMIOS_IPROC_CONTINUE;
    }
  }

[Chris Johns]

I am attempting to follow the flow of this ticket to see what the issue is. Sebastian, Joel pushed the patch on 2 Oct 2019, then you updated the ticket with comment:4 around Feb 1 2020 plus a patch to fix the change Joel pushed removing references to one of the enum values. The change only updated the ticket. It was confusing for me to determining the status and action that needs to happen. I have now taken the time to do this.

In regards to enum's one of the fixes for ticket #3969 highlights a subtle issue with bool as a return type then changing code to need more return values. There is no upgrade path I consider clean or safe (?) if there is a need to add more return states than a bool's true and false. The libdl ticket shows what happens if you move to an enum from bool as compilers, coverity etc cannot see enough to know any code that was if (!returned_bool_now_enum()) ... maybe wrong. In the case of libdl the true or 1 became .*_no_error and 0 so a valid or no error result was incorrectly handled in code not updated. I think in important interfaces an enum should be considered desirable even if there is only two states and even if only one state is currently being used, i.e. what we now have.

I agree the documentation should indicate how to use the enum results returned. Can someone please update the enum's comments so a user knows how to handle them if returned?

Thanks

[Sebastian Huber <sebastian.huber@…>]

In 153b2669/rtems:

termios: Replace rtems_termios_isig_status_code

Merge the rtems_termios_isig_status_code and
rtems_termios_iproc_status_code enums into a single
rtems_termios_iproc_status_code which is now a part of the API.

Simplify rtems_termios_posix_isig_handler() to avoid unreachable code.

Close #3800.