Re: [PATCH 02/11] migration/hmp: Fix postcopy-blocktime per-vCPU results

[Peter Xu <peterx@redhat.com>]

On Wed, May 28, 2025 at 03:08:38PM +0000, Dr. David Alan Gilbert wrote:
> * Markus Armbruster (armbru@redhat.com) wrote:
> > Peter Xu <peterx@redhat.com> writes:
> > 
> > > Unfortunately, it was never correctly shown..
> > >
> > > This is only found when I started to look into making the blocktime feature
> > > more useful (so as to avoid using bpftrace, even though I'm not sure which
> > > one will be harder to use..).
> > >
> > > So the old dump would look like this:
> > >
> > >   Postcopy vCPU Blocktime: 0-1,4,10,21,33,46,48,59
> > >
> > > Even though there're actually 40 vcpus, and the string will merge same
> > > elements and also sort them.
> > >
> > > To fix it, simply loop over the uint32List manually.  Now it looks like:
> > >
> > >   Postcopy vCPU Blocktime (ms):
> > >    [15, 0, 0, 43, 29, 34, 36, 29, 37, 41,
> > >     33, 37, 45, 52, 50, 38, 40, 37, 40, 49,
> > >     40, 35, 35, 35, 81, 19, 18, 19, 18, 30,
> > >     22, 3, 0, 0, 0, 0, 0, 0, 0, 0]
> > >
> > > Cc: Dr. David Alan Gilbert <dave@treblig.org>
> > > Cc: Alexey Perevalov <a.perevalov@samsung.com>
> > > Cc: Markus Armbruster <armbru@redhat.com>
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > >  migration/migration-hmp-cmds.c | 23 ++++++++++++++---------
> > >  1 file changed, 14 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
> > > index 367ff6037f..3cf890b887 100644
> > > --- a/migration/migration-hmp-cmds.c
> > > +++ b/migration/migration-hmp-cmds.c
> > > @@ -208,15 +208,20 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
> > >      }
> > >  
> > >      if (info->has_postcopy_vcpu_blocktime) {
> > > -        Visitor *v;
> > > -        char *str;
> > > -        v = string_output_visitor_new(false, &str);
> > > -        visit_type_uint32List(v, NULL, &info->postcopy_vcpu_blocktime,
> > > -                              &error_abort);
> > > -        visit_complete(v, &str);
> > > -        monitor_printf(mon, "Postcopy vCPU Blocktime: %s\n", str);
> > > -        g_free(str);
> > > -        visit_free(v);
> > > +        uint32List *item = info->postcopy_vcpu_blocktime;
> > > +        int count = 0;
> > > +
> > > +        monitor_printf(mon, "Postcopy vCPU Blocktime (ms): \n [");
> > > +
> > > +        while (item) {
> > > +            monitor_printf(mon, "%"PRIu32", ", item->value);
> > > +            item = item->next;
> > > +            /* Each line 10 vcpu results, newline if there's more */
> > 
> > The list can be arbitrarily long?
> 
> One per vCPU, so a small arbitrary.
> 
> > > +            if ((++count % 10 == 0) && item) {
> > > +                monitor_printf(mon, "\n  ");
> > > +            }
> > > +        }
> > > +        monitor_printf(mon, "\b\b]\n");
> > 
> > Uh, backspace?
> 
> Agreed!

Agreed on.. using backspace? :)

> 
> Dave
> 
> > I usually do something like
> > 
> >     sep = "";
> >     for (...) {
> >         printf("%s...", sep, ...);
> >         sep = ", "
> >     }
> > 
> > To add line breaks, I'd use something like
> > 
> >         sep = ... ? ", " : ",\n";

Thanks for the suggestion!  Definitely better..

I'll squash below into this patch when repost, comments on top always
welcomed.

diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index 3cf890b887..6c36e202a0 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -209,19 +209,18 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
 
     if (info->has_postcopy_vcpu_blocktime) {
         uint32List *item = info->postcopy_vcpu_blocktime;
+        const char *sep = "";
         int count = 0;
 
         monitor_printf(mon, "Postcopy vCPU Blocktime (ms): \n [");
 
         while (item) {
-            monitor_printf(mon, "%"PRIu32", ", item->value);
+            monitor_printf(mon, "%s%"PRIu32, sep, item->value);
             item = item->next;
             /* Each line 10 vcpu results, newline if there's more */
-            if ((++count % 10 == 0) && item) {
-                monitor_printf(mon, "\n  ");
-            }
+            sep = ((++count % 10 == 0) && item) ? ",\n  " : ", ";
         }
-        monitor_printf(mon, "\b\b]\n");
+        monitor_printf(mon, "]\n");
     }

-- 
Peter Xu