[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Juan Quintela
Subject: Re: [Qemu-devel] [PATCH 0/3] Remove QEMUFile abuse
Date: Mon, 19 Sep 2011 23:03:09 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux)

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).


> 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

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

>  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, 

TraceRecord record;

../trace/simple.c:141:            unused = fwrite(&record, sizeof(record), 1, 


../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);


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

>> Can you say what you will preffer?
> "Use stdio instead of QEMUFile"


Thanks for the review, Juan.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]