| Message ID | 1350838081-6351-2-git-send-email-lilei@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 10/21/2012 10:47 AM, Lei Li wrote: > Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> > --- > qemu-char.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 72 insertions(+), 0 deletions(-) > > diff --git a/qemu-char.c b/qemu-char.c > index b082bae..b174da1 100644 > --- a/qemu-char.c > +++ b/qemu-char.c > @@ -2588,6 +2588,78 @@ size_t qemu_chr_mem_osize(const CharDriverState *chr) > return d->outbuf_size; > } > > +/*********************************************************/ > +/*CircularMemoryr chardev*/ s/CircularMemoryr/CircularMemory/ > +static int cirmem_chr_write(CharDriverState *chr, const uint8_t *buf, int len) > +{ > + CirMemCharDriver *d = chr->opaque; > + int i; > + > + if (len < 0) { > + return -1; > + } > + > + /* The size should be a power of 2. */ Shouldn't you enforce that, then?
On Mon, 22 Oct 2012 00:47:57 +0800 Lei Li <lilei@linux.vnet.ibm.com> wrote: > Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> This patch should be squashed in the next one. More comments below. > --- > qemu-char.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 72 insertions(+), 0 deletions(-) > > diff --git a/qemu-char.c b/qemu-char.c > index b082bae..b174da1 100644 > --- a/qemu-char.c > +++ b/qemu-char.c > @@ -2588,6 +2588,78 @@ size_t qemu_chr_mem_osize(const CharDriverState *chr) > return d->outbuf_size; > } > > +/*********************************************************/ > +/*CircularMemoryr chardev*/ s/Memoryr/Memory > + > +typedef struct { > + size_t size; > + size_t producer; > + size_t consumer; > + uint8_t *cbuf; > +} CirMemCharDriver; > + > +static bool cirmem_chr_is_empty(const CharDriverState *chr) > +{ > + const CirMemCharDriver *d = chr->opaque; > + > + return d->producer == d->consumer; > +} > + > +static bool cirmem_chr_is_full(const CharDriverState *chr) > +{ > + const CirMemCharDriver *d = chr->opaque; > + > + return (d->producer - d->consumer) >= d->size; > +} > + > +static int cirmem_chr_write(CharDriverState *chr, const uint8_t *buf, int len) > +{ > + CirMemCharDriver *d = chr->opaque; > + int i; > + > + if (len < 0) { > + return -1; > + } > + > + /* The size should be a power of 2. */ > + for (i = 0; i < len; i++ ) { > + d->cbuf[d->producer % d->size] = buf[i]; > + d->producer++; > + } > + > + return 0; > +} > + > +static int cirmem_chr_read(CharDriverState *chr, uint8_t *buf, int len) > +{ > + CirMemCharDriver *d = chr->opaque; > + int i; > + > + if (cirmem_chr_is_empty(chr) || len < 0) { > + return -1; > + } > + > + if (len > d->size) { > + len = d->size; > + } > + > + for (i = 0; i < len; i++) { > + buf[i] = d->cbuf[d->consumer % d->size]; > + d->consumer++; > + } > + > + return 0; > +} You don't seem to reset producer/consumer anywhere, I wonder if it's possible for a long running VM to trigger the limit here. > + > +static void cirmem_chr_close(struct CharDriverState *chr) > +{ > + CirMemCharDriver *d = chr->opaque; > + > + g_free(d); > + g_free(chr->opaque); Double free. I think you want to free cbuf, like: g_free(d->cbuf); g_free(d); > + chr->opaque = NULL; > +} > + > QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename) > { > char host[65], port[33], width[8], height[8];
On 10/22/2012 10:08 PM, Eric Blake wrote: > On 10/21/2012 10:47 AM, Lei Li wrote: >> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> >> --- >> qemu-char.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 files changed, 72 insertions(+), 0 deletions(-) >> >> diff --git a/qemu-char.c b/qemu-char.c >> index b082bae..b174da1 100644 >> --- a/qemu-char.c >> +++ b/qemu-char.c >> @@ -2588,6 +2588,78 @@ size_t qemu_chr_mem_osize(const CharDriverState *chr) >> return d->outbuf_size; >> } >> >> +/*********************************************************/ >> +/*CircularMemoryr chardev*/ > s/CircularMemoryr/CircularMemory/ > Yeah, I just found it... Thanks! >> +static int cirmem_chr_write(CharDriverState *chr, const uint8_t *buf, int len) >> +{ >> + CirMemCharDriver *d = chr->opaque; >> + int i; >> + >> + if (len < 0) { >> + return -1; >> + } >> + >> + /* The size should be a power of 2. */ > Shouldn't you enforce that, then? Yes, it has been checked when open the CirMemChar backend in patch 2/5, as code below: if (d->size & (d->size -1)) { return NULL; }
On 10/23/2012 02:14 AM, Luiz Capitulino wrote: > On Mon, 22 Oct 2012 00:47:57 +0800 > Lei Li <lilei@linux.vnet.ibm.com> wrote: > >> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> > This patch should be squashed in the next one. More comments below. > >> --- >> qemu-char.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 files changed, 72 insertions(+), 0 deletions(-) >> >> diff --git a/qemu-char.c b/qemu-char.c >> index b082bae..b174da1 100644 >> --- a/qemu-char.c >> +++ b/qemu-char.c >> @@ -2588,6 +2588,78 @@ size_t qemu_chr_mem_osize(const CharDriverState *chr) >> return d->outbuf_size; >> } >> >> +/*********************************************************/ >> +/*CircularMemoryr chardev*/ > s/Memoryr/Memory > >> + >> +typedef struct { >> + size_t size; >> + size_t producer; >> + size_t consumer; >> + uint8_t *cbuf; >> +} CirMemCharDriver; >> + >> +static bool cirmem_chr_is_empty(const CharDriverState *chr) >> +{ >> + const CirMemCharDriver *d = chr->opaque; >> + >> + return d->producer == d->consumer; >> +} >> + >> +static bool cirmem_chr_is_full(const CharDriverState *chr) >> +{ >> + const CirMemCharDriver *d = chr->opaque; >> + >> + return (d->producer - d->consumer) >= d->size; >> +} >> + >> +static int cirmem_chr_write(CharDriverState *chr, const uint8_t *buf, int len) >> +{ >> + CirMemCharDriver *d = chr->opaque; >> + int i; >> + >> + if (len < 0) { >> + return -1; >> + } >> + >> + /* The size should be a power of 2. */ >> + for (i = 0; i < len; i++ ) { >> + d->cbuf[d->producer % d->size] = buf[i]; >> + d->producer++; >> + } >> + >> + return 0; >> +} >> + >> +static int cirmem_chr_read(CharDriverState *chr, uint8_t *buf, int len) >> +{ >> + CirMemCharDriver *d = chr->opaque; >> + int i; >> + >> + if (cirmem_chr_is_empty(chr) || len < 0) { >> + return -1; >> + } >> + >> + if (len > d->size) { >> + len = d->size; >> + } >> + >> + for (i = 0; i < len; i++) { >> + buf[i] = d->cbuf[d->consumer % d->size]; >> + d->consumer++; >> + } >> + >> + return 0; >> +} > You don't seem to reset producer/consumer anywhere, I wonder if it's possible > for a long running VM to trigger the limit here. Yes, it make sense.
On Tue, 23 Oct 2012 13:40:13 +0800 Lei Li <lilei@linux.vnet.ibm.com> wrote: > >> +static int cirmem_chr_write(CharDriverState *chr, const uint8_t *buf, int len) > >> +{ > >> + CirMemCharDriver *d = chr->opaque; > >> + int i; > >> + > >> + if (len < 0) { > >> + return -1; > >> + } > >> + > >> + /* The size should be a power of 2. */ > > Shouldn't you enforce that, then? > > Yes, it has been checked when open the CirMemChar backend in patch 2/5, > as code below: > > if (d->size & (d->size -1)) { > return NULL; > } You could add an assert() in cirmem_chr_write() then.
diff --git a/qemu-char.c b/qemu-char.c index b082bae..b174da1 100644 --- a/qemu-char.c +++ b/qemu-char.c @@ -2588,6 +2588,78 @@ size_t qemu_chr_mem_osize(const CharDriverState *chr) return d->outbuf_size; } +/*********************************************************/ +/*CircularMemoryr chardev*/ + +typedef struct { + size_t size; + size_t producer; + size_t consumer; + uint8_t *cbuf; +} CirMemCharDriver; + +static bool cirmem_chr_is_empty(const CharDriverState *chr) +{ + const CirMemCharDriver *d = chr->opaque; + + return d->producer == d->consumer; +} + +static bool cirmem_chr_is_full(const CharDriverState *chr) +{ + const CirMemCharDriver *d = chr->opaque; + + return (d->producer - d->consumer) >= d->size; +} + +static int cirmem_chr_write(CharDriverState *chr, const uint8_t *buf, int len) +{ + CirMemCharDriver *d = chr->opaque; + int i; + + if (len < 0) { + return -1; + } + + /* The size should be a power of 2. */ + for (i = 0; i < len; i++ ) { + d->cbuf[d->producer % d->size] = buf[i]; + d->producer++; + } + + return 0; +} + +static int cirmem_chr_read(CharDriverState *chr, uint8_t *buf, int len) +{ + CirMemCharDriver *d = chr->opaque; + int i; + + if (cirmem_chr_is_empty(chr) || len < 0) { + return -1; + } + + if (len > d->size) { + len = d->size; + } + + for (i = 0; i < len; i++) { + buf[i] = d->cbuf[d->consumer % d->size]; + d->consumer++; + } + + return 0; +} + +static void cirmem_chr_close(struct CharDriverState *chr) +{ + CirMemCharDriver *d = chr->opaque; + + g_free(d); + g_free(chr->opaque); + chr->opaque = NULL; +} + QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename) { char host[65], port[33], width[8], height[8];
Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> --- qemu-char.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 72 insertions(+), 0 deletions(-)