Re: [Qemu-devel] [PATCH] hmp: Add '\n' in monitor_printf()

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 15.10.2013 um 05:38 hat mike geschrieben:
>> On 10/14/2013 10:36 PM, Markus Armbruster wrote:
>> >Mike Qiu <address@hidden> writes:
>> >
>> >>Without this, output of 'info block'
>> >>
>> >>scsi0-hd0: /images/f18-ppc64.qcow2 (qcow2)
>> >>  [not inserted]
>> >>scsi0-cd2: [not inserted]
>> >>     Removable device: not locked, tray closed
>> >>
>> >>floppy0: [not inserted]
>> >>     Removable device: not locked, tray closed
>> >>
>> >>sd0: [not inserted]
>> >>     Removable device: not locked, tray closed
>> >>
>> >>There will be no additional lines between scsi0-hd0 scsi0-cd2,
>> >>and break the info style.
>> >Just saw a similar one:
>> >
>> >     (qemu) info block
>> >     disk0: test.img (raw)
>> >      [not inserted]
>> >     cd: [not inserted]
>> >         Removable device: not locked, tray closed
>> >
>> >     foo: tmp.img (raw)
>> >         Removable device: not locked, tray closed
>> >      [not inserted](qemu)
>> >
>> >>This patch is to solve this.
>> >>
>> >>Signed-off-by: Mike Qiu <address@hidden>
>> >>---
>> >>  hmp.c | 2 +-
>> >>  1 file changed, 1 insertion(+), 1 deletion(-)
>> >>
>> >>diff --git a/hmp.c b/hmp.c
>> >>index 5891507..2d2e5f8 100644
>> >>--- a/hmp.c
>> >>+++ b/hmp.c
>> >>@@ -367,7 +367,7 @@ void hmp_info_block(Monitor *mon, const QDict *qdict)
>> >>                              info->value->inserted->iops_wr_max,
>> >>                              info->value->inserted->iops_size);
>> >>          } else {
>> >>-            monitor_printf(mon, " [not inserted]");
>> >>+            monitor_printf(mon, " [not inserted]\n");
>> >>          }
>> >>          if (verbose) {
>> >                monitor_printf(mon, "\nImages:\n");
>> >
>> >What about removing the newline before "Images"?
>> A good idea I think, it no need to add addition lines in one info.
>>
>> But see commit id: fbe2e26c15af35e4d157874dc80f6a19eebaa83b
>> [...]
>> It was changed to add this, so there maybe some reasons I think,
>
> Like everything else in that commit, I did that change because I found it
> more readable.
>
> The problem seems to be commit 3e9fab69 ('block: Add support for
> throttling burst max in QMP and the command line'), which added a bogus
> "[not inserted]" message. We simply need to drop it altogether instead of
> adding a newline.
>
>> > I think we should also drop this newline:
>> >
>> >          if (info->value->removable) {
>> >              monitor_printf(mon, "    Removable device: %slocked, tray 
>> > %s\n",
>> >                             info->value->locked ? "" : "not ",
>> >                             info->value->tray_open ? "open" : "closed");
>> >          }
>
> Why? Look:
>
> (qemu) info block
> scsi0-cd0: /tmp/cdrom.qcow2 (qcow2)
>     Removable device: not locked, tray closed
>     Backing file:     
> /home/kwolf/images/iso/Fedora-18-x86_64-Live-Desktop.iso (chain depth: 1)
>     I/O throttling:   bps=1048576 bps_rd=0 bps_wr=0 bps_max=104857 
> bps_rd_max=0 bps_wr_max=0 iops=0 iops_rd=0 iops_wr=0 iops_max=0 iops_rd_max=0 
> iops_wr_max=0 iops_size=0
>
> Do you really want to remove the newline?

This one made me think I do:

    foo: tmp.img (raw)
        Removable device: not locked, tray closed
     [not inserted](qemu) 

If the '[not inserted]' is wrong and needs to go, then I actually don't.