Re: [Qemu-devel] [PATCH 0/3] Remove QEMUFile abuse

[Juan Quintela]

malc <address@hidden> wrote:
> On Sun, 18 Sep 2011, Juan Quintela wrote:
>
>> malc <address@hidden> wrote:
>> > On Fri, 16 Sep 2011, Anthony Liguori wrote:
>> >
>> >> Reviewed-by: Anthony Liguori <address@hidden>
>> >> 
>> >> 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.