Re: [Qemu-devel] [PATCH 007/124] vmstate: Return error in case of error

[Juan Quintela]

"Dr. David Alan Gilbert" <address@hidden> wrote:
> * Juan Quintela (address@hidden) wrote:
>> "Dr. David Alan Gilbert" <address@hidden> wrote:
>> > * Juan Quintela (address@hidden) wrote:
>> >> If there is an error while loading a field, we should stop reading and
>> >> not continue with the rest of fields.  And we should also set an error
>> >> in qemu_file.
>> >> 
>> >> Signed-off-by: Juan Quintela <address@hidden>
>> >> ---
>> >>  vmstate.c | 6 ++++++
>> >>  1 file changed, 6 insertions(+)
>> >> 
>> >> diff --git a/vmstate.c b/vmstate.c
>> >> index bfa34cc..bcf1cde 100644
>> >> --- a/vmstate.c
>> >> +++ b/vmstate.c
>> >> @@ -74,7 +74,13 @@ int vmstate_load_state(QEMUFile *f, const 
>> >> VMStateDescription *vmsd,
>> >>                      ret = field->info->get(f, addr, size);
>> >> 
>> >>                  }
>> >> +                if (ret >= 0) {
>> >> +                    ret = qemu_file_get_error(f);
>> >> +                }
>> >>                  if (ret < 0) {
>> >> +                    if (!qemu_file_get_error(f)) {
>> >> +                        qemu_file_set_error(f, ret);
>> >> +                    }
>> >
>> > qemu_file_set_error already checks for an existing error, so
>> > you don't need that check.
>> 
>> ret could already be less than zero and qemu_file error not being set
>> yet.  Problem found during testing.  That is the reason that I have to
>> "improve" the previous patch.
>
> but you can do:
>
> if (ret < 0) {
>   qemu_file_set_error(f, ret);
>
> There is no need for the extra qemu_file_get_error, since
> qemu_file_set_error does
> that internally.

Then we lost the previous error if there is one.


cases:

ret >=0 && qemu_file_get_error() == 0

   success

ret >=0 && qeum_file_get_error() != 0

    we got error on the 1st branch
    and now ret < 0 & qemu_file_get_error() < 0

ret < 0 && qemu_file_get_error() < 0

    In this case, we don't want to set qemu_file error with ret
    By convention, 1st user that sets qemu_file_set_error() wins until
    there is a reset.

    If we set this unconditionally, we do this case wrong

ret < 0 && qemu_file_get_error() == 0


    We want ret <0 & set qemu_file error


I think that the patch that I posted got the 4 cases right.  Your
solution gets the 3rd case wrong (for definition of wrong that means
current usage).

And people wonders why I don't like the qemu_file_get_error() bussiness.
To not having to check/propagate errors in some cases, we end having
code like


if (qemu_file_get_error()) {
    return error;
}

ret = qemu_file_foo();

if (ret >= 0) {
   ret = qemu_file_get_error(f);
}
if (ret < 0) {
   if (!qemu_file_get_error(f)) {
      qemu_file_set_error(f, ret);
}


This is the proper usage of calling a funtion that works with
qemu_file() (much of them just don't return errors at all.)


Later, Juan.