Magnus Hagander wrote:
> > >> Now, I still twist my head around the lines:
> > >> if ((fd = _open_osfhandle((long) h, fileFlags & O_APPEND)) < 0
> > >> ||
> > >> (fileFlags & (O_TEXT | O_BINARY) && (_setmode(fd, fileFlags &
> > (O_TEXT
> > >> | O_BINARY)) < 0)))
> >
> > > Without having studied it closely, it might also highlight a bug
> > on
> > > failure of the second clause -- if the _setmode fails, shouldn't
> > > _close be called instead of CloseHandle, and -1 returned?
> > > (CloseHandle would still be called on failure of the
> > _open_osfhandle,
> > > obviously)
> >
> > I agree that this code is both wrong and unreadable (although in
> > practice the _setmode will probably never fail, which is why our
> > attention hasn't been drawn to it). Is someone going to submit a
> > patch? I'm hesitant to change the code myself since I'm not in a
> > position to test it.
>
> I can look at fixing that. Is it something we want to do for 8.2, or
> wait until 8.3? If there's a hidden bug, I guess 8.2?
Magnus, is this the right fix?
--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Index: src/port/open.c
===================================================================
RCS file: /cvsroot/pgsql/src/port/open.c,v
retrieving revision 1.15
diff -c -c -r1.15 open.c
*** src/port/open.c 24 Sep 2006 17:19:53 -0000 1.15
--- src/port/open.c 3 Oct 2006 01:16:43 -0000
***************
*** 105,113 ****
}
/* _open_osfhandle will, on error, set errno accordingly */
! if ((fd = _open_osfhandle((long) h, fileFlags & O_APPEND)) < 0 ||
! (fileFlags & (O_TEXT | O_BINARY) && (_setmode(fd, fileFlags & (O_TEXT | O_BINARY)) < 0)))
CloseHandle(h); /* will not affect errno */
return fd;
}
--- 105,119 ----
}
/* _open_osfhandle will, on error, set errno accordingly */
! if ((fd = _open_osfhandle((long) h, fileFlags & O_APPEND)) < 0)
CloseHandle(h); /* will not affect errno */
+ else if (fileFlags & (O_TEXT | O_BINARY) &&
+ _setmode(fd, fileFlags & (O_TEXT | O_BINARY)) < 0)
+ {
+ _close(fd) /* will not affect errno */
+ return -1;
+ }
+
return fd;
}