Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

std::cin doesn't read pipe redirection #320

Open
wmarini opened this issue Oct 24, 2023 · 8 comments
Open

std::cin doesn't read pipe redirection #320

wmarini opened this issue Oct 24, 2023 · 8 comments

Comments

@wmarini
Copy link

wmarini commented Oct 24, 2023

Hello,

I'd like to present a use case that I'm currently working on.

I'm trying to create a program that changes the color of some words based on a defined pattern. I'd like to use this program with pipe redirection. Something similar to the following bash script below:

#!/usr/bin/env bash

RED=$(echo -e '\033[1;31m')
CLEAR=$(echo -e '\033[0m')

sed -E "s/\b(Jan|Feb|Mar|Apr|May|Jun|Jul|Aug|Sep|Oct|Nov|Dec)\b/${RED}&${CLEAR}/g"

The direct use of this script is using unamed pipe redirection, like (manually edited to illustrate the months written in red color):

$ cat /tmp/test.txt | ./hlight.bash
\33[0;31mJanJan\33[0m 31st
\33[0;31mFebFeb\33[0m 01st
\33[0;31mMayMay\33[0m 15th

I tried making a C++20 program in Linux (Ubuntu 22.04, xterm) with that same behavior and also using pipe redirection, but it seems that program never reads the pipe input because the cpp-terminal library modifies the std::cin.rdbuf in iostream.cpp and discards the initial stdin content during the program initialization.

void Term::StreamInitializer::init()
{
  if(m_counter++ == 0)
  {
    std::ios_base::Init();
    new(&Term::cout) TOstream(Term::Buffer::Type::FullBuffered, BUFSIZ);
    new(&Term::clog) TOstream(Term::Buffer::Type::LineBuffered, BUFSIZ);
    new(&Term::cerr) TOstream(Term::Buffer::Type::Unbuffered, 0);
    new(&Term::cin) TIstream(Term::Buffer::Type::FullBuffered, BUFSIZ);
    std::cin.rdbuf(Term::cin.rdbuf());
  }
}

void Term::Private::FileInitializer::init()
{   
    // ...
#if defined(_WIN32)
    // ...
#else
    new(&Term::Private::in) InputFileHandler(io_mutex, "/dev/tty");
    new(&Term::Private::out) OutputFileHandler(io_mutex, "/dev/tty");
#endif
    // ...
}

I believe this is a project decision, but instead of always opening a new /dev/tty in file.cpp::Term::Private::FileInitializer::init, it might be useful to reuse the already opened fd = 0 (stdin) when the program uses pipe redirection. A possible solution could be addind a new ctor Term::Private::FileHandler::FileHandler that reuses stdin instead.

To ensure that the program correctly opens a pipe redirection for stdin, you can verify it by using the isatty function. So the Term::Private::FileInitializer::init can be modified to something like:

void Term::Private::FileInitializer::init()
{   
    // ...
#if defined(_WIN32)
    // ...
#else
    if (isatty(STDIN_FILENO)) {
        new(&Term::Private::in) InputFileHandler(io_mutex, stdin);
    } else {
        new(&Term::Private::in) InputFileHandler(io_mutex, "/dev/tty");
    }
    new(&Term::Private::out) OutputFileHandler(io_mutex, "/dev/tty");
#endif
    // ...
}

I aim to enable cpp-terminal to maintain the same expected behavior for std::cin in all scenarios.

Thank you!

@flagarde
Copy link
Collaborator

flagarde commented Oct 24, 2023

@wmarini Thx for the report, In fact it is a bug I think. You are correct saying that Term::cout Term::clog Term::cin are always connected to the terminal by design.

I would like to let the Term::cin always be bind to the terminal /dev/tty. I think what could be done is to redesign the buffer class to be able to change to whom is talking to and do :

  if(m_counter++ == 0)
  {
    std::ios_base::Init();
    new(&Term::cout) TOstream(Term::Buffer::Type::FullBuffered, BUFSIZ);
    new(&Term::clog) TOstream(Term::Buffer::Type::LineBuffered, BUFSIZ);
    new(&Term::cerr) TOstream(Term::Buffer::Type::Unbuffered, 0);
    new(&Term::cin) TIstream(Term::Buffer::Type::FullBuffered, BUFSIZ);
    std::cin.rdbuf(Term::rdbuf(stdin));
  }

The use of the buffer class in this case std::cin.rdbuf(Term::cin.rdbuf()); is to be able to read form std::cin even when the terminal is in raw mode. The fact that is always reading to /dev/tty is not intentionnal but just a lack of modularity of the class buffer.

I think we could redesigne the buffer class to add the possibility to change to whom is talking and this would solve your problem right ?

Could you provide a very basic code you wanted it to work? We could add it as example or test to avoid such problem in the future

In my humble opinion, to assure that Term::cin clog cerr etc are always binded to /dev/tty is something important. User can then choose std::cout if he want the consumer to be able to redirect some messages

@flagarde
Copy link
Collaborator

flagarde commented Oct 24, 2023

The buffer class.. is not optimal; it is my first time dealing with IOStreams internals and the design is a bit hard to follow for me, at least for now. I need time to study and digest the Standard C++ Iostreams and Locales: Advanced Programmer's Guide and Reference.

@flagarde
Copy link
Collaborator

Some toy code : master...flagarde:cpp-terminal:master

It seems to do what you want on linux, need to check windows and there is many bugs too, we need to deal with raw and cooked by ourself, but using the example ./cin_raw or ./cin_cooked it seems to do what you want :

echo "1234 toto" | ./cin_cooked

but then we trigger the last issue you mentionned before (#316). It the path I would like to follow but it need more cleaning

And #316 need a better fix, I just did a naive one, I knew it would be triggered in some corner cases.

@wmarini
Copy link
Author

wmarini commented Oct 24, 2023

Hi, sure, I can provide an example program that handles that scenario. Let me review the example directory first and prepare something that is not covered in other example program.

@flagarde
Copy link
Collaborator

Thanks, I will try to fix #316 for all cases. It's necessary for the buffer changes. I hope I will be able to have a better buffer class soon

@flagarde
Copy link
Collaborator

@wmarini I have made some change on the code. It seems it is closer to what you want to do. Doing :

echo "42 Hello" | ./cin_raw

or :

echo "42 Hello" | ./cin_cooked

you can catch the value using std::cin. Don't use Term::cin as it is always binded to the terminal. Maybe need more polishing but the behaviour you want seems possible now.

@wmarini
Copy link
Author

wmarini commented Oct 28, 2023

Excellent! It's working now!
My preliminary test works as expected:

$ echo -e "test std::cin\ntest stdin" | ./pipe_cin 
Checking if std::cin is empty
std::cin => "test std::cin"
Checking if stdin is empty
stdin    => "test stdin"

I did that simple test just to make sure stdin and std::cin are using the pipe to read input:

#include "cpp-terminal/color.hpp"
#include "cpp-terminal/terminal.hpp"

#include <iostream>
#include <string>
#include <sys/select.h>
#include <unistd.h>

namespace {

bool isStdinEmpty()
{
  struct timeval tv{0,0};
  fd_set rfds;
  FD_ZERO(&rfds);
  FD_SET(STDIN_FILENO, &rfds);
  return select(STDIN_FILENO + 1, &rfds, nullptr, nullptr, &tv) == 0;
}

std::string ReadStdin()
{
  if (isStdinEmpty()) {
    return "stdin is empty";
  }
  std::string stdin_result;
  char c;
  while((c = static_cast<char>(std::fgetc(stdin))) != EOF) {
    if (c == '\n')
      break;
    stdin_result += c;
  }
  return stdin_result;
}

bool isCinPipe()
{
  return isatty(fileno(stdin)) == 0;
}

std::string ReadCin()
{
  if (!isCinPipe()) {
    return "std::cin isn't an unamed pipe";
  }
  std::string cin_result;
  char c;
  while((c = static_cast<char>(std::cin.get())) != '\n') {
      cin_result += c;
  }
  return cin_result;
}

std::ostream& PrintOutColor(std::ostream& ostr, std::string const& str, Term::Color::Name color)
{
  ostr << "\"" <<
    Term::color_fg(color) << str << Term::color_fg(Term::Color::Name::Default) 
    << "\"" << std::endl;
  return ostr;
}

} // namespace

int main()
{
  std::string cinBuff{};
  
  std::cout << "Checking if std::cin is empty" << std::endl;
  cinBuff = ReadCin();
  std::cout << "std::cin => ";
  PrintOutColor(std::cout, cinBuff, Term::Color::Name::Red);

  
  std::cout << "Checking if stdin is empty" << std::endl;
  cinBuff = ReadStdin();
  std::cout << "stdin    => ";
  PrintOutColor(std::cout, cinBuff, Term::Color::Name::Red);

  return 0;
}

I'm going to continue working to finish my original program that tries to highlight some selected words on my terminal from an unamed pipe.

Thank you!!!

@flagarde
Copy link
Collaborator

Great, I'm not yet sure if the fix is working on all cases but it's a first move in the right direction.

Thx for the code, would you allow us to take it and use it as basis for some example or test in cpp-terminal ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants