[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 09:56:41 -0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Wed, Nov 02, 2011 at 08:33:05AM +0100, Paolo Bonzini wrote:
> On 11/01/2011 08:20 PM, Eduardo Habkost wrote:
> >+/** Calls close function and set last_error if needed
> >+ *
> >+ * Internal function. qemu_fflush() must be called before this.
> >+ *
> >+ * Returns f->close() return value, or 0 if close function is not set.
> >+ */
> >+static int qemu_close(QEMUFile *f)
> > {
> > int ret = 0;
> >- qemu_fflush(f);
> >- if (f->close)
> >+ if (f->close) {
> > ret = f->close(f->opaque);
> >+ qemu_file_set_if_error(f, ret);
> >+ }
> >+ return ret;
> >+}
> >+
> >+/** Closes the file
> >+ *
> >+ * Returns negative error value if any error happened on previous
> >operations or
> >+ * while closing the file. Returns 0 or positive number on success.
> >+ *
> >+ * The meaning of return value on success depends on the specific backend
> >+ * being used.
> >+ */
> >+int qemu_fclose(QEMUFile *f)
> >+{
> >+ int ret;
> >+ qemu_fflush(f);
> >+ ret = qemu_close(f);
>
> Isn't the return value of qemu_close useless, because if nonzero it
> will always be present in last_error too? With this change, I'd
> leave it inline.
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).
It could be inlined, yes. I just wanted to isolate the "call f->close if
set, handling errors" part from the flush+close+g_free logic, to try to
make functions shorter and easier to read and analyze (please let me
know if I failed to do that :-).
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) {
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.
I am happy with either version, so I am open to suggestions.
>
> >+ if (f->last_error)
> >+ ret = f->last_error;
Oops, I have to fix coding style and add braces here.
> > g_free(f);
> > return ret;
>
> Paolo
>
--
Eduardo
- [Qemu-devel] [RFC PATCH 10/11] tcp_close(): check for close() errors too, (continued)
- [Qemu-devel] [RFC PATCH 10/11] tcp_close(): check for close() errors too, Eduardo Habkost, 2011/11/01
- [Qemu-devel] [RFC PATCH 02/11] QEMUFileCloseFunc: add return value documentation, Eduardo Habkost, 2011/11/01
- [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