[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: |
Fri, 12 Nov 2010 13:40:26 -0200 |
On Fri, 12 Nov 2010 16:04:39 +0100
Markus Armbruster <address@hidden> wrote:
> Luiz Capitulino <address@hidden> writes:
>
> > On Fri, 12 Nov 2010 15:16:33 +0100
> > Markus Armbruster <address@hidden> wrote:
> >
> >> Luiz Capitulino <address@hidden> writes:
> >>
> >> > On Fri, 12 Nov 2010 11:21:57 +0100
> >> > Markus Armbruster <address@hidden> wrote:
> >> >
> >> >> Luiz Capitulino <address@hidden> writes:
> >> [...]
> >> >> > +QString *qemu_chr_mem_to_qs(CharDriverState *chr)
> >> >> > +{
> >> >> > + MemoryDriver *d = chr->opaque;
> >> >> > +
> >> >> > + if (d->outbuf_size == 0) {
> >> >> > + return qstring_new();
> >> >> > + }
> >> >>
> >> >> Why is this necessary? Is qstring_from_substr() broken for empty
> >> >> substrings? If it is, it ought to be fixed!
> >> >
> >> > qstring_from_substr() takes a character range; outbuf_size stores a size,
> >> > not a string length. So we do:
> >> >
> >> >> > + return qstring_from_substr((char *) d->outbuf, 0, d->outbuf_size
> >> >> > - 1);
> >> >
> >> > If outbuf_size is 0, we'll be passing a negative value down.
> >>
> >> What's wrong with that?
> >
> > Although it's going to work with the current QString implementation, I don't
> > think it's it's a good idea to rely on a negative index.
>
> How should I extract the substring of S beginning at index B with length
> L? If I cant't do this for any B, L with interval [B,B+L-1] fully
> within [0,length(S)], then the API is flawed, and ought to be replaced.
Not sure we're talking about the same problem, anymore. When you said:
> >> What's wrong with that?
What did you mean? Did you mean 'let's not decrement outbuf_size' or did
you mean 'let's pass -1 anyway'?
Both seem wrong to me: the substring [0,-1] should be invalid and not
decrementing outbuf_size is wrong, because it contains the buffer size and
qstring_from_substr() will consume an additional char from the buffer (which
should be '\0' today, but we shouldn't count on that).
>
> > Maybe, we could have:
> >
> > return qstring_from_substr((char *) d->outbuf, 0,
> > d->outbuf_size > 0 ? d->outbuf_size - 1 : 0);
> >
> > A bit harder to read, but makes the function smaller.
>
> Err, doesn't qstring_from_substr(s, 0, 0) extract a substring of length
> 1?
Yeah, it's a bug. But that doesn't change my suggestion, can we do this way?
This should fix the bug (not even compiled tested):
diff --git a/qstring.c b/qstring.c
index 4e2ba08..72a25de 100644
--- a/qstring.c
+++ b/qstring.c
@@ -42,10 +42,10 @@ QString *qstring_from_substr(const char *str, int start,
int end)
qstring = qemu_malloc(sizeof(*qstring));
- qstring->length = end - start + 1;
- qstring->capacity = qstring->length;
+ qstring->length = end - start;
+ qstring->capacity = qstring->length + 1;
- qstring->string = qemu_malloc(qstring->capacity + 1);
+ qstring->string = qemu_malloc(qstring->capacity);
memcpy(qstring->string, str + start, qstring->length);
qstring->string[qstring->length] = 0;
- [Qemu-devel] [PATCH v3 0/3]: QMP: Human Monitor passthrough, Luiz Capitulino, 2010/11/11
- [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/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
- 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 <=
- 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/11
[Qemu-devel] [PATCH 3/3] QMP/qmp-shell: Introduce HMP mode, Luiz Capitulino, 2010/11/11