diff mbox

Suggested patch: reset errno after isatty()

Message ID AANLkTimfxjrtV4eks5tK68h6TQ20soib+6gaTC+QyB4a@mail.gmail.com
State New, archived
Headers show

Commit Message

Ketil Froyn Nov. 3, 2010, 1:16 p.m. UTC
On Wed, Nov 3, 2010 at 9:23 AM, Matthieu CASTET
<matthieu.castet@parrot.com> wrote:

> Why errno is checked when pread return 0 ?
> Errno should be checked only when return is -1.

Good point, and I guess that was why I was getting weird errors in the
first place, because errno was checked when no error had occurred. Odd
that pread() returned 0 when trying to read on my android device -
maybe a bionic bug?

This code has been rewritten, but the new master isn't working for me
(yet). This patch against v1.4.1 seems to have solved my problems for
now. I've just replaced pread() with read(), because it works, and
fixed up the error checking.

                /* ECC stats available ? */

Comments

Mike Frysinger Nov. 6, 2010, 8:54 a.m. UTC | #1
On Wed, Nov 3, 2010 at 09:16, Ketil Froyn wrote:
> On Wed, Nov 3, 2010 at 9:23 AM, Matthieu CASTET wrote:
> This code has been rewritten, but the new master isn't working for me
> (yet). This patch against v1.4.1 seems to have solved my problems for
> now. I've just replaced pread() with read(), because it works, and
> fixed up the error checking.
>
> --- a/nanddump.c
> +++ b/nanddump.c
> @@ -412,10 +412,25 @@ int main(int argc, char * const argv[])
>                        memset (readbuf, 0xff, bs);
>                } else {
>                        /* Read page data and exit on failure */
> -                       if (pread(fd, readbuf, bs, ofs) != bs) {
> -                               perror("pread");
> -                               goto closeall;
> -                       }
> +                       do {
> +                               ret = read(fd, readbuf, bs);
> +                               if (ret == -1) {
> +                                       if (errno == EAGAIN || errno == EINTR) {
> +                                               continue;
> +                                       }
> +                                       perror("read");
> +                                       goto closeall;
> +                               }
> +                               if (ret == 0) {
> +                                       printf("No more data to read\n");
> +                                       break;
> +                               }
> +                               if (ret != bs) {
> +                                       fprintf(stderr, "read() got
> wrong number of bytes: got %i, expected %i\n", ret, bs);
> +                                       goto closeall;
> +                               }
> +                               break;
> +                       } while (1);
>                }

doesnt libmtd provide read functions already ?
-mike
Ketil Froyn Nov. 7, 2010, 10:36 p.m. UTC | #2
On Sat, Nov 6, 2010 at 9:54 AM, Mike Frysinger <vapier.adi@gmail.com> wrote:
>
> doesnt libmtd provide read functions already ?
> -mike

I'm not familiar with libmtd, but the latest release (v1.4.1) just did
it itself with pread, and since that failed I fixed it.

The new code in the repo, which appears to use libmtd, didn't work for
me, and the fix appeared more complicated. My patch is for v1.4.1,
which should also make it easier to integrate for package maintainers,
if they wish, pending a working rewrite.

Ketil
Mike Frysinger Nov. 9, 2010, 9:40 a.m. UTC | #3
On Sun, Nov 7, 2010 at 17:36, Ketil Froyn wrote:
> On Sat, Nov 6, 2010 at 9:54 AM, Mike Frysinger wrote:
>> doesnt libmtd provide read functions already ?
>
> I'm not familiar with libmtd, but the latest release (v1.4.1) just did
> it itself with pread, and since that failed I fixed it.
>
> The new code in the repo, which appears to use libmtd, didn't work for
> me, and the fix appeared more complicated. My patch is for v1.4.1,
> which should also make it easier to integrate for package maintainers,
> if they wish, pending a working rewrite.

the focus is on the master branch.  stable fixes should largely be
back ports, not a complete fork.
-mike
Artem Bityutskiy Nov. 13, 2010, 11:07 a.m. UTC | #4
On Sun, 2010-11-07 at 23:36 +0100, Ketil Froyn wrote:
> On Sat, Nov 6, 2010 at 9:54 AM, Mike Frysinger <vapier.adi@gmail.com> wrote:
> >
> > doesnt libmtd provide read functions already ?
> > -mike
> 
> I'm not familiar with libmtd, but the latest release (v1.4.1) just did
> it itself with pread, and since that failed I fixed it.
> 
> The new code in the repo, which appears to use libmtd, didn't work for
> me, and the fix appeared more complicated. My patch is for v1.4.1,
> which should also make it easier to integrate for package maintainers,
> if they wish, pending a working rewrite.

Ketil, you have a point, 

however, Mike is right, fixes should be back-ports, or if you fix a
problem in the older release, you should fix it in the master branch as
well, or show that it does not exist in the master branch. This is the
only way it is possible to keep development under control.

We really need to be sure that whatever is fixed in older releases is
also fixed in the newer. Yes, this is more work for you but hey, I'm
also doing this stuff in my spare time.
Artem Bityutskiy Nov. 13, 2010, 11:55 a.m. UTC | #5
On Sat, 2010-11-13 at 13:07 +0200, Artem Bityutskiy wrote:
> On Sun, 2010-11-07 at 23:36 +0100, Ketil Froyn wrote:
> > On Sat, Nov 6, 2010 at 9:54 AM, Mike Frysinger <vapier.adi@gmail.com> wrote:
> > >
> > > doesnt libmtd provide read functions already ?
> > > -mike
> > 
> > I'm not familiar with libmtd, but the latest release (v1.4.1) just did
> > it itself with pread, and since that failed I fixed it.
> > 
> > The new code in the repo, which appears to use libmtd, didn't work for
> > me, and the fix appeared more complicated. My patch is for v1.4.1,
> > which should also make it easier to integrate for package maintainers,
> > if they wish, pending a working rewrite.
> 
> Ketil, you have a point, 
> 
> however, Mike is right, fixes should be back-ports, or if you fix a
> problem in the older release, you should fix it in the master branch as
> well, or show that it does not exist in the master branch. This is the
> only way it is possible to keep development under control.
> 
> We really need to be sure that whatever is fixed in older releases is
> also fixed in the newer. Yes, this is more work for you but hey, I'm
> also doing this stuff in my spare time.

BTW, with patches from Brian Norris which I just pushed the master
branch may work better for you.
diff mbox

Patch

--- a/nanddump.c
+++ b/nanddump.c
@@ -412,10 +412,25 @@  int main(int argc, char * const argv[])
                        memset (readbuf, 0xff, bs);
                } else {
                        /* Read page data and exit on failure */
-                       if (pread(fd, readbuf, bs, ofs) != bs) {
-                               perror("pread");
-                               goto closeall;
-                       }
+                       do {
+                               ret = read(fd, readbuf, bs);
+                               if (ret == -1) {
+                                       if (errno == EAGAIN || errno == EINTR) {
+                                               continue;
+                                       }
+                                       perror("read");
+                                       goto closeall;
+                               }
+                               if (ret == 0) {
+                                       printf("No more data to read\n");
+                                       break;
+                               }
+                               if (ret != bs) {
+                                       fprintf(stderr, "read() got
wrong number of bytes: got %i, expected %i\n", ret, bs);
+                                       goto closeall;
+                               }
+                               break;
+                       } while (1);
                }