"Dr. David Alan Gilbert" <dgilb...@redhat.com> wrote:
> * Juan Quintela (quint...@redhat.com) wrote:
>> "Dr. David Alan Gilbert" <dgilb...@redhat.com> wrote:
>> > * Juan Quintela (quint...@redhat.com) 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 <quint...@redhat.com>
>> >> ---
>> >> 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.
