Re: [Qemu-devel] [PATCH 2/6] monitor: Adjust qmp_human_monitor_command to new MemCharDriver

[Luiz Capitulino <lcapitulino@redhat.com>]

On Tue, 04 Sep 2012 00:14:03 +0800
Lei Li <lilei@linux.vnet.ibm.com> wrote:

> On 08/31/2012 02:51 AM, Luiz Capitulino wrote:
> > On Thu, 23 Aug 2012 13:14:22 +0800
> > Lei Li <lilei@linux.vnet.ibm.com> wrote:
> >
> >> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> >> ---
> >>   monitor.c |    8 +++++++-
> >>   1 files changed, 7 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/monitor.c b/monitor.c
> >> index 480f583..ab4650b 100644
> >> --- a/monitor.c
> >> +++ b/monitor.c
> >> @@ -642,7 +642,13 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
> >>       CharDriverState mchar;
> >>   
> >>       memset(&hmp, 0, sizeof(hmp));
> >> -    qemu_chr_init_mem(&mchar);
> >> +
> >> +    /* Since the backend of MemCharDriver convert to a circular
> >> +     * buffer with fixed size, so should indicate the init memory
> >> +     * size.
> >> +     *
> >> +     * XXX:  is 4096 as init memory enough for this? */
> >> +    qemu_chr_init_mem(&mchar, 4096);
> > I'm not sure I like this. The end result will be that hmp commands writing
> > more than 4096 bytes will simply fail or return garbage (if the circular buffer
> > is changed to allow writing more than it supports) today they would just work.
> >
> > Although it's always possible to increase the buffer size, we would only realize
> > this is needed when the bug is triggered, which means it has a high chance
> > of happening in production. IOW, this would be a regression.
> >
> > The only solution I can think of is to make the circular buffer and the
> > current MemoryDriver live in parallel. Actually, you really seem to be
> > adding something else.
> 
> Hi Luiz,
> 
> Thanks for your review. Yes, I agree with that it's not a good solution for the
> old user human command. Make the circular buffer and the current MemoryDriver
> live in parallel, do you mean don't change the current MemoryDriver, choose to
> expose a new char device backend with circular buffer?

Yes, you could add CircularMemoryDriver for example.

> 
> >>       hmp.chr = &mchar;
> >>   
> >>       old_mon = cur_mon;
> >
> 
>