[PATCH 0/2] Enabling support for the FIFO based console in sandbox

[PATCH 0/2] Enabling support for the FIFO based console in sandbox

[Aleksander Morgado]

Hey,

I've been trying to make the FIFO based extra sandbox console work for some time now, and ended up preparing a couple of patches that seem to serve the purpose. I'm not totally sure whether this hasn't been working for a long time or if I was doing somethin wrong myself (possibly!).

The first one is a fix to allow registering multiple sandbox consoles; looks like this was not possible with the current codebase as the additional consoles were all being registered as devices with the same name and id.

The second patch is open for discussion. I wasn't able to make the setup work with separate console devices registered for input and output, the only way I could make it work was registering a console that did both input and output, so I ended up modifying it so that the logic of the application allows only that, an extra bidirectional console using two separate FIFO files. If either --stdin or --stdout is not given, or if either of them gets given multiple times, an error is issued. With this setup it works for me, I can run barebox and use either the default stdin/stdout console or the FIFO based one, but maybe we're losing other useful usecases.

What do you think?

Aleksander Morgado (2):
  sandbox: fix registering multiple consoles
  sandbox: --stdin and --stdout allow max one bidirectional console

 arch/sandbox/board/console.c                   |  5 ++-
 arch/sandbox/mach-sandbox/include/mach/linux.h |  2 +-
 arch/sandbox/os/common.c                       | 50 +++++++++++++++++++-------
 drivers/serial/linux_console.c                 |  3 ++
 4 files changed, 44 insertions(+), 16 deletions(-)

--
2.13.0

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/2] sandbox: fix registering multiple consoles

[Aleksander Morgado]

Consoles need to be registered with the "console" device name so that
they are probed by the correct driver. The barebox_register_console()
was already forcing this as it was overwriting the name that was being
passed as argument, but it was failing to provide a unique id for
each new console, so the underlying register_device() would just
return an error when wanting to re-register a device with device name
"console" and id 0.

We remove the unused name parameter from barebox_register_console() as
it is really nowhere used, and also specify DEVICE_ID_DYNAMIC as id,
so that a new unique device id is given to each newly registered
console device.

Signed-off-by: Aleksander Morgado <aleksander@aleksander.es>
---
 arch/sandbox/board/console.c                   | 5 ++---
 arch/sandbox/mach-sandbox/include/mach/linux.h | 2 +-
 arch/sandbox/os/common.c                       | 6 +++---
 drivers/serial/linux_console.c                 | 3 +++
 4 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/arch/sandbox/board/console.c b/arch/sandbox/board/console.c
index cd5ad5723..cf1781d15 100644
--- a/arch/sandbox/board/console.c
+++ b/arch/sandbox/board/console.c
@@ -22,7 +22,7 @@
 #include <mach/linux.h>
 #include <xfuncs.h>
 
-int barebox_register_console(char *name, int stdinfd, int stdoutfd)
+int barebox_register_console(int stdinfd, int stdoutfd)
 {
 	struct device_d *dev;
 	struct linux_console_data *data;
@@ -32,9 +32,8 @@ int barebox_register_console(char *name, int stdinfd, int stdoutfd)
 	data = (struct linux_console_data *)(dev + 1);
 
 	dev->platform_data = data;
-	strcpy(dev->name, name);
-
 	strcpy(dev->name, "console");
+	dev->id = DEVICE_ID_DYNAMIC;
 
 	data->stdoutfd = stdoutfd;
 	data->stdinfd  = stdinfd;
diff --git a/arch/sandbox/mach-sandbox/include/mach/linux.h b/arch/sandbox/mach-sandbox/include/mach/linux.h
index 1f11ed449..1327a56ca 100644
--- a/arch/sandbox/mach-sandbox/include/mach/linux.h
+++ b/arch/sandbox/mach-sandbox/include/mach/linux.h
@@ -19,7 +19,7 @@ void __attribute__((noreturn)) linux_exit(void);
 
 int linux_execve(const char * filename, char *const argv[], char *const envp[]);
 
-int barebox_register_console(char *name_template, int stdinfd, int stdoutfd);
+int barebox_register_console(int stdinfd, int stdoutfd);
 
 int barebox_register_dtb(const void *dtb);
 
diff --git a/arch/sandbox/os/common.c b/arch/sandbox/os/common.c
index 67667d40d..192917ac2 100644
--- a/arch/sandbox/os/common.c
+++ b/arch/sandbox/os/common.c
@@ -366,7 +366,7 @@ int main(int argc, char *argv[])
 				exit(1);
 			}
 
