On 2023/01/25 1:12, Peter Maydell wrote:
On Thu, 1 Dec 2022 at 10:33, Akihiko Odaki <akihiko.odaki@xxxxxxxxxx> wrote:
Before this change, write_kvmstate_to_list() and
write_list_to_kvmstate() tolerated even if it failed to access some
register, and returned a bool indicating whether one of the register
accesses failed. However, it does not make sen not to fail early as the
the callers check the returned value and fail early anyway.
So let write_kvmstate_to_list() and write_list_to_kvmstate() fail early
too. This will allow to propagate errno to the callers and log it if
appropriate.
(Sorry this one didn't get reviewed earlier.)
I agree that all the callers of these functions check for
failure, so there's no major benefit from doing the
don't-fail-early logic. But is there a reason why we should
actively make this change?
In particular, these functions form part of a family with the
similar write_cpustate_to_list() and write_list_to_cpustate(),
and it's inconsistent to have the kvmstate ones return
negative-errno while the cpustate ones still return bool.
For the cpustate ones we *do* rely in some places on
the "don't fail early" behaviour. The kvmstate ones do the
same thing I think mostly for consistency.
So unless there's a specific reason why changing these
functions improves behaviour as seen by users, I think
I favour retaining the consistency.
thanks
-- PMM
I withdraw this patch. The only reason is that it allows to log errno
when reporting the error, and the benefit is negligible when compared to
the consistency.
Regards,
Akihiko Odaki
