Re: [RFC v4] edit: Add basic support for input line editing

On Dec 22, 2023, at 2:14 PM, Marcel Holtmann <[email protected]> wrote:
> 
> This allows for simple line editing with history capabilities. On
> purpose this has no concept of terminal input or terminal output and
> just allows manipulation of an internal wide character string.
> 
> The debug option is something that might need to be removed or at least
> changed a little bit, but right now it is nice to see the internal
> states.
> 
> Following features are missing:
> 
> 1) Work with words (delete, move etc.)
> 2) Tab completion
> 3) Hints system
> 
> Following features are left to the user:
> 
> 1) Showing the prompt
> 2) Switching to masked input
> 
> The demo-edit is just for demonstration purposes and requires Curses to
> be available without any autoconf magic.
> 
> With demo-cli there is a really simple command line handling example
> that uses an internal terminal abstraction.

Marcel:

Thank you again for pulling this together. This looks to be in strong shape for production use. This certainly bests libedit/editline for being non-blocking, readline for licensing friendliness, and linenoise for being far more customizable and flexible.

I’ve encountered a few areas that I think could improve use and integration along with a few unknowns that I’ve yet been able to track down, with or without analysis tools.

Happy to post a full diff against v4 if that’s helpful.

1. The fatal errors from demo-cli and demo-edit are probably best as warnings since not having a UTF-8 locale (my system has the C and POSIX locales but that’s it) does not seem fatal as long as the application is restricted to ASCII (and even then, it still may not be fatal):

    if (strcmp(nl_langinfo(CODESET), ENCODING_UTF8)) {
        fprintf(stderr, "WARNING: no %s codeset / character map; non-ASCII characters may not work\n", ENCODING_UTF8);
    }

2. Other than introspection functions, ideally all of the functions that can have would have an ‘int’ or ‘int32_t’ return signature and return ‘0’ on success or ‘-<some POSIX error>’ on failure. With the Booleans, I find I have to synthesize an error any way for my application. It’d be easier to just to pass along whatever error l_edit or l_term encountered.

3. Ideally, all callbacks would take an instance pointer as their first argument, for example:

    typedef void (*l_edit_display_func_t) (struct l_edit *edit, const wchar_t *wstr, size_t wlen, size_t pos, void *user_data);

    typedef void (*l_term_key_func_t) (struct l_term *term, wint_t wch, void *user_data);

4. Ideally, the following would be public functions:

    int l_term_set_input(struct l_term *term, int fd);
    int l_term_set_output(struct l_term *term, int fd);

My application already has weak pointers to ’stdin’ and ’stdout’. I am able to use the above to just do:

    l_term_set_input(fileno(my_input_stream));
    l_term_set_output(fileno(my_output_stream));

5. Ideally, l_term would be more run loop adaptable. Toward that end, I would propose:

    typedef void (*l_term_io_func_t)(struct l_term *term,
                                     int fd,
                                     bool readable,
                                     bool writable,
                                     void *user_data);

    int l_term_set_io_handler(struct l_term *term,
                               l_term_io_func_t handler,
                               void *user_data);

with the following implementation added to / changed in term.c:

    #define IO_HANDLER(term, fd, readable, writable) \
        do {                                         \
            if (term && term->io_handler) {          \
                term->io_handler(term,               \
                                 fd,                 \
                                 readable,           \
                                 writable,           \
                                 term->io_data);     \
            }                                        \
        } while (0);

This is invoked with:

    IO_HANDLER(term, term->in_fd, 1, 0);
    IO_HANDLER(term, term->out_fd, 0, 1);

at the end of l_term_open and:

    IO_HANDLER(term, term->in_fd, 0, 0);
    IO_HANDLER(term, term->out_fd, 0, 0);

at the start of l_term_close. Both in lieu of the existing l_io calls.

It is also used in the newly-proposed l_term_set_input and l_term_set_output calls below.

    LIB_EXPORT int l_term_set_io_handler(struct l_term *term,
                    l_term_io_func_t handler, void *user_data)
    {
        if (!term)
            return -EINVAL;
    
        term->io_handler = handler;
        term->io_data = user_data;
    
        return 0;
    }
    
    LIB_EXPORT int l_term_set_input(struct l_term *term, int fd)
    {
        if (!term)
            return -EINVAL;
    
        if (fd < 0)
            return -EBADF;
    
        term->in_fd = fd;
        term->in_ops = NULL;
    
        IO_HANDLER(term, fd, 1, 0);
    
        return 0;
    }
    
    LIB_EXPORT int l_term_set_output(struct l_term *term, int fd)
    {
        if (!term)
            return -EINVAL;
    
        if (fd < 0)
            return -EBADF;
    
        term->out_fd = fd;
        term->out_ops = NULL;
    
        IO_HANDLER(term, fd, 0, 1);
    
        return 0;
    }
    
    LIB_EXPORT bool l_term_set_input_stdin(struct l_term *term)
    {
        return l_term_set_input(term, STDIN_FILENO) == 0;
    }
    
    LIB_EXPORT bool l_term_set_output_stdout(struct l_term *term)
    {
        return l_term_set_output(term, STDOUT_FILENO) == 0;
    }
    
