~emersion/mrsh-dev

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
4 2

[PATCH mrsh v2] Add optional readline/libedit support

Details
Message ID
<20180916190045.11499-1-sir@cmpwn.com>
Sender timestamp
1537124445
DKIM signature
missing
Download raw message
Patch: +111 -14
This adds support for line editing, history, etc, either with GNU
readline or BSD libedit.
---
Differences from the last patch:

- The interactive interface has been somewhat generalized to support
  multiple frontends, and the readline code has accordingly been moved
  into its own file.
- History can now be turned off at runtime and is written after every
  command, instead of all at the end.

Future directions:

- Completion

 basic.c               | 17 +++++++++++++++++
 include/mrsh.h        | 11 +++++++++++
 include/mrsh/parser.h |  1 +
 main.c                | 31 +++++++++++++++++++++----------
 meson.build           | 18 ++++++++++++++----
 parser/parser.c       |  4 ++++
 readline.c            | 43 +++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 111 insertions(+), 14 deletions(-)
 create mode 100644 basic.c
 create mode 100644 include/mrsh.h
 create mode 100644 readline.c

diff --git a/basic.c b/basic.c
new file mode 100644
index 0000000..de25eed
--- /dev/null
@@ -0,0 +1,17 @@
/* Basic, strictly POSIX interactive line interface */
#include <stdio.h>
#include <stdlib.h>
#include <mrsh/shell.h>
#include "mrsh.h"

FILE *interactive_init(FILE *input, struct mrsh_state *state) {
	return input;
}

FILE *interactive_next(FILE *input, struct mrsh_state *state) {
	char *ps1 = get_ps1(state);
	fprintf(stderr, "%s", ps1);
	fflush(stderr);
	free(ps1);
	return input;
}
diff --git a/include/mrsh.h b/include/mrsh.h
new file mode 100644
index 0000000..fa6a01d
--- /dev/null
+++ b/include/mrsh.h
@@ -0,0 +1,11 @@
#ifndef _MRSH_H
#define _MRSH_H

// From main.c
char *get_ps1(struct mrsh_state *state);

// From frontend drivers
FILE *interactive_init(FILE *input, struct mrsh_state *state);
FILE *interactive_next(FILE *input, struct mrsh_state *state);

#endif
diff --git a/include/mrsh/parser.h b/include/mrsh/parser.h
index 695d7a4..33f6edc 100644
--- a/include/mrsh/parser.h
+++ b/include/mrsh/parser.h
@@ -15,6 +15,7 @@ void mrsh_parser_destroy(struct mrsh_parser *state);
struct mrsh_program *mrsh_parse_program(struct mrsh_parser *state);
struct mrsh_program *mrsh_parse_line(struct mrsh_parser *state);
struct mrsh_word *mrsh_parse_word(struct mrsh_parser *state);
void mrsh_parser_set_file(struct mrsh_parser *parser, FILE *f);
bool mrsh_parser_eof(struct mrsh_parser *state);
void mrsh_parser_set_alias(struct mrsh_parser *state,
	mrsh_parser_alias_func_t alias, void *user_data);
diff --git a/main.c b/main.c
index cf8a049..11e6acc 100644
--- a/main.c
+++ b/main.c
@@ -6,9 +6,11 @@
#include <mrsh/parser.h>
#include <mrsh/shell.h>
#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#include <unistd.h>
#include "builtin.h"
#include "mrsh.h"

