[boost] [Process] List of small issues

Subject: [boost] [Process] List of small issues
From: Artyom (artyomtnk_at_[hidden])
Date: 2011-01-13 09:35:53

Hello,

I've made a micro review of Boost.Process, there are small
issues I've found:

- stream buffer implementation:

   underflow()

   you need to check the errno if it is EINTR,
   returning -1 and getting EINTR is not a problem and thus
   it should retry.
   
   sync();

   Same as in underflow() - check for EINTR and retry, but also
   there is a problem that you do not check that you had fully
   written the data.

   For example what I write out << std::flush sync() is called
   and I expect the all data should be written to device, so
   if return value is less then the size of the buffer you should
   retry and write again till buffer is empty, you get error or EOF.

-  Windows and Unicode.

   You are using CreateProcessA. I would recommend to always use
   wide API and convert narrow strings to wide similarly to what
   boost::filesystem::v3 does, so for example where the global
   locale as utf-8 facet you would convert narrow strings to wide
   and run it.

   Notes:

   1. You can also always assume that strings under windows are UTF-8
      and always convert them to wide string before system calls.
      
      This is I think better approach, but it is different from what
      most of boost does.

   2. I do not recommend adding wide API - makes the code much uglier,
      rather convert normal strigns to wide strings before system call.

-  It may be very good addition to implement full support of putback.

Additional point:
-----------------

I've noticed that you planned asynchronous notification and so on
but I think it is quite important to add feature that provide
an ability to wait for multiple processes to terminate
and have timeout.

It can be done using sigtimedwait/sigwait and assigned signal handlers
for SIGCHLD

Artyom

P.S.: Good luck with the review library looks overall very nice.