Re: [Qemu-devel] question: about exec/poison.h

[Paolo Bonzini]

On 30/11/2015 04:47, Peter Xu wrote:
> 
> I met one problem when trying to add a new public function in dump.h
> named "dump_state_get_global" and using it in hmp.c.

Don't do that. :)

hmp.c functions should in general use the QMP commands as the base.  In
your case, hmp_info_dump should call qmp_query_dump.

Paolo

Re: [Qemu-devel] question: about exec/poison.h

[Markus Armbruster]

Paolo Bonzini  writes:

> On 30/11/2015 04:47, Peter Xu wrote:
>> 
>> I met one problem when trying to add a new public function in dump.h
>> named "dump_state_get_global" and using it in hmp.c.
>
> Don't do that. :)
>
> hmp.c functions should in general use the QMP commands as the base.  In
> your case, hmp_info_dump should call qmp_query_dump.

Yes.  HMP commands need to be implemented on top of QMP commands, to
avoid a feature gap.  Exception: HMP commands that make no sense for
QMP, like "print" or "cpu".

Re: [Qemu-devel] question: about exec/poison.h

[Peter Xu]

On Mon, Nov 30, 2015 at 10:12:10AM +0100, Paolo Bonzini wrote:
> 
> 
> On 30/11/2015 04:47, Peter Xu wrote:
> > 
> > I met one problem when trying to add a new public function in dump.h
> > named "dump_state_get_global" and using it in hmp.c.
> 
> Don't do that. :)
> 
> hmp.c functions should in general use the QMP commands as the base.  In
> your case, hmp_info_dump should call qmp_query_dump.

Hi, Paolo,

That becomes a problem only if I do not have "percentage" (or
written/total) in QMP queries, which is suggested in the previous
review message:

https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg06088.html

"""
The percentage is not necessary as part of the QMP return value.  You
can compute it in hmp_info_dump however and print it to HMP only.
"""

So from what I understand from your reply, I could include the
percentage value into "query-dump", right? :)

Actually, I think it would be cooler if we could have them in both
QMP/HMP messages (maybe I would better prefer percentage comparing
to written/total bytes, since it is more directly human
readable).

Thanks!
Peter

> 
> Paolo

Re: [Qemu-devel] question: about exec/poison.h

[Peter Xu]

On Mon, Nov 30, 2015 at 10:28:08AM +0100, Markus Armbruster wrote:
> Paolo Bonzini  writes:
> 
> > On 30/11/2015 04:47, Peter Xu wrote:
> >> 
> >> I met one problem when trying to add a new public function in dump.h
> >> named "dump_state_get_global" and using it in hmp.c.
> >
> > Don't do that. :)
> >
> > hmp.c functions should in general use the QMP commands as the base.  In
> > your case, hmp_info_dump should call qmp_query_dump.
> 
> Yes.  HMP commands need to be implemented on top of QMP commands, to
> avoid a feature gap.  Exception: HMP commands that make no sense for
> QMP, like "print" or "cpu".

I see. Thanks Markus!

Peter

Re: [Qemu-devel] question: about exec/poison.h

[Markus Armbruster]

Peter Xu  writes:

> Hi, all,
>
> I met one problem when trying to add a new public function in dump.h
> named "dump_state_get_global" and using it in hmp.c. What I got is
> something like:
>
> In file included from /root/git/qemu/hmp.c:35:0:
> /root/git/qemu/include/sysemu/dump.h:26:34: error: attempt to use
> poisoned "TARGET_PAGE_BITS"
>  (((unsigned long long)(X) >> TARGET_PAGE_BITS) - ARCH_PFN_OFFSET)
>
> I did a quick look on the poison.h file, seeing that it should be
> used to avoid using arch-depentent macros in arch-independent
> codes. That's cool. However, that's also problem to me.
>
> The problem is: First of all, dump itself is arch
> dependent. Meanwhile, hmp.c is not. Also, what I am trying to add is
> arch indepentent too (just like hmp.c). Now if I include "dump.h" in
> hmp.c to use that function, I may encounter the error message.
>
> I got one idea, which is to split dump.h into two header files:
> dump.h and dump-arch-indep.h (the latter name could be of course
> shorter). So that I can move arch independent declarations into that
> new header file and use it in hmp.h. Not sure whether this is the
> good one to go.
>
> Does anyone have suggestion on what I should do?