char *expand_ps1(struct mrsh_state *state, const char *ps1) {
	struct mrsh_parser *parser =
@@ -24,18 +26,16 @@ char *expand_ps1(struct mrsh_state *state, const char *ps1) {
	return mrsh_word_str(word);
}

static void print_ps1(struct mrsh_state *state) {
char *get_ps1(struct mrsh_state *state) {
	const char *ps1 = mrsh_hashtable_get(&state->variables, "PS1");
	if (ps1 != NULL) {
		char *expanded_ps1 = expand_ps1(state, ps1);
		// TODO: Replace ! with next history ID
		fprintf(stderr, "%s", expanded_ps1);
		free(expanded_ps1);
		return expand_ps1(state, ps1);
	} else {
		fprintf(stderr, "%s", getuid() ? "$ " : "# ");
		char *p = malloc(3);
		sprintf(p, "%s", getuid() ? "$ " : "# ");
		return p;
	}

	fflush(stderr);
}

static void source_profile(struct mrsh_state *state) {
@@ -91,14 +91,24 @@ int main(int argc, char *argv[]) {
	source_profile(&state);

	// TODO: set PWD
	
	FILE *input = state.input;
	if (state.interactive) {
		input = interactive_init(input, &state);
	}

	struct mrsh_parser *parser = mrsh_parser_create(state.input);
	struct mrsh_parser *parser = mrsh_parser_create(input);
	mrsh_parser_set_alias(parser, get_alias, &state);
	while (state.exit == -1) {
		struct mrsh_program *prog;
		if (state.interactive) {
			print_ps1(&state);
			FILE *_input = interactive_next(input, &state);
			if (_input != input) {
				mrsh_parser_set_file(parser, input);
				input = _input;
			}
		}
		struct mrsh_program *prog = mrsh_parse_line(parser);
		prog = mrsh_parse_line(parser);
		if (prog == NULL) {
			struct mrsh_position err_pos;
			const char *err_msg = mrsh_parser_error(parser, &err_pos);
@@ -129,5 +139,6 @@ int main(int argc, char *argv[]) {
	mrsh_parser_destroy(parser);
	mrsh_state_finish(&state);
	fclose(state.input);

	return state.exit;
}
diff --git a/meson.build b/meson.build
index 66b5d2c..3283d16 100644
--- a/meson.build
+++ b/meson.build
@@ -17,6 +17,7 @@ add_project_arguments('-Wno-missing-field-initializers', language: 'c')

cc = meson.get_compiler('c')

readline = cc.find_library('readline', required: false)
rt = cc.find_library('rt')

mrsh_inc = include_directories('include')
@@ -83,12 +84,21 @@ mrsh = declare_dependency(
	include_directories: mrsh_inc,
)

shell_deps = [mrsh_static]
shell_files = [
	'main.c'
]
if readline.found()
	shell_deps += [readline]
	shell_files += ['readline.c']
else
	shell_files += ['basic.c']
endif

executable(
	'mrsh',
	files([
		'main.c',
	]),
	dependencies: [mrsh_static],
	files(shell_files),
	dependencies: shell_deps,
	install: true,
)

diff --git a/parser/parser.c b/parser/parser.c
index a091af6..409d156 100644
--- a/parser/parser.c
+++ b/parser/parser.c
@@ -71,6 +71,10 @@ struct mrsh_parser *mrsh_parser_create_from_buffer(const char *buf, size_t len)
	return state;
}

void mrsh_parser_set_file(struct mrsh_parser *parser, FILE *f) {
	parser->f = f;
}

void mrsh_parser_destroy(struct mrsh_parser *state) {
	if (state == NULL) {
		return;
diff --git a/readline.c b/readline.c
new file mode 100644
index 0000000..e605e36
--- /dev/null
+++ b/readline.c
@@ -0,0 +1,43 @@
/* readline/libedit interactive line interface */
#define _POSIX_C_SOURCE 200809L
#include <limits.h>
#include <stdio.h>
#include <stdlib.h>
#include <readline/history.h>
#include <readline/readline.h>
#include <mrsh/shell.h>
#include <mrsh/parser.h>
#include "mrsh.h"

static const size_t rl_buf_size = 4096;

static const char *get_history_path() {
	static char history_path[PATH_MAX + 1];
	snprintf(history_path, sizeof(history_path),
			"%s/.mrsh_history", getenv("HOME"));
	return history_path;
}

FILE *interactive_init(FILE *input, struct mrsh_state *state) {
	input = fmemopen(NULL, rl_buf_size, "w+");
	rl_initialize();
	read_history(get_history_path());
	return input;
}

FILE *interactive_next(FILE *input, struct mrsh_state *state) {
	char *ps1 = get_ps1(state);
	char *line = readline(ps1);
	free(ps1);
	if (line != NULL) {
		fclose(input);
		input = fmemopen(NULL, rl_buf_size, "w+");
		fprintf(input, "%s\n", line);
		fseek(input, 0, SEEK_SET);
		if (!(state->options & MRSH_OPT_NOLOG)) {
			add_history(line);
			write_history(get_history_path());
		}
	}
	return input;
}
-- 
2.19.0
Details
Message ID
<lKAQDZVcFpYqE9Waibf-HqyQbqvsfvcYBKLUcshR-qbyObW8RFwrC68O97el0f5abZYF9hL862MhrvlEckJyZCrvcWbSD1lcYlpcoQLS47o=@emersion.fr>
In-Reply-To
<20180916190045.11499-1-sir@cmpwn.com> (view parent)
Sender timestamp
1537129860
DKIM signature
missing
Download raw message
> This adds support for line editing, history, etc, either with GNU
> readline or BSD libedit.
> ---
> Differences from the last patch:
>
> - The interactive interface has been somewhat generalized to support
>   multiple frontends, and the readline code has accordingly been moved
>   into its own file.
> - History can now be turned off at runtime and is written after every
>   command, instead of all at the end.
>
> Future directions:
>
> - Completion

Thanks for this patch! This is something that we'll need sooner or
later anyway for interactive shells built on top of mrsh.

The main issue with this patch is continuation lines. Here are some
problematic commands:

  echo a \
  b c

or

  echo "a
  b c"

or

  cat <<EOF
  a b c
  EOF

We probably need a way for the parser to tell "I need more lines". That way,
the shell could buffer the current line, read the next one and parse again
the input.
Details
Message ID
<20180916203341.GA4102@homura.localdomain>
In-Reply-To
<lKAQDZVcFpYqE9Waibf-HqyQbqvsfvcYBKLUcshR-qbyObW8RFwrC68O97el0f5abZYF9hL862MhrvlEckJyZCrvcWbSD1lcYlpcoQLS47o=@emersion.fr> (view parent)
Sender timestamp
1537130021
DKIM signature
missing
Download raw message
On 2018-09-16  8:31 PM, Simon Ser wrote:
> We probably need a way for the parser to tell "I need more lines". That way,
> the shell could buffer the current line, read the next one and parse again
> the input.

I think that's kind of out of scope for this patch, can we merge this
as-is and address that later?
Details
Message ID
<G_ZOUWCJltu86EOYzyb-wOm9pmOE93F53xt-plPYuYdf-QQjAuscR3Q2akuUHcvs_eLfx8BQ82zZST2qBqO8AZ1t5imvxP5qZA8hLIxzVHg=@emersion.fr>
In-Reply-To
<20180916203341.GA4102@homura.localdomain> (view parent)
Sender timestamp
1537132738
DKIM signature
missing
Download raw message
On Sunday, September 16, 2018 10:33 PM, Drew DeVault <sir@cmpwn.com> wrote:
> On 2018-09-16 8:31 PM, Simon Ser wrote:
>
> > We probably need a way for the parser to tell "I need more lines". That way,
> > the shell could buffer the current line, read the next one and parse again
> > the input.
>
> I think that's kind of out of scope for this patch, can we merge this
> as-is and address that later?

Yeah, I'm fine with this. I'm not a fan of mrsh_parser_set_file,
because it might end up in a bad state (if something's left in the
internal buffer for instance). It's not that of a big deal though.

Since it's not complete yet, is it possible to add a build option to
disable it by default for now?
Details
Message ID
<DFYGfwkWgJrdS7UVvQjAZrxUiDBN6bn2IKZ3sXabLrjMlpqH8pV8g5UOCdTkvBNzLMpwOPPIExFiulRbFD6_pHFTkqLJB5oGJmdmAeWvhe0=@emersion.fr>
In-Reply-To
<G_ZOUWCJltu86EOYzyb-wOm9pmOE93F53xt-plPYuYdf-QQjAuscR3Q2akuUHcvs_eLfx8BQ82zZST2qBqO8AZ1t5imvxP5qZA8hLIxzVHg=@emersion.fr> (view parent)
Sender timestamp
1537373576
DKIM signature
missing
Download raw message
Just added a new mrsh_parser_continuation_line function that
checks if the input ends on a continuation line. I think it
should be enough to call it after parsing and buffer the line
in case it returns true.
Review patch Export thread (mbox)