From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=50657 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PGwUI-0002Ss-PS for qemu-devel@nongnu.org; Fri, 12 Nov 2010 11:28:32 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PGwUD-0005OL-Sg for qemu-devel@nongnu.org; Fri, 12 Nov 2010 11:28:27 -0500 Received: from mx1.redhat.com ([209.132.183.28]:13291) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PGwUD-0005O9-IE for qemu-devel@nongnu.org; Fri, 12 Nov 2010 11:28:25 -0500 Date: Fri, 12 Nov 2010 14:28:16 -0200 From: Luiz Capitulino Subject: Re: [Qemu-devel] [PATCH 1/3] qemu-char: Introduce Memory driver Message-ID: <20101112142816.06de170c@doriath> In-Reply-To: References: <1289415546-19105-1-git-send-email-lcapitulino@redhat.com> <1289415546-19105-2-git-send-email-lcapitulino@redhat.com> <20101111134827.35be7944@doriath> <20101111164413.13966ae4@doriath> <20101112115212.68aed4dd@doriath> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org On Fri, 12 Nov 2010 16:54:14 +0100 Markus Armbruster wrote: > Luiz Capitulino writes: > > > On Fri, 12 Nov 2010 11:16:54 +0100 > > Markus Armbruster wrote: > > > >> Luiz Capitulino writes: > >> > >> > On Thu, 11 Nov 2010 17:32:06 +0100 > >> > Markus Armbruster wrote: > >> > > >> >> Luiz Capitulino writes: > >> >> > >> >> > On Thu, 11 Nov 2010 16:30:26 +0100 > >> >> > Markus Armbruster wrote: > >> >> > > >> >> >> Luiz Capitulino 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 > >> >> >> > --- > >> >> >> > 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. > > Actually, this function works just fine for embedded '\0's (I tested > it): it returns the correct QString, with full length and '\0' embedded. Good. > Only later, when we attempt to put that QString on the wire do we screw > up, in to_json(). It fails to consider the length, and stops at the > first 0. In fact, there's not even a way to get the length of a > QString! There's only qstring_get_str(). I'd call that an API bug. > You might call it a restriction instead ;) Whatever it is, let's do what has to be done: just add it. > If anything needs a comment, it's qobject_to_json(). But I think that > one needs a bug fix instead. Care to send a patch then? > Alternatively, we could document that QString and its users can't cope > with embedded '\0'. That depend on QString users, doesn't it? > > >> 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). > > It's in to_json(). > > [...] >