Hi,
Thanks for the review.
On Mon, Jul 21, 2025 at 09:43:30PM +0900, Akihiko Odaki wrote:
> On 2025/07/21 20:29, Arun Menon wrote:
> > This is an incremental step in converting vmstate loading
> > code to report error via Error objects instead of directly
> > printing it to console/monitor.
> > It is ensured that loadvm_postcopy_handle_advise() must report an error
> > in errp, in case of failure.
> >
> > Reviewed-by: Daniel P. Berrangé <berra...@redhat.com>
> > Signed-off-by: Arun Menon <arme...@redhat.com>
> > ---
> > migration/savevm.c | 39 +++++++++++++++++----------------------
> > 1 file changed, 17 insertions(+), 22 deletions(-)
> >
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index
> > 6b8c78bfb9bde2a8e7500b0342cd386b0d12db97..4a3db9498678a19597257e683029cd3f6c8d1a65
> > 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -1912,39 +1912,39 @@ enum LoadVMExitCodes {
> > * quickly.
> > */
> > static int loadvm_postcopy_handle_advise(MigrationIncomingState *mis,
> > - uint16_t len)
> > + uint16_t len, Error **errp)
> > {
> > PostcopyState ps = postcopy_state_set(POSTCOPY_INCOMING_ADVISE);
> > uint64_t remote_pagesize_summary, local_pagesize_summary, remote_tps;
> > size_t page_size = qemu_target_page_size();
> > - Error *local_err = NULL;
> > trace_loadvm_postcopy_handle_advise();
> > if (ps != POSTCOPY_INCOMING_NONE) {
> > - error_report("CMD_POSTCOPY_ADVISE in wrong postcopy state (%d)",
> > ps);
> > + error_setg(errp, "CMD_POSTCOPY_ADVISE in wrong postcopy "
> > + "state (%d)", ps);
> > return -1;
> > }
> > switch (len) {
> > case 0:
> > if (migrate_postcopy_ram()) {
> > - error_report("RAM postcopy is enabled but have 0 byte advise");
> > + error_setg(errp, "RAM postcopy is enabled but have 0 byte
> > advise");
> > return -EINVAL;
> > }
> > return 0;
> > case 8 + 8:
> > if (!migrate_postcopy_ram()) {
> > - error_report("RAM postcopy is disabled but have 16 byte
> > advise");
> > + error_setg(errp, "RAM postcopy is disabled but have 16 "
> > + "byte advise");
> > return -EINVAL;
> > }
> > break;
> > default:
> > - error_report("CMD_POSTCOPY_ADVISE invalid length (%d)", len);
> > + error_setg(errp, "CMD_POSTCOPY_ADVISE invalid length (%d)", len);
> > return -EINVAL;
> > }
> > - if (!postcopy_ram_supported_by_host(mis, &local_err)) {
> > - error_report_err(local_err);
> > + if (!postcopy_ram_supported_by_host(mis, errp)) {
> > postcopy_state_set(POSTCOPY_INCOMING_NONE);
> > return -1;
> > }
> > @@ -1967,9 +1967,9 @@ static int
> > loadvm_postcopy_handle_advise(MigrationIncomingState *mis,
> > * also fails when passed to an older qemu that doesn't
> > * do huge pages.
> > */
> > - error_report("Postcopy needs matching RAM page sizes (s=%" PRIx64
> > - " d=%" PRIx64
> > ")",
> > - remote_pagesize_summary, local_pagesize_summary);
> > + error_setg(errp, "Postcopy needs matching RAM "
> > + "page sizes (s=%" PRIx64 " d=%" PRIx64 ")",
> > + remote_pagesize_summary, local_pagesize_summary);
> > return -1;
> > }
> > @@ -1979,17 +1979,17 @@ static int
> > loadvm_postcopy_handle_advise(MigrationIncomingState *mis,
> > * Again, some differences could be dealt with, but for now keep
> > it
> > * simple.
> > */
> > - error_report("Postcopy needs matching target page sizes (s=%d
> > d=%zd)",
> > - (int)remote_tps, page_size);
> > + error_setg(errp, "Postcopy needs matching target "
> > + "page sizes (s=%d d=%zd)", (int)remote_tps, page_size);
> > return -1;
> > }
> > - if (postcopy_notify(POSTCOPY_NOTIFY_INBOUND_ADVISE, &local_err)) {
> > - error_report_err(local_err);
> > + if (postcopy_notify(POSTCOPY_NOTIFY_INBOUND_ADVISE, errp)) {
> > return -1;
> > }
> > - if (ram_postcopy_incoming_init(mis, NULL) < 0) {
> > + if (ram_postcopy_incoming_init(mis, errp) < 0) {
> > + error_prepend(errp, "PostCopy RAM incoming init failed ");
>
> Nitpick: s/PostCopy/Postcopy/ for consistency with other error messages.
>
Good catch, thanks.
Might as well amend it while working on the new version.
Regards,
Arun
