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

[Aleksander Morgado <aleksander@aleksander.es>]

> 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