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

[mike]

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

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