[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] util: Detect more I/O errors
From: |
Colin Watson |
Subject: |
Re: [PATCH] util: Detect more I/O errors |
Date: |
Fri, 1 Mar 2019 09:53:51 +0000 |
User-agent: |
NeoMutt/20170113 (1.7.2) |
On Thu, Feb 28, 2019 at 04:32:44PM +0000, Elliott, Robert (Persistent Memory)
wrote:
> > -----Original Message-----
> > From: Grub-devel <address@hidden> On
> > Behalf Of Colin Watson
> ...
> > -void
> > +int
> > grub_util_file_sync (FILE *f)
> > {
> > - fflush (f);
> > + if (fflush (f) != 0)
> > + return -1;
> > if (!allow_fd_syncs)
> > - return;
> > - fsync (fileno (f));
> > + return 0;
> > + return fsync (fileno (f));
> > }
>
> Since that's just returning -1 for an error (both for fflush and fsync),
> not errno which contains the reason for the error...
>
> > diff --git a/util/editenv.c b/util/editenv.c
> > index c6f8d2298..eb2d0c03a 100644
> > --- a/util/editenv.c
> > +++ b/util/editenv.c
> > @@ -55,7 +55,8 @@ grub_util_create_envblk_file (const char *name)
> > strerror (errno));
> >
> >
> > - grub_util_file_sync (fp);
> > + if (grub_util_file_sync (fp) < 0)
> > + grub_util_error (_("cannot sync `%s': %s"), namenew, strerror
> > (errno));
>
> callers like this will interpret the -1 as EPERM, which isn't the
> true reason.
No, this is a mistaken analysis. The code you're quoting passes errno
to strerror when rendering the error message, *not* the return code of
grub_util_file_sync. It is therefore not possible for it to
misinterpret -1 as EPERM. The same holds for all similar call sites in
my patch. (In any case, the value of EPERM is typically 1, not -1; you
may be thinking of the kernel practice of returning -errno, which is not
common in userspace code.)
The pattern I've followed for grub_util_file_sync and other similar
functions is the commonplace one used by many C library functions: it
either returns 0, or it returns some other value (in this case -1) and
sets errno to indicate the error. It is of course necessary to ensure
that it only returns -1 when something has already set errno, but I
believe that's the case throughout my patch.
--
Colin Watson address@hidden
- Re: [PATCH] util: Detect more I/O errors,
Colin Watson <=