possible problem with boost::iostreams::file_descriptor
nested file_descriptor implementation class is defined as follows: struct impl { impl() : #ifdef BOOST_IOSTREAMS_WINDOWS handle_(reinterpret_cast<handle_type>(-1)), #else handle_(-1), #endif flags_(0) { } impl(handle_type fd, bool close_on_exit) : handle_(fd), flags_(0) { if (close_on_exit) flags_ |= impl::close_on_exit; } ~impl() { if (flags_ & close_on_exit) close_impl(*this); } enum flags { close_on_exit = 1, append = 4 }; handle_type handle_; int flags_; }; In the second constructor in particular we have this: impl(handle_type fd, bool close_on_exit) : handle_(fd), flags_(0) { if (close_on_exit) flags_ |= impl::close_on_exit; } I think this should be changed. Just because close_on_exit is specified to false does not mean a file is not open. It just means don't close it if it is. I think flags enumeration should be changed to: enum flags { close_on_exit = 1, good = 2, append = 4 }; and constructors / code should be changed accordingly. Otherwise following code (Windows) asserts, even though I don't think it should: bool is_open(HANDLE h) { return h && h!=INVALID_HANDLE_VALUE;} HANDLE h = ::CreateFile("C:\\autoexec.bat", GENERIC_READ, FILE_SHARE_READ|FILE_SHARE_WRITE,NULL,OPEN_EXISTING,FILE_ATTRIBUTE_NORMAL,NULL); boost::iostreams::file_descriptor fd(h, false); BOOST_ASSERT(fd.is_open() == is_open(h)); Thoughts?
Zachary Turner wrote:
... and constructors / code should be changed accordingly. Otherwise following code (Windows) asserts, even though I don't think it should:
bool is_open(HANDLE h) { return h && h!=INVALID_HANDLE_VALUE;}
HANDLE h = ::CreateFile("C:\\autoexec.bat", GENERIC_READ, FILE_SHARE_READ|FILE_SHARE_WRITE,NULL,OPEN_EXISTING,FILE_ATTRIBUTE_NORMAL,NULL);
boost::iostreams::file_descriptor fd(h, false);
BOOST_ASSERT(fd.is_open() == is_open(h));
Thoughts?
I think there may be a related bug report : https://svn.boost.org/trac/boost/ticket/2817
Zachary Turner wrote:
nested file_descriptor implementation class is defined as follows: [snip] In the second constructor in particular we have this:
impl(handle_type fd, bool close_on_exit) : handle_(fd), flags_(0) { if (close_on_exit) flags_ |= impl::close_on_exit; }
I think this should be changed. Just because close_on_exit is specified to false does not mean a file is not open. It just means don't close it if it is. I think flags enumeration should be changed to:
enum flags { close_on_exit = 1, good = 2, append = 4 };
and constructors / code should be changed accordingly. Otherwise following code (Windows) asserts, even though I don't think it should:
bool is_open(HANDLE h) { return h && h!=INVALID_HANDLE_VALUE;}
HANDLE h = ::CreateFile("C:\\autoexec.bat", GENERIC_READ, FILE_SHARE_READ|FILE_SHARE_WRITE,NULL,OPEN_EXISTING,FILE_ATTRI BUTE_NORMAL,NULL);
boost::iostreams::file_descriptor fd(h, false);
BOOST_ASSERT(fd.is_open() == is_open(h));
Thoughts?
Wouldn't changing: bool is_open() const { return pimpl_->flags_ != 0; } to: bool is_open() const { return pimpl_->handle_ != -1; } be a better and/or simpler fix?
On Thu, Jun 11, 2009 at 8:22 AM, Eric MALENFANT < Eric.Malenfant@sagem-interstar.com> wrote:
Zachary Turner wrote:
nested file_descriptor implementation class is defined as follows: [snip] In the second constructor in particular we have this:
impl(handle_type fd, bool close_on_exit) : handle_(fd), flags_(0) { if (close_on_exit) flags_ |= impl::close_on_exit; }
I think this should be changed. Just because close_on_exit is specified to false does not mean a file is not open. It just means don't close it if it is. I think flags enumeration should be changed to:
enum flags { close_on_exit = 1, good = 2, append = 4 };
and constructors / code should be changed accordingly. Otherwise following code (Windows) asserts, even though I don't think it should:
bool is_open(HANDLE h) { return h && h!=INVALID_HANDLE_VALUE;}
HANDLE h = ::CreateFile("C:\\autoexec.bat", GENERIC_READ, FILE_SHARE_READ|FILE_SHARE_WRITE,NULL,OPEN_EXISTING,FILE_ATTRI BUTE_NORMAL,NULL);
boost::iostreams::file_descriptor fd(h, false);
BOOST_ASSERT(fd.is_open() == is_open(h));
Thoughts?
Wouldn't changing: bool is_open() const { return pimpl_->flags_ != 0; } to: bool is_open() const { return pimpl_->handle_ != -1; } be a better and/or simpler fix?
I blame the fact that it was 1 AM when I wrote that. Not really sure what I was thinking tbh :D Although I think it should compare against both -1 and 0, since neither indicates a valid handle.
Zachary Turner wrote:
Eric MALENFANT wrote:
Zachary Turner wrote:
Otherwise following code (Windows) asserts, even though I don't think it should:
bool is_open(HANDLE h) { return h && h!=INVALID_HANDLE_VALUE;}
HANDLE h = ::CreateFile("C:\\autoexec.bat", GENERIC_READ, FILE_SHARE_READ|FILE_SHARE_WRITE,NULL,OPEN_EXISTING,FILE_ATTRI BUTE_NORMAL,NULL);
boost::iostreams::file_descriptor fd(h, false);
BOOST_ASSERT(fd.is_open() == is_open(h));
Wouldn't changing: bool is_open() const { return pimpl_->flags_ != 0; } to: bool is_open() const { return pimpl_->handle_ != -1; } be a better and/or simpler fix?
I blame the fact that it was 1 AM when I wrote that. Not really sure what I was thinking tbh :D
... and I blame the fact that I was not wearing my glasses while checking if my reply was redundant: eg wrote:
I think there may be a related bug report : https://svn.boost.org/trac/boost/ticket/2817
The ticket eg refers to also suggests to "fix" is_open() by validating handle_'s value.
Although I think it should compare against both -1 and 0, since neither indicates a valid handle.
For a Windows HANDLE maybe, but ain't 0 a valid file descriptor value (I tought it was stdin)?
On Thu, Jun 11, 2009 at 8:50 AM, Eric MALENFANT < Eric.Malenfant@sagem-interstar.com> wrote:
Zachary Turner wrote:
Eric MALENFANT wrote:
Zachary Turner wrote:
Otherwise following code (Windows) asserts, even though I don't think it should:
bool is_open(HANDLE h) { return h && h!=INVALID_HANDLE_VALUE;}
HANDLE h = ::CreateFile("C:\\autoexec.bat", GENERIC_READ, FILE_SHARE_READ|FILE_SHARE_WRITE,NULL,OPEN_EXISTING,FILE_ATTRI BUTE_NORMAL,NULL);
boost::iostreams::file_descriptor fd(h, false);
BOOST_ASSERT(fd.is_open() == is_open(h));
Wouldn't changing: bool is_open() const { return pimpl_->flags_ != 0; } to: bool is_open() const { return pimpl_->handle_ != -1; } be a better and/or simpler fix?
I blame the fact that it was 1 AM when I wrote that. Not really sure what I was thinking tbh :D
... and I blame the fact that I was not wearing my glasses while checking if my reply was redundant:
eg wrote:
I think there may be a related bug report : https://svn.boost.org/trac/boost/ticket/2817
The ticket eg refers to also suggests to "fix" is_open() by validating handle_'s value.
Although I think it should compare against both -1 and 0, since neither indicates a valid handle.
For a Windows HANDLE maybe, but ain't 0 a valid file descriptor value (I tought it was stdin)? ______________________________
Yea it would have to be #ifdef'd around BOOST_IOSTREAMS_WINDOWS or whatever the define is called.
participants (3)
-
eg -
Eric MALENFANT -
Zachary Turner