| Message ID | 1381757489-3310-1-git-send-email-qiudayu@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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.
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. > >
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
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.
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 > > >
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 > >
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 > > > > > > > >
于 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.
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.
于 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. >
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. >> > > > >
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) {
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(-)