Re: [Qemu-devel] [PATCH] check return value from read() and write() properly
Anthony Liguori writes (Re: [Qemu-devel] [PATCH] check return value from
read() and write() properly):
Ian Jackson wrote:
The system calls read and write may return less than the whole amount
requested for a number of reasons. So the idioms
if (read(fd, object, sizeof(object)) != sizeof(object)) goto fail;
and even worse
if (read(fd, object, sizeof(object)) 0) goto fail;
are wrong. Additionally, read and write may sometimes return EINTR on
some systems so interruption is not desired or expected a loop is
needed.
In the attached patch I introduce two new pairs of functions:
qemu_{read,write} which are like read and write but never
return partial answers unnecessarily
and which never fail with EINTR
qemu_{read,write}_ok which returns -1 for any failure or
incomplete read, or +1 for success,
reducing repetition at calling points
There is already a unix_write function that serves this purpose. I
think a better approach would be to define unix_read/unix_write and
remove the EAGAIN handling and instead only spin on EINTR.
Is this just an argument about the name ? I chose names like
qemu_read and qemu_write by analogy with qemu_malloc. unix_write's
name is a bit of an anomaly. It is better to call it qemu_write
because that makes it clearer that these functions should be used
pretty much everywhere.
Also, unix_write is in the wrong file. It has to be in osdep.c so
that programs like qemu-img pick it up.
I don't really like the _ok thing as it's not a very common idiom.
Picking a random example from the code (loader.c near line 50):
-if (read(fd, addr, size) != size) {
+if (qemu_read_ok(fd, addr, size) 0) {
This is an improvement because you only have to write `size' once.
The idiom being replaced above is very common in the existing code and
is very clumsy when we get to things like this (dyngen.c near line
1130, indent removed to make it more readable):
- if(read(fd, dysymtabcmd, sizeof(struct dysymtab_command)) != sizeof(struct
dysymtab_command))
+ if(qemu_read_ok(fd, dysymtabcmd, sizeof(struct dysymtab_command)) 0)
As I say the former idiom is common in qemu but it is cumbersome.
As ever, facilities should be provided - and used - to improve
cumbersome idioms.
Ian.
Re: [Qemu-devel] [PATCH] check return value from read() and write() properly
Ian Jackson wrote:
The system calls read and write may return less than the whole amount
requested for a number of reasons. So the idioms
if (read(fd, object, sizeof(object)) != sizeof(object)) goto fail;
and even worse
if (read(fd, object, sizeof(object)) 0) goto fail;
are wrong. Additionally, read and write may sometimes return EINTR on
some systems so interruption is not desired or expected a loop is
needed.
In the attached patch I introduce two new pairs of functions:
qemu_{read,write} which are like read and write but never
return partial answers unnecessarily
and which never fail with EINTR
qemu_{read,write}_ok which returns -1 for any failure or
incomplete read, or +1 for success,
reducing repetition at calling points
There is already a unix_write function that serves this purpose. I
think a better approach would be to define unix_read/unix_write and
remove the EAGAIN handling and instead only spin on EINTR.
I don't really like the _ok thing as it's not a very common idiom.
Regards,
Anthony Liguori
I added these to osdep.c, and there are many calls in the block
drivers, so osdep.o needs to be in BLOCK_OBJS as I do in my previous
patch (getting rid of the duplicate definitions of qemu_malloc c).
The patch then uses these new functions whereever appropriate. I
think I have got each calling point correct but for obvious reasons I
haven't done a thorough test.
The resulting code is I think both smaller and more correct. In most
cases the correct behaviour was obvious.
There was one nonobvious case: I removed unix_write from vl.c and
replaced calls to it with calls to qemu_write. unix_write looped on
EAGAIN (though qemu_write doesn't) but I think this is wrong since
that simply results in spinning if the fd is nonblocking and the write
cannot complete immediately. Callers with nonblocking fds have to
cope with partial results and retry later. Since unix_write doesn't
do that I assume that its callers don't really have nonblocking fds;
if they do then the old code is buggy and my new code is buggy too but
in a different way.
Also, the Makefile rule for dyngen$(EXESUF) referred to dyngen.c
rather than dyngen.o, which appears to have been a mistake which I
have fixed since I had to add osdep.o anyway.
Regards,
Ian.
[Qemu-devel] [PATCH] check return value from read() and write() properly
The system calls read and write may return less than the whole amount
requested for a number of reasons. So the idioms
if (read(fd, object, sizeof(object)) != sizeof(object)) goto fail;
and even worse
if (read(fd, object, sizeof(object)) 0) goto fail;
are wrong. Additionally, read and write may sometimes return EINTR on
some systems so interruption is not desired or expected a loop is
needed.
In the attached patch I introduce two new pairs of functions:
qemu_{read,write} which are like read and write but never
return partial answers unnecessarily
and which never fail with EINTR
qemu_{read,write}_ok which returns -1 for any failure or
incomplete read, or +1 for success,
reducing repetition at calling points
I added these to osdep.c, and there are many calls in the block
drivers, so osdep.o needs to be in BLOCK_OBJS as I do in my previous
patch (getting rid of the duplicate definitions of qemu_malloc c).
The patch then uses these new functions whereever appropriate. I
think I have got each calling point correct but for obvious reasons I
haven't done a thorough test.
The resulting code is I think both smaller and more correct. In most
cases the correct behaviour was obvious.
There was one nonobvious case: I removed unix_write from vl.c and
replaced calls to it with calls to qemu_write. unix_write looped on
EAGAIN (though qemu_write doesn't) but I think this is wrong since
that simply results in spinning if the fd is nonblocking and the write
cannot complete immediately. Callers with nonblocking fds have to
cope with partial results and retry later. Since unix_write doesn't
do that I assume that its callers don't really have nonblocking fds;
if they do then the old code is buggy and my new code is buggy too but
in a different way.
Also, the Makefile rule for dyngen$(EXESUF) referred to dyngen.c
rather than dyngen.o, which appears to have been a mistake which I
have fixed since I had to add osdep.o anyway.
Regards,
Ian.
Index: Makefile
===
RCS file: /sources/qemu/qemu/Makefile,v
retrieving revision 1.143
diff -u -r1.143 Makefile
--- Makefile14 Jan 2008 04:24:27 - 1.143
+++ Makefile25 Jan 2008 13:25:27 -
@@ -151,7 +151,7 @@
$(CC) $(CFLAGS) $(CPPFLAGS) $(BASE_CFLAGS) -c -o $@ $
# dyngen host tool
-dyngen$(EXESUF): dyngen.c
+dyngen$(EXESUF): dyngen.o osdep.o
$(HOST_CC) $(CFLAGS) $(CPPFLAGS) $(BASE_CFLAGS) -o $@ $^
clean:
Index: block-bochs.c
===
RCS file: /sources/qemu/qemu/block-bochs.c,v
retrieving revision 1.6
diff -u -r1.6 block-bochs.c
--- block-bochs.c 11 Nov 2007 02:51:16 - 1.6
+++ block-bochs.c 25 Jan 2008 13:25:27 -
@@ -126,7 +126,7 @@
s-fd = fd;
-if (read(fd, bochs, sizeof(bochs)) != sizeof(bochs)) {
+if (qemu_read(fd, bochs, sizeof(bochs)) != sizeof(bochs)) {
goto fail;
}
@@ -151,7 +151,7 @@
s-catalog_bitmap = qemu_malloc(s-catalog_size * 4);
if (!s-catalog_bitmap)
goto fail;
-if (read(s-fd, s-catalog_bitmap, s-catalog_size * 4) !=
+if (qemu_read(s-fd, s-catalog_bitmap, s-catalog_size * 4) !=
s-catalog_size * 4)
goto fail;
for (i = 0; i s-catalog_size; i++)
@@ -176,6 +176,7 @@
int64_t offset = sector_num * 512;
int64_t extent_index, extent_offset, bitmap_offset, block_offset;
char bitmap_entry;
+int r;
// seek to sector
extent_index = offset / s-extent_size;
@@ -200,7 +201,9 @@
// read in bitmap for current extent
lseek(s-fd, bitmap_offset + (extent_offset / 8), SEEK_SET);
-read(s-fd, bitmap_entry, 1);
+r = read(s-fd, bitmap_entry, 1);
+if (r 0) return -1;
+if (r == 0) { errno= EIO; return -1; }
if (!((bitmap_entry (extent_offset % 8)) 1))
{
@@ -223,8 +226,8 @@
while (nb_sectors 0) {
if (!seek_to_sector(bs, sector_num))
{
- ret = read(s-fd, buf, 512);
- if (ret != 512)
+ ret = qemu_read_ok(s-fd, buf, 512);
+ if (ret 0)
return -1;
}
else
Index: block-cloop.c
===
RCS file: /sources/qemu/qemu/block-cloop.c,v
retrieving revision 1.7
diff -u -r1.7 block-cloop.c
--- block-cloop.c 11 Nov 2007 02:51:16 - 1.7
+++ block-cloop.c 25 Jan 2008 13:25:27 -
@@ -66,10 +66,10 @@
close(s-fd);
return -1;
}
-if(read(s-fd,s-block_size,4)4)
+if(qemu_read_ok(s-fd,s-block_size,4)0)
goto cloop_close;
s-block_size=be32_to_cpu(s-block_size);
-if(read(s-fd,s-n_blocks,4)4)
+if(qemu_read_ok(s-fd,s-n_blocks,4)0)
goto cloop_close;
s-n_blocks=be32_to_cpu(s-n_blocks);
@@ -77,7 +77,7 @@
