On Fri, 12 Nov 2010 16:54:14 +0100
Markus Armbruster <arm...@redhat.com> wrote:
> Luiz Capitulino <lcapitul...@redhat.com> writes:
>
> > 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.
>
> 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().
>
> [...]
>
