Peter Xu <pet...@redhat.com> writes:
> On Wed, Jul 05, 2023 at 06:54:37PM -0300, Fabiano Rosas wrote:
>> Peter Xu <pet...@redhat.com> writes:
>>
>> > There're a lot of cases where we only have an errno set in last_error but
>> > without a detailed error description. When this happens, try to generate
>> > an error contains the errno as a descriptive error.
>> >
>> > This will be helpful in cases where one relies on the Error*. E.g.,
>> > migration state only caches Error* in MigrationState.error. With this,
>> > we'll display correct error messages in e.g. query-migrate when the error
>> > was only set by qemu_file_set_error().
>> >
>> > Signed-off-by: Peter Xu <pet...@redhat.com>
>> > ---
>> > migration/qemu-file.c | 15 ++++++++++++---
>> > 1 file changed, 12 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/migration/qemu-file.c b/migration/qemu-file.c
>> > index acc282654a..419b4092e7 100644
>> > --- a/migration/qemu-file.c
>> > +++ b/migration/qemu-file.c
>> > @@ -156,15 +156,24 @@ void qemu_file_set_hooks(QEMUFile *f, const
>> > QEMUFileHooks *hooks)
>> > *
>> > * Return negative error value if there has been an error on previous
>> > * operations, return 0 if no error happened.
>> > - * Optional, it returns Error* in errp, but it may be NULL even if return
>> > value
>> > - * is not 0.
>> > *
>> > + * If errp is specified, a verbose error message will be copied over.
>> > */
>> > int qemu_file_get_error_obj(QEMUFile *f, Error **errp)
>> > {
>> > + if (!f->last_error) {
>> > + return 0;
>> > + }
>> > +
>> > + /* There is an error */
>> > if (errp) {
>> > - *errp = f->last_error_obj ? error_copy(f->last_error_obj) : NULL;
>> > + if (f->last_error_obj) {
>> > + *errp = error_copy(f->last_error_obj);
>> > + } else {
>> > + error_setg_errno(errp, -f->last_error, "Channel error");
>>
>> There are a couple of places that do:
>>
>> ret = vmstate_save(f, se, ms->vmdesc);
>> if (ret) {
>> qemu_file_set_error(f, ret);
>> break;
>> }
>>
>> and vmstate_save() can return > 0 on error. This would make this message
>> say "Unknown error". This is minor.
>>
>> But take a look at qemu_fclose(). It can return f->last_error while the
>> function documentation says it should return negative on error.
>>
>> Should we make qemu_file_set_error() check 'ret' and always set a
>> negative value for f->last_error?
>
> Yeah, maybe we can add a sanity check, but logically it's better we just
> fix vmstate_save() to make sure it always returns a <0 error.
>
> It seems to me there're so many hooks in vmstate_save_state_v() that it can
> return random things. What's the one you spot? If it's an obvious issue
> we can fix them.
I see at least:
ret = field->info->put(f, curr_elem, size, field, vmdesc_loop);
with put_power() from target/arm/machine.c returning 1.
Since vmstate_save_state_v() is quite involved I don't think we should
block this series because of it. I can do a closer audit and send a
separate patch with it.
So:
Reviewed-by: Fabiano Rosas <faro...@suse.de>
