Bug in pread/pwrite ?

Hi,

After upgrading my compiler from cygwin 1.5.17-1 to 1.5.24-2,
TestDisk & PhotoRec, my GPL data recovery programs, were
unable to read data!

I have written a little program (see attachment) to reproduce
the problem. As administrator, run "test_pread /dev/sda".

The program use lseek() to go the disk end, the function failed.
Now pread will now always failed, because it ends
(cf cygwin-1.5.24-2/newlib/libc/unix/pread.c) by
an lseek to the backuped location. The same problem also applies
to pwrite.

I don't know if the following patch is a good idea:

--- cygwin-1.5.24-2/newlib/libc/unix/pread.org.c  2002-05-06 22:29:28.000000000 +0200
+++ cygwin-1.5.24-2/newlib/libc/unix/pread.c      2007-03-08 23:37:34.000000000 +0100
 <at>  <at>  -70,8 +70,7  <at>  <at>  _DEFUN (_pread_r, (rptr, fd, buf, n, off

    num_read = _read_r (rptr, fd, buf, n);

-  if (_lseek_r (rptr, fd, cur_pos, SEEK_SET) == (off_t)-1)
-    return -1;
+  _lseek_r (rptr, fd, cur_pos, SEEK_SET);

    return (ssize_t)num_read;
  }

Re: Bug in pread/pwrite ?

On Fri, Mar 09, 2007 at 12:05:47AM +0100, Christophe GRENIER wrote:
>After upgrading my compiler from cygwin 1.5.17-1 to 1.5.24-2, TestDisk
>& PhotoRec, my GPL data recovery programs, were unable to read data!

Please calm down.

>I have written a little program (see attachment) to reproduce the
>problem.  As administrator, run "test_pread /dev/sda".
>
>The program use lseek() to go the disk end, the function failed.  Now
>pread will now always failed, because it ends (cf
>cygwin-1.5.24-2/newlib/libc/unix/pread.c) by an lseek to the backuped
>location.  The same problem also applies to pwrite.
>
>I don't know if the following patch is a good idea:

Patches sent here are really supposed to be tested.  You obviously
didn't test this because your proposed patch doesn't modify a function
that Cygwin actually uses.

The pread() that Cygwin does uses is in fhandler_disk_file.cc.

All of that aside, I don't see how ignoring an lseek() failure
could be considered to be a good thing.

cgf

Re: Bug in pread/pwrite ?

On Thu, 8 Mar 2007, Christopher Faylor wrote:

> The pread() that Cygwin does uses is in fhandler_disk_file.cc.

Thanks, now i know where the culprit is and understand why
I had a problem with my proposed patch 

> All of that aside, I don't see how ignoring an lseek() failure
> could be considered to be a good thing.

I have done more research since, have a look to this glibc
pread implementation:

ssize_t
__libc_pread (int fd, void *buf, size_t nbyte, off_t offset)
{
   /* Since we must not change the file pointer preserve the value so that
      we can restore it later.  */
   int save_errno;
   ssize_t result;
   off_t old_offset = __libc_lseek (fd, 0, SEEK_CUR);
   if (old_offset == (off_t) -1)
     return -1;

   /* Set to wanted position.  */
   if (__libc_lseek (fd, offset, SEEK_SET) == (off_t) -1)
     return -1;

   /* Write out the data.  */
   result = __libc_read (fd, buf, nbyte);

Re: Bug in pread/pwrite ?

On Mar  9 08:57, Christophe GRENIER wrote:
> On Thu, 8 Mar 2007, Christopher Faylor wrote:
> >All of that aside, I don't see how ignoring an lseek() failure
> >could be considered to be a good thing.
> 
> I have done more research since, have a look to this glibc
> pread implementation:
> 
> ssize_t
> __libc_pread (int fd, void *buf, size_t nbyte, off_t offset)
> {
>   /* Since we must not change the file pointer preserve the value so that
>      we can restore it later.  */
>   int save_errno;
>   ssize_t result;
>   off_t old_offset = __libc_lseek (fd, 0, SEEK_CUR);
>   if (old_offset == (off_t) -1)
>     return -1;
    ^^^^^^^^^^^^

>   /* Set to wanted position.  */
>   if (__libc_lseek (fd, offset, SEEK_SET) == (off_t) -1)
>     return -1;
    ^^^^^^^^^^^^

>   /* Write out the data.  */
>   result = __libc_read (fd, buf, nbyte);
> 
>   /* Now we have to restore the position.  If this fails we have to
>      return this as an error.  But if the writing also failed we

compile warning in cygwin/stat.h

This patch: http://cygwin.com/ml/cygwin-cvs/2007-q1/msg00123.html
breaks compilation of coreutils against the latest snapshot when using
-Wall -Werror, due to an unused expression on the left of a comma.

2007-03-13  Eric Blake  <ebb9 <at> byu.net>

	* include/cygwin/stat.h (S_TYPEISSHM, S_TYPEISSEM, S_TYPEISSHM):
	Avoid compiler warnings.

--
Don't work too hard, make some time for fun as well!

Eric Blake             ebb9 <at> byu.net

Index: include/cygwin/stat.h
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/include/cygwin/stat.h,v
retrieving revision 1.10
diff -u -p -r1.10 stat.h
--- include/cygwin/stat.h	6 Mar 2007 14:56:44 -0000	1.10
+++ include/cygwin/stat.h	13 Mar 2007 12:26:19 -0000
 <at>  <at>  -91,9 +91,9  <at>  <at>  struct stat
 /* POSIX IPC objects are not implemented as distinct file types, so the
    below macros have to return 0.  The expression is supposed to catch
    illegal usage with non-stat parameters. */
-#define S_TYPEISMQ(buf)  ((buf)->st_mode,0)
-#define S_TYPEISSEM(buf) ((buf)->st_mode,0)
-#define S_TYPEISSHM(buf) ((buf)->st_mode,0)

Re: compile warning in cygwin/stat.h

On Mar 13 06:30, Eric Blake wrote:
> 	* include/cygwin/stat.h (S_TYPEISSHM, S_TYPEISSEM, S_TYPEISSHM):
> 	Avoid compiler warnings.

Thanks, applied.

Corinna

--

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          cygwin AT cygwin DOT com
Red Hat

Re: compile warning in cygwin/stat.h

According to Corinna Vinschen on 3/13/2007 7:22 AM:
> On Mar 13 06:30, Eric Blake wrote:
>> 	* include/cygwin/stat.h (S_TYPEISSHM, S_TYPEISSEM, S_TYPEISSHM):
>> 	Avoid compiler warnings.
> 
> Thanks, applied.

For all that, and I still got the changelog wrong.  I listed S_TYPEISSHM
twice and missed S_TYPEISMQ.  I'm spending more typing on the patch
procedure than the patch itself :)

--
Don't work too hard, make some time for fun as well!

Eric Blake             ebb9 <at> byu.net

Re: compile warning in cygwin/stat.h

On Mar 13 07:28, Eric Blake wrote:
> According to Corinna Vinschen on 3/13/2007 7:22 AM:
> > On Mar 13 06:30, Eric Blake wrote:
> >> 	* include/cygwin/stat.h (S_TYPEISSHM, S_TYPEISSEM, S_TYPEISSHM):
> >> 	Avoid compiler warnings.
> > 
> > Thanks, applied.
> 
> For all that, and I still got the changelog wrong.  I listed S_TYPEISSHM
> twice and missed S_TYPEISMQ.  I'm spending more typing on the patch
> procedure than the patch itself :)

... and I didn't notice so it was sort of a collective effort to get it
wrong :)

