Re: [libvirt] [PATCH] iohelper: Don't report errors on special FDs

On 11/05/2012 07:54 AM, Michal Privoznik wrote: > Some FDs may not implement fdatasync() functionality, e.g. > pipes or stdout. In that case EINVAL or EROFS is returned. Don't mention 'stdout'. It is not an inherent property of fd 1 that it can't support fdatasync(); rather, it is a property of WHAT the fd is. You don't know if stdout is a pipe or a regular file (at least, not without an fstat()). > We don't want to fail then nor report any error. > > Reported-by: Christophe Fergeau <cfergeau(a)redhat.com&gt; > --- > > I know that those two 'if-s' can be joined together but it just looks weird to me. > > src/util/iohelper.c | 7 +++++-- > 1 files changed, 5 insertions(+), 2 deletions(-) > > diff --git a/src/util/iohelper.c b/src/util/iohelper.c > index 860e14a..b8c91aa 100644 > --- a/src/util/iohelper.c > +++ b/src/util/iohelper.c > @@ -181,8 +181,11 @@ runIO(const char *path, int fd, int oflags, unsigned long long length) > > /* Ensure all data is written */ > if (fdatasync(fdout) < 0) { > - virReportSystemError(errno, _("unable to fsync %s"), fdoutname); > - goto cleanup; > + if (errno != EINVAL && errno != EROFS) { We're highly unlikely to get EROFS this late in the game (we would have already failed at write()ing to stdout earlier). But going off the man page, I see why you did it: EROFS, EINVAL fd is bound to a special file which does not support synchro‐ nization. > + /* fdatasync() may fail on some special FDs like stdout or pipes */ Again, don't mention stdout. Mentioning just pipes is sufficient. > + virReportSystemError(errno, _("unable to fsync %s"), fdoutname); > + goto cleanup; > + } ACK if you clean up the comment and commit message.