[libvirt] [PATCH] iohelper: Don't report errors on special FDs
Some FDs may not implement fdatasync() functionality, e.g. pipes or stdout. In that case EINVAL or EROFS is returned. We don't want to fail then nor report any error. Reported-by: Christophe Fergeau <cfergeau@redhat.com> --- 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) { + /* fdatasync() may fail on some special FDs like stdout or pipes */ + virReportSystemError(errno, _("unable to fsync %s"), fdoutname); + goto cleanup; + } } ret = 0; -- 1.7.8.6
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@redhat.com> ---
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. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05.11.2012 16:40, Eric Blake wrote:
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@redhat.com> ---
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.
Okay, cleaned up and pushed. Thanks. Michal
participants (2)
-
Eric Blake -
Michal Privoznik