[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH 05/11] qemu_fclose: return last_error if set
From: |
Eduardo Habkost |
Subject: |
Re: [Qemu-devel] [RFC PATCH 05/11] qemu_fclose: return last_error if set |
Date: |
Wed, 2 Nov 2011 10:26:42 -0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Wed, Nov 02, 2011 at 01:15:46PM +0100, Paolo Bonzini wrote:
> On 11/02/2011 12:56 PM, Eduardo Habkost wrote:
> >No, if it's positive it won't be set on last_error, so we have to save
> >it somewhere other than last_error (that's what the qemu_close() return
> >value is used for).
>
> Ok, I was confused by your patch 6, which basically removes the only
> case when qemu_fclose was returning a positive, nonzero value. :) I
> guess the problem is there?
Yes! We must return the pclose() return value there, as exec_close()
needs it. Thanks for spotting it. :-)
>
> >Without a separate function and qemu_file_set_if_error(), the function
> >will look like:
> >
> >int qemu_fclose(QEMUFile *f)
> >{
> > int ret = 0;
> > qemu_fflush();
> > if (f->close) {
> > ret = f->close(f->opaque);
> > }
> > if (f->last_error) {
> ^^^^^^^^^^^^^
>
> "if (ret >= 0 && f->last_error)" perhaps?
I don't think so: if f->close() fails but we have already got an error
on any previous operation (in other words, if f->last_error was already
set), I think we should return info about the first error (that will
probably be more informative), instead of the close() error.
>
> > ret = f->last_error;
> > }
> > g_free(f);
> > return ret;
> >}
> >
> >
> >Now that I am looking at the resulting code, it doesn't look too bad. I
> >guess I was simply too eager to encapsulate every bit of logic (in this
> >case the "if (f->close) ..." part) into separate functions. I find the
> >two-function version slightly easier to analyze, though.
>
> Yes, it's the same for me too now that I actually understand what's
> going on.
Good. :-)
--
Eduardo
- [Qemu-devel] [RFC PATCH 01/11] savevm: use qemu_file_set_error() instead of setting last_error directly, (continued)
- [Qemu-devel] [RFC PATCH 01/11] savevm: use qemu_file_set_error() instead of setting last_error directly, Eduardo Habkost, 2011/11/01
- [Qemu-devel] [RFC PATCH 11/11] unix_close(): check for close() errors too, Eduardo Habkost, 2011/11/01
- [Qemu-devel] [RFC PATCH 07/11] stdio_fclose: return -errno on errors, Eduardo Habkost, 2011/11/01
- [Qemu-devel] [RFC PATCH 03/11] exec_close(): accept any negative value as qemu_fclose() error, Eduardo Habkost, 2011/11/01
- [Qemu-devel] [RFC PATCH 09/11] fd_close(): check for close() errors too, Eduardo Habkost, 2011/11/01
- [Qemu-devel] [RFC PATCH 08/11] exec_close(): return -errno on errors, Eduardo Habkost, 2011/11/01
- [Qemu-devel] [RFC PATCH 05/11] qemu_fclose: return last_error if set, Eduardo Habkost, 2011/11/01
[Qemu-devel] [RFC PATCH 06/11] stdio_pclose: return -errno on error, Eduardo Habkost, 2011/11/01
Re: [Qemu-devel] [RFC PATCH 00/11] qemu_fclose() error handling fixes, Juan Quintela, 2011/11/02
Re: [Qemu-devel] [RFC PATCH 00/11] qemu_fclose() error handling fixes, Michael Roth, 2011/11/04