* set_open_status @ 2021-02-17 21:35 Ken Brown 2021-02-18 9:07 ` set_open_status Corinna Vinschen 0 siblings, 1 reply; 5+ messages in thread From: Ken Brown @ 2021-02-17 21:35 UTC (permalink / raw) To: cygwin-devel Most fhandlers call set_open_status in their 'open' method before a successful return. I just noticed that fhandler_fifo::open doesn't do this. I thought at first that it was an oversight on my part from when I overhauled the FIFO implementation, but I just checked fhandler_fifo.cc as of cygwin 2.9.0, and I don't see it there either. Is there some reason this would be wrong for FIFOs? Ken ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: set_open_status 2021-02-17 21:35 set_open_status Ken Brown @ 2021-02-18 9:07 ` Corinna Vinschen 2021-02-18 13:07 ` set_open_status Corinna Vinschen 0 siblings, 1 reply; 5+ messages in thread From: Corinna Vinschen @ 2021-02-18 9:07 UTC (permalink / raw) To: cygwin-developers On Feb 17 16:35, Ken Brown via Cygwin-developers wrote: > Most fhandlers call set_open_status in their 'open' method before a > successful return. I just noticed that fhandler_fifo::open doesn't do this. > I thought at first that it was an oversight on my part from when I > overhauled the FIFO implementation, but I just checked fhandler_fifo.cc as > of cygwin 2.9.0, and I don't see it there either. > > Is there some reason this would be wrong for FIFOs? I guess this was just an oversight. It's certainly not fatal, given how open_status is used exclusively by fhandler_base::reset_to_open_binmode() in turn *only* called from setmode(fd, 0). This is quite a bordercase. Combined with FIFOs supporting only O_BINARY mode anyway... It won't hurt to add the set_open_mode call to FIFOs but it won't change anything, except avoiding to report O_TEXT mode after a call to setmode(fd, 0). Maybe it would be better to change fhandler_base::reset_to_open_binmode instead. Right now, if open_status hasn't been initialized, it prefers O_TEXT over O_BINARY. That's unfortunate, because it forces us to call set_open_mode everywhere to make sure the mode is stored correctly for this single, and seldom, invocation of setmode(fd, 0). It would be nice if set_open_mode would only be necessary on fhandlers supporting O_TEXT at all. Corinna ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: set_open_status 2021-02-18 9:07 ` set_open_status Corinna Vinschen @ 2021-02-18 13:07 ` Corinna Vinschen 2021-02-18 21:23 ` set_open_status Ken Brown 0 siblings, 1 reply; 5+ messages in thread From: Corinna Vinschen @ 2021-02-18 13:07 UTC (permalink / raw) To: cygwin-developers On Feb 18 10:07, Corinna Vinschen via Cygwin-developers wrote: > On Feb 17 16:35, Ken Brown via Cygwin-developers wrote: > > Most fhandlers call set_open_status in their 'open' method before a > > successful return. I just noticed that fhandler_fifo::open doesn't do this. > > I thought at first that it was an oversight on my part from when I > > overhauled the FIFO implementation, but I just checked fhandler_fifo.cc as > > of cygwin 2.9.0, and I don't see it there either. > > > > Is there some reason this would be wrong for FIFOs? > > I guess this was just an oversight. It's certainly not fatal, given how > open_status is used exclusively by fhandler_base::reset_to_open_binmode() > in turn *only* called from setmode(fd, 0). This is quite a bordercase. > Combined with FIFOs supporting only O_BINARY mode anyway... > > It won't hurt to add the set_open_mode call to FIFOs but it won't change > anything, except avoiding to report O_TEXT mode after a call to > setmode(fd, 0). > > Maybe it would be better to change fhandler_base::reset_to_open_binmode > instead. Right now, if open_status hasn't been initialized, it prefers > O_TEXT over O_BINARY. That's unfortunate, because it forces us to call > set_open_mode everywhere to make sure the mode is stored correctly for > this single, and seldom, invocation of setmode(fd, 0). It would be nice > if set_open_mode would only be necessary on fhandlers supporting O_TEXT > at all. On second thought, this should do the trick: diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h index e457e2785e9b..ac9ee7c9e787 100644 --- a/winsup/cygwin/fhandler.h +++ b/winsup/cygwin/fhandler.h @@ -274,7 +274,8 @@ class fhandler_base void reset_to_open_binmode () { set_flags ((get_flags () & ~(O_TEXT | O_BINARY)) - | ((open_status.wbinary || open_status.rbinary) + | (((open_status.wbinset ? open_status.wbinary : 1) + || (open_status.rbinset ? open_status.rbinary : 1)) ? O_BINARY : O_TEXT)); } Corinna ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: set_open_status 2021-02-18 13:07 ` set_open_status Corinna Vinschen @ 2021-02-18 21:23 ` Ken Brown 2021-02-19 17:15 ` set_open_status Corinna Vinschen 0 siblings, 1 reply; 5+ messages in thread From: Ken Brown @ 2021-02-18 21:23 UTC (permalink / raw) To: cygwin-developers On 2/18/2021 8:07 AM, Corinna Vinschen via Cygwin-developers wrote: > On Feb 18 10:07, Corinna Vinschen via Cygwin-developers wrote: >> On Feb 17 16:35, Ken Brown via Cygwin-developers wrote: >>> Most fhandlers call set_open_status in their 'open' method before a >>> successful return. I just noticed that fhandler_fifo::open doesn't do this. >>> I thought at first that it was an oversight on my part from when I >>> overhauled the FIFO implementation, but I just checked fhandler_fifo.cc as >>> of cygwin 2.9.0, and I don't see it there either. >>> >>> Is there some reason this would be wrong for FIFOs? >> >> I guess this was just an oversight. It's certainly not fatal, given how >> open_status is used exclusively by fhandler_base::reset_to_open_binmode() >> in turn *only* called from setmode(fd, 0). This is quite a bordercase. >> Combined with FIFOs supporting only O_BINARY mode anyway... >> >> It won't hurt to add the set_open_mode call to FIFOs but it won't change >> anything, except avoiding to report O_TEXT mode after a call to >> setmode(fd, 0). >> >> Maybe it would be better to change fhandler_base::reset_to_open_binmode >> instead. Right now, if open_status hasn't been initialized, it prefers >> O_TEXT over O_BINARY. That's unfortunate, because it forces us to call >> set_open_mode everywhere to make sure the mode is stored correctly for >> this single, and seldom, invocation of setmode(fd, 0). It would be nice >> if set_open_mode would only be necessary on fhandlers supporting O_TEXT >> at all. > > On second thought, this should do the trick: > > diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h > index e457e2785e9b..ac9ee7c9e787 100644 > --- a/winsup/cygwin/fhandler.h > +++ b/winsup/cygwin/fhandler.h > @@ -274,7 +274,8 @@ class fhandler_base > void reset_to_open_binmode () > { > set_flags ((get_flags () & ~(O_TEXT | O_BINARY)) > - | ((open_status.wbinary || open_status.rbinary) > + | (((open_status.wbinset ? open_status.wbinary : 1) > + || (open_status.rbinset ? open_status.rbinary : 1)) > ? O_BINARY : O_TEXT)); > } Yes, this looks right. Ken ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: set_open_status 2021-02-18 21:23 ` set_open_status Ken Brown @ 2021-02-19 17:15 ` Corinna Vinschen 0 siblings, 0 replies; 5+ messages in thread From: Corinna Vinschen @ 2021-02-19 17:15 UTC (permalink / raw) To: cygwin-developers On Feb 18 16:23, Ken Brown via Cygwin-developers wrote: > On 2/18/2021 8:07 AM, Corinna Vinschen via Cygwin-developers wrote: > > On Feb 18 10:07, Corinna Vinschen via Cygwin-developers wrote: > > > On Feb 17 16:35, Ken Brown via Cygwin-developers wrote: > > > > Most fhandlers call set_open_status in their 'open' method before a > > > > successful return. I just noticed that fhandler_fifo::open doesn't do this. > > > > I thought at first that it was an oversight on my part from when I > > > > overhauled the FIFO implementation, but I just checked fhandler_fifo.cc as > > > > of cygwin 2.9.0, and I don't see it there either. > > > > > > > > Is there some reason this would be wrong for FIFOs? > > > > > > I guess this was just an oversight. It's certainly not fatal, given how > > > open_status is used exclusively by fhandler_base::reset_to_open_binmode() > > > in turn *only* called from setmode(fd, 0). This is quite a bordercase. > > > Combined with FIFOs supporting only O_BINARY mode anyway... > > > > > > It won't hurt to add the set_open_mode call to FIFOs but it won't change > > > anything, except avoiding to report O_TEXT mode after a call to > > > setmode(fd, 0). > > > > > > Maybe it would be better to change fhandler_base::reset_to_open_binmode > > > instead. Right now, if open_status hasn't been initialized, it prefers > > > O_TEXT over O_BINARY. That's unfortunate, because it forces us to call > > > set_open_mode everywhere to make sure the mode is stored correctly for > > > this single, and seldom, invocation of setmode(fd, 0). It would be nice > > > if set_open_mode would only be necessary on fhandlers supporting O_TEXT > > > at all. > > > > On second thought, this should do the trick: > > > > diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h > > index e457e2785e9b..ac9ee7c9e787 100644 > > --- a/winsup/cygwin/fhandler.h > > +++ b/winsup/cygwin/fhandler.h > > @@ -274,7 +274,8 @@ class fhandler_base > > void reset_to_open_binmode () > > { > > set_flags ((get_flags () & ~(O_TEXT | O_BINARY)) > > - | ((open_status.wbinary || open_status.rbinary) > > + | (((open_status.wbinset ? open_status.wbinary : 1) > > + || (open_status.rbinset ? open_status.rbinary : 1)) > > ? O_BINARY : O_TEXT)); > > } > > Yes, this looks right. > > Ken Thanks, I pushed it. Corinna ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-02-19 17:15 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-17 21:35 set_open_status Ken Brown 2021-02-18 9:07 ` set_open_status Corinna Vinschen 2021-02-18 13:07 ` set_open_status Corinna Vinschen 2021-02-18 21:23 ` set_open_status Ken Brown 2021-02-19 17:15 ` set_open_status Corinna Vinschen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).