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

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

[Mike Qiu]

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.

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) {
-- 
1.8.3.1

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

[Markus Armbruster]

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"?

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.

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

[mike]

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

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

[Kevin Wolf]

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.

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

Kevin

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

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

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

This one made me think I do:

    foo: tmp.img (raw)
        Removable device: not locked, tray closed
     [not inserted](qemu) 

If the '[not inserted]' is wrong and needs to go, then I actually don't.

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

[mike]

On 10/15/2013 05:31 PM, Markus Armbruster wrote:
> Kevin Wolf <kwolf@redhat.com> writes:
>
>> 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.
>>
>>>> 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?
> This one made me think I do:
>
>      foo: tmp.img (raw)
>          Removable device: not locked, tray closed
>       [not inserted](qemu)
>
> If the '[not inserted]' is wrong and needs to go, then I actually don't.
Here '[not inserted]' is very strange, if the commit author has some 
reasonable reasons,
we can keep it, otherwise, I think we should remove it.

Thanks
Mike
>
>

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

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

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

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

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

[Wenchao Xia]

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

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

[Markus Armbruster]

Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:

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

I want this fixed:

    $ qemu -nodefaults -nographic -monitor stdio -S -drive if=none,file=tmp.img 
    QEMU 1.6.50 monitor - type 'help' for more information
    (qemu) info block
    none0: tmp.img (raw)
        Removable device: not locked, tray closed
     [not inserted](qemu) 

Output doesn't end with newline, messing up the prompt.

Elswhere in the thread, we concluded that the offending '[not inserted]'
is bogus, and should be dropped.

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

[Wenchao Xia]

于 2013/10/16 15:47, Markus Armbruster 写道:
> Wenchao Xia<xiawenc@linux.vnet.ibm.com>  writes:
>
>> 于 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.
>>>
>> 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.
> I want this fixed:
>
>      $ qemu -nodefaults -nographic -monitor stdio -S -drive if=none,file=tmp.img
>      QEMU 1.6.50 monitor - type 'help' for more information
>      (qemu) info block
>      none0: tmp.img (raw)
>          Removable device: not locked, tray closed
>       [not inserted](qemu)
>
> Output doesn't end with newline, messing up the prompt.
>
   I see, no end with newline is bad, should fix.


> Elswhere in the thread, we concluded that the offending '[not inserted]'
> is bogus, and should be dropped.
>

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

[mike]

On 10/16/2013 03:56 PM, Wenchao Xia wrote:
> 于 2013/10/16 15:47, Markus Armbruster 写道:
>> Wenchao Xia<xiawenc@linux.vnet.ibm.com>  writes:
>>
>>> 于 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.
>>>>
>>> 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.
>> I want this fixed:
>>
>>      $ qemu -nodefaults -nographic -monitor stdio -S -drive 
>> if=none,file=tmp.img
>>      QEMU 1.6.50 monitor - type 'help' for more information
>>      (qemu) info block
>>      none0: tmp.img (raw)
>>          Removable device: not locked, tray closed
>>       [not inserted](qemu)
>>
>> Output doesn't end with newline, messing up the prompt.
>>
>   I see, no end with newline is bad, should fix.
>
>
OK, I will make a patch to drop this ' [not inserted]' line instead of 
add a new line.

Thanks
Mike
>> Elswhere in the thread, we concluded that the offending '[not inserted]'
>> is bogus, and should be dropped.
>>
>
>
>
>