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@xxxxxxxxxx>
>> ---
>>
>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list