Hi Marc-André,
Thank you for the review.
On Wed, Aug 06, 2025 at 11:54:36AM +0400, Marc-André Lureau wrote:
> Hi
>
> On Tue, Aug 5, 2025 at 10:30 PM Arun Menon <armenon@redhat.com> 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_ram_handle_discard() must report an
> > error
> > in errp, in case of failure.
> >
> > Signed-off-by: Arun Menon <armenon@redhat.com>
> >
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
>
> > ---
> > migration/savevm.c | 26 +++++++++++++-------------
> > 1 file changed, 13 insertions(+), 13 deletions(-)
> >
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index
> > eb90873a750ded354b3db31cba40b44d1be79864..3abe4193e02aae9c813ff07fb388a7ee470c8a6a
> > 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -2004,7 +2004,7 @@ static int
> > loadvm_postcopy_handle_advise(MigrationIncomingState *mis,
> > * There can be 0..many of these messages, each encoding multiple pages.
> > */
> > static int loadvm_postcopy_ram_handle_discard(MigrationIncomingState *mis,
> > - uint16_t len)
> > + uint16_t len, Error **errp)
> > {
> > int tmp;
> > char ramid[256];
> > @@ -2017,6 +2017,7 @@ static int
> > loadvm_postcopy_ram_handle_discard(MigrationIncomingState *mis,
> > /* 1st discard */
> > tmp = postcopy_ram_prepare_discard(mis);
> > if (tmp) {
> > + error_setg(errp, "Failed to prepare for RAM discard: %d",
> > tmp);
> > return tmp;
> > }
> > break;
> > @@ -2026,8 +2027,9 @@ static int
> > loadvm_postcopy_ram_handle_discard(MigrationIncomingState *mis,
> > break;
> >
> > default:
> > - error_report("CMD_POSTCOPY_RAM_DISCARD in wrong postcopy state
> > (%d)",
> > - ps);
> > + error_setg(errp,
> > + "CMD_POSTCOPY_RAM_DISCARD in wrong postcopy state
> > (%d)",
> > + ps);
> > return -1;
> > }
> > /* We're expecting a
> > @@ -2036,29 +2038,30 @@ static int
> > loadvm_postcopy_ram_handle_discard(MigrationIncomingState *mis,
> > * then at least 1 16 byte chunk
> > */
> > if (len < (1 + 1 + 1 + 1 + 2 * 8)) {
> > - error_report("CMD_POSTCOPY_RAM_DISCARD invalid length (%d)", len);
> > + error_setg(errp, "CMD_POSTCOPY_RAM_DISCARD invalid length (%d)",
> > len);
> > return -1;
> > }
> >
> > tmp = qemu_get_byte(mis->from_src_file);
> > if (tmp != postcopy_ram_discard_version) {
> > - error_report("CMD_POSTCOPY_RAM_DISCARD invalid version (%d)",
> > tmp);
> > + error_setg(errp, "CMD_POSTCOPY_RAM_DISCARD invalid version (%d)",
> > tmp);
> > return -1;
> > }
> >
> > if (!qemu_get_counted_string(mis->from_src_file, ramid)) {
> > - error_report("CMD_POSTCOPY_RAM_DISCARD Failed to read RAMBlock
> > ID");
> > + error_setg(errp,
> > + "CMD_POSTCOPY_RAM_DISCARD Failed to read RAMBlock ID");
> > return -1;
> > }
> > tmp = qemu_get_byte(mis->from_src_file);
> > if (tmp != 0) {
> > - error_report("CMD_POSTCOPY_RAM_DISCARD missing nil (%d)", tmp);
> > + error_setg(errp, "CMD_POSTCOPY_RAM_DISCARD missing nil (%d)",
> > tmp);
> > return -1;
> > }
> >
> > len -= 3 + strlen(ramid);
> > if (len % 16) {
> > - error_report("CMD_POSTCOPY_RAM_DISCARD invalid length (%d)", len);
> > + error_setg(errp, "CMD_POSTCOPY_RAM_DISCARD invalid length (%d)",
> > len);
> > return -1;
> > }
> > trace_loadvm_postcopy_ram_handle_discard_header(ramid, len);
> > @@ -2070,6 +2073,7 @@ static int
> > loadvm_postcopy_ram_handle_discard(MigrationIncomingState *mis,
> > len -= 16;
> > int ret = ram_discard_range(ramid, start_addr, block_length);
> > if (ret) {
> > + error_setg(errp, "Failed to discard RAM range %s: %d", ramid,
> > ret);
> >
>
> note: the ram_discard_range() and ram_block_discard_range() functions also
> calls error_report() Maybe they can be converted too.., let's not do this
> in this already long series.
Agreed.
>
>
>
>
> > return ret;
> > }
> > }
> > @@ -2629,11 +2633,7 @@ static int loadvm_process_command(QEMUFile *f,
> > Error **errp)
> > return loadvm_postcopy_handle_run(mis, errp);
> >
> > case MIG_CMD_POSTCOPY_RAM_DISCARD:
> > - ret = loadvm_postcopy_ram_handle_discard(mis, len);
> > - if (ret < 0) {
> > - error_setg(errp, "Failed to load device state command: %d",
> > ret);
> > - }
> > - return ret;
> > + return loadvm_postcopy_ram_handle_discard(mis, len, errp);
> >
> > case MIG_CMD_POSTCOPY_RESUME:
> > loadvm_postcopy_handle_resume(mis);
> >
> > --
> > 2.50.1
> >
> >
Regards,
Arun