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

[mike <qiudayu@linux.vnet.ibm.com>]

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 <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.
>
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.
>>> 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
>
>
>