-			barebox_register_console("cout", -1, fd);
+			barebox_register_console(-1, fd);
 			break;
 		case 'I':
 			fd = open(optarg, O_RDWR);
@@ -375,7 +375,7 @@ int main(int argc, char *argv[])
 				exit(1);
 			}
 
-			barebox_register_console("cin", fd, -1);
+			barebox_register_console(fd, -1);
 			break;
 		case 'x':
 			sdl_xres = strtoul(optarg, NULL, 0);
@@ -426,7 +426,7 @@ int main(int argc, char *argv[])
 		}
 	}
 
-	barebox_register_console("console", fileno(stdin), fileno(stdout));
+	barebox_register_console(fileno(stdin), fileno(stdout));
 
 	rawmode();
 	start_barebox();
diff --git a/drivers/serial/linux_console.c b/drivers/serial/linux_console.c
index 760b3b81f..0d5da9d1b 100644
--- a/drivers/serial/linux_console.c
+++ b/drivers/serial/linux_console.c
@@ -73,6 +73,9 @@ static int linux_console_probe(struct device_d *dev)
 
 	console_register(cdev);
 
+	pr_info("%s: registered as %s%d\n", dev->name, cdev->class_dev.name,
+		cdev->class_dev.id);
+
 	return 0;
 }
 
-- 
2.13.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/2] sandbox: --stdin and --stdout allow max one bidirectional console

[Aleksander Morgado]

Allow up to one bidirectional FIFO/file based console in addition to
the default stdin/stdout console that is always registered.

We avoid opening the FIFO files while parsing the options because the
whole logic may block if e.g. trying to open the --stdout FIFO and
there is no reader in the other end. So instead, we store the --stdout
and --stdin file paths given, and we open both sequentially once all
the options have been parsed. This also allows us to validate that at
most a single pair of --stdin and --stdout paths has been given.
e.g.:
    term1 $ mkfifo /tmp/bbstdin
    term1 $ mkfifo /tmp/bbstdout
    term1 $ ./barebox -I /tmp/bbstdin -O /tmp/bbstdout
       (blocks until a reader is available in the stdout FIFO)

    term2 $ cat /tmp/bbstdout & cat > /tmp/bbstdin
       (starts reader and writer, which triggers barebox to continue)

If only one console is activated (CONFIG_CONSOLE_ACTIVATE_ALL=n), the
default stdin/stdout console will be preferred instead of the new
FIFO/file based, which would need to be activated explicitly later on.
e.g.:
    barebox@barebox sandbox:/ cs1.active=ioe

Signed-off-by: Aleksander Morgado <aleksander@aleksander.es>
---
 arch/sandbox/os/common.c | 48 +++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 37 insertions(+), 11 deletions(-)

diff --git a/arch/sandbox/os/common.c b/arch/sandbox/os/common.c
index 192917ac2..0461abaac 100644
--- a/arch/sandbox/os/common.c
+++ b/arch/sandbox/os/common.c
@@ -39,6 +39,7 @@
 #include <signal.h>
 #include <sys/select.h>
 #include <sys/wait.h>
+#include <linux/limits.h>
 /*
  * ...except the ones needed to connect with barebox
  */