Finally, I propose ‘l_term_process’ (or whatever it might be appropriate to name it as):

    LIB_EXPORT void l_term_process(struct l_term *term)
    {
        wchar_t wstr[2];
        ssize_t len;
        mbstate_t ps;
    
        if (!term)
            return;
    
        len = read(term->in_fd, term->key_buf + term->key_len,
                        sizeof(term->key_buf) - term->key_len);
        if (len < 0)
            return;
    
        term->key_len += len;
    
        while (term->key_len > 0) {
            memset(&ps, 0, sizeof(ps));
    
            len = mbrtowc(wstr, term->key_buf, term->key_len, &ps);
            if (len < 0)
                break;
    
            memmove(term->key_buf, term->key_buf + len,
                            term->key_len - len);
            term->key_len -= len;
    
            if (term->key_handler) {
                wint_t wch = wstr[0];
                term->key_handler(term, wch, term->key_data);
            }
        }
    }
    
which is effectively the main body of ‘in_callback’ which could be preserved and made public as an adjunct helper function for applications that want “all-in” on ELL:

    LIB_EXPORT bool l_term_io_callback(struct l_io *io, void *user_data)
    {
        struct l_term *term = user_data;
        
        l_term_process(term);
    
        return true;
    }

The addition of ‘l_term_set_bounds’:

    int l_term_set_bounds(struct l_term *term, uint16_t rows, uint16_t columns);

would likewise allow the bounds changing and SIGWINCH handling to be outside of l_term with a public helper that could / can be leveraged for those that want “all-in” on ELL. I haven’t fully completed that refactoring yet.

6. I suspect integrators will end up copy-and-pasting the whole of handle_input from demo-cli frequently which is mostly an emacs-style key binding “default". That is certainly where I started with hooks added for handling LF (basically “handle input”, the crux of the application) and Ctrl-C (which stops and quits my run loop). I’ve not yet forecast the right API, but it seems like there is a l_edit_emacs_handler or some such that can be set with a companion input_handler and control_handler or some such which pass along the input line and control sequence for application-specific handling.

7. l_edit_set_max_display_length unsurprisingly calls update_display which calls display_handler. Consequently, calling l_edit_set_max_display_length from display_handler creates an infinite loop. A warning comment probably serve future integrators well.

8. Finally, there seems to be a memory smasher somewhere. When I integrate l_edit and l_term, my application crashes during initialization with heap corruption sometime after I do initialization which amounts to:

    status_t ElementController :: InitEll(
        FILE *inInputStream,
        FILE *inOutputStream,
        FILE *inErrorStream
    )
    {
        struct l_term *  lEllTerm = nullptr;
        struct l_edit *  lEllEdit = nullptr;
        bool             lOpened  = false;
        bool             lStatus;
        status_t         lRetval  = STATUS_SUCCESS;
    
        nlREQUIRE_ACTION(inInputStream  != nullptr, done, lRetval = -EINVAL);
        nlREQUIRE_ACTION(inOutputStream != nullptr, done, lRetval = -EINVAL);
        nlREQUIRE_ACTION(inErrorStream  != nullptr, done, lRetval = -EINVAL);
    
        mDescriptorEventMap.clear();
    
        lEllTerm = l_term_new();
        nlREQUIRE_ACTION(lEllTerm != nullptr, done, lRetval = -ENOMEM);
    
        lEllEdit = l_edit_new();
        nlREQUIRE_ACTION(lEllEdit != nullptr, done, lRetval = -ENOMEM);
    
        lStatus = l_term_set_io_handler(lEllTerm, DescriptorCallback, this);
        nlREQUIRE_ACTION(lStatus, done, lRetval = ERROR_INITIALIZATION_FAILED);
    
        lStatus = l_term_set_key_handler(lEllTerm, KeyCallback, this);
        nlREQUIRE_ACTION(lStatus, done, lRetval = ERROR_INITIALIZATION_FAILED);
    
        lRetval = l_term_set_input(lEllTerm, fileno(inInputStream));
        nlREQUIRE_SUCCESS(lRetval, done);
    
        lRetval = l_term_set_output(lEllTerm, fileno(inOutputStream));
        nlREQUIRE_SUCCESS(lRetval, done);
    
        lOpened = l_term_open(lEllTerm);
        nlREQUIRE_ACTION(lOpened, done, lRetval = ERROR_INITIALIZATION_FAILED);
    
        lStatus = l_term_get_max_columns(lEllTerm, &mTermColumnsMax);
        nlREQUIRE_ACTION(lStatus, done, lRetval = ERROR_INITIALIZATION_FAILED);
    
        lStatus = l_edit_set_max_display_length(lEllEdit, mTermColumnsMax);
        nlREQUIRE_ACTION(lStatus, done, lRetval = ERROR_INITIALIZATION_FAILED);
    
        lStatus = l_edit_set_display_handler(lEllEdit, DisplayCallback, this);
        nlREQUIRE_ACTION(lStatus, done, lRetval = ERROR_INITIALIZATION_FAILED);
    
        mEllTerm = lEllTerm;
        mEllEdit = lEllEdit;
    
     done:
        if (lRetval < STATUS_SUCCESS)
        {
            if (lEllEdit != nullptr)
            {
                l_edit_free(lEllEdit);
            }
    
            if (lEllTerm != nullptr)
            {
                if (lOpened)
                    l_term_close(lEllTerm);
    
                l_term_free(lEllTerm);
            }
        }
        
        return (lRetval);
    }

If I return to using libedit/editline, it’s fine. If I just completely remove l_edit/l_term, it’s fine. Unfortunately and surprisingly, neither clang scan-build nor asan are able to zero in fully on the issue. Though, when it crashes on Armv7, asan indicates a read on 0x0 and often a corrupted PC and stack pointer of 0x0. My suspicion is there is a null or garbage dereference happening somewhere.

Best,

Grant

-- 
Principal
Nuovations

[email protected]
https://www.nuovations.com/