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

[malc]

On Mon, 19 Sep 2011, Juan Quintela wrote:

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

The code was using QEMUFile abstraction which didn't have an option to
return errors, stdio does, so it must be used (look at how file open
was handled before, it could fail and when it does old code tries to
notify the user, new code for all replaced QEMUFile function must
follow suit)

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

errno

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

And i don't much care what the code i do not maintain/wrote does things
should be done properly.

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

For one thing it greatly simplifies error checking, i.e. you _always_
compare the returned value with 1 instead of some random parameters.

> 
> 
> >> 
> >> Can you say what you will preffer?
> >> 
> >
> > "Use stdio instead of QEMUFile"
> 
> Done.
> 
> Thanks for the review, Juan.
> 

-- 
mailto:address@hidden