On Fri, 12 Nov 2010 11:16:54 +0100 Markus Armbruster <arm...@redhat.com> wrote:
> Luiz Capitulino <lcapitul...@redhat.com> writes: > > > On Thu, 11 Nov 2010 17:32:06 +0100 > > Markus Armbruster <arm...@redhat.com> wrote: > > > >> 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? > > > > 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. > > When I see "assumes X" in a function comment, I immediately ask "and > what happens when !X?" The default answer is "it explodes, so don't do > that". That answer is wrong here. Therefore, I find the comment > misleading. That's how you interpret it, my interpretation is that I might not get the expected behavior. > Let's figure out what really happens. The human command's output is > sent to the client as a JSON string (response object member return). > JSON strings can consist of Unicode characters, "except for the > characters that must be escaped: quotation mark, reverse solidus, and > the control characters (U+0000 through U+001F)" (RFC 4627, section 2.5). > > Do we escape these characters? Where in the code? Should be in the json parser, but qemu_chr_mem_to_qs() doesn't assume its users (and it obviously shouldn't). > > >> >> > +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. > > Fair question. > > Semantically, we assert that close is safe and does the job. > > For your memory driver, it's not safe, because the QTAILQ_REMOVE() is > safe only when chr is in chardevs, which it isn't. And it doesn't do > the job, because it doesn't free resources. > > We can detect the "not safe" condition: search chardevs for chr. Might > want to put it in a function qemu_chr_is_internal(). > > If we don't want to search, we can add a flag that reflects "is in > chardevs". > > Taking a step back: "external" character devices are in chardevs, and > are to be closed with qemu_chr_close(). "internal" ones are not, and > are to be closed differently. > > [...] >