@@ -329,9 +330,11 @@ static const char optstring[] = "hm:i:e:d:O:I:x:y:";
 int main(int argc, char *argv[])
 {
 	void *ram;
-	int opt, ret, fd;
+	int opt, ret;
 	int malloc_size = CONFIG_MALLOC_SIZE;
 	int fdno = 0, envno = 0, option_index = 0;
+	char path_stdout[PATH_MAX + 1] = { 0 };
+	char path_stdin[PATH_MAX + 1] = { 0 };
 
 	while (1) {
 		option_index = 0;
@@ -360,22 +363,18 @@ int main(int argc, char *argv[])
 			}
 			break;
 		case 'O':
-			fd = open(optarg, O_WRONLY);
-			if (fd < 0) {
-				perror("open");
+			if (path_stdout[0]) {
+				printf("error: cannot specify -O,--stdout multiple times\n");
 				exit(1);
 			}
-
-			barebox_register_console(-1, fd);
+			strncpy(path_stdout, optarg, PATH_MAX);
 			break;
 		case 'I':
-			fd = open(optarg, O_RDWR);
-			if (fd < 0) {
-				perror("open");
+			if (path_stdin[0]) {
+				printf("error: cannot specify -I,--stdin multiple times\n");
 				exit(1);
 			}
-
-			barebox_register_console(fd, -1);
+			strncpy(path_stdin, optarg, PATH_MAX);
 			break;
 		case 'x':
 			sdl_xres = strtoul(optarg, NULL, 0);
@@ -426,6 +425,33 @@ int main(int argc, char *argv[])
 		}
 	}
 
+	/* Register additional FIFO console */
+	if (path_stdout[0] || path_stdin[0]) {
+		int fdout, fdin;
+
+		/* Both must be given to build a FIFO console */
+		if (!path_stdin[0] || !path_stdout[0]) {
+			printf("error: both -I,--stdin and -O,--stdout must be"
+			       "specified to enable the FIFO console\n");
+			exit(1);
+		}
+
+		fdout = open(path_stdout, O_WRONLY);
+		if (fdout < 0) {
+			perror("open stdout");
+			exit(1);
+		}
+
+		fdin = open(path_stdin, O_RDWR);
+		if (fdin < 0) {
+			perror("open stdin");
+			exit(1);
+		}
+
+		barebox_register_console(fdin, fdout);
+	}
+
+	/* Register default stdin/stdout console */
 	barebox_register_console(fileno(stdin), fileno(stdout));
 
 	rawmode();
-- 
2.13.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] sandbox: --stdin and --stdout allow max one bidirectional console

[Sascha Hauer]

Hi Aleksander,

On Wed, May 31, 2017 at 06:12:41PM +0200, Aleksander Morgado wrote:
> Allow up to one bidirectional FIFO/file based console in addition to
> the default stdin/stdout console that is always registered.
> 
> We avoid opening the FIFO files while parsing the options because the
> whole logic may block if e.g. trying to open the --stdout FIFO and
> there is no reader in the other end. So instead, we store the --stdout
> and --stdin file paths given, and we open both sequentially once all
> the options have been parsed. This also allows us to validate that at
> most a single pair of --stdin and --stdout paths has been given.
> e.g.:
>     term1 $ mkfifo /tmp/bbstdin
>     term1 $ mkfifo /tmp/bbstdout
>     term1 $ ./barebox -I /tmp/bbstdin -O /tmp/bbstdout
>        (blocks until a reader is available in the stdout FIFO)
> 
>     term2 $ cat /tmp/bbstdout & cat > /tmp/bbstdin
>        (starts reader and writer, which triggers barebox to continue)
> 
> If only one console is activated (CONFIG_CONSOLE_ACTIVATE_ALL=n), the
> default stdin/stdout console will be preferred instead of the new
> FIFO/file based, which would need to be activated explicitly later on.
> e.g.:
>     barebox@barebox sandbox:/ cs1.active=ioe
> 
> Signed-off-by: Aleksander Morgado <aleksander@aleksander.es>
> ---
>  arch/sandbox/os/common.c | 48 +++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 37 insertions(+), 11 deletions(-)

The first patch is ok and I applied it. Before this patch though I'd like
to have the attached patch. It merely fixes what we already have: The -I
and -O options got broken over time, we ended up using the barebox
malloc pool before it was initialized.

So next we have to create a patch for what you want to do: Have an
option to create a bidirectional console in barebox. Can we have a
--file-console option that takes two path arguments instead of one, with
some separator in between?

Sascha

------------------------8<-----------------------------

From 8e5a534df784ab91ffde55ed5038e66e8dbb36c8 Mon Sep 17 00:00:00 2001
From: Sascha Hauer <s.hauer@pengutronix.de>
Date: Thu, 1 Jun 2017 10:08:31 +0200
Subject: [PATCH] sandbox: Fix registering file/fifo consoles

barebox_register_console() uses xzalloc which requires the malloc pool
to be initialized, so call it during the second option parsing when
this is already done.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 arch/sandbox/os/common.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/arch/sandbox/os/common.c b/arch/sandbox/os/common.c
index 192917ac22..8cf0873130 100644
--- a/arch/sandbox/os/common.c
+++ b/arch/sandbox/os/common.c
@@ -359,24 +359,6 @@ int main(int argc, char *argv[])
 				exit(1);
 			}
 			break;
-		case 'O':
-			fd = open(optarg, O_WRONLY);
-			if (fd < 0) {
-				perror("open");
-				exit(1);
-			}
-
-			barebox_register_console(-1, fd);
-			break;
-		case 'I':
-			fd = open(optarg, O_RDWR);
-			if (fd < 0) {
-				perror("open");
-				exit(1);
-			}
-
-			barebox_register_console(fd, -1);
-			break;
 		case 'x':
 			sdl_xres = strtoul(optarg, NULL, 0);
 			break;
