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

[Benoît Canet]

Hello,

>Le Tuesday 15 Oct 2013 à 18:07:16 (+0800), mike a écrit :
> On 10/15/2013 04:58 PM, Kevin Wolf wrote:
> >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.
> >
> Yes, I agree with you. but maybe need the author of the commit 3e9fab69
> ('block: Add support for throttling burst max in QMP and the command line')
> to have some comments on this line, I think.

Hello,

I do not remember even thinking about adding this monitor_printf in 3e9fab69.
It must be the result of a bad conflict resolve or something like that.
Sorry for adding this.
Do you want me to send a two liner to remove this ? 

Best regards

Benoît

> >>>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?
> I'm not, but Markus suggest to do so.
> 
> Thanks
> Mike
> >Kevin
> >
> >
> >
> 
>