Patchwork [1/5] qemu-char: Add new char backend CircularMemCharDriver

login
register
mail settings
Submitter Lei Li
Date Oct. 21, 2012, 4:47 p.m.
Message ID <1350838081-6351-2-git-send-email-lilei@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/193044/
State New
Headers show

Comments

Lei Li - Oct. 21, 2012, 4:47 p.m.
Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 qemu-char.c |   72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 72 insertions(+), 0 deletions(-)
Eric Blake - Oct. 22, 2012, 2:08 p.m.
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?
Luiz Capitulino - Oct. 22, 2012, 6:14 p.m.
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];
Lei Li - Oct. 23, 2012, 5:40 a.m.
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;
}
Lei Li - Oct. 23, 2012, 6:36 a.m.
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.
Luiz Capitulino - Oct. 23, 2012, 12:42 p.m.
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.

Patch

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];