@@ -384,7 +366,7 @@ int main(int argc, char *argv[])
 			sdl_yres = strtoul(optarg, NULL, 0);
 			break;
 		default:
-			exit(1);
+			break;
 		}
 	}
 
@@ -421,6 +403,24 @@ int main(int argc, char *argv[])
 			if (ret)
 				exit(1);
 			break;
+		case 'O':
+			fd = open(optarg, O_WRONLY);
+			if (fd < 0) {
+				perror("open");
+				exit(1);
+			}
+
+			barebox_register_console(-1, fd);
+			break;
+		case 'I':
+			fd = open(optarg, O_RDWR);
+			if (fd < 0) {
+				perror("open");
+				exit(1);
+			}
+
+			barebox_register_console(fd, -1);
+			break;
 		default:
 			break;
 		}
-- 
2.11.0


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] sandbox: --stdin and --stdout allow max one bidirectional console

[Aleksander Morgado]

> On Wed, May 31, 2017 at 06:12:41PM +0200, Aleksander Morgado wrote:
>> Allow up to one bidirectional FIFO/file based console in addition to
>> the default stdin/stdout console that is always registered.
>>
>> We avoid opening the FIFO files while parsing the options because the
>> whole logic may block if e.g. trying to open the --stdout FIFO and
>> there is no reader in the other end. So instead, we store the --stdout
>> and --stdin file paths given, and we open both sequentially once all
>> the options have been parsed. This also allows us to validate that at
>> most a single pair of --stdin and --stdout paths has been given.
>> e.g.:
>>     term1 $ mkfifo /tmp/bbstdin
>>     term1 $ mkfifo /tmp/bbstdout
>>     term1 $ ./barebox -I /tmp/bbstdin -O /tmp/bbstdout
>>        (blocks until a reader is available in the stdout FIFO)
>>
>>     term2 $ cat /tmp/bbstdout & cat > /tmp/bbstdin
>>        (starts reader and writer, which triggers barebox to continue)
>>
>> If only one console is activated (CONFIG_CONSOLE_ACTIVATE_ALL=n), the
>> default stdin/stdout console will be preferred instead of the new
>> FIFO/file based, which would need to be activated explicitly later on.
>> e.g.:
>>     barebox@barebox sandbox:/ cs1.active=ioe
>>
>> Signed-off-by: Aleksander Morgado <aleksander@aleksander.es>
>> ---
>>  arch/sandbox/os/common.c | 48 +++++++++++++++++++++++++++++++++++++-----------
>>  1 file changed, 37 insertions(+), 11 deletions(-)
>
> The first patch is ok and I applied it. Before this patch though I'd like
> to have the attached patch. It merely fixes what we already have: The -I
> and -O options got broken over time, we ended up using the barebox
> malloc pool before it was initialized.
>

Ah! I see, just moving the 'O' and 'I' parsing to the second getopt
makes it work. I actually knew about the malloc pool being prepared in
between both getopt runs, as my attempts to strdup() the file paths in
the first run didn't succeed, I just didn't realize it was also
affecting the other logic :)

> So next we have to create a patch for what you want to do: Have an
> option to create a bidirectional console in barebox. Can we have a
> --file-console option that takes two path arguments instead of one, with
> some separator in between?
>

Well, is this even needed any more? I'm fine having separate in/out
consoles registered, as long as they work.

