Message ID | 1345698866-19794-3-git-send-email-lilei@linux.vnet.ibm.com |
---|---|
State | New |
Headers | show |
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. > hmp.chr = &mchar; > > old_mon = cur_mon;
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? >> hmp.chr = &mchar; >> >> old_mon = cur_mon; >
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; > > > >
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); hmp.chr = &mchar; old_mon = cur_mon;
Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> --- monitor.c | 8 +++++++- 1 files changed, 7 insertions(+), 1 deletions(-)