Re: time for 1.5.11? (/dev/dsp problems -- Gerd take note)

[Igor Pechtchanski <pechtcha at cs dot nyu dot edu>]

While we're on the topic, here's what seems like another potential bug
(just from eyeballing the code):

fhandler_dev_dsp::open (int flags, mode_t mode)
{
  open_count++;
  if (open_count > 1)
    {
      set_errno (EBUSY);
      return 0;
      ^^^^^^^^^
    }

If the device is busy, open_count will never get decremented (it's only
decremented on close(), and who'd think of closing an fd that failed to
open?), so nobody will ever be able to open() it afterwards, even if it
becomes available.  I believe open_count should be decremented just before
the return above.  Comments?
	Igor

On Wed, 9 Jun 2004, Christopher Faylor wrote:

> Gerd,
> What do you think about Igor's findings?
>
> cgf
>
> On Wed, Jun 09, 2004 at 08:56:13PM -0400, Igor Pechtchanski wrote:
> >On Wed, 9 Jun 2004, Christopher Faylor wrote:
> >
> >> On Wed, Jun 09, 2004 at 04:28:37PM -0400, Igor Pechtchanski wrote:
> >> >On Wed, 9 Jun 2004, Igor Pechtchanski wrote:
> >> >
> >> >> On Wed, 9 Jun 2004, Corinna Vinschen wrote:
> >> >>
> >> >> > On Jun  9 13:01, Christopher Faylor wrote:
> >> >> > > Anyway, on to the future...  Would it make sense to release a 1.5.11?
> >> >> > > Do you think we've hit all of the fallout from the path reorg/speedup?
> >> >> >
> >> >> > Looking through the reports on the Cygwin list, I see that we never
> >> >> > got a reply if the MapViewOfFileEx problem has been solved.  Also,
> >> >> > what about the report that /dev/dsp isn't working?  Igor was going to
> >> >> > investigate this further.  Any news, Igor?
> >> >>
> >> >> I'm still investigating.  So far it looks like /dev/dsp doesn't like to be
> >> >> dup'd (but then, who does? ;-)).  I'm attaching a short test program to
> >> >> demostrate this, but I'll keep digging...
> >> >
> >> >...And moments after I sent the above I found the culprit (I think).
> >> >Seems like fhandler_dsp::dup() neglects to call fhandler_base::dup(), so
> >> >the IO handle is unset.  It should be a trivial 1-line patch, but just in
> >> >case, can someone with a copyright assignment take care of this?
> >>
> >> You should be safe if it is really a one line patch.  Don't you want to
> >> get your name into the Cygwin changelog limelight?
> >>
> >> cgf
> >
> >Ok, it turns out that to fix this properly, we'll need a much larger patch
> >than the one line I was looking at...  Here are some thoughts.
> >
> >First off, my guess above was wrong.  The IO handle has nothing to do with
> >this -- fhandler_dev_dsp doesn't use it.  However, the intuition (I still
> >think) was correct: some fields, namely the audio_in_ and audio_out_
> >pointers, weren't replicated on dup(), so the new (duped) object ended up
> >with NULL fields, and couldn't write to them.
> >
> >The simplest solution to the above is to simply copy the pointers.  This
> >will work for the test program I provided earlier, but won't for another
> >legitimate test program (attached) that closes the original handle before
> >using the duped one (I think this behavior is allowed).
> >
> >The "proper" way to fix this, IMO, would be to allow sharing the audio_in_
> >and audio_out_ objects, and have some sort of reference count in them,
> >instead of deleting them outright in close().  I'm not sure this will
> >cover the situations when a process is spawned (e.g., fork), but it should
> >suffice for redirection.  This, however, promises to be a non-trivial
> >patch.
> >
> >Comments?
> >	Igor
> >
> >
> >
> >#include <fcntl.h>
> >#include <unistd.h>
> >
> >#define BUFSIZE	1024
> >#define WORKS	"This works\n"
> >#define NWORKS	"This doesn\'t work\n"
> >
> >int main(int ac, char *av[]) {
> >  char buf[BUFSIZE+1];
> >  int num, src, dst;
> >
> >  write(2, WORKS, sizeof(WORKS)-1);
> >  src = open("/cygdrive/c/WINNT/Media/tada.wav", O_RDONLY);
> >  dst = open("/dev/dsp", 0x601);
> >  while ((num = read(src, buf, BUFSIZE)) > 0)
> >    write(dst, buf, num);
> >  close(dst);
> >  close(src);
> >
> >  write(2, NWORKS, sizeof(NWORKS)-1);
> >  src = open("/cygdrive/c/WINNT/Media/tada.wav", O_RDONLY);
> >  dst = open("/dev/dsp", 0x601);
> >  if (dup2(dst, 1) != -1) {
> >    close(dst);
> >    dst = 1;
> >    while ((num = read(src, buf, BUFSIZE)) > 0)
> >      write(dst, buf, num);
> >  }
> >  close(dst);
> >  close(src);
> >}

-- 
				http://cs.nyu.edu/~pechtcha/
      |\      _,,,---,,_		pechtcha@cs.nyu.edu
ZZZzz /,`.-'`'    -.  ;-;;,_		igor@watson.ibm.com
     |,4-  ) )-,_. ,\ (  `'-'		Igor Pechtchanski, Ph.D.
    '---''(_/--'  `-'\_) fL	a.k.a JaguaR-R-R-r-r-r-.-.-.  Meow!

"I have since come to realize that being between your mentor and his route
to the bathroom is a major career booster."  -- Patrick Naughton