[RFC,05/11] qemu_fclose: return last_error if set

This will make sure no error will be missed as long as callers always
check for qemu_fclose() return value. For reference, this is the
complete list of qemu_fclose() callers:

 - exec_close(): already fixed to check for negative values, not -1
 - migrate_fd_cleanup(): already fixed to consider only negative values
   as error, not any non-zero value
 - exec_accept_incoming_migration(): no return value check (yet)
 - fd_accept_incoming_migration(): no return value check (yet)
 - tcp_accept_incoming_migration(): no return value check (yet)
 - unix_accept_incoming_migration(): no return value check (yet)
 - do_savevm(): no return value check (yet)
 - load_vmstate(): no return value check (yet)

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 savevm.c |   46 +++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 43 insertions(+), 3 deletions(-)

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.

> +    if (f->last_error)
> +        ret = f->last_error;
>       g_free(f);
>       return ret;

Paolo

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
>

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?

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

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

Paolo

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