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

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

于 2013/10/15 18:07, mike 写道:
> 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
>>
>>
>>
>
>
Why the new line matters? I think there is a QMP interface available, so 
the hmp output format
would not bother much, just need to tip clear the info.