On 02/18/2014 08:56 AM, Juan Quintela wrote: > fwrite() returns the number of items written. But when there is one > error, it can return a short write. > > In the particular bug that I was tracking, I did a migration to a > read-only filesystem. And it was able to finish the migration > correctly. fwrite() never returned a negative error code, nor zero, > always 4096. (migration writes chunks of about 14000 bytes). And it > was able to "complete" the migration with success (yes, reading the > file was a bit more difficult). > > To add insult to injury, if your amount of memory was big enough (12GB > on my case), it overwrote some important structure, and from them, > malloc failed. This check makes the problem go away. > > Signed-off-by: Juan Quintela <[email protected]> > --- > > v3: fwrite don't set errno in all cases.
Huh? What cases would those be? http://pubs.opengroup.org/onlinepubs/9699919799/functions/fwrite.html is quite clear - POSIX requires that the only time fwrite() can return a smaller number of elements written is if it encountered a write failure as if by failed fputc(), and since fputc() is required to set errno on write failure, likewise, fwrite() is guaranteed to set errno on short writes. >> The fwrite() function shall return the number of elements successfully >> written, which may be less than nitems if a write error is encountered. If >> size or nitems is 0, fwrite() shall return 0 and the state of the stream >> remains unchanged. Otherwise, if a write error occurs, the error indicator >> for the stream shall be set, [CX] [Option Start] and errno shall be set to >> indicate the error. [Option End] > +++ b/qemu-file.c > @@ -100,7 +100,17 @@ static int stdio_put_buffer(void *opaque, const uint8_t > *buf, int64_t pos, > int size) > { > QEMUFileStdio *s = opaque; > - return fwrite(buf, 1, size, s->stdio_file); > + int res; > + > + res = fwrite(buf, 1, size, s->stdio_file); > + > + if (res != size) { > + if (errno) { > + return -errno; Even if your claim that fwrite doesn't always set errno on short writes were true, your patch here is wrong. Remember, POSIX states that you CANNOT assume the value of errno is reliable unless a function returned an error that indicates it was set, or for certain functions (like strtol) that are required to leave errno unchanged on success. POSIX is also clear that no library function (including fwrite) may explicitly set errno to 0 - they can only preserve its former value, or set it to a non-zero value. Furthermore, for functions which are not required to leave errno unchanged on success, it is perfectly acceptable for those functions (including fwrite) to clobber errno to some other value, even when returning success, all because you cannot rely on the errno value except when the function returned explicit failure. So, since fwrite is not required to keep errno unchanged on success, if you were right that there really is a case where it is not required to set errno on failure, then this patch has a problem - you do not pre-set errno=0, therefore, you cannot guarantee that you are not leaking some prior errno value from some prior function call. NAK to this version; v2 was better. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
