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

[Kevin Wolf <kwolf@redhat.com>]

Am 15.10.2013 um 05:38 hat mike geschrieben:
> On 10/14/2013 10:36 PM, Markus Armbruster wrote:
> >Mike Qiu <qiudayu@linux.vnet.ibm.com> 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 <qiudayu@linux.vnet.ibm.com>
> >>---
> >>  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?

Kevin