Patchwork [1/3] qemu-char: Introduce Memory driver

login
register
mail settings
Submitter Luiz Capitulino
Date Nov. 12, 2010, 3:40 p.m.
Message ID <20101112134026.0d825aa4@doriath>
Download mbox | patch
Permalink /patch/70979/
State New
Headers show

Comments

Luiz Capitulino - Nov. 12, 2010, 3:40 p.m.
On Fri, 12 Nov 2010 16:04:39 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Fri, 12 Nov 2010 15:16:33 +0100
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> 
> >> > On Fri, 12 Nov 2010 11:21:57 +0100
> >> > Markus Armbruster <armbru@redhat.com> wrote:
> >> >
> >> >> Luiz Capitulino <lcapitulino@redhat.com> 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):
Markus Armbruster - Nov. 12, 2010, 4:06 p.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Fri, 12 Nov 2010 16:04:39 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > On Fri, 12 Nov 2010 15:16:33 +0100
>> > Markus Armbruster <armbru@redhat.com> wrote:
>> >
>> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> >> 
>> >> > On Fri, 12 Nov 2010 11:21:57 +0100
>> >> > Markus Armbruster <armbru@redhat.com> wrote:
>> >> >
>> >> >> Luiz Capitulino <lcapitulino@redhat.com> 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'?

Yes, what's wrong with qstring_from_substr(S, 0, -1)?

Its function comment is imprecise, it doesn't tell us whether the END-th
character is included in the substring or not.

The code, however, is clear enough: it *is* included.  And the unit test
checks that.

Therefore, qstring_from_substr("abc", 0, 0) returns the qstring "a".

> Both seem wrong to me: the substring [0,-1] should be invalid

Why?

How do you express "the empty substring starting at 0" then?

>                                                               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;

I suspect this will fail your unit test.
Luiz Capitulino - Nov. 12, 2010, 4:54 p.m.
On Fri, 12 Nov 2010 17:06:16 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Fri, 12 Nov 2010 16:04:39 +0100
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> 
> >> > On Fri, 12 Nov 2010 15:16:33 +0100
> >> > Markus Armbruster <armbru@redhat.com> wrote:
> >> >
> >> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> >> 
> >> >> > On Fri, 12 Nov 2010 11:21:57 +0100
> >> >> > Markus Armbruster <armbru@redhat.com> wrote:
> >> >> >
> >> >> >> Luiz Capitulino <lcapitulino@redhat.com> 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'?
> 
> Yes, what's wrong with qstring_from_substr(S, 0, -1)?
> 
> Its function comment is imprecise, it doesn't tell us whether the END-th
> character is included in the substring or not.
> 
> The code, however, is clear enough: it *is* included.  And the unit test
> checks that.
> 
> Therefore, qstring_from_substr("abc", 0, 0) returns the qstring "a".
> 
> > Both seem wrong to me: the substring [0,-1] should be invalid
> 
> Why?
> 
> How do you express "the empty substring starting at 0" then?

I didn't consider that when I wrote the code, so it's a matter a defining
the behavior we want it to have.


> 
> >                                                               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;
> 
> I suspect this will fail your unit test.

Haven't checked it yet, but maybe it has to be fixed too.

Patch

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;