Re: [PATCH] util: Detect more I/O errors

[Colin Watson]

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