>
> ------------------------8<-----------------------------
>
> From 8e5a534df784ab91ffde55ed5038e66e8dbb36c8 Mon Sep 17 00:00:00 2001
> From: Sascha Hauer <s.hauer@pengutronix.de>
> Date: Thu, 1 Jun 2017 10:08:31 +0200
> Subject: [PATCH] sandbox: Fix registering file/fifo consoles
>
> barebox_register_console() uses xzalloc which requires the malloc pool
> to be initialized, so call it during the second option parsing when
> this is already done.
>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  arch/sandbox/os/common.c | 38 +++++++++++++++++++-------------------
>  1 file changed, 19 insertions(+), 19 deletions(-)
>
> diff --git a/arch/sandbox/os/common.c b/arch/sandbox/os/common.c
> index 192917ac22..8cf0873130 100644
> --- a/arch/sandbox/os/common.c
> +++ b/arch/sandbox/os/common.c
> @@ -359,24 +359,6 @@ int main(int argc, char *argv[])
>                                 exit(1);
>                         }
>                         break;
> -               case 'O':
> -                       fd = open(optarg, O_WRONLY);
> -                       if (fd < 0) {
> -                               perror("open");
> -                               exit(1);
> -                       }
> -
> -                       barebox_register_console(-1, fd);
> -                       break;
> -               case 'I':
> -                       fd = open(optarg, O_RDWR);
> -                       if (fd < 0) {
> -                               perror("open");
> -                               exit(1);
> -                       }
> -
> -                       barebox_register_console(fd, -1);
> -                       break;
>                 case 'x':
>                         sdl_xres = strtoul(optarg, NULL, 0);
>                         break;
> @@ -384,7 +366,7 @@ int main(int argc, char *argv[])
>                         sdl_yres = strtoul(optarg, NULL, 0);
>                         break;
>                 default:
> -                       exit(1);
> +                       break;
>                 }
>         }
>
> @@ -421,6 +403,24 @@ int main(int argc, char *argv[])
>                         if (ret)
>                                 exit(1);
>                         break;
> +               case 'O':
> +                       fd = open(optarg, O_WRONLY);
> +                       if (fd < 0) {
> +                               perror("open");
> +                               exit(1);
> +                       }
> +
> +                       barebox_register_console(-1, fd);
> +                       break;
> +               case 'I':
> +                       fd = open(optarg, O_RDWR);
> +                       if (fd < 0) {
> +                               perror("open");
> +                               exit(1);
> +                       }
> +
> +                       barebox_register_console(fd, -1);
> +                       break;
>                 default:
>                         break;
>                 }
> --
> 2.11.0
>
>
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |



-- 
Aleksander
https://aleksander.es

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] sandbox: --stdin and --stdout allow max one bidirectional console

[Sascha Hauer]

On Thu, Jun 01, 2017 at 10:28:42AM +0200, Aleksander Morgado wrote:
> > On Wed, May 31, 2017 at 06:12:41PM +0200, Aleksander Morgado wrote:
> >> Allow up to one bidirectional FIFO/file based console in addition to
> >> the default stdin/stdout console that is always registered.
> >>
> >> We avoid opening the FIFO files while parsing the options because the
> >> whole logic may block if e.g. trying to open the --stdout FIFO and
> >> there is no reader in the other end. So instead, we store the --stdout
> >> and --stdin file paths given, and we open both sequentially once all
> >> the options have been parsed. This also allows us to validate that at
> >> most a single pair of --stdin and --stdout paths has been given.
> >> e.g.:
> >>     term1 $ mkfifo /tmp/bbstdin
> >>     term1 $ mkfifo /tmp/bbstdout
> >>     term1 $ ./barebox -I /tmp/bbstdin -O /tmp/bbstdout
> >>        (blocks until a reader is available in the stdout FIFO)
> >>
> >>     term2 $ cat /tmp/bbstdout & cat > /tmp/bbstdin
> >>        (starts reader and writer, which triggers barebox to continue)
> >>
> >> If only one console is activated (CONFIG_CONSOLE_ACTIVATE_ALL=n), the
> >> default stdin/stdout console will be preferred instead of the new
> >> FIFO/file based, which would need to be activated explicitly later on.
> >> e.g.:
> >>     barebox@barebox sandbox:/ cs1.active=ioe
> >>
> >> Signed-off-by: Aleksander Morgado <aleksander@aleksander.es>
> >> ---
> >>  arch/sandbox/os/common.c | 48 +++++++++++++++++++++++++++++++++++++-----------
> >>  1 file changed, 37 insertions(+), 11 deletions(-)
> >
> > The first patch is ok and I applied it. Before this patch though I'd like
> > to have the attached patch. It merely fixes what we already have: The -I
> > and -O options got broken over time, we ended up using the barebox
> > malloc pool before it was initialized.
> >
> 
> Ah! I see, just moving the 'O' and 'I' parsing to the second getopt
> makes it work. I actually knew about the malloc pool being prepared in
> between both getopt runs, as my attempts to strdup() the file paths in
> the first run didn't succeed, I just didn't realize it was also
> affecting the other logic :)
> 
> > So next we have to create a patch for what you want to do: Have an
> > option to create a bidirectional console in barebox. Can we have a
> > --file-console option that takes two path arguments instead of one, with
> > some separator in between?
> >
> 
> Well, is this even needed any more? I'm fine having separate in/out
> consoles registered, as long as they work.

Ah, great, then we're done. I thought your goal was to have a single
console in barebox capable of both input and output.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox