Re: [Qemu-devel] [PATCH] hmp/info_migration: formatting migration capability output

[Wei Yang]

On Fri, Aug 02, 2019 at 11:02:20AM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (address@hidden) wrote:
>> On Fri, Aug 02, 2019 at 07:24:34AM +0200, Markus Armbruster wrote:
>> >Subject doesn't quite conform to conventions.  Suggest
>> >
>> >    hmp: Improve how "info migrate" formats capabilities
>> 
>> Thanks
>> 
>> >
>> >Wei Yang <address@hidden> writes:
>> >
>> >> Current we put all migration capability in one line, which make it hard
>> >> to read them and someone them are missed due to terminal width.
>> >>
>> >> This patch formats it to print 4 in one line, which looks like this now:
>> >>
>> >> capabilities:
>> >>               xbzrle: off         rdma-pin-all: off        auto-converge: 
>> >> off          zero-blocks: off
>> >>             compress:  on               events: off         postcopy-ram: 
>> >> off               x-colo: off
>> >>          release-ram: off                block: off          return-path: 
>> >> off pause-before-switchover: off
>> >>              multifd: off        dirty-bitmaps: off   postcopy-blocktime: 
>> >> off  late-block-activate: off
>> >>      x-ignore-shared: off
>> >>
>> >> Signed-off-by: Wei Yang <address@hidden>
>> >> ---
>> >>  monitor/hmp-cmds.c | 8 ++++++--
>> >>  1 file changed, 6 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
>> >> index 5ca3ebe942..29ce5b73e4 100644
>> >> --- a/monitor/hmp-cmds.c
>> >> +++ b/monitor/hmp-cmds.c
>> >> @@ -229,9 +229,13 @@ void hmp_info_migrate(Monitor *mon, const QDict 
>> >> *qdict)
>> >>  
>> >>      /* do not display parameters during setup */
>> >>      if (info->has_status && caps) {
>> >> +        int index = 0;
>> >>          monitor_printf(mon, "capabilities: ");
>> >> -        for (cap = caps; cap; cap = cap->next) {
>> >> -            monitor_printf(mon, "%s: %s ",
>> >> +        for (cap = caps; cap; cap = cap->next, index++) {
>> >> +            if (!(index % 4)) {
>> >> +                monitor_printf(mon, "\n");
>> >> +            }
>> >> +            monitor_printf(mon, "%20s: %3s ",
>> >>                             
>> >> MigrationCapability_str(cap->value->capability),
>> >>                             cap->value->state ? "on" : "off");
>> >>          }
>> >
>> >This assumes migration capability names are at most 20 characters long.
>> >late-block-activate is pushing it already: 19 characters.
>> >
>> >It adds up to 104 characters per line, which is rather wide.
>> >
>> >What about putting each capability on its own line, just like globals,
>> >and just like "info migrate_capabilities"?
>> >
>> >(qemu) info migrate
>> >globals:
>> >store-global-state: on
>> >only-migratable: off
>> >send-configuration: on
>> >send-section-footer: on
>> >decompress-error-check: on
>> >clear-bitmap-shift: 18
>> >capabilities:
>> >xbzrle: off
>> >rdma-pin-all: off
>> >auto-converge: off
>> >[...]
>> 
>> This looks good, while would reader may feel difficult to clarify from which
>> one the capabilities stops. 
>> 
>> How about add 4 space at the beginning of each capability?
>> 
>> Would the output be too long? How about wrap two capability into one line?
>
>IMHO the simpler way is just not to display the capabilities on
>'info migrate' at all.  If people want the list of capabilities there's
>already 'info migrate_capabiltiies' they can use.
>
>It was nice displaying them at the start of 'info migrate'
>when we only had a few, but nowe we've got so many it's perhaps
>easier just to omit them.
>

Got it.

>Dave
>
>> -- 
>> Wei Yang
>> Help you, Help me
>--
>Dr. David Alan Gilbert / address@hidden / Manchester, UK

-- 
Wei Yang
Help you, Help me