Luiz Capitulino <lcapitul...@redhat.com> writes:
> On Thu, 11 Nov 2010 16:30:26 +0100
> Markus Armbruster <arm...@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitul...@redhat.com> writes:
>>
>> > This driver handles in-memory chardev operations. That's, all writes
>> > to this driver are stored in an internal buffer and it 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 is going to be used by the monitor's "HMP passthrough via QMP"
>> > feature, which needs to run monitor handlers without a backing
>> > device.
>> >
>> > Signed-off-by: Luiz Capitulino <lcapitul...@redhat.com>
>> > ---
>> > qemu-char.c | 66
>> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> > qemu-char.h | 6 +++++
>> > 2 files changed, 72 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/qemu-char.c b/qemu-char.c
>> > index 88997f9..896df14 100644
>> > --- a/qemu-char.c
>> > +++ b/qemu-char.c
>> > @@ -2275,6 +2275,72 @@ static CharDriverState
>> > *qemu_chr_open_socket(QemuOpts *opts)
>> > return NULL;
>> > }
>> >
>> > +/***********************************************************/
>> > +/* Memory chardev */
>> > +typedef struct {
>> > + size_t outbuf_size;
>> > + size_t outbuf_capacity;
>> > + uint8_t *outbuf;
>> > +} MemoryDriver;
>> > +
>> > +static int mem_chr_write(CharDriverState *chr, const uint8_t *buf, int
>> > len)
>> > +{
>> > + MemoryDriver *d = chr->opaque;
>> > +
>> > + /* TODO: the QString implementation has the same code, we should
>> > + * introduce a generic way to do this in cutils.c */
>> > + if (d->outbuf_capacity < d->outbuf_size + len) {
>> > + /* grown outbuf */
>>
>> Used to say "grow" (sans n) here. Intentional change?
>
> Hum, no. I think I've squashed an older commit while rebasing (but this seems
> to be the only problem).
>
>>
>> > + 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 len;
>> > +}
>> > +
>> > +#define DEFAULT_BUF_SIZE 4096
>>
>> It's the *initial* buffer size, isn't it?
>
> Yes.
Could we make the name reflect that then?
>> Doubt it's worth a #define (there's just one user), but that's a matter
>> of taste.
>>
>> > +
>> > +void qemu_chr_init_mem(CharDriverState *chr)
>> > +{
>> > + MemoryDriver *d;
>> > +
>> > + d = qemu_malloc(sizeof(*d));
>> > + d->outbuf_size = 0;
>> > + d->outbuf_capacity = DEFAULT_BUF_SIZE;
>> > + d->outbuf = qemu_mallocz(d->outbuf_capacity);
>> > +
>> > + memset(chr, 0, sizeof(*chr));
>> > + chr->opaque = d;
>> > + chr->chr_write = mem_chr_write;
>> > +}
>> > +
>> > +/* assumes the stored data is a string */
>>
>> What else could it be? Worrying about embedded '\0's?
>
> Yes, as the driver itself doesn't interpret the contents of its
> buffer.
What happens if there are embedded '\0's?
>> > +QString *qemu_chr_mem_to_qs(CharDriverState *chr)
>> > +{
>> > + MemoryDriver *d = chr->opaque;
>> > +
>> > + if (d->outbuf_size == 0) {
>> > + return NULL;
>> > + }
>>
>> Did you forget to change this? We agreed to return an empty QString
>> when chr contains an empty string.
>
> I've changed my mind and forgot to mention it: I thought that we would
> need to return NULL on error conditions, but turns out that this function
> never fails.
>
> So, I do think it's better to let it that way for two reasons:
>
> 1. An empty has at least the '\0' character, but in this case the buffer
> is really empty
qstring_from_substr() copies the contents of the buffer (any length
works, including 0), then appends a '\0'. I'm afraid I don't get the
problem here...
> 2. Returning an empty string for this case will add unneeded complexity
> to the caller, ie. checking if the QString's length is 0 and decref'ing it
I strongly recommend not to screw up the interface of a generally useful
function like qemu_chr_mem_to_qs() just to make its initial user
marginally simpler.
If you decide not to follow my recommendation, please document the
unusual mapping of empty string to null pointer in a function comment.
>> > +
>> > + return qstring_from_substr((char *) d->outbuf, 0, d->outbuf_size - 1);
>> > +}
>> > +
>> > +/* NOTE: this driver can not be closed with qemu_chr_close()! */
>> > +void qemu_chr_close_mem(CharDriverState *chr)
>> > +{
>> > + MemoryDriver *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(). It probably explodes if you try. Please add a
>> suitable assertion to qemu_chr_close() to document the fact, and to
>> ensure misuse fails in a controlled, obvious manner.
>
> Ah forgot, but that can be done as a separate patch, so if I don't respin
> this series I'll send an additional patch for that.
Okay.
>>
>> > 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..c4e55b4 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;
>> >
>> > +/* memory chardev */
>> > +void qemu_chr_init_mem(CharDriverState *chr);
>> > +void qemu_chr_close_mem(CharDriverState *chr);
>> > +QString *qemu_chr_mem_to_qs(CharDriverState *chr);
>> > +
>> > /* async I/O support */
>> >
>> > int qemu_set_fd_handler2(int fd,
>>
