On Thu, Jul 17, 2025 at 05:54:41PM +0100, Daniel P. Berrangé wrote:
> On Thu, Jul 17, 2025 at 06:07:30AM +0530, 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_process_command() must report an error
> > in errp, in case of failure.
> >
> > Signed-off-by: Arun Menon <arme...@redhat.com>
> > ---
> > migration/savevm.c | 89
> > ++++++++++++++++++++++++++++++++++++++++--------------
> > 1 file changed, 67 insertions(+), 22 deletions(-)
> >
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index
> > 22d73999595384519c755c9416b74ba1263a8bb9..98711bb38a4548d4f168459f729f604a78716c25
> > 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > case MIG_CMD_PACKAGED:
> > - return loadvm_handle_cmd_packaged(mis);
> > + ret = loadvm_handle_cmd_packaged(mis);
> > + if (ret < 0) {
> > + error_setg(errp, "Failed to load device state command: %d",
> > ret);
> > + return -1;
> > + }
> > + return ret;
> >
> > case MIG_CMD_POSTCOPY_ADVISE:
> > - return loadvm_postcopy_handle_advise(mis, len);
> > + ret = loadvm_postcopy_handle_advise(mis, len);
> > + if (ret < 0) {
> > + error_setg(errp, "Failed to load device state command: %d",
> > ret);
> > + return -1;
> > + }
> > + return ret;
> >
> > case MIG_CMD_POSTCOPY_LISTEN:
> > - return loadvm_postcopy_handle_listen(mis);
> > + ret = loadvm_postcopy_handle_listen(mis);
> > + if (ret < 0) {
> > + error_setg(errp, "Failed to load device state command: %d",
> > ret);
> > + return -1;
> > + }
> > + return ret;
> >
> > case MIG_CMD_POSTCOPY_RUN:
> > - return loadvm_postcopy_handle_run(mis);
> > + ret = loadvm_postcopy_handle_run(mis);
> > + if (ret < 0) {
> > + error_setg(errp, "Failed to load device state command: %d",
> > ret);
> > + return -1;
> > + }
> > + return ret;
> >
> > case MIG_CMD_POSTCOPY_RAM_DISCARD:
> > - return loadvm_postcopy_ram_handle_discard(mis, len);
> > + ret = loadvm_postcopy_ram_handle_discard(mis, len);
> > + if (ret < 0) {
> > + error_setg(errp, "Failed to load device state command: %d",
> > ret);
> > + return -1;
> > + }
> > + return ret;
> >
> > case MIG_CMD_POSTCOPY_RESUME:
> > - return loadvm_postcopy_handle_resume(mis);
> > + ret = loadvm_postcopy_handle_resume(mis);
> > + if (ret < 0) {
> > + error_setg(errp, "Failed to load device state command: %d",
> > ret);
> > + return -1;
> > + }
> > + return ret;
> >
> > case MIG_CMD_RECV_BITMAP:
> > - return loadvm_handle_recv_bitmap(mis, len);
> > + ret = loadvm_handle_recv_bitmap(mis, len);
> > + if (ret < 0) {
> > + error_setg(errp, "Failed to load device state command: %d",
> > ret);
> > + return -1;
> > + }
> > + return ret;
> >
> > case MIG_CMD_ENABLE_COLO:
> > - return loadvm_process_enable_colo(mis);
> > + ret = loadvm_process_enable_colo(mis);
> > + if (ret < 0) {
> > + error_setg(errp, "Failed to load device state command: %d",
> > ret);
> > + return -1;
> > + }
> > + return ret;
> >
> > case MIG_CMD_SWITCHOVER_START:
> > - return loadvm_postcopy_handle_switchover_start();
> > + ret = loadvm_postcopy_handle_switchover_start();
> > + if (ret < 0) {
> > + error_setg(errp, "Failed to load device state command: %d",
> > ret);
> > + return -1;
> > + }
> > + return ret;
> > }
>
> Can you ensure that each of these have unique error message strings,
> as if they ever appear, it will be important to distinguish which of
> these nine commands actually failed.
Ignore this comment. I see you eliminate all these generic error messages
in following patches, so this is only ever a transient issue half way
through the patch series.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