What would the contents of an arch-independent dump.h be?  If it's
interesting, keeping it in its own header probably makes sense.  If not,
perhaps we can find an existing header to use.  Can't say more than that
without seeing the actual contents.

Re: [Qemu-devel] question: about exec/poison.h

[Peter Xu]

On Mon, Nov 30, 2015 at 11:36:41AM +0100, Paolo Bonzini wrote:
> 
> 
> On 30/11/2015 10:47, Peter Xu wrote:
> > So from what I understand from your reply, I could include the
> > percentage value into "query-dump", right? :)
> 
> If you have the written and total bytes in the QMP reply, you can use
> total*100/written to compute the percentage in the HMP return value.
> 
> If total and written do not produce the percentage, you need to add
> another numerator and denominator pair to the QMP reply.

Then I will use written/total numbers in QMP reply.

> 
> > Actually, I think it would be cooler if we could have them in both
> > QMP/HMP messages (maybe I would better prefer percentage comparing
> > to written/total bytes, since it is more directly human
> > readable).
> 
> For HMP yes.  QMP need not be human readable, it only needs to be a good
> API (which sometimes means being less human readable).

Ok. Thanks.

Peter

> 
> Paolo

Re: [Qemu-devel] question: about exec/poison.h

[Paolo Bonzini]

On 30/11/2015 10:47, Peter Xu wrote:
> So from what I understand from your reply, I could include the
> percentage value into "query-dump", right? :)

If you have the written and total bytes in the QMP reply, you can use
total*100/written to compute the percentage in the HMP return value.

If total and written do not produce the percentage, you need to add
another numerator and denominator pair to the QMP reply.

> Actually, I think it would be cooler if we could have them in both
> QMP/HMP messages (maybe I would better prefer percentage comparing
> to written/total bytes, since it is more directly human
> readable).

For HMP yes.  QMP need not be human readable, it only needs to be a good
API (which sometimes means being less human readable).

Paolo

Re: [Qemu-devel] question: about exec/poison.h

[Peter Xu]

On Mon, Nov 30, 2015 at 09:06:45AM +0100, Markus Armbruster wrote:
> Peter Xu  writes:
> 
> > Hi, all,
> >
> > I met one problem when trying to add a new public function in dump.h
> > named "dump_state_get_global" and using it in hmp.c. What I got is
> > something like:
> >
> > In file included from /root/git/qemu/hmp.c:35:0:
> > /root/git/qemu/include/sysemu/dump.h:26:34: error: attempt to use
> > poisoned "TARGET_PAGE_BITS"
> >  (((unsigned long long)(X) >> TARGET_PAGE_BITS) - ARCH_PFN_OFFSET)
> >
> > I did a quick look on the poison.h file, seeing that it should be
> > used to avoid using arch-depentent macros in arch-independent
> > codes. That's cool. However, that's also problem to me.
> >
> > The problem is: First of all, dump itself is arch
> > dependent. Meanwhile, hmp.c is not. Also, what I am trying to add is
> > arch indepentent too (just like hmp.c). Now if I include "dump.h" in
> > hmp.c to use that function, I may encounter the error message.
> >
> > I got one idea, which is to split dump.h into two header files:
> > dump.h and dump-arch-indep.h (the latter name could be of course
> > shorter). So that I can move arch independent declarations into that
> > new header file and use it in hmp.h. Not sure whether this is the
> > good one to go.
> >
> > Does anyone have suggestion on what I should do?
> 
> What would the contents of an arch-independent dump.h be?  If it's
> interesting, keeping it in its own header probably makes sense.  If not,
> perhaps we can find an existing header to use.  Can't say more than that
> without seeing the actual contents.

Hi, Markus,

It's related to dump detach support patch set. Then I think I could
first leverage an existing header and post the patch first.

Thanks for the reply.
Peter