[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/3] qemu-char: Introduce Memory driver
From: |
Luiz Capitulino |
Subject: |
Re: [Qemu-devel] [PATCH 1/3] qemu-char: Introduce Memory driver |
Date: |
Thu, 11 Nov 2010 16:44:13 -0200 |
On Thu, 11 Nov 2010 17:32:06 +0100
Markus Armbruster <address@hidden> wrote:
> Luiz Capitulino <address@hidden> writes:
>
> > On Thu, 11 Nov 2010 16:30:26 +0100
> > Markus Armbruster <address@hidden> wrote:
> >
> >> Luiz Capitulino <address@hidden> 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 <address@hidden>
> >> > ---
> >> > 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?
The string will be shorter than expected? And what if it contains
non-printable characters?
It's just a cautionary comment to help the user identify such problems, I think
we're making a whole argument about a quite minor thing.
>
> >> > +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.
Okay, found a different way of doing this that should make us both happy.
Very exciting changes in v3!
>
> 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.
Btw, what should we assert() for? We're going to have to access QTAILQ
member I guess.
>
> >>
> >> > 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,
> >>
>
- [Qemu-devel] [PATCH v2 0/3]: QMP: Human Monitor passthrough, Luiz Capitulino, 2010/11/10
- [Qemu-devel] [PATCH 1/3] qemu-char: Introduce Memory driver, Luiz Capitulino, 2010/11/10
- Re: [Qemu-devel] [PATCH 1/3] qemu-char: Introduce Memory driver, Markus Armbruster, 2010/11/11
- Re: [Qemu-devel] [PATCH 1/3] qemu-char: Introduce Memory driver, Luiz Capitulino, 2010/11/11
- Re: [Qemu-devel] [PATCH 1/3] qemu-char: Introduce Memory driver, Markus Armbruster, 2010/11/11
- Re: [Qemu-devel] [PATCH 1/3] qemu-char: Introduce Memory driver,
Luiz Capitulino <=
- Re: [Qemu-devel] [PATCH 1/3] qemu-char: Introduce Memory driver, Markus Armbruster, 2010/11/12
- Re: [Qemu-devel] [PATCH 1/3] qemu-char: Introduce Memory driver, Luiz Capitulino, 2010/11/12
- Re: [Qemu-devel] [PATCH 1/3] qemu-char: Introduce Memory driver, Markus Armbruster, 2010/11/12
- Re: [Qemu-devel] [PATCH 1/3] qemu-char: Introduce Memory driver, Luiz Capitulino, 2010/11/12
[Qemu-devel] [PATCH 2/3] QMP: Introduce Human Monitor passthrough command, Luiz Capitulino, 2010/11/10
[Qemu-devel] [PATCH 3/3] QMP/qmp-shell: Introduce HMP mode, Luiz Capitulino, 2010/11/10