malc <av1...@comtv.ru> wrote:
> On Sun, 18 Sep 2011, Juan Quintela wrote:
>
>> malc <av1...@comtv.ru> wrote:
>> > On Fri, 16 Sep 2011, Anthony Liguori wrote:
>> >
>> >> Reviewed-by: Anthony Liguori <aligu...@us.ibm.com>
>> >>
>> >> malc, please Ack.
>> >>
>> >
>> > I don't like the commit message.
>>
>> Can you be more specific?
>
> QEMUFile predates migration by a few years so could have never been
> inteneded to be used for it (leave alone only).
See, I feel young again O:-). Nowadays it is true, though.
Improved comment.
> There's no such thing
> as "vawaudio" (i.e. v vs w).
Fixed.
> Commentary aside: fcalls (seek/tell/read/close) can fail and the code
It is the same code that it is today. There were no handling of errors
before, and adding that means changing infrastructure.
> in the patch doesn't handle it, error path for fwrite does not supply
> information on why the call has failed
It prints: "name_of_function: short write"
Man page on my fedora linux puts:
fread() and fwrite() return the number of items successfully read or
written (i.e., not the number of characters). If an error occurs, or
the end-of-file is reached, the return value is a short item count (or
zero).
Not a lot that I can do :-(
Several of the functions return void, so there is not easy to add error
handling. In the functions that handle errors, error code was added and
handled.
> and furthermore does it via printf,
> also, i believe i mentioned this once before, fwrite (p, 1, n, f) should
> really be (p, n, 1, f).
I don't care one way or another. But qemu source code uses it the other
way around.
[ Removed the ones that I introduced on my patch ]
../hw/vga.c:2370: ret = fwrite(linebuf, 1, pbuf - linebuf, f);
char *linebuf;
../qga/guest-agent-commands.c:259: write_count = fwrite(buf, 1, count, fh);
guchar *buf;
../trace/simple.c:134: unused = fwrite(&record, sizeof(record), 1,
trace_fp);
TraceRecord record;
../trace/simple.c:141: unused = fwrite(&record, sizeof(record), 1,
trace_fp);
same
../trace/simple.c:239: if (fwrite(&header, sizeof header, 1, trace_fp)
!= 1) {
TraceRecord header;
../monitor.c:1609: if (fwrite(buf, 1, l, f) != l) {
uint8_t buf[];
../monitor.c:1645: if (fwrite(buf, 1, l, f) != l) {
uint8_t buf[];
../savevm.c:216: return fwrite(buf, 1, size, s->stdio_file);
uint8_t *buf;
../savevm.c:340: return fwrite(buf, 1, size, s->stdio_file);
same.
BTW, what is the reason that "size of element" for a char string is not
1, and number of elements is not number of elements of array? I have to
admit that I have always used fwrite/read the other way around that you
suggest.
>>
>> Can you say what you will preffer?
>>
>
> "Use stdio instead of QEMUFile"
Done.
Thanks for the review, Juan.
