| Message ID | 1288037204-27768-2-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
Luiz Capitulino <lcapitulino@redhat.com> writes: > This driver handles in-memory qemu-char driver operations, that's, > all writes to this driver are stored in an internal buffer. The > driver doesn't talk to the external world in any way. I'm not so happy with the name "buffered driver". "Bufferedness" isn't what this kind of character device is about. It's about being connected to a memory buffer. Consider stdio streams or C++ IOstreams: there are various kinds of streams, and they can be buffered or unbuffered. One kind is the "memory" or "string" stream. What about "memory driver"? "membuf driver"? "string driver"? > Right now it's very simple: it supports only writes. But it can > be easily extended to support more operations. > > This driver is going to be used by the monitor subsystem, which > needs to 'emulate' a qemu-char device, so that monitor handlers > can be ran without a backing device and have their output stored. > > TODO: Move buffer growth logic to cutils. Would be nice to have this TODO as comment in the code. > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > --- > qemu-char.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > qemu-char.h | 6 +++++ > 2 files changed, 74 insertions(+), 0 deletions(-) > > diff --git a/qemu-char.c b/qemu-char.c > index 6d2dce7..1ca9ccc 100644 > --- a/qemu-char.c > +++ b/qemu-char.c > @@ -2279,6 +2279,74 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts) > return NULL; > } > > +/***********************************************************/ > +/* Buffered chardev */ > +typedef struct { > + size_t outbuf_size; > + size_t outbuf_capacity; > + uint8_t *outbuf; > +} BufferedDriver; Out of curiosity: how do you think input should work? Second buffer? Loopback to same buffer? Maybe the buffer should be a separate data type, with the character device type wrapped around its buffer(s). I'm not asking you to do that now. > + > +static int buffered_chr_write(CharDriverState *chr, const uint8_t *buf, int len) > +{ > + BufferedDriver *d = chr->opaque; > + > + if (d->outbuf_capacity < (d->outbuf_size + len)) { Superfluous parenthesis around right operand of <. > + /* grow outbuf */ > + d->outbuf_capacity += len; > + d->outbuf_capacity *= 2; > + d->outbuf = qemu_realloc(d->outbuf, d->outbuf_capacity); > + } > + > + memcpy(d->outbuf + d->outbuf_size, buf, len); > + d->outbuf_size += len; > + > + return 0; Uh, aren't you supposed to return len here? > +} > + > +void qemu_chr_init_buffered(CharDriverState *chr) > +{ > + BufferedDriver *d; > + > + d = qemu_mallocz(sizeof(BufferedDriver)); > + d->outbuf_capacity = 4096; > + d->outbuf = qemu_mallocz(d->outbuf_capacity); > + > + memset(chr, 0, sizeof(*chr)); > + chr->opaque = d; > + chr->chr_write = buffered_chr_write; > +} Unlike normal character drivers, this one isn't accessible via qemu_chr_open(). That's why you have qemu_chr_init_buffered() rather than qemu_chr_open_buffered(). > + > +QString *qemu_chr_buffered_to_qs(CharDriverState *chr) > +{ > + char *str; > + QString *qs; > + BufferedDriver *d = chr->opaque; > + > + if (d->outbuf_size == 0) { > + return NULL; > + } This is weird. Shouldn't we return an empty QString when chr contains an empty string? > + > + str = qemu_malloc(d->outbuf_size + 1); > + memcpy(str, d->outbuf, d->outbuf_size); > + str[d->outbuf_size] = '\0'; > + > + qs = qstring_from_str(str); > + qemu_free(str); > + > + return qs; > +} While a QString is exactly what you need in PATCH 2/2, it's rather special. Let's split it into the elementary building blocks: (1) Find the string stored within the chr. (2) Copy it to a newly malloc'ed buffer. (3) Wrap a QString around it. Already exists: qstring_from_str(). Maybe you'd prefer to keep (1) and (2) fused. Fine with me. > + > +void qemu_chr_close_buffered(CharDriverState *chr) > +{ > + BufferedDriver *d = chr->opaque; > + > + qemu_free(d->outbuf); > + qemu_free(chr->opaque); > + chr->opaque = NULL; > + chr->chr_write = NULL; > +} Unlike normal character drivers, this one can't be closed with qemu_chr_close(), can it? What happens if someone calls qemu_chr_close() on it? > + > QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename) > { > char host[65], port[33], width[8], height[8]; > diff --git a/qemu-char.h b/qemu-char.h > index 18ad12b..4467641 100644 > --- a/qemu-char.h > +++ b/qemu-char.h > @@ -6,6 +6,7 @@ > #include "qemu-option.h" > #include "qemu-config.h" > #include "qobject.h" > +#include "qstring.h" > > /* character device */ > > @@ -100,6 +101,11 @@ CharDriverState *qemu_chr_open_eventfd(int eventfd); > > extern int term_escape_char; > > +/* buffered chardev */ > +void qemu_chr_init_buffered(CharDriverState *chr); > +void qemu_chr_close_buffered(CharDriverState *chr); > +QString *qemu_chr_buffered_to_qs(CharDriverState *chr); > + > /* async I/O support */ > > int qemu_set_fd_handler2(int fd,
On Wed, 10 Nov 2010 10:26:15 +0100 Markus Armbruster <armbru@redhat.com> wrote: > Luiz Capitulino <lcapitulino@redhat.com> writes: > > > This driver handles in-memory qemu-char driver operations, that's, > > all writes to this driver are stored in an internal buffer. The > > driver doesn't talk to the external world in any way. > > I'm not so happy with the name "buffered driver". "Bufferedness" isn't > what this kind of character device is about. It's about being connected > to a memory buffer. > > Consider stdio streams or C++ IOstreams: there are various kinds of > streams, and they can be buffered or unbuffered. One kind is the > "memory" or "string" stream. Makes sense. > What about "memory driver"? "membuf driver"? "string driver"? Let's call it MemoryDriver then. > > Right now it's very simple: it supports only writes. But it can > > be easily extended to support more operations. > > > > This driver is going to be used by the monitor subsystem, which > > needs to 'emulate' a qemu-char device, so that monitor handlers > > can be ran without a backing device and have their output stored. > > > > TODO: Move buffer growth logic to cutils. > > Would be nice to have this TODO as comment in the code. True. > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > > --- > > qemu-char.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > qemu-char.h | 6 +++++ > > 2 files changed, 74 insertions(+), 0 deletions(-) > > > > diff --git a/qemu-char.c b/qemu-char.c > > index 6d2dce7..1ca9ccc 100644 > > --- a/qemu-char.c > > +++ b/qemu-char.c > > @@ -2279,6 +2279,74 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts) > > return NULL; > > } > > > > +/***********************************************************/ > > +/* Buffered chardev */ > > +typedef struct { > > + size_t outbuf_size; > > + size_t outbuf_capacity; > > + uint8_t *outbuf; > > +} BufferedDriver; > > Out of curiosity: how do you think input should work? Second buffer? > Loopback to same buffer? I was thinking in having a second buffer. > Maybe the buffer should be a separate data type, with the character > device type wrapped around its buffer(s). I'm not asking you to do that > now. Seems interesting. > > + > > +static int buffered_chr_write(CharDriverState *chr, const uint8_t *buf, int len) > > +{ > > + BufferedDriver *d = chr->opaque; > > + > > + if (d->outbuf_capacity < (d->outbuf_size + len)) { > > Superfluous parenthesis around right operand of <. > > > + /* grow outbuf */ > > + d->outbuf_capacity += len; > > + d->outbuf_capacity *= 2; > > + d->outbuf = qemu_realloc(d->outbuf, d->outbuf_capacity); > > + } > > + > > + memcpy(d->outbuf + d->outbuf_size, buf, len); > > + d->outbuf_size += len; > > + > > + return 0; > > Uh, aren't you supposed to return len here? Yes, but you're reviewing my RFC, I've posted an updated version already: http://lists.gnu.org/archive/html/qemu-devel/2010-10/msg02232.html I think most of your comments still apply, if not please say. > > +} > > + > > +void qemu_chr_init_buffered(CharDriverState *chr) > > +{ > > + BufferedDriver *d; > > + > > + d = qemu_mallocz(sizeof(BufferedDriver)); > > + d->outbuf_capacity = 4096; > > + d->outbuf = qemu_mallocz(d->outbuf_capacity); > > + > > + memset(chr, 0, sizeof(*chr)); > > + chr->opaque = d; > > + chr->chr_write = buffered_chr_write; > > +} > > Unlike normal character drivers, this one isn't accessible via > qemu_chr_open(). That's why you have qemu_chr_init_buffered() rather > than qemu_chr_open_buffered(). Yes, and it's simpler that way. > > +QString *qemu_chr_buffered_to_qs(CharDriverState *chr) > > +{ > > + char *str; > > + QString *qs; > > + BufferedDriver *d = chr->opaque; > > + > > + if (d->outbuf_size == 0) { > > + return NULL; > > + } > > This is weird. Shouldn't we return an empty QString when chr contains > an empty string? Yeah, will fix. > > + str = qemu_malloc(d->outbuf_size + 1); > > + memcpy(str, d->outbuf, d->outbuf_size); > > + str[d->outbuf_size] = '\0'; > > + > > + qs = qstring_from_str(str); > > + qemu_free(str); > > + > > + return qs; > > +} > > While a QString is exactly what you need in PATCH 2/2, it's rather > special. Let's split it into the elementary building blocks: > > (1) Find the string stored within the chr. > (2) Copy it to a newly malloc'ed buffer. > (3) Wrap a QString around it. Already exists: qstring_from_str(). > > Maybe you'd prefer to keep (1) and (2) fused. Fine with me. This function is different in v1, it's quite simple, but it still returns a QString: /* assumes the stored data is a string */ QString *qemu_chr_buffered_to_qs(CharDriverState *chr) { BufferedDriver *d = chr->opaque; if (d->outbuf_size == 0) { return NULL; } return qstring_from_substr((char *) d->outbuf, 0, d->outbuf_size - 1); } I'd like to keep things as simple as possible for now, maybe future users will want to get a raw buffer, maybe not. Let's add it when needed. > > + > > +void qemu_chr_close_buffered(CharDriverState *chr) > > +{ > > + BufferedDriver *d = chr->opaque; > > + > > + qemu_free(d->outbuf); > > + qemu_free(chr->opaque); > > + chr->opaque = NULL; > > + chr->chr_write = NULL; > > +} > > Unlike normal character drivers, this one can't be closed with > qemu_chr_close(), can it? What happens if someone calls > qemu_chr_close() on it? I guess it will explode, because this driver is not in the chardevs list and our CharDriverState instance is allocated on the stack. Does a function comment solves the problem or do you have something else in mind? > > > + > > QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename) > > { > > char host[65], port[33], width[8], height[8]; > > diff --git a/qemu-char.h b/qemu-char.h > > index 18ad12b..4467641 100644 > > --- a/qemu-char.h > > +++ b/qemu-char.h > > @@ -6,6 +6,7 @@ > > #include "qemu-option.h" > > #include "qemu-config.h" > > #include "qobject.h" > > +#include "qstring.h" > > > > /* character device */ > > > > @@ -100,6 +101,11 @@ CharDriverState *qemu_chr_open_eventfd(int eventfd); > > > > extern int term_escape_char; > > > > +/* buffered chardev */ > > +void qemu_chr_init_buffered(CharDriverState *chr); > > +void qemu_chr_close_buffered(CharDriverState *chr); > > +QString *qemu_chr_buffered_to_qs(CharDriverState *chr); > > + > > /* async I/O support */ > > > > int qemu_set_fd_handler2(int fd, >
Luiz Capitulino <lcapitulino@redhat.com> writes: > On Wed, 10 Nov 2010 10:26:15 +0100 > Markus Armbruster <armbru@redhat.com> wrote: > >> Luiz Capitulino <lcapitulino@redhat.com> writes: >> >> > This driver handles in-memory qemu-char driver operations, that's, >> > all writes to this driver are stored in an internal buffer. The >> > driver doesn't talk to the external world in any way. >> >> I'm not so happy with the name "buffered driver". "Bufferedness" isn't >> what this kind of character device is about. It's about being connected >> to a memory buffer. >> >> Consider stdio streams or C++ IOstreams: there are various kinds of >> streams, and they can be buffered or unbuffered. One kind is the >> "memory" or "string" stream. > > Makes sense. > >> What about "memory driver"? "membuf driver"? "string driver"? > > Let's call it MemoryDriver then. Works for me. >> > Right now it's very simple: it supports only writes. But it can >> > be easily extended to support more operations. >> > >> > This driver is going to be used by the monitor subsystem, which >> > needs to 'emulate' a qemu-char device, so that monitor handlers >> > can be ran without a backing device and have their output stored. >> > >> > TODO: Move buffer growth logic to cutils. >> >> Would be nice to have this TODO as comment in the code. > > True. > >> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> >> > --- >> > qemu-char.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> > qemu-char.h | 6 +++++ >> > 2 files changed, 74 insertions(+), 0 deletions(-) >> > >> > diff --git a/qemu-char.c b/qemu-char.c >> > index 6d2dce7..1ca9ccc 100644 >> > --- a/qemu-char.c >> > +++ b/qemu-char.c >> > @@ -2279,6 +2279,74 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts) >> > return NULL; >> > } >> > >> > +/***********************************************************/ >> > +/* Buffered chardev */ >> > +typedef struct { >> > + size_t outbuf_size; >> > + size_t outbuf_capacity; >> > + uint8_t *outbuf; >> > +} BufferedDriver; >> >> Out of curiosity: how do you think input should work? Second buffer? >> Loopback to same buffer? > > I was thinking in having a second buffer. > >> Maybe the buffer should be a separate data type, with the character >> device type wrapped around its buffer(s). I'm not asking you to do that >> now. > > Seems interesting. > >> > + >> > +static int buffered_chr_write(CharDriverState *chr, const uint8_t *buf, int len) >> > +{ >> > + BufferedDriver *d = chr->opaque; >> > + >> > + if (d->outbuf_capacity < (d->outbuf_size + len)) { >> >> Superfluous parenthesis around right operand of <. >> >> > + /* grow outbuf */ >> > + d->outbuf_capacity += len; >> > + d->outbuf_capacity *= 2; >> > + d->outbuf = qemu_realloc(d->outbuf, d->outbuf_capacity); >> > + } >> > + >> > + memcpy(d->outbuf + d->outbuf_size, buf, len); >> > + d->outbuf_size += len; >> > + >> > + return 0; >> >> Uh, aren't you supposed to return len here? > > Yes, but you're reviewing my RFC, I've posted an updated version already: > > http://lists.gnu.org/archive/html/qemu-devel/2010-10/msg02232.html Meh. Sorry about that. > I think most of your comments still apply, if not please say. Okay to simply review your respin when it comes? >> > +} >> > + >> > +void qemu_chr_init_buffered(CharDriverState *chr) >> > +{ >> > + BufferedDriver *d; >> > + >> > + d = qemu_mallocz(sizeof(BufferedDriver)); >> > + d->outbuf_capacity = 4096; >> > + d->outbuf = qemu_mallocz(d->outbuf_capacity); >> > + >> > + memset(chr, 0, sizeof(*chr)); >> > + chr->opaque = d; >> > + chr->chr_write = buffered_chr_write; >> > +} >> >> Unlike normal character drivers, this one isn't accessible via >> qemu_chr_open(). That's why you have qemu_chr_init_buffered() rather >> than qemu_chr_open_buffered(). > > Yes, and it's simpler that way. > >> > +QString *qemu_chr_buffered_to_qs(CharDriverState *chr) >> > +{ >> > + char *str; >> > + QString *qs; >> > + BufferedDriver *d = chr->opaque; >> > + >> > + if (d->outbuf_size == 0) { >> > + return NULL; >> > + } >> >> This is weird. Shouldn't we return an empty QString when chr contains >> an empty string? > > Yeah, will fix. > >> > + str = qemu_malloc(d->outbuf_size + 1); >> > + memcpy(str, d->outbuf, d->outbuf_size); >> > + str[d->outbuf_size] = '\0'; >> > + >> > + qs = qstring_from_str(str); >> > + qemu_free(str); >> > + >> > + return qs; >> > +} >> >> While a QString is exactly what you need in PATCH 2/2, it's rather >> special. Let's split it into the elementary building blocks: >> >> (1) Find the string stored within the chr. >> (2) Copy it to a newly malloc'ed buffer. >> (3) Wrap a QString around it. Already exists: qstring_from_str(). >> >> Maybe you'd prefer to keep (1) and (2) fused. Fine with me. > > This function is different in v1, it's quite simple, but it still > returns a QString: > > /* assumes the stored data is a string */ What else could it be? Worrying about embedded '\0's? > QString *qemu_chr_buffered_to_qs(CharDriverState *chr) > { > BufferedDriver *d = chr->opaque; > > if (d->outbuf_size == 0) { > return NULL; > } > > return qstring_from_substr((char *) d->outbuf, 0, d->outbuf_size - 1); > } > > I'd like to keep things as simple as possible for now, maybe future > users will want to get a raw buffer, maybe not. Let's add it when needed. > >> > + >> > +void qemu_chr_close_buffered(CharDriverState *chr) >> > +{ >> > + BufferedDriver *d = chr->opaque; >> > + >> > + qemu_free(d->outbuf); >> > + qemu_free(chr->opaque); >> > + chr->opaque = NULL; >> > + chr->chr_write = NULL; >> > +} >> >> Unlike normal character drivers, this one can't be closed with >> qemu_chr_close(), can it? What happens if someone calls >> qemu_chr_close() on it? > > I guess it will explode, because this driver is not in the chardevs list > and our CharDriverState instance is allocated on the stack. > > Does a function comment solves the problem or do you have something else > in mind? A general OO rule: Having different constructors for different sub-types is okay, but once constructed, you should be able to use the objects without knowing of what sub-type they are. That includes destruction. Exceptions prove the rule. Maybe this is one, maybe not. >> > + >> > QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename) >> > { >> > char host[65], port[33], width[8], height[8]; >> > diff --git a/qemu-char.h b/qemu-char.h >> > index 18ad12b..4467641 100644 >> > --- a/qemu-char.h >> > +++ b/qemu-char.h >> > @@ -6,6 +6,7 @@ >> > #include "qemu-option.h" >> > #include "qemu-config.h" >> > #include "qobject.h" >> > +#include "qstring.h" >> > >> > /* character device */ >> > >> > @@ -100,6 +101,11 @@ CharDriverState *qemu_chr_open_eventfd(int eventfd); >> > >> > extern int term_escape_char; >> > >> > +/* buffered chardev */ >> > +void qemu_chr_init_buffered(CharDriverState *chr); >> > +void qemu_chr_close_buffered(CharDriverState *chr); >> > +QString *qemu_chr_buffered_to_qs(CharDriverState *chr); >> > + >> > /* async I/O support */ >> > >> > int qemu_set_fd_handler2(int fd, >>
On Wed, 10 Nov 2010 13:56:39 +0100 Markus Armbruster <armbru@redhat.com> wrote: > Luiz Capitulino <lcapitulino@redhat.com> writes: > > > On Wed, 10 Nov 2010 10:26:15 +0100 > > Markus Armbruster <armbru@redhat.com> wrote: > > > >> Luiz Capitulino <lcapitulino@redhat.com> writes: > >> > >> > This driver handles in-memory qemu-char driver operations, that's, > >> > all writes to this driver are stored in an internal buffer. The > >> > driver doesn't talk to the external world in any way. > >> > >> I'm not so happy with the name "buffered driver". "Bufferedness" isn't > >> what this kind of character device is about. It's about being connected > >> to a memory buffer. > >> > >> Consider stdio streams or C++ IOstreams: there are various kinds of > >> streams, and they can be buffered or unbuffered. One kind is the > >> "memory" or "string" stream. > > > > Makes sense. > > > >> What about "memory driver"? "membuf driver"? "string driver"? > > > > Let's call it MemoryDriver then. > > Works for me. > > >> > Right now it's very simple: it supports only writes. But it can > >> > be easily extended to support more operations. > >> > > >> > This driver is going to be used by the monitor subsystem, which > >> > needs to 'emulate' a qemu-char device, so that monitor handlers > >> > can be ran without a backing device and have their output stored. > >> > > >> > TODO: Move buffer growth logic to cutils. > >> > >> Would be nice to have this TODO as comment in the code. > > > > True. > > > >> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > >> > --- > >> > qemu-char.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > >> > qemu-char.h | 6 +++++ > >> > 2 files changed, 74 insertions(+), 0 deletions(-) > >> > > >> > diff --git a/qemu-char.c b/qemu-char.c > >> > index 6d2dce7..1ca9ccc 100644 > >> > --- a/qemu-char.c > >> > +++ b/qemu-char.c > >> > @@ -2279,6 +2279,74 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts) > >> > return NULL; > >> > } > >> > > >> > +/***********************************************************/ > >> > +/* Buffered chardev */ > >> > +typedef struct { > >> > + size_t outbuf_size; > >> > + size_t outbuf_capacity; > >> > + uint8_t *outbuf; > >> > +} BufferedDriver; > >> > >> Out of curiosity: how do you think input should work? Second buffer? > >> Loopback to same buffer? > > > > I was thinking in having a second buffer. > > > >> Maybe the buffer should be a separate data type, with the character > >> device type wrapped around its buffer(s). I'm not asking you to do that > >> now. > > > > Seems interesting. > > > >> > + > >> > +static int buffered_chr_write(CharDriverState *chr, const uint8_t *buf, int len) > >> > +{ > >> > + BufferedDriver *d = chr->opaque; > >> > + > >> > + if (d->outbuf_capacity < (d->outbuf_size + len)) { > >> > >> Superfluous parenthesis around right operand of <. > >> > >> > + /* grow outbuf */ > >> > + d->outbuf_capacity += len; > >> > + d->outbuf_capacity *= 2; > >> > + d->outbuf = qemu_realloc(d->outbuf, d->outbuf_capacity); > >> > + } > >> > + > >> > + memcpy(d->outbuf + d->outbuf_size, buf, len); > >> > + d->outbuf_size += len; > >> > + > >> > + return 0; > >> > >> Uh, aren't you supposed to return len here? > > > > Yes, but you're reviewing my RFC, I've posted an updated version already: > > > > http://lists.gnu.org/archive/html/qemu-devel/2010-10/msg02232.html > > Meh. Sorry about that. > > > I think most of your comments still apply, if not please say. > > Okay to simply review your respin when it comes? > > >> > +} > >> > + > >> > +void qemu_chr_init_buffered(CharDriverState *chr) > >> > +{ > >> > + BufferedDriver *d; > >> > + > >> > + d = qemu_mallocz(sizeof(BufferedDriver)); > >> > + d->outbuf_capacity = 4096; > >> > + d->outbuf = qemu_mallocz(d->outbuf_capacity); > >> > + > >> > + memset(chr, 0, sizeof(*chr)); > >> > + chr->opaque = d; > >> > + chr->chr_write = buffered_chr_write; > >> > +} > >> > >> Unlike normal character drivers, this one isn't accessible via > >> qemu_chr_open(). That's why you have qemu_chr_init_buffered() rather > >> than qemu_chr_open_buffered(). > > > > Yes, and it's simpler that way. > > > >> > +QString *qemu_chr_buffered_to_qs(CharDriverState *chr) > >> > +{ > >> > + char *str; > >> > + QString *qs; > >> > + BufferedDriver *d = chr->opaque; > >> > + > >> > + if (d->outbuf_size == 0) { > >> > + return NULL; > >> > + } > >> > >> This is weird. Shouldn't we return an empty QString when chr contains > >> an empty string? > > > > Yeah, will fix. > > > >> > + str = qemu_malloc(d->outbuf_size + 1); > >> > + memcpy(str, d->outbuf, d->outbuf_size); > >> > + str[d->outbuf_size] = '\0'; > >> > + > >> > + qs = qstring_from_str(str); > >> > + qemu_free(str); > >> > + > >> > + return qs; > >> > +} > >> > >> While a QString is exactly what you need in PATCH 2/2, it's rather > >> special. Let's split it into the elementary building blocks: > >> > >> (1) Find the string stored within the chr. > >> (2) Copy it to a newly malloc'ed buffer. > >> (3) Wrap a QString around it. Already exists: qstring_from_str(). > >> > >> Maybe you'd prefer to keep (1) and (2) fused. Fine with me. > > > > This function is different in v1, it's quite simple, but it still > > returns a QString: > > > > /* assumes the stored data is a string */ > > What else could it be? Worrying about embedded '\0's? > > > QString *qemu_chr_buffered_to_qs(CharDriverState *chr) > > { > > BufferedDriver *d = chr->opaque; > > > > if (d->outbuf_size == 0) { > > return NULL; > > } > > > > return qstring_from_substr((char *) d->outbuf, 0, d->outbuf_size - 1); > > } > > > > I'd like to keep things as simple as possible for now, maybe future > > users will want to get a raw buffer, maybe not. Let's add it when needed. > > > >> > + > >> > +void qemu_chr_close_buffered(CharDriverState *chr) > >> > +{ > >> > + BufferedDriver *d = chr->opaque; > >> > + > >> > + qemu_free(d->outbuf); > >> > + qemu_free(chr->opaque); > >> > + chr->opaque = NULL; > >> > + chr->chr_write = NULL; > >> > +} > >> > >> Unlike normal character drivers, this one can't be closed with > >> qemu_chr_close(), can it? What happens if someone calls > >> qemu_chr_close() on it? > > > > I guess it will explode, because this driver is not in the chardevs list > > and our CharDriverState instance is allocated on the stack. > > > > Does a function comment solves the problem or do you have something else > > in mind? > > A general OO rule: Having different constructors for different sub-types > is okay, but once constructed, you should be able to use the objects > without knowing of what sub-type they are. That includes destruction. We will have to add our MemoryDriver to the chardevs list, this has some implications like being visible in qemu_chr_info() and qemu_chr_find(), likely to also imply that we should choose a chr->filename. Another detail is that we'll have to dynamically alocate our CharDriverState instance. Not a problem, but adds a few more lines of code and a qemu_free(). None of this is needed today. Really worth it? > > Exceptions prove the rule. Maybe this is one, maybe not. > > >> > + > >> > QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename) > >> > { > >> > char host[65], port[33], width[8], height[8]; > >> > diff --git a/qemu-char.h b/qemu-char.h > >> > index 18ad12b..4467641 100644 > >> > --- a/qemu-char.h > >> > +++ b/qemu-char.h > >> > @@ -6,6 +6,7 @@ > >> > #include "qemu-option.h" > >> > #include "qemu-config.h" > >> > #include "qobject.h" > >> > +#include "qstring.h" > >> > > >> > /* character device */ > >> > > >> > @@ -100,6 +101,11 @@ CharDriverState *qemu_chr_open_eventfd(int eventfd); > >> > > >> > extern int term_escape_char; > >> > > >> > +/* buffered chardev */ > >> > +void qemu_chr_init_buffered(CharDriverState *chr); > >> > +void qemu_chr_close_buffered(CharDriverState *chr); > >> > +QString *qemu_chr_buffered_to_qs(CharDriverState *chr); > >> > + > >> > /* async I/O support */ > >> > > >> > int qemu_set_fd_handler2(int fd, > >> >
Luiz Capitulino <lcapitulino@redhat.com> writes: > On Wed, 10 Nov 2010 13:56:39 +0100 > Markus Armbruster <armbru@redhat.com> wrote: > >> Luiz Capitulino <lcapitulino@redhat.com> writes: >> >> > On Wed, 10 Nov 2010 10:26:15 +0100 >> > Markus Armbruster <armbru@redhat.com> wrote: [...] >> >> Unlike normal character drivers, this one can't be closed with >> >> qemu_chr_close(), can it? What happens if someone calls >> >> qemu_chr_close() on it? >> > >> > I guess it will explode, because this driver is not in the chardevs list >> > and our CharDriverState instance is allocated on the stack. >> > >> > Does a function comment solves the problem or do you have something else >> > in mind? >> >> A general OO rule: Having different constructors for different sub-types >> is okay, but once constructed, you should be able to use the objects >> without knowing of what sub-type they are. That includes destruction. > > We will have to add our MemoryDriver to the chardevs list, this has some > implications like being visible in qemu_chr_info() and qemu_chr_find(), > likely to also imply that we should choose a chr->filename. Not if we formalize the notion of an "internal use only" character device. Say, !chr->filename means it's internal, and internal ones aren't in chardevs. Make qemu_chr_close()'s QTAILQ_REMOVE() conditional !chr->filename. > Another detail is that we'll have to dynamically alocate our CharDriverState > instance. Not a problem, but adds a few more lines of code and a > qemu_free(). None of this is needed today. I doubt the alloc/free matters. > Really worth it? Your call. But if you decide not to, please add a suitable assertion to qemu_chr_close(), to make it obvious what went wrong when an internal character device explodes there. >> Exceptions prove the rule. Maybe this is one, maybe not. [...]
On Wed, 10 Nov 2010 14:33:47 +0100 Markus Armbruster <armbru@redhat.com> wrote: > Luiz Capitulino <lcapitulino@redhat.com> writes: > > > On Wed, 10 Nov 2010 13:56:39 +0100 > > Markus Armbruster <armbru@redhat.com> wrote: > > > >> Luiz Capitulino <lcapitulino@redhat.com> writes: > >> > >> > On Wed, 10 Nov 2010 10:26:15 +0100 > >> > Markus Armbruster <armbru@redhat.com> wrote: > [...] > >> >> Unlike normal character drivers, this one can't be closed with > >> >> qemu_chr_close(), can it? What happens if someone calls > >> >> qemu_chr_close() on it? > >> > > >> > I guess it will explode, because this driver is not in the chardevs list > >> > and our CharDriverState instance is allocated on the stack. > >> > > >> > Does a function comment solves the problem or do you have something else > >> > in mind? > >> > >> A general OO rule: Having different constructors for different sub-types > >> is okay, but once constructed, you should be able to use the objects > >> without knowing of what sub-type they are. That includes destruction. > > > > We will have to add our MemoryDriver to the chardevs list, this has some > > implications like being visible in qemu_chr_info() and qemu_chr_find(), > > likely to also imply that we should choose a chr->filename. > > Not if we formalize the notion of an "internal use only" character > device. Say, !chr->filename means it's internal, and internal ones > aren't in chardevs. Make qemu_chr_close()'s QTAILQ_REMOVE() conditional > !chr->filename. Yes, it's doable. But this kind of change will make this series intrusive, for example, is it really impossible to create !chr->filename via the normal means (eg. from the user)? What if we break something else with this change? > > Another detail is that we'll have to dynamically alocate our CharDriverState > > instance. Not a problem, but adds a few more lines of code and a > > qemu_free(). None of this is needed today. > > I doubt the alloc/free matters. > > > Really worth it? > > Your call. I don't think so, unless we have a real need for it (and this can be done later anyway). > But if you decide not to, please add a suitable assertion to > qemu_chr_close(), to make it obvious what went wrong when an internal > character device explodes there. > > >> Exceptions prove the rule. Maybe this is one, maybe not. > [...] >
diff --git a/qemu-char.c b/qemu-char.c index 6d2dce7..1ca9ccc 100644 --- a/qemu-char.c +++ b/qemu-char.c @@ -2279,6 +2279,74 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts) return NULL; } +/***********************************************************/ +/* Buffered chardev */ +typedef struct { + size_t outbuf_size; + size_t outbuf_capacity; + uint8_t *outbuf; +} BufferedDriver; + +static int buffered_chr_write(CharDriverState *chr, const uint8_t *buf, int len) +{ + BufferedDriver *d = chr->opaque; + + if (d->outbuf_capacity < (d->outbuf_size + len)) { + /* grow outbuf */ + d->outbuf_capacity += len; + d->outbuf_capacity *= 2; + d->outbuf = qemu_realloc(d->outbuf, d->outbuf_capacity); + } + + memcpy(d->outbuf + d->outbuf_size, buf, len); + d->outbuf_size += len; + + return 0; +} + +void qemu_chr_init_buffered(CharDriverState *chr) +{ + BufferedDriver *d; + + d = qemu_mallocz(sizeof(BufferedDriver)); + d->outbuf_capacity = 4096; + d->outbuf = qemu_mallocz(d->outbuf_capacity); + + memset(chr, 0, sizeof(*chr)); + chr->opaque = d; + chr->chr_write = buffered_chr_write; +} + +QString *qemu_chr_buffered_to_qs(CharDriverState *chr) +{ + char *str; + QString *qs; + BufferedDriver *d = chr->opaque; + + if (d->outbuf_size == 0) { + return NULL; + } + + str = qemu_malloc(d->outbuf_size + 1); + memcpy(str, d->outbuf, d->outbuf_size); + str[d->outbuf_size] = '\0'; + + qs = qstring_from_str(str); + qemu_free(str); + + return qs; +} + +void qemu_chr_close_buffered(CharDriverState *chr) +{ + BufferedDriver *d = chr->opaque; + + qemu_free(d->outbuf); + qemu_free(chr->opaque); + chr->opaque = NULL; + chr->chr_write = NULL; +} + QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename) { char host[65], port[33], width[8], height[8]; diff --git a/qemu-char.h b/qemu-char.h index 18ad12b..4467641 100644 --- a/qemu-char.h +++ b/qemu-char.h @@ -6,6 +6,7 @@ #include "qemu-option.h" #include "qemu-config.h" #include "qobject.h" +#include "qstring.h" /* character device */ @@ -100,6 +101,11 @@ CharDriverState *qemu_chr_open_eventfd(int eventfd); extern int term_escape_char; +/* buffered chardev */ +void qemu_chr_init_buffered(CharDriverState *chr); +void qemu_chr_close_buffered(CharDriverState *chr); +QString *qemu_chr_buffered_to_qs(CharDriverState *chr); + /* async I/O support */ int qemu_set_fd_handler2(int fd,
This driver handles in-memory qemu-char driver operations, that's, all writes to this driver are stored in an internal buffer. The driver doesn't talk to the external world in any way. Right now it's very simple: it supports only writes. But it can be easily extended to support more operations. This driver is going to be used by the monitor subsystem, which needs to 'emulate' a qemu-char device, so that monitor handlers can be ran without a backing device and have their output stored. TODO: Move buffer growth logic to cutils. Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> --- qemu-char.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ qemu-char.h | 6 +++++ 2 files changed, 74 insertions(+), 0 deletions(-)