Corinna

--

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          cygwin AT cygwin DOT com
Red Hat

[PATCH] getmntent()->mnt_type values that match Linux...

There was a discussion quite some time ago about getmntent()'s mnt_type 
field:

    http://www.cygwin.com/ml/cygwin-developers/2002-09/msg00078.html

...but nothing seems to have come of it. I noticed that Cygwin builds of 
PhysicsFS (http://icculus.org/physfs/) don't detect CD-ROM drives since 
mnt_type is always "system" or "user" ... this patch changes this to 
make an earnest effort to match what a GNU/Linux system would report, 
and moves the system/user string to mnt_opts.

Without some solution like this, code external to Cygwin would have to 
take heroic measures (#ifdefs and calls into the Win32 API) to figure 
out what type of filesystem /cygdrive/f really is.

After patching, here's the output from "mount" for a hard drive with two 
NTFS partitions (C: and D:), a CD-ROM drive (E:), a FAT memory stick 
(F:), and a Samba share (Z:) ...

$ mount
C:\cygwin\bin on /usr/bin type ntfs (binmode,system)
C:\cygwin\lib on /usr/lib type ntfs (binmode,system)
C:\cygwin on / type ntfs (binmode,system)
c: on /cygdrive/c type ntfs (binmode,noumount,system)
d: on /cygdrive/d type ntfs (binmode,noumount,system)
e: on /cygdrive/e type iso9660 (binmode,noumount,system)
f: on /cygdrive/f type vfat (binmode,noumount,system)
z: on /cygdrive/z type smbfs (binmode,noumount,system)

Re: [PATCH] getmntent()->mnt_type values that match Linux...

On Mon, Mar 19, 2007 at 02:30:16AM -0400, Ryan C. Gordon wrote:
>
>There was a discussion quite some time ago about getmntent()'s mnt_type 
>field:
>
>   http://www.cygwin.com/ml/cygwin-developers/2002-09/msg00078.html
>
>...but nothing seems to have come of it. I noticed that Cygwin builds of 
>PhysicsFS (http://icculus.org/physfs/) don't detect CD-ROM drives since 
>mnt_type is always "system" or "user" ... this patch changes this to 
>make an earnest effort to match what a GNU/Linux system would report, 
>and moves the system/user string to mnt_opts.
>
>Without some solution like this, code external to Cygwin would have to 
>take heroic measures (#ifdefs and calls into the Win32 API) to figure 
>out what type of filesystem /cygdrive/f really is.
>
>After patching, here's the output from "mount" for a hard drive with two 
>NTFS partitions (C: and D:), a CD-ROM drive (E:), a FAT memory stick 
>(F:), and a Samba share (Z:) ...
>
>$ mount
>C:\cygwin\bin on /usr/bin type ntfs (binmode,system)
>C:\cygwin\lib on /usr/lib type ntfs (binmode,system)
>C:\cygwin on / type ntfs (binmode,system)
>c: on /cygdrive/c type ntfs (binmode,noumount,system)
>d: on /cygdrive/d type ntfs (binmode,noumount,system)
>e: on /cygdrive/e type iso9660 (binmode,noumount,system)
>f: on /cygdrive/f type vfat (binmode,noumount,system)
>z: on /cygdrive/z type smbfs (binmode,noumount,system)