Hi Fabiano,
Thanks for the review.

On Fri, Aug 15, 2025 at 04:51:04PM -0300, Fabiano Rosas wrote:
> Arun Menon <arme...@redhat.com> writes:
> 
> > 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_handle_recv_bitmap() 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 | 21 ++++++++++-----------
> >  1 file changed, 10 insertions(+), 11 deletions(-)
> >
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index 
> > 9098c4bd3394d7b9ed77e20afbb26fd9c9be6550..a7aede1b3df9164e322e68f3889df7c4166876f5
> >  100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -2480,32 +2480,35 @@ static int 
> > loadvm_handle_cmd_packaged(MigrationIncomingState *mis, Error **errp)
> >   * len (1 byte) + ramblock_name (<255 bytes)
> >   */
> >  static int loadvm_handle_recv_bitmap(MigrationIncomingState *mis,
> > -                                     uint16_t len)
> > +                                     uint16_t len, Error **errp)
> >  {
> >      QEMUFile *file = mis->from_src_file;
> >      RAMBlock *rb;
> >      char block_name[256];
> >      size_t cnt;
> > +    int ret;
> >  
> >      cnt = qemu_get_counted_string(file, block_name);
> >      if (!cnt) {
> > -        error_report("%s: failed to read block name", __func__);
> > +        error_setg(errp, "failed to read block name: %s", block_name);
> 
> Could we not print the buffer that's just failed to be written? As a
> matter of principle =)
yes, we must not, its content will be empty. Thanks
> 
> >          return -EINVAL;
> >      }
> >  
> >      /* Validate before using the data */
> > -    if (qemu_file_get_error(file)) {
> > -        return qemu_file_get_error(file);
> > +    ret = qemu_file_get_error(file);
> > +    if (ret < 0) {
> > +        error_setg(errp, "migration stream has error: %d", ret);
> 
> I've been suggesting "stream error:", probably best to keep it uniform.
Sure, will do.
> 
> > +        return ret;
> >      }
> >  
> >      if (len != cnt + 1) {
> > -        error_report("%s: invalid payload length (%d)", __func__, len);
> > +        error_setg(errp, "invalid payload length (%d)", len);
> >          return -EINVAL;
> >      }
> >  
> >      rb = qemu_ram_block_by_name(block_name);
> >      if (!rb) {
> > -        error_report("%s: block '%s' not found", __func__, block_name);
> > +        error_setg(errp, "block '%s' not found", block_name);
> >          return -EINVAL;
> >      }
> >  
> > @@ -2642,11 +2645,7 @@ static int loadvm_process_command(QEMUFile *f, Error 
> > **errp)
> >          return 0;
> >  
> >      case MIG_CMD_RECV_BITMAP:
> > -        ret = loadvm_handle_recv_bitmap(mis, len);
> > -        if (ret < 0) {
> > -            error_setg(errp, "Failed to load device state command: %d", 
> > ret);
> > -        }
> > -        return ret;
> > +        return loadvm_handle_recv_bitmap(mis, len, errp);
> >  
> >      case MIG_CMD_ENABLE_COLO:
> >          ret = loadvm_process_enable_colo(mis);
> 

Regards,
Arun


Reply via email to