* [Qemu-devel] [PATCH] check return value from read() and write() properly
@ 2008-01-25 14:10 Ian Jackson
2008-02-06 17:14 ` [Qemu-devel] " Ian Jackson
2008-02-06 19:18 ` [Qemu-devel] " Anthony Liguori
0 siblings, 2 replies; 4+ messages in thread
From: Ian Jackson @ 2008-01-25 14:10 UTC (permalink / raw)
To: qemu-devel
[-- Attachment #1: message body text --]
[-- Type: text/plain, Size: 2089 bytes --]
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.
[-- Attachment #2: handle return value from read/write correctly --]
[-- Type: text/plain, Size: 37295 bytes --]
Index: Makefile
===================================================================
RCS file: /sources/qemu/qemu/Makefile,v
retrieving revision 1.143
diff -u -r1.143 Makefile
--- Makefile 14 Jan 2008 04:24:27 -0000 1.143
+++ Makefile 25 Jan 2008 13:25:27 -0000
@@ -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 -0000 1.6
+++ block-bochs.c 25 Jan 2008 13:25:27 -0000
@@ -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 -0000 1.7
+++ block-cloop.c 25 Jan 2008 13:25:27 -0000
@@ -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 @@
offsets_size=s->n_blocks*sizeof(uint64_t);
if(!(s->offsets=(uint64_t*)malloc(offsets_size)))
goto cloop_close;
- if(read(s->fd,s->offsets,offsets_size)<offsets_size)
+ if(qemu_read_ok(s->fd,s->offsets,offsets_size)<0)
goto cloop_close;
for(i=0;i<s->n_blocks;i++) {
s->offsets[i]=be64_to_cpu(s->offsets[i]);
@@ -109,8 +109,8 @@
uint32_t bytes = s->offsets[block_num+1]-s->offsets[block_num];
lseek(s->fd, s->offsets[block_num], SEEK_SET);
- ret = read(s->fd, s->compressed_block, bytes);
- if (ret != bytes)
+ ret = qemu_read_ok(s->fd, s->compressed_block, bytes);
+ if (ret < 0)
return -1;
s->zstream.next_in = s->compressed_block;
Index: block-cow.c
===================================================================
RCS file: /sources/qemu/qemu/block-cow.c,v
retrieving revision 1.10
diff -u -r1.10 block-cow.c
--- block-cow.c 11 Nov 2007 02:51:16 -0000 1.10
+++ block-cow.c 25 Jan 2008 13:25:28 -0000
@@ -77,7 +77,7 @@
}
s->fd = fd;
/* see if it is a cow image */
- if (read(fd, &cow_header, sizeof(cow_header)) != sizeof(cow_header)) {
+ if (qemu_read_ok(fd, &cow_header, sizeof(cow_header)) < 0) {
goto fail;
}
@@ -159,8 +159,8 @@
while (nb_sectors > 0) {
if (is_changed(s->cow_bitmap, sector_num, nb_sectors, &n)) {
lseek(s->fd, s->cow_sectors_offset + sector_num * 512, SEEK_SET);
- ret = read(s->fd, buf, n * 512);
- if (ret != n * 512)
+ ret = qemu_read_ok(s->fd, buf, n * 512);
+ if (ret < 0)
return -1;
} else {
if (bs->backing_hd) {
Index: block-dmg.c
===================================================================
RCS file: /sources/qemu/qemu/block-dmg.c,v
retrieving revision 1.8
diff -u -r1.8 block-dmg.c
--- block-dmg.c 11 Nov 2007 02:51:16 -0000 1.8
+++ block-dmg.c 25 Jan 2008 13:25:28 -0000
@@ -60,7 +60,7 @@
static off_t read_off(int fd)
{
uint64_t buffer;
- if(read(fd,&buffer,8)<8)
+ if(qemu_read_ok(fd,&buffer,8)<0)
return 0;
return be64_to_cpu(buffer);
}
@@ -68,7 +68,7 @@
static off_t read_uint32(int fd)
{
uint32_t buffer;
- if(read(fd,&buffer,4)<4)
+ if(qemu_read_ok(fd,&buffer,4)<0)
return 0;
return be32_to_cpu(buffer);
}
@@ -209,24 +209,15 @@
s->current_chunk = s->n_chunks;
switch(s->types[chunk]) {
case 0x80000005: { /* zlib compressed */
- int i;
-
ret = lseek(s->fd, s->offsets[chunk], SEEK_SET);
if(ret<0)
return -1;
/* we need to buffer, because only the chunk as whole can be
* inflated. */
- i=0;
- do {
- ret = read(s->fd, s->compressed_chunk+i, s->lengths[chunk]-i);
- if(ret<0 && errno==EINTR)
- ret=0;
- i+=ret;
- } while(ret>=0 && ret+i<s->lengths[chunk]);
-
- if (ret != s->lengths[chunk])
- return -1;
+ ret = qemu_read_ok(s->fd, s->compressed_chunk, s->lengths[chunk]);
+ if (ret < 0)
+ return -1;
s->zstream.next_in = s->compressed_chunk;
s->zstream.avail_in = s->lengths[chunk];
@@ -240,8 +231,8 @@
return -1;
break; }
case 1: /* copy */
- ret = read(s->fd, s->uncompressed_chunk, s->lengths[chunk]);
- if (ret != s->lengths[chunk])
+ ret = qemu_read_ok(s->fd, s->uncompressed_chunk, s->lengths[chunk]);
+ if (ret < 0)
return -1;
break;
case 2: /* zero */
Index: block-parallels.c
===================================================================
RCS file: /sources/qemu/qemu/block-parallels.c,v
retrieving revision 1.2
diff -u -r1.2 block-parallels.c
--- block-parallels.c 11 Nov 2007 02:51:16 -0000 1.2
+++ block-parallels.c 25 Jan 2008 13:25:28 -0000
@@ -84,7 +84,7 @@
s->fd = fd;
- if (read(fd, &ph, sizeof(ph)) != sizeof(ph))
+ if (qemu_read_ok(fd, &ph, sizeof(ph)) < 0)
goto fail;
if (memcmp(ph.magic, HEADER_MAGIC, 16) ||
@@ -103,8 +103,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) !=
- s->catalog_size * 4)
+ if (qemu_read_ok(s->fd, s->catalog_bitmap, s->catalog_size * 4) < 0)
goto fail;
for (i = 0; i < s->catalog_size; i++)
le32_to_cpus(&s->catalog_bitmap[i]);
@@ -147,7 +146,7 @@
while (nb_sectors > 0) {
if (!seek_to_sector(bs, sector_num)) {
- if (read(s->fd, buf, 512) != 512)
+ if (qemu_read_ok(s->fd, buf, 512) < 0)
return -1;
} else
memset(buf, 0, 512);
Index: block-qcow.c
===================================================================
RCS file: /sources/qemu/qemu/block-qcow.c,v
retrieving revision 1.15
diff -u -r1.15 block-qcow.c
--- block-qcow.c 11 Nov 2007 02:51:16 -0000 1.15
+++ block-qcow.c 25 Jan 2008 13:25:28 -0000
@@ -738,7 +738,7 @@
static int qcow_create(const char *filename, int64_t total_size,
const char *backing_file, int flags)
{
- int fd, header_size, backing_filename_len, l1_size, i, shift;
+ int fd, header_size, backing_filename_len, l1_size, i, shift, errno_save;
QCowHeader header;
uint64_t tmp;
@@ -775,18 +775,28 @@
header.crypt_method = cpu_to_be32(QCOW_CRYPT_NONE);
}
+
/* write all the data */
- write(fd, &header, sizeof(header));
+ if (qemu_write_ok(fd, &header, sizeof(header)) < 0)
+ goto fail;
if (backing_file) {
- write(fd, backing_file, backing_filename_len);
+ if (qemu_write_ok(fd, backing_file, backing_filename_len) < 0)
+ goto fail;
}
lseek(fd, header_size, SEEK_SET);
tmp = 0;
for(i = 0;i < l1_size; i++) {
- write(fd, &tmp, sizeof(tmp));
+ if (qemu_write_ok(fd, &tmp, sizeof(tmp)) < 0)
+ goto fail;
}
close(fd);
return 0;
+
+ fail:
+ errno_save= errno;
+ close(fd);
+ errno= errno_save;
+ return -1;
}
static int qcow_make_empty(BlockDriverState *bs)
Index: block-raw-posix.c
===================================================================
RCS file: /sources/qemu/qemu/block-raw-posix.c,v
retrieving revision 1.4
diff -u -r1.4 block-raw-posix.c
--- block-raw-posix.c 6 Jan 2008 18:53:07 -0000 1.4
+++ block-raw-posix.c 25 Jan 2008 13:25:28 -0000
@@ -163,7 +163,7 @@
}
s->lseek_err_cnt=0;
- ret = read(s->fd, buf, count);
+ ret = qemu_read(s->fd, buf, count);
if (ret == count)
goto label__raw_read__success;
@@ -175,11 +175,11 @@
/* Try harder for CDrom. */
if (bs->type == BDRV_TYPE_CDROM) {
lseek(s->fd, offset, SEEK_SET);
- ret = read(s->fd, buf, count);
+ ret = qemu_read(s->fd, buf, count);
if (ret == count)
goto label__raw_read__success;
lseek(s->fd, offset, SEEK_SET);
- ret = read(s->fd, buf, count);
+ ret = qemu_read(s->fd, buf, count);
if (ret == count)
goto label__raw_read__success;
@@ -216,7 +216,7 @@
}
s->lseek_err_cnt = 0;
- ret = write(s->fd, buf, count);
+ ret = qemu_write(s->fd, buf, count);
if (ret == count)
goto label__raw_write__success;
Index: block-vmdk.c
===================================================================
RCS file: /sources/qemu/qemu/block-vmdk.c,v
retrieving revision 1.19
diff -u -r1.19 block-vmdk.c
--- block-vmdk.c 14 Jan 2008 03:48:37 -0000 1.19
+++ block-vmdk.c 25 Jan 2008 13:25:29 -0000
@@ -220,13 +220,13 @@
/* read the header */
if (lseek(p_fd, 0x0, SEEK_SET) == -1)
goto fail;
- if (read(p_fd, hdr, HEADER_SIZE) != HEADER_SIZE)
+ if (qemu_read_ok(p_fd, hdr, HEADER_SIZE) < 0)
goto fail;
/* write the header */
if (lseek(snp_fd, 0x0, SEEK_SET) == -1)
goto fail;
- if (write(snp_fd, hdr, HEADER_SIZE) == -1)
+ if (qemu_write_ok(snp_fd, hdr, HEADER_SIZE) == -1)
goto fail;
memset(&header, 0, sizeof(header));
@@ -236,7 +236,7 @@
/* the descriptor offset = 0x200 */
if (lseek(p_fd, 0x200, SEEK_SET) == -1)
goto fail;
- if (read(p_fd, p_desc, DESC_SIZE) != DESC_SIZE)
+ if (qemu_read_ok(p_fd, p_desc, DESC_SIZE) < 0)
goto fail;
if ((p_name = strstr(p_desc,"CID")) != 0) {
@@ -258,7 +258,7 @@
/* write the descriptor */
if (lseek(snp_fd, 0x200, SEEK_SET) == -1)
goto fail;
- if (write(snp_fd, s_desc, strlen(s_desc)) == -1)
+ if (qemu_write_ok(snp_fd, s_desc, strlen(s_desc)) == -1)
goto fail;
gd_offset = header.gd_offset * SECTOR_SIZE; // offset of GD table
@@ -280,11 +280,11 @@
goto fail;
if (lseek(p_fd, rgd_offset, SEEK_SET) == -1)
goto fail_rgd;
- if (read(p_fd, rgd_buf, gd_size) != gd_size)
+ if (qemu_read_ok(p_fd, rgd_buf, gd_size) < 0)
goto fail_rgd;
if (lseek(snp_fd, rgd_offset, SEEK_SET) == -1)
goto fail_rgd;
- if (write(snp_fd, rgd_buf, gd_size) == -1)
+ if (qemu_write_ok(snp_fd, rgd_buf, gd_size) == -1)
goto fail_rgd;
qemu_free(rgd_buf);
@@ -294,11 +294,11 @@
goto fail_rgd;
if (lseek(p_fd, gd_offset, SEEK_SET) == -1)
goto fail_gd;
- if (read(p_fd, gd_buf, gd_size) != gd_size)
+ if (qemu_read_ok(p_fd, gd_buf, gd_size) < 0)
goto fail_gd;
if (lseek(snp_fd, gd_offset, SEEK_SET) == -1)
goto fail_gd;
- if (write(snp_fd, gd_buf, gd_size) == -1)
+ if (qemu_write_ok(snp_fd, gd_buf, gd_size) == -1)
goto fail_gd;
qemu_free(gd_buf);
@@ -765,6 +765,8 @@
header.check_bytes[2] = 0xd;
header.check_bytes[3] = 0xa;
+ /* BUG XXX No error checking anywhere here! XXX BUG */
+
/* write all the data */
write(fd, &magic, sizeof(magic));
write(fd, &header, sizeof(header));
Index: block-vpc.c
===================================================================
RCS file: /sources/qemu/qemu/block-vpc.c,v
retrieving revision 1.8
diff -u -r1.8 block-vpc.c
--- block-vpc.c 16 Dec 2007 03:16:05 -0000 1.8
+++ block-vpc.c 25 Jan 2008 13:25:29 -0000
@@ -100,14 +100,14 @@
s->fd = fd;
- if (read(fd, &header, HEADER_SIZE) != HEADER_SIZE)
+ if (qemu_read_ok(fd, &header, HEADER_SIZE) < 0)
goto fail;
if (strncmp(header.magic, "conectix", 8))
goto fail;
lseek(s->fd, be32_to_cpu(header.type.main.subheader_offset), SEEK_SET);
- if (read(fd, &header, HEADER_SIZE) != HEADER_SIZE)
+ if (qemu_read_ok(fd, &header, HEADER_SIZE) < 0)
goto fail;
if (strncmp(header.magic, "cxsparse", 8))
@@ -122,8 +122,7 @@
s->pagetable = qemu_malloc(s->pagetable_entries * 4);
if (!s->pagetable)
goto fail;
- if (read(s->fd, s->pagetable, s->pagetable_entries * 4) !=
- s->pagetable_entries * 4)
+ if (qemu_read_ok(s->fd, s->pagetable, s->pagetable_entries * 4) < 0)
goto fail;
for (i = 0; i < s->pagetable_entries; i++)
be32_to_cpus(&s->pagetable[i]);
@@ -175,7 +174,7 @@
// Scary! Bitmap is stored as big endian 32bit entries,
// while we used to look it up byte by byte
- read(s->fd, s->pageentry_u8, 512);
+ read(s->fd, s->pageentry_u8, 512); // no error handling ?!
for (i = 0; i < 128; i++)
be32_to_cpus(&s->pageentry_u32[i]);
}
@@ -185,7 +184,7 @@
#else
lseek(s->fd, bitmap_offset + (pageentry_index / 8), SEEK_SET);
- read(s->fd, &bitmap_entry, 1);
+ read(s->fd, &bitmap_entry, 1); // no error handling ?!
if ((bitmap_entry >> (pageentry_index % 8)) & 1)
return -1; // not allocated
@@ -205,8 +204,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-vvfat.c
===================================================================
RCS file: /sources/qemu/qemu/block-vvfat.c,v
retrieving revision 1.16
diff -u -r1.16 block-vvfat.c
--- block-vvfat.c 24 Dec 2007 13:26:04 -0000 1.16
+++ block-vvfat.c 25 Jan 2008 13:25:30 -0000
@@ -1173,7 +1173,6 @@
static inline int read_cluster(BDRVVVFATState *s,int cluster_num)
{
if(s->current_cluster != cluster_num) {
- int result=0;
off_t offset;
assert(!s->current_mapping || s->current_fd || (s->current_mapping->mode & MODE_DIRECTORY));
if(!s->current_mapping
@@ -1208,8 +1207,7 @@
if(lseek(s->current_fd, offset, SEEK_SET)!=offset)
return -3;
s->cluster=s->cluster_buffer;
- result=read(s->current_fd,s->cluster,s->cluster_size);
- if(result<0) {
+ if (qemu_read_ok(s->current_fd,s->cluster,s->cluster_size) < 0) {
s->current_cluster = -1;
return -1;
}
@@ -2241,7 +2239,7 @@
if (ret < 0)
return ret;
- if (write(fd, cluster, rest_size) < 0)
+ if (qemu_write_ok(fd, cluster, rest_size) < 0)
return -2;
offset += rest_size;
Index: dyngen.c
===================================================================
RCS file: /sources/qemu/qemu/dyngen.c,v
retrieving revision 1.59
diff -u -r1.59 dyngen.c
--- dyngen.c 18 Nov 2007 21:22:10 -0000 1.59
+++ dyngen.c 25 Jan 2008 13:25:32 -0000
@@ -31,6 +31,7 @@
#include <fcntl.h>
#include "config-host.h"
+#include "osdep.h"
/* NOTE: we test CONFIG_WIN32 instead of _WIN32 to enabled cross
compilation */
@@ -251,7 +252,7 @@
if (!data)
return NULL;
lseek(fd, offset, SEEK_SET);
- if (read(fd, data, size) != size) {
+ if (qemu_read_ok(fd, data, size) < 0) {
free(data);
return NULL;
}
@@ -489,7 +490,7 @@
error("can't open file '%s'", filename);
/* Read ELF header. */
- if (read(fd, &ehdr, sizeof (ehdr)) != sizeof (ehdr))
+ if (qemu_read_ok(fd, &ehdr, sizeof (ehdr)) < 0)
error("unable to read file header");
/* Check ELF identification. */
@@ -713,7 +714,7 @@
error("can't open file '%s'", filename);
/* Read COFF header. */
- if (read(fd, &fhdr, sizeof (fhdr)) != sizeof (fhdr))
+ if (qemu_read_ok(fd, &fhdr, sizeof (fhdr)) < 0)
error("unable to read file header");
/* Check COFF identification. */
@@ -1086,7 +1087,7 @@
error("can't open file '%s'", filename);
/* Read Mach header. */
- if (read(fd, &mach_hdr, sizeof (mach_hdr)) != sizeof (mach_hdr))
+ if (qemu_read_ok(fd, &mach_hdr, sizeof (mach_hdr)) < 0)
error("unable to read file header");
/* Check Mach identification. */
@@ -1103,14 +1104,14 @@
/* read segment headers */
for(i=0, j=sizeof(mach_hdr); i<mach_hdr.ncmds ; i++)
{
- if(read(fd, &lc, sizeof(struct load_command)) != sizeof(struct load_command))
+ if(qemu_read_ok(fd, &lc, sizeof(struct load_command)) < 0)
error("unable to read load_command");
if(lc.cmd == LC_SEGMENT)
{
offset_to_segment = j;
lseek(fd, offset_to_segment, SEEK_SET);
segment = malloc(sizeof(struct segment_command));
- if(read(fd, segment, sizeof(struct segment_command)) != sizeof(struct segment_command))
+ if(qemu_read_ok(fd, segment, sizeof(struct segment_command)) < 0)
error("unable to read LC_SEGMENT");
}
if(lc.cmd == LC_DYSYMTAB)
@@ -1118,7 +1119,7 @@
offset_to_dysymtab = j;
lseek(fd, offset_to_dysymtab, SEEK_SET);
dysymtabcmd = malloc(sizeof(struct dysymtab_command));
- if(read(fd, dysymtabcmd, sizeof(struct dysymtab_command)) != sizeof(struct dysymtab_command))
+ if(qemu_read_ok(fd, dysymtabcmd, sizeof(struct dysymtab_command)) < 0)
error("unable to read LC_DYSYMTAB");
}
if(lc.cmd == LC_SYMTAB)
@@ -1126,7 +1127,7 @@
offset_to_symtab = j;
lseek(fd, offset_to_symtab, SEEK_SET);
symtabcmd = malloc(sizeof(struct symtab_command));
- if(read(fd, symtabcmd, sizeof(struct symtab_command)) != sizeof(struct symtab_command))
+ if(qemu_read_ok(fd, symtabcmd, sizeof(struct symtab_command)) < 0)
error("unable to read LC_SYMTAB");
}
j+=lc.cmdsize;
Index: elf_ops.h
===================================================================
RCS file: /sources/qemu/qemu/elf_ops.h,v
retrieving revision 1.12
diff -u -r1.12 elf_ops.h
--- elf_ops.h 18 Nov 2007 01:44:35 -0000 1.12
+++ elf_ops.h 25 Jan 2008 13:25:32 -0000
@@ -149,7 +149,7 @@
uint64_t addr, low = 0, high = 0;
uint8_t *data = NULL;
- if (read(fd, &ehdr, sizeof(ehdr)) != sizeof(ehdr))
+ if (qemu_read_ok(fd, &ehdr, sizeof(ehdr)) < 0)
goto fail;
if (must_swab) {
glue(bswap_ehdr, SZ)(&ehdr);
@@ -168,7 +168,7 @@
phdr = qemu_mallocz(size);
if (!phdr)
goto fail;
- if (read(fd, phdr, size) != size)
+ if (qemu_read_ok(fd, phdr, size) < 0)
goto fail;
if (must_swab) {
for(i = 0; i < ehdr.e_phnum; i++) {
@@ -187,7 +187,7 @@
if (ph->p_filesz > 0) {
if (lseek(fd, ph->p_offset, SEEK_SET) < 0)
goto fail;
- if (read(fd, data, ph->p_filesz) != ph->p_filesz)
+ if (qemu_read_ok(fd, data, ph->p_filesz) < 0)
goto fail;
}
addr = ph->p_vaddr + virt_to_phys_addend;
Index: loader.c
===================================================================
RCS file: /sources/qemu/qemu/loader.c,v
retrieving revision 1.11
diff -u -r1.11 loader.c
--- loader.c 18 Nov 2007 01:44:35 -0000 1.11
+++ loader.c 25 Jan 2008 13:25:34 -0000
@@ -47,7 +47,7 @@
return -1;
size = lseek(fd, 0, SEEK_END);
lseek(fd, 0, SEEK_SET);
- if (read(fd, addr, size) != size) {
+ if (qemu_read_ok(fd, addr, size) < 0) {
close(fd);
return -1;
}
@@ -115,8 +115,7 @@
if (fd < 0)
return -1;
- size = read(fd, &e, sizeof(e));
- if (size < 0)
+ if (qemu_read_ok(fd, &e, sizeof(e)) < 0)
goto fail;
bswap_ahdr(&e);
@@ -127,17 +126,17 @@
case QMAGIC:
case OMAGIC:
lseek(fd, N_TXTOFF(e), SEEK_SET);
- size = read(fd, addr, e.a_text + e.a_data);
- if (size < 0)
+ size = qemu_read(fd, addr, e.a_text + e.a_data);
+ if (size != e.a_text + e.a_data)
goto fail;
break;
case NMAGIC:
lseek(fd, N_TXTOFF(e), SEEK_SET);
- size = read(fd, addr, e.a_text);
- if (size < 0)
+ size = qemu_read(fd, addr, e.a_text);
+ if (size != e.a_text)
goto fail;
- ret = read(fd, addr + N_DATADDR(e), e.a_data);
- if (ret < 0)
+ ret = qemu_read(fd, addr + N_DATADDR(e), e.a_data);
+ if (ret != e.a_data)
goto fail;
size += ret;
break;
@@ -161,7 +160,7 @@
ptr = qemu_malloc(size);
if (!ptr)
return NULL;
- if (read(fd, ptr, size) != size) {
+ if (qemu_read_ok(fd, ptr, size) < 0) {
qemu_free(ptr);
return NULL;
}
@@ -210,7 +209,7 @@
perror(filename);
return -1;
}
- if (read(fd, e_ident, sizeof(e_ident)) != sizeof(e_ident))
+ if (qemu_read_ok(fd, e_ident, sizeof(e_ident)) < 0)
goto fail;
if (e_ident[0] != ELFMAG0 ||
e_ident[1] != ELFMAG1 ||
@@ -276,7 +275,7 @@
if (fd < 0)
return -1;
- size = read(fd, hdr, sizeof(uboot_image_header_t));
+ size = qemu_read(fd, hdr, sizeof(uboot_image_header_t));
if (size < 0)
goto fail;
@@ -310,7 +309,7 @@
if (!data)
goto fail;
- if (read(fd, data, hdr->ih_size) != hdr->ih_size) {
+ if (qemu_read_ok(fd, data, hdr->ih_size) < 0) {
fprintf(stderr, "Error reading file\n");
goto fail;
}
Index: osdep.c
===================================================================
RCS file: /sources/qemu/qemu/osdep.c,v
retrieving revision 1.22
diff -u -r1.22 osdep.c
--- osdep.c 24 Dec 2007 14:33:23 -0000 1.22
+++ osdep.c 25 Jan 2008 13:25:34 -0000
@@ -28,6 +28,7 @@
#include <errno.h>
#include <unistd.h>
#include <fcntl.h>
+#include <assert.h>
#ifdef HOST_SOLARIS
#include <sys/types.h>
#include <sys/statvfs.h>
@@ -60,6 +61,33 @@
return malloc(size);
}
+#define define_readwrite(rw, cnst) \
+ssize_t qemu_##rw(int fd, cnst void *buf, size_t count) { \
+ ssize_t got, done; \
+ done = 0; \
+ while (count>0) { \
+ got = rw(fd, buf, count); \
+ if (got == 0) { errno = 0; return done; } \
+ if (got < 0) { \
+ if (errno==EINTR) continue; \
+ return done ? done : -1; \
+ } \
+ assert(got <= count); \
+ done += got; \
+ count -= got; \
+ buf = (cnst char*)buf + got; \
+ } \
+ return done; \
+} \
+int qemu_##rw##_ok(int fd, cnst void *buf, size_t count) { \
+ ssize_t got; \
+ got = qemu_##rw(fd, buf, count); \
+ if (got == count) return 1; \
+ return -1; \
+}
+define_readwrite(read, /* empty */)
+define_readwrite(write, const)
+
#if defined(_WIN32)
void *qemu_memalign(size_t alignment, size_t size)
{
Index: osdep.h
===================================================================
RCS file: /sources/qemu/qemu/osdep.h,v
retrieving revision 1.13
diff -u -r1.13 osdep.h
--- osdep.h 24 Dec 2007 14:33:23 -0000 1.13
+++ osdep.h 25 Jan 2008 13:25:34 -0000
@@ -2,6 +2,7 @@
#define QEMU_OSDEP_H
#include <stdarg.h>
+#include <sys/types.h>
#ifndef glue
#define xglue(x, y) x ## y
@@ -52,6 +53,17 @@
void *qemu_vmalloc(size_t size);
void qemu_vfree(void *ptr);
+ssize_t qemu_read(int fd, void *buf, size_t count);
+ssize_t qemu_write(int fd, const void *buf, size_t count);
+ /* Repeatedly call read/write until the request is satisfied or an error
+ * occurs, and then returns what read would have done. If it returns
+ * a short read then errno is set, or zero if it was EOF. */
+
+int qemu_read_ok(int fd, void *buf, size_t count);
+int qemu_write_ok(int fd, const void *buf, size_t count);
+ /* Even more simplified versions which return 1 on success or -1 on
+ * failure. EOF counts as failure but then errno is set to 0. */
+
void *get_mmap_addr(unsigned long size);
int qemu_create_pidfile(const char *filename);
Index: usb-linux.c
===================================================================
RCS file: /sources/qemu/qemu/usb-linux.c,v
retrieving revision 1.17
diff -u -r1.17 usb-linux.c
--- usb-linux.c 18 Nov 2007 01:44:36 -0000 1.17
+++ usb-linux.c 25 Jan 2008 13:25:35 -0000
@@ -348,8 +348,8 @@
struct usbdevfs_urb *purb = NULL;
int len, ret;
- len = read(s->pipe_fds[0], &pending_urb, sizeof(pending_urb));
- if (len != sizeof(pending_urb)) {
+ ret = qemu_read_ok(s->pipe_fds[0], &pending_urb, sizeof(pending_urb));
+ if (ret < 0) {
printf("urb_completion: error reading pending_urb, len=%d\n", len);
return;
}
Index: vl.c
===================================================================
RCS file: /sources/qemu/qemu/vl.c,v
retrieving revision 1.401
diff -u -r1.401 vl.c
--- vl.c 23 Jan 2008 19:01:12 -0000 1.401
+++ vl.c 25 Jan 2008 13:25:38 -0000
@@ -1960,29 +1960,9 @@
#else
-static int unix_write(int fd, const uint8_t *buf, int len1)
-{
- int ret, len;
-
- len = len1;
- while (len > 0) {
- ret = write(fd, buf, len);
- if (ret < 0) {
- if (errno != EINTR && errno != EAGAIN)
- return -1;
- } else if (ret == 0) {
- break;
- } else {
- buf += ret;
- len -= ret;
- }
- }
- return len1 - len;
-}
-
static inline int send_all(int fd, const uint8_t *buf, int len1)
{
- return unix_write(fd, buf, len1);
+ return qemu_write(fd, buf, len1);
}
void socket_set_nonblock(int fd)
@@ -2004,7 +1984,7 @@
static int fd_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
{
FDCharDriver *s = chr->opaque;
- return unix_write(s->fd_out, buf, len);
+ return qemu_write(s->fd_out, buf, len);
}
static int fd_chr_read_poll(void *opaque)
@@ -2440,8 +2420,7 @@
case CHR_IOCTL_PP_EPP_READ_ADDR:
if (pp_hw_mode(drv, IEEE1284_MODE_EPP|IEEE1284_ADDR)) {
struct ParallelIOArg *parg = arg;
- int n = read(fd, parg->buffer, parg->count);
- if (n != parg->count) {
+ if (qemu_read_ok(fd, parg->buffer, parg->count) < 0) {
return -EIO;
}
}
@@ -2449,8 +2428,7 @@
case CHR_IOCTL_PP_EPP_READ:
if (pp_hw_mode(drv, IEEE1284_MODE_EPP)) {
struct ParallelIOArg *parg = arg;
- int n = read(fd, parg->buffer, parg->count);
- if (n != parg->count) {
+ if (qemu_read_ok(fd, parg->buffer, parg->count) < 0) {
return -EIO;
}
}
@@ -2458,8 +2436,7 @@
case CHR_IOCTL_PP_EPP_WRITE_ADDR:
if (pp_hw_mode(drv, IEEE1284_MODE_EPP|IEEE1284_ADDR)) {
struct ParallelIOArg *parg = arg;
- int n = write(fd, parg->buffer, parg->count);
- if (n != parg->count) {
+ if (qemu_write(fd, parg->buffer, parg->count) < 0) {
return -EIO;
}
}
@@ -2467,8 +2444,7 @@
case CHR_IOCTL_PP_EPP_WRITE:
if (pp_hw_mode(drv, IEEE1284_MODE_EPP)) {
struct ParallelIOArg *parg = arg;
- int n = write(fd, parg->buffer, parg->count);
- if (n != parg->count) {
+ if (qemu_write_ok(fd, parg->buffer, parg->count) < 0) {
return -EIO;
}
}
@@ -3899,7 +3875,7 @@
TAPState *s = opaque;
int ret;
for(;;) {
- ret = write(s->fd, buf, size);
+ ret = write(s->fd, buf, size); /* No error handling ?! */
if (ret < 0 && (errno == EINTR || errno == EAGAIN)) {
} else {
break;
Index: darwin-user/machload.c
===================================================================
RCS file: /sources/qemu/qemu/darwin-user/machload.c,v
retrieving revision 1.1
diff -u -r1.1 machload.c
--- darwin-user/machload.c 18 Jan 2007 20:06:33 -0000 1.1
+++ darwin-user/machload.c 25 Jan 2008 13:25:42 -0000
@@ -440,7 +440,7 @@
if (!data)
return NULL;
lseek(fd, offset, SEEK_SET);
- if (read(fd, data, size) != size) {
+ if (qemu_read_ok(fd, data, size) < 0) {
free(data);
return NULL;
}
@@ -472,7 +472,7 @@
qerror("can't open file '%s'", filename);
/* Read magic header. */
- if (read(fd, &magic, sizeof (magic)) != sizeof (magic))
+ if (qemu_read_ok(fd, &magic, sizeof (magic)) < 0)
qerror("unable to read Magic of '%s'", filename);
/* Check Mach identification. */
@@ -506,7 +506,7 @@
lseek(fd, 0, SEEK_SET);
/* Read Fat header. */
- if (read(fd, &fh, sizeof (fh)) != sizeof (fh))
+ if (qemu_read_ok(fd, &fh, sizeof (fh)) < 0)
qerror("unable to read file header");
if(need_bswap)
@@ -515,7 +515,7 @@
/* Read Fat Arch. */
fa = malloc(sizeof(struct fat_arch)*fh.nfat_arch);
- if (read(fd, fa, sizeof(struct fat_arch)*fh.nfat_arch) != sizeof(struct fat_arch)*fh.nfat_arch)
+ if (qemu_read_ok(fd, fa, sizeof(struct fat_arch)*fh.nfat_arch) < 0)
qerror("unable to read file header");
for( i = 0; i < fh.nfat_arch; i++, fa++)
@@ -529,7 +529,7 @@
/* Read Mach header. */
- if (read(fd, &mach_hdr, sizeof(struct mach_header)) != sizeof (struct mach_header))
+ if (qemu_read_ok(fd, &mach_hdr, sizeof(struct mach_header)) < 0)
qerror("unable to read file header");
if(mach_hdr.magic == MH_MAGIC)
@@ -549,7 +549,7 @@
{
lseek(fd, 0, SEEK_SET);
/* Read Mach header */
- if (read(fd, &mach_hdr, sizeof (mach_hdr)) != sizeof (mach_hdr))
+ if (qemu_read_ok(fd, &mach_hdr, sizeof (mach_hdr)) < 0)
qerror("%s: unable to read file header", filename);
}
@@ -573,7 +573,7 @@
/* read segment headers */
lcmds = malloc(mach_hdr.sizeofcmds);
- if(read(fd, lcmds, mach_hdr.sizeofcmds) != mach_hdr.sizeofcmds)
+ if(qemu_read_ok(fd, lcmds, mach_hdr.sizeofcmds) < 0)
qerror("%s: unable to read load_command", filename);
slide = 0;
mmapfixed = 0;
Index: hw/pc.c
===================================================================
RCS file: /sources/qemu/qemu/hw/pc.c,v
retrieving revision 1.97
diff -u -r1.97 pc.c
--- hw/pc.c 16 Dec 2007 03:16:05 -0000 1.97
+++ hw/pc.c 25 Jan 2008 13:25:42 -0000
@@ -446,17 +446,16 @@
return -1;
/* load 16 bit code */
- if (read(fd, real_addr, 512) != 512)
+ if (qemu_read_ok(fd, real_addr, 512) < 0)
goto fail;
setup_sects = real_addr[0x1F1];
if (!setup_sects)
setup_sects = 4;
- if (read(fd, real_addr + 512, setup_sects * 512) !=
- setup_sects * 512)
+ if (qemu_read_ok(fd, real_addr + 512, setup_sects * 512) < 0)
goto fail;
/* load 32 bit code */
- size = read(fd, addr, 16 * 1024 * 1024);
+ size = qemu_read(fd, addr, 16 * 1024 * 1024);
if (size < 0)
goto fail;
close(fd);
Index: linux-user/elfload.c
===================================================================
RCS file: /sources/qemu/qemu/linux-user/elfload.c,v
retrieving revision 1.57
diff -u -r1.57 elfload.c
--- linux-user/elfload.c 16 Nov 2007 10:46:04 -0000 1.57
+++ linux-user/elfload.c 25 Jan 2008 13:25:43 -0000
@@ -871,7 +871,7 @@
retval = lseek(interpreter_fd, interp_elf_ex->e_phoff, SEEK_SET);
if(retval >= 0) {
- retval = read(interpreter_fd,
+ retval = qemu_read_ok(interpreter_fd,
(char *) elf_phdata,
sizeof(struct elf_phdr) * interp_elf_ex->e_phnum);
}
@@ -991,7 +991,7 @@
lseek(fd, hdr->e_shoff, SEEK_SET);
for (i = 0; i < hdr->e_shnum; i++) {
- if (read(fd, &sechdr, sizeof(sechdr)) != sizeof(sechdr))
+ if (qemu_read_ok(fd, &sechdr, sizeof(sechdr)) < 0)
return;
#ifdef BSWAP_NEEDED
bswap_shdr(&sechdr);
@@ -1000,8 +1000,7 @@
symtab = sechdr;
lseek(fd, hdr->e_shoff
+ sizeof(sechdr) * sechdr.sh_link, SEEK_SET);
- if (read(fd, &strtab, sizeof(strtab))
- != sizeof(strtab))
+ if (qemu_read_ok(fd, &strtab, sizeof(strtab)) < 0)
return;
#ifdef BSWAP_NEEDED
bswap_shdr(&strtab);
@@ -1024,7 +1023,7 @@
return;
lseek(fd, symtab.sh_offset, SEEK_SET);
- if (read(fd, s->disas_symtab, symtab.sh_size) != symtab.sh_size)
+ if (qemu_read_ok(fd, s->disas_symtab, symtab.sh_size) < 0)
return;
for (i = 0; i < symtab.sh_size / sizeof(struct elf_sym); i++) {
@@ -1047,7 +1046,7 @@
s->disas_symtab = syms32;
#endif
lseek(fd, strtab.sh_offset, SEEK_SET);
- if (read(fd, strings, strtab.sh_size) != strtab.sh_size)
+ if (qemu_read_ok(fd, strings, strtab.sh_size) < 0)
return;
s->disas_num_syms = symtab.sh_size / sizeof(struct elf_sym);
s->next = syminfos;
@@ -1109,7 +1108,7 @@
retval = lseek(bprm->fd, elf_ex.e_phoff, SEEK_SET);
if(retval > 0) {
- retval = read(bprm->fd, (char *) elf_phdata,
+ retval = qemu_read_ok(bprm->fd, (char *) elf_phdata,
elf_ex.e_phentsize * elf_ex.e_phnum);
}
@@ -1164,7 +1163,7 @@
retval = lseek(bprm->fd, elf_ppnt->p_offset, SEEK_SET);
if(retval >= 0) {
- retval = read(bprm->fd, elf_interpreter, elf_ppnt->p_filesz);
+ retval = qemu_read(bprm->fd, elf_interpreter, elf_ppnt->p_filesz);
}
if(retval < 0) {
perror("load_elf_binary2");
@@ -1200,7 +1199,7 @@
if (retval >= 0) {
retval = lseek(interpreter_fd, 0, SEEK_SET);
if(retval >= 0) {
- retval = read(interpreter_fd,bprm->buf,128);
+ retval = qemu_read_ok(interpreter_fd,bprm->buf,128);
}
}
if (retval >= 0) {
Index: linux-user/linuxload.c
===================================================================
RCS file: /sources/qemu/qemu/linux-user/linuxload.c,v
retrieving revision 1.9
diff -u -r1.9 linuxload.c
--- linux-user/linuxload.c 16 Nov 2007 10:46:05 -0000 1.9
+++ linux-user/linuxload.c 25 Jan 2008 13:25:43 -0000
@@ -99,7 +99,7 @@
memset(bprm->buf, 0, sizeof(bprm->buf));
retval = lseek(bprm->fd, 0L, SEEK_SET);
if(retval >= 0) {
- retval = read(bprm->fd, bprm->buf, 128);
+ retval = qemu_read_ok(bprm->fd, bprm->buf, 128);
}
if(retval < 0) {
perror("prepare_binprm");
Index: tests/hello-arm.c
===================================================================
RCS file: /sources/qemu/qemu/tests/hello-arm.c,v
retrieving revision 1.3
diff -u -r1.3 hello-arm.c
--- tests/hello-arm.c 17 Sep 2007 08:09:54 -0000 1.3
+++ tests/hello-arm.c 25 Jan 2008 13:25:43 -0000
@@ -108,6 +108,6 @@
void _start(void)
{
- write(1, "Hello World\n", 12);
+ qemu_write(1, "Hello World\n", 12);
exit1(0);
}
Index: tests/linux-test.c
===================================================================
RCS file: /sources/qemu/qemu/tests/linux-test.c,v
retrieving revision 1.5
diff -u -r1.5 linux-test.c
--- tests/linux-test.c 17 Sep 2007 08:09:54 -0000 1.5
+++ tests/linux-test.c 25 Jan 2008 13:25:43 -0000
@@ -103,7 +103,7 @@
fd = chk_error(open("file1", O_WRONLY | O_TRUNC | O_CREAT, 0644));
for(i=0;i < FILE_BUF_SIZE; i++)
buf[i] = i;
- len = chk_error(write(fd, buf, FILE_BUF_SIZE / 2));
+ len = chk_error(qemu_write(fd, buf, FILE_BUF_SIZE / 2));
if (len != (FILE_BUF_SIZE / 2))
error("write");
vecs[0].iov_base = buf + (FILE_BUF_SIZE / 2);
Index: tests/qruncom.c
===================================================================
RCS file: /sources/qemu/qemu/tests/qruncom.c,v
retrieving revision 1.8
diff -u -r1.8 qruncom.c
--- tests/qruncom.c 10 Nov 2007 15:15:54 -0000 1.8
+++ tests/qruncom.c 25 Jan 2008 13:25:43 -0000
@@ -175,7 +175,7 @@
perror(filename);
exit(1);
}
- ret = read(fd, vm86_mem + COM_BASE_ADDR, 65536 - 256);
+ ret = qemu_read_ok(fd, vm86_mem + COM_BASE_ADDR, 65536 - 256);
if (ret < 0) {
perror("read");
exit(1);
Index: tests/testthread.c
===================================================================
RCS file: /sources/qemu/qemu/tests/testthread.c,v
retrieving revision 1.2
diff -u -r1.2 testthread.c
--- tests/testthread.c 11 Apr 2003 00:13:04 -0000 1.2
+++ tests/testthread.c 25 Jan 2008 13:25:43 -0000
@@ -15,7 +15,7 @@
for(i=0;i<10;i++) {
snprintf(buf, sizeof(buf), "thread1: %d %s\n", i, (char *)arg);
- write(1, buf, strlen(buf));
+ qemu_write(1, buf, strlen(buf));
usleep(100 * 1000);
}
return NULL;
@@ -27,7 +27,7 @@
char buf[512];
for(i=0;i<20;i++) {
snprintf(buf, sizeof(buf), "thread2: %d %s\n", i, (char *)arg);
- write(1, buf, strlen(buf));
+ qemu_write(1, buf, strlen(buf));
usleep(150 * 1000);
}
return NULL;
^ permalink raw reply [flat|nested] 4+ messages in thread
* [Qemu-devel] Re: [PATCH] check return value from read() and write() properly
2008-01-25 14:10 [Qemu-devel] [PATCH] check return value from read() and write() properly Ian Jackson
@ 2008-02-06 17:14 ` Ian Jackson
2008-02-06 19:18 ` [Qemu-devel] " Anthony Liguori
1 sibling, 0 replies; 4+ messages in thread
From: Ian Jackson @ 2008-02-06 17:14 UTC (permalink / raw)
To: qemu-devel
iwj writes ("[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:
I think this fix should be applied because it corrects bugs which
might conceivably data loss.
Thanks,
Ian.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] check return value from read() and write() properly
2008-01-25 14:10 [Qemu-devel] [PATCH] check return value from read() and write() properly Ian Jackson
2008-02-06 17:14 ` [Qemu-devel] " Ian Jackson
@ 2008-02-06 19:18 ` Anthony Liguori
2008-02-07 10:32 ` Ian Jackson
1 sibling, 1 reply; 4+ messages in thread
From: Anthony Liguori @ 2008-02-06 19:18 UTC (permalink / raw)
To: qemu-devel
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.
>
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] check return value from read() and write() properly
2008-02-06 19:18 ` [Qemu-devel] " Anthony Liguori
@ 2008-02-07 10:32 ` Ian Jackson
0 siblings, 0 replies; 4+ messages in thread
From: Ian Jackson @ 2008-02-07 10:32 UTC (permalink / raw)
To: qemu-devel
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2008-02-07 10:34 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-01-25 14:10 [Qemu-devel] [PATCH] check return value from read() and write() properly Ian Jackson
2008-02-06 17:14 ` [Qemu-devel] " Ian Jackson
2008-02-06 19:18 ` [Qemu-devel] " Anthony Liguori
2008-02-07 10:32 ` Ian Jackson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.