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