[libvirt] [PATCH] safe{read,write}: Don't lie on nonblocking FD
Currently, whenever somebody calls saferead() on nonblocking FD (safewrite() is totally interchangeable for purpose of this message) he might get wrong return value. For instance, in the first iteration some data is read. The number of bytes read is stored into local variable 'nread'. However, in next iterations we can get -1 from read() with errno == EAGAIN, in which case the -1 is returned despite fact some data has already been read. So the caller gets confused. Moreover, the comment just above the functions says, they act like regular read() with nicer handling of EINTR. Well, they don't now. --- src/util/virutil.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/util/virutil.c b/src/util/virutil.c index b36e9d3..cd19971 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -105,6 +105,8 @@ saferead(int fd, void *buf, size_t count) size_t nread = 0; while (count > 0) { ssize_t r = read(fd, buf, count); + if (r < 0 && nread && errno == EAGAIN) + return nread; if (r < 0 && errno == EINTR) continue; if (r < 0) @@ -125,7 +127,8 @@ safewrite(int fd, const void *buf, size_t count) size_t nwritten = 0; while (count > 0) { ssize_t r = write(fd, buf, count); - + if (r < 0 && nwritten && errno == EAGAIN) + return nwritten; if (r < 0 && errno == EINTR) continue; if (r < 0) -- 1.8.0.2
On Wed, Jan 16, 2013 at 07:27:46PM +0100, Michal Privoznik wrote:
Currently, whenever somebody calls saferead() on nonblocking FD (safewrite() is totally interchangeable for purpose of this message) he might get wrong return value. For instance, in the first iteration some data is read. The number of bytes read is stored into local variable 'nread'. However, in next iterations we can get -1 from read() with errno == EAGAIN, in which case the -1 is returned despite fact some data has already been read. So the caller gets confused.
Moreover, the comment just above the functions says, they act like regular read() with nicer handling of EINTR. Well, they don't now.
I think that it is correct that these APIs return -1 on EAGAIN. These APIs should *not* be used on non-blocking FDs. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 16.01.2013 19:31, Daniel P. Berrange wrote:
On Wed, Jan 16, 2013 at 07:27:46PM +0100, Michal Privoznik wrote:
Currently, whenever somebody calls saferead() on nonblocking FD (safewrite() is totally interchangeable for purpose of this message) he might get wrong return value. For instance, in the first iteration some data is read. The number of bytes read is stored into local variable 'nread'. However, in next iterations we can get -1 from read() with errno == EAGAIN, in which case the -1 is returned despite fact some data has already been read. So the caller gets confused.
Moreover, the comment just above the functions says, they act like regular read() with nicer handling of EINTR. Well, they don't now.
I think that it is correct that these APIs return -1 on EAGAIN. These APIs should *not* be used on non-blocking FDs.
Daniel
In that case I think we have to note it explicitly in the comments. Michal
On Wed, Jan 16, 2013 at 07:39:53PM +0100, Michal Privoznik wrote:
On 16.01.2013 19:31, Daniel P. Berrange wrote:
On Wed, Jan 16, 2013 at 07:27:46PM +0100, Michal Privoznik wrote:
Currently, whenever somebody calls saferead() on nonblocking FD (safewrite() is totally interchangeable for purpose of this message) he might get wrong return value. For instance, in the first iteration some data is read. The number of bytes read is stored into local variable 'nread'. However, in next iterations we can get -1 from read() with errno == EAGAIN, in which case the -1 is returned despite fact some data has already been read. So the caller gets confused.
Moreover, the comment just above the functions says, they act like regular read() with nicer handling of EINTR. Well, they don't now.
I think that it is correct that these APIs return -1 on EAGAIN. These APIs should *not* be used on non-blocking FDs.
In that case I think we have to note it explicitly in the comments.
BTW, what code did you encounter that was using this with non-blocking fds ? Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 16.01.2013 19:40, Daniel P. Berrange wrote:
On Wed, Jan 16, 2013 at 07:39:53PM +0100, Michal Privoznik wrote:
On 16.01.2013 19:31, Daniel P. Berrange wrote:
On Wed, Jan 16, 2013 at 07:27:46PM +0100, Michal Privoznik wrote:
Currently, whenever somebody calls saferead() on nonblocking FD (safewrite() is totally interchangeable for purpose of this message) he might get wrong return value. For instance, in the first iteration some data is read. The number of bytes read is stored into local variable 'nread'. However, in next iterations we can get -1 from read() with errno == EAGAIN, in which case the -1 is returned despite fact some data has already been read. So the caller gets confused.
Moreover, the comment just above the functions says, they act like regular read() with nicer handling of EINTR. Well, they don't now.
I think that it is correct that these APIs return -1 on EAGAIN. These APIs should *not* be used on non-blocking FDs.
In that case I think we have to note it explicitly in the comments.
BTW, what code did you encounter that was using this with non-blocking fds ?
Daniel
My new code which I am working on. Basically, from the event loop I was trying to read from a FD (hence a nonblocking FD) and I used saferead(fd, ...) instead of read(fd, ...). It took me a while to find out why am I not getting anything else than -1/EAGAIN. Michal
participants (2)
-
Daniel P. Berrange -
Michal Privoznik