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

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

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

-            if (verbose) {
-                monitor_printf(mon, " images:\n");
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-                image_info = info->value->inserted->image;
-                while (1) {
- bdrv_image_info_dump((fprintf_function)monitor_printf,
-                                             mon, image_info);
-                    if (image_info->has_backing_image) {
-                        image_info = image_info->backing_image;
-                    } else {
-                        break;
-                    }
+        if (verbose) {
+            monitor_printf(mon, "\nImages:\n");
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+            image_info = info->value->inserted->image;
+            while (1) {
+ bdrv_image_info_dump((fprintf_function)monitor_printf,
+                                         mon, image_info);
+                if (image_info->has_backing_image) {
+                    image_info = image_info->backing_image;
+                } else {
+                    break;
                  }
              }
-        } else {
-            monitor_printf(mon, " [not inserted]");
          }

It was changed to add this, so there maybe some reasons I think,

Thanks
Mike
> 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");
>          }
>
> Maybe more.  The function probably needs a systematic newline review